Re: [Qemu-devel] [PATCH 1/3] ide: don't lose pending dma state

[John Snow]

On 03/28/2016 07:48 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <address@hidden>
> 
> If the migration occurs after the IDE DMA has been set up but before it
> has been initiated, the state gets lost upon save/restore. Specifically,
> ->dma_cb callback gets cleared, so, when the guest eventually starts bus
> mastering, the DMA never completes, causing the guest to time out the
> operation.
> 
> OTOH all the infrastructure is already in place to restart the DMA if
> the migration happens while the DMA is in progress.
> 
> So reuse that infrastructure, by setting bus->error_status based on
> ->dma_cmd in pre_save if ->dma_cb callback is already set but DMAING is
> clear. This will indicate the need for restart and make sure ->dma_cb is
> restored in ide_restart_bh(); however since DMAING is clear the state
> upon restore will be exactly "ready for DMA" as before the save.
> 
> 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/core.c     |  9 +--------
>  hw/ide/internal.h | 15 +++++++++++++++
>  hw/ide/pci.c      |  4 ++++
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 241e840..8f86036 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -803,14 +803,7 @@ static void ide_dma_cb(void *opaque, int ret)
>          return;
>      }
>      if (ret < 0) {
> -        int op = IDE_RETRY_DMA;
> -
> -        if (s->dma_cmd == IDE_DMA_READ)
> -            op |= IDE_RETRY_READ;
> -        else if (s->dma_cmd == IDE_DMA_TRIM)
> -            op |= IDE_RETRY_TRIM;
> -
> -        if (ide_handle_rw_error(s, -ret, op)) {
> +        if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
>              return;
>          }
>      }
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 86bde26..68c7d0d 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -513,6 +513,21 @@ struct IDEDevice {
>  #define IDE_RETRY_TRIM 0x80
>  #define IDE_RETRY_HBA  0x100
>  
> +static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
> +{
> +    switch (dma_cmd) {
> +    case IDE_DMA_READ:
> +        return IDE_RETRY_DMA | IDE_RETRY_READ;
> +    case IDE_DMA_WRITE:
> +        return IDE_RETRY_DMA;
> +    case IDE_DMA_TRIM:
> +        return IDE_RETRY_DMA | IDE_RETRY_TRIM;
> +    default:
> +        break;
> +    }
> +    return 0;
> +}
> +
>  static inline IDEState *idebus_active_if(IDEBus *bus)
>  {
>      return bus->ifs + bus->unit;
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 92ffee7..8d56a00 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -308,6 +308,10 @@ static void ide_bmdma_pre_save(void *opaque)
>      BMDMAState *bm = opaque;
>      uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
>  
> +    if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
> +        bm->bus->error_status =
> +            ide_dma_cmd_to_retry(bmdma_active_if(bm)->dma_cmd);
> +    }
>      bm->migration_retry_unit = bm->bus->retry_unit;
>      bm->migration_retry_sector_num = bm->bus->retry_sector_num;
>      bm->migration_retry_nsector = bm->bus->retry_nsector;
> 

_this_ is the one I meant to R-B, thanks Denis. (Testing the others now,
but having some laptop issues. Please stand by.)

Reviewed-by: John Snow <address@hidden>