Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma

[John Snow]

On 03/28/2016 07:48 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <address@hidden>
> 
> Restart of ATAPI DMA used to be unreachable, because the request to do
> so wasn't indicated in bus->error_status due to the lack of spare bits, and
> ide_restart_bh() would return early doing nothing.
> 
> This patch makes use of the observation that not all bit combinations were
> possible in ->error_status. In particular, IDE_RETRY_READ only made sense
> together with IDE_RETRY_DMA or IDE_RETRY_PIO. This allows to re-use
> IDE_RETRY_READ alone as an indicator of ATAPI DMA restart request.
> 
> To makes things more uniform, ATAPI DMA gets its own value for ->dma_cmd.
> As a means against confusion, macros are added to test the state of
> ->error_status.
> 
> The patch fixes the restart of both in-flight and pending ATAPI DMA,
> following the scheme similar to that of IDE DMA.
> 
> Signed-off-by: Pavel Butsykin <address@hidden>
> Reviewed-by: Roman Kagan <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: John Snow <address@hidden>
> ---
>  hw/ide/atapi.c    | 15 ++++++++-------
>  hw/ide/core.c     | 27 ++++++++++++---------------
>  hw/ide/internal.h | 21 +++++++++++++++++++++
>  3 files changed, 41 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index acc52cd..fb9ae43 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -342,6 +342,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, 
> int max_size)
>          block_acct_start(blk_get_stats(s->blk), &s->acct, size,
>                           BLOCK_ACCT_READ);
>          s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
> +        s->dma_cmd = IDE_DMA_ATAPI;
>          ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
>      } else {
>          s->status = READY_STAT | SEEK_STAT;
> @@ -375,15 +376,18 @@ static void ide_atapi_cmd_check_status(IDEState *s)
>  }
>  /* ATAPI DMA support */
>  
> -/* XXX: handle read errors */
>  static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
>  {
>      IDEState *s = opaque;
>      int data_offset, n;
>  
>      if (ret < 0) {
> -        ide_atapi_io_error(s, ret);
> -        goto eot;
> +        if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
> +            if (s->bus->error_status) {
> +                return;
> +            }
> +            goto eot;
> +        }
>      }
>  
>      if (s->io_buffer_size > 0) {
> @@ -464,6 +468,7 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int lba, 
> int nb_sectors,
>  
>      /* XXX: check if BUSY_STAT should be set */
>      s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
> +    s->dma_cmd = IDE_DMA_ATAPI;
>      ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
>  }
>  
> @@ -481,10 +486,6 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int 
> nb_sectors,
>      }
>  }
>  
> -
> -/* Called by *_restart_bh when the transfer function points
> - * to ide_atapi_cmd
> - */
>  void ide_atapi_dma_restart(IDEState *s)
>  {
>      /*
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 8f86036..0425d86 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -56,7 +56,6 @@ static const int smart_attributes[][12] = {
>      { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 
> 0x32},
>  };
>  
> -static int ide_handle_rw_error(IDEState *s, int error, int op);
>  static void ide_dummy_transfer_stop(IDEState *s);
>  
>  static void padstr(char *str, const char *src, int len)
> @@ -772,7 +771,7 @@ void ide_dma_error(IDEState *s)
>      ide_set_irq(s->bus);
>  }
>  
> -static int ide_handle_rw_error(IDEState *s, int error, int op)
> +int ide_handle_rw_error(IDEState *s, int error, int op)
>  {
>      bool is_read = (op & IDE_RETRY_READ) != 0;
>      BlockErrorAction action = blk_get_error_action(s->blk, is_read, error);
> @@ -782,8 +781,10 @@ static int ide_handle_rw_error(IDEState *s, int error, 
> int op)
>          s->bus->error_status = op;
>      } else if (action == BLOCK_ERROR_ACTION_REPORT) {
>          block_acct_failed(blk_get_stats(s->blk), &s->acct);
> -        if (op & IDE_RETRY_DMA) {
> +        if (IS_IDE_RETRY_DMA(op)) {
>              ide_dma_error(s);
> +        } else if (IS_IDE_RETRY_ATAPI(op)) {
> +            ide_atapi_io_error(s, -error);
>          } else {
>              ide_rw_error(s);
>          }
> @@ -871,6 +872,8 @@ static void ide_dma_cb(void *opaque, int ret)
>                                          ide_issue_trim, ide_dma_cb, s,
>                                          DMA_DIRECTION_TO_DEVICE);
>          break;
> +    default:
> +        abort();
>      }
>      return;
>  
> @@ -2517,15 +2520,13 @@ static void ide_restart_bh(void *opaque)
>          if (s->bus->dma->ops->restart) {
>              s->bus->dma->ops->restart(s->bus->dma);
>          }
> -    }
> -
> -    if (error_status & IDE_RETRY_DMA) {
> +    } else if (IS_IDE_RETRY_DMA(error_status)) {
>          if (error_status & IDE_RETRY_TRIM) {
>              ide_restart_dma(s, IDE_DMA_TRIM);
>          } else {
>              ide_restart_dma(s, is_read ? IDE_DMA_READ : IDE_DMA_WRITE);
>          }
> -    } else if (error_status & IDE_RETRY_PIO) {
> +    } else if (IS_IDE_RETRY_PIO(error_status)) {
>          if (is_read) {
>              ide_sector_read(s);
>          } else {
> @@ -2533,15 +2534,11 @@ static void ide_restart_bh(void *opaque)
>          }
>      } else if (error_status & IDE_RETRY_FLUSH) {
>          ide_flush_cache(s);
> +    } else if (IS_IDE_RETRY_ATAPI(error_status)) {
> +        assert(s->end_transfer_func == ide_atapi_cmd);
> +        ide_atapi_dma_restart(s);
>      } else {
> -        /*
> -         * We've not got any bits to tell us about ATAPI - but
> -         * we do have the end_transfer_func that tells us what
> -         * we're trying to do.
> -         */
> -        if (s->end_transfer_func == ide_atapi_cmd) {
> -            ide_atapi_dma_restart(s);
> -        }
> +        abort();
>      }
>  }
>  
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 68c7d0d..eb006c2 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -338,6 +338,7 @@ enum ide_dma_cmd {
>      IDE_DMA_READ,
>      IDE_DMA_WRITE,
>      IDE_DMA_TRIM,
> +    IDE_DMA_ATAPI
>  };
>  
>  #define ide_cmd_is_read(s) \
> @@ -508,11 +509,27 @@ struct IDEDevice {
>  /* These are used for the error_status field of IDEBus */
>  #define IDE_RETRY_DMA  0x08
>  #define IDE_RETRY_PIO  0x10
> +#define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */
>  #define IDE_RETRY_READ  0x20
>  #define IDE_RETRY_FLUSH 0x40
>  #define IDE_RETRY_TRIM 0x80
>  #define IDE_RETRY_HBA  0x100
>  
> +#define IS_IDE_RETRY_DMA(_status) \
> +    ((_status) & IDE_RETRY_DMA)
> +
> +#define IS_IDE_RETRY_PIO(_status) \
> +    ((_status) & IDE_RETRY_PIO)
> +
> +/*
> + * The method of the IDE_RETRY_ATAPI determination is to use a previously
> + * impossible bit combination as a new status value.
> + */
> +#define IS_IDE_RETRY_ATAPI(_status)   \
> +    (((_status) & IDE_RETRY_ATAPI) && \
> +     !IS_IDE_RETRY_DMA(_status) &&    \
> +     !IS_IDE_RETRY_PIO(_status))
> +
>  static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
>  {
>      switch (dma_cmd) {
> @@ -522,6 +539,8 @@ static inline uint8_t ide_dma_cmd_to_retry(uint8_t 
> dma_cmd)
>          return IDE_RETRY_DMA;
>      case IDE_DMA_TRIM:
>          return IDE_RETRY_DMA | IDE_RETRY_TRIM;
> +    case IDE_DMA_ATAPI:
> +        return IDE_RETRY_ATAPI;
>      default:
>          break;
>      }
> @@ -612,4 +631,6 @@ void ide_bus_new(IDEBus *idebus, size_t idebus_size, 
> DeviceState *dev,
>                   int bus_id, int max_units);
>  IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
>  
> +int ide_handle_rw_error(IDEState *s, int error, int op);
> +
>  #endif /* HW_IDE_INTERNAL_H */
> 


Sorry, I'm afraid this doesn't compile:

/home/bos/jhuston/src/qemu/hw/ide/macio.c: In function
‘pmac_ide_transfer_cb’:
/home/bos/jhuston/src/qemu/hw/ide/macio.c:339:5: error: enumeration
value ‘IDE_DMA_ATAPI’ not handled in switch [-Werror=switch]
switch (s->dma_cmd) {
^
cc1: all warnings being treated as errors
/home/bos/jhuston/src/qemu/rules.mak:57: recipe for target
'hw/ide/macio.o' failed
make: *** [hw/ide/macio.o] Error 1
make: *** Waiting for unfinished jobs....