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

[John Snow <jsnow@redhat.com>]

On 03/30/2016 01:38 PM, Denis V. Lunev wrote:
> On 03/30/2016 08:35 PM, John Snow wrote:
>>
>> On 03/23/2016 06:26 AM, Denis V. Lunev wrote:
>>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>>
>>> 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 calling the DMA callback with an
>>> artificial error code in pre_save if the callback is already set but
>>> DMAING is clear. The callback then sets bus->error_status to indicate
>>> the need for restart; however since DMAING is clear the state upon
>>> restore will be exactly "ready forDMA" as before the save.
>>>
>>> This patch only fixes the IDE DMA case; the ATAPI DMA is left with a
>>> stub
>>> for now since its restart hadn't been implemented yet. It will be
>>> addressed in the followup patch.
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: John Snow <jsnow@redhat.com>
>>> ---
>>>   hw/ide/atapi.c    | 9 ++++++++-
>>>   hw/ide/core.c     | 9 ++++++++-
>>>   hw/ide/internal.h | 7 +++++++
>>>   hw/ide/pci.c      | 3 +++
>>>   4 files changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>>> index 1fe58ab..268220d 100644
>>> --- a/hw/ide/atapi.c
>>> +++ b/hw/ide/atapi.c
>>> @@ -381,7 +381,14 @@ static void ide_atapi_cmd_read_dma_cb(void
>>> *opaque, int ret)
>>>       IDEState *s = opaque;
>>>       int data_offset, n;
>>>   -    if (ret < 0) {
>>> +    if (ret < 0) { /* XXX: handle -ESTATESAVE for migration */
>>> +        if (ret == -ESTATESAVE) {
>>> +            /*
>>> +             * This case is not really an error
>>> +             * but a request to save the state.
>>> +             */
>>> +            return;
>>> +        }
>>>           ide_atapi_io_error(s, ret);
>>>           goto eot;
>>>       }
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 241e840..872e11f 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -810,7 +810,14 @@ static void ide_dma_cb(void *opaque, int ret)
>>>           else if (s->dma_cmd == IDE_DMA_TRIM)
>>>               op |= IDE_RETRY_TRIM;
>>>   -        if (ide_handle_rw_error(s, -ret, op)) {
>>> +        if (ret == -ESTATESAVE) {
>>> +            /*
>>> +             * This case is not really an error
>>> +             * but a request to save the state.
>>> +             */
>>> +            s->bus->error_status = op;
>>> +            return;
>>> +        } else if (ide_handle_rw_error(s, -ret, op)) {
>>>               return;
>>>           }
>>>       }
>>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>>> index 86bde26..dcd8627 100644
>>> --- a/hw/ide/internal.h
>>> +++ b/hw/ide/internal.h
>>> @@ -513,6 +513,13 @@ struct IDEDevice {
>>>   #define IDE_RETRY_TRIM 0x80
>>>   #define IDE_RETRY_HBA  0x100
>>>   +/*
>>> + * a code to trigger entering error path and save/restore the "ready
>>> to DMA"
>>> + * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not
>>> supposed to
>>> + * come from the block layer
>>> + */
>>> +#define ESTATESAVE EINPROGRESS
>>> +
>>>   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..e1f89fa 100644
>>> --- a/hw/ide/pci.c
>>> +++ b/hw/ide/pci.c
>>> @@ -308,6 +308,9 @@ 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->dma_cb(bmdma_active_if(bm), -ESTATESAVE);
>>> +    }
>>>       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;
>>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
> there is never patch sent [PATCH 1/3] ide: don't lose pending dma state
> 28.03
> as v2.
> 
> Den

My mistake. I tested the new one but replied to the wrong mail.