[1/3] ide: don't loose pending dma state
diff mbox

Message ID 1458728792-15779-2-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev March 23, 2016, 10:26 a.m. UTC
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(-)

Comments

Paolo Bonzini March 23, 2016, 8:34 p.m. UTC | #1
On 23/03/2016 11:26, Denis V. Lunev wrote:
> 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.

Could you just use the dma_cmd field to build the error_status?

For extra points, make ide_handle_rw_error convert IDE_DMA_* to
IDE_RETRY_* so that the callers only need to pass in IDE_RETRY_DMA (like
they only need to pass in IDE_RETRY_PIO).

Paolo
Pavel Butsykin March 24, 2016, 9:24 a.m. UTC | #2
On 23.03.2016 23:34, Paolo Bonzini wrote:
>
>
> On 23/03/2016 11:26, Denis V. Lunev wrote:
>> 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.
>
> Could you just use the dma_cmd field to build the error_status?
>
> For extra points, make ide_handle_rw_error convert IDE_DMA_* to
> IDE_RETRY_* so that the callers only need to pass in IDE_RETRY_DMA (like
> they only need to pass in IDE_RETRY_PIO).
>
> Paolo
>
You mean to do something like that:

ide_handle_rw_error(s, -ret, s->dma_cmd | IDE_RETRY_DMA)

?
Paolo Bonzini March 24, 2016, 11:23 a.m. UTC | #3
On 24/03/2016 10:24, Pavel Butsykin wrote:
>> For extra points, make ide_handle_rw_error convert IDE_DMA_* to
>> IDE_RETRY_* so that the callers only need to pass in IDE_RETRY_DMA
>>
> You mean to do something like that:
> 
> ide_handle_rw_error(s, -ret, s->dma_cmd | IDE_RETRY_DMA)

Just ide_handle_rw_error(s, -ret, IDE_RETRY_DMA)

and in ide_handle_rw_error

    if (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;
    }

But I'm not sure anymore this is a good idea.

However, if it works, using dma_cmd would be better than using the dma_cb.

Paolo
Pavel Butsykin March 24, 2016, 1:38 p.m. UTC | #4
On 24.03.2016 14:23, Paolo Bonzini wrote:
>
>
> On 24/03/2016 10:24, Pavel Butsykin wrote:
>>> For extra points, make ide_handle_rw_error convert IDE_DMA_* to
>>> IDE_RETRY_* so that the callers only need to pass in IDE_RETRY_DMA
>>>
>> You mean to do something like that:
>>
>> ide_handle_rw_error(s, -ret, s->dma_cmd | IDE_RETRY_DMA)
>
> Just ide_handle_rw_error(s, -ret, IDE_RETRY_DMA)
>
> and in ide_handle_rw_error
>
>      if (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;
>      }
>
> But I'm not sure anymore this is a good idea.
>
Look at the ide_sector_read_cb function. The ->dma_cmd field is used 
only for the DMA, we can't use the ide_cmd_is_read macro for PIO or
ATAPI. In fact, the ide_handle_rw_error looks like a generic function
and I do not see any good reason to move the handling of the dma_cmd
field(especially in the context of bug fixes).

> However, if it works, using dma_cmd would be better than using the dma_cb.
>
We can't use the dma_cmd field to build the ATAPI ->error_status. Your
proposal is quite logical, but please look at the 3rd patch, there to
build ->error_status we can use the same dma_cb, it is very convenient.

> Paolo
>
Paolo Bonzini March 24, 2016, 2:07 p.m. UTC | #5
On 24/03/2016 14:38, Pavel Butsykin wrote:
> Look at the ide_sector_read_cb function. The ->dma_cmd field is used
> only for the DMA, we can't use the ide_cmd_is_read macro for PIO or
> ATAPI. In fact, the ide_handle_rw_error looks like a generic function
> and I do not see any good reason to move the handling of the dma_cmd
> field(especially in the context of bug fixes).

I agree now.

>> However, if it works, using dma_cmd would be better than using the
>> dma_cb.
>>
> We can't use the dma_cmd field to build the ATAPI ->error_status. Your
> proposal is quite logical, but please look at the 3rd patch, there to
> build ->error_status we can use the same dma_cb, it is very convenient.

Couldn't you just add a new dma_cmd value IDE_DMA_ATAPI, and set it
before every call to ide_start_dma(s, ide_atapi_cmd_read_dma_cb)?

Thanks,

Paolo
Eric Blake March 24, 2016, 2:47 p.m. UTC | #6
On 03/24/2016 03:24 AM, Pavel Butsykin wrote:
> On 23.03.2016 23:34, Paolo Bonzini wrote:

s/loose/lose/ in the subject line (lose rhymes with "use" and deals with
loss; loose rhymes with "goose" and is the opposite of tighten)
Pavel Butsykin March 24, 2016, 3:11 p.m. UTC | #7
On 24.03.2016 17:07, Paolo Bonzini wrote:
>
>
> On 24/03/2016 14:38, Pavel Butsykin wrote:
>> Look at the ide_sector_read_cb function. The ->dma_cmd field is used
>> only for the DMA, we can't use the ide_cmd_is_read macro for PIO or
>> ATAPI. In fact, the ide_handle_rw_error looks like a generic function
>> and I do not see any good reason to move the handling of the dma_cmd
>> field(especially in the context of bug fixes).
>
> I agree now.
>
>>> However, if it works, using dma_cmd would be better than using the
>>> dma_cb.
>>>
>> We can't use the dma_cmd field to build the ATAPI ->error_status. Your
>> proposal is quite logical, but please look at the 3rd patch, there to
>> build ->error_status we can use the same dma_cb, it is very convenient.
>
> Couldn't you just add a new dma_cmd value IDE_DMA_ATAPI, and set it
> before every call to ide_start_dma(s, ide_atapi_cmd_read_dma_cb)?
>
You want something like this:
dma_cb()
{
...
     if (ret < 0) {
         if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s))) {
             return;
         }
     }
...
}

static void ide_bmdma_pre_save(void *opaque)
{
...
     if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
         bm->bus->error_status = ide_dma_cmd_to_retry(bmdma_active_if(bm));
     }
...
}

??

> Thanks,
>
> Paolo
>
Paolo Bonzini March 24, 2016, 3:12 p.m. UTC | #8
On 24/03/2016 16:11, Pavel Butsykin wrote:
>>
> You want something like this:
> dma_cb()
> {
> ...
>     if (ret < 0) {
>         if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s))) {
>             return;
>         }
>     }
> ...
> }
> 
> static void ide_bmdma_pre_save(void *opaque)
> {
> ...
>     if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
>         bm->bus->error_status = ide_dma_cmd_to_retry(bmdma_active_if(bm));
>     }
> ...
> }

Yes, that would do.

Paolo
Pavel Butsykin March 24, 2016, 3:20 p.m. UTC | #9
On 24.03.2016 18:12, Paolo Bonzini wrote:
>
>
> On 24/03/2016 16:11, Pavel Butsykin wrote:
>>>
>> You want something like this:
>> dma_cb()
>> {
>> ...
>>      if (ret < 0) {
>>          if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s))) {
>>              return;
>>          }
>>      }
>> ...
>> }
>>
>> static void ide_bmdma_pre_save(void *opaque)
>> {
>> ...
>>      if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
>>          bm->bus->error_status = ide_dma_cmd_to_retry(bmdma_active_if(bm));
>>      }
>> ...
>> }
>
> Yes, that would do.
>
Well, I can do it, looks quite ok.

> Paolo
>
Pavel Butsykin March 24, 2016, 3:40 p.m. UTC | #10
On 24.03.2016 17:47, Eric Blake wrote:
> On 03/24/2016 03:24 AM, Pavel Butsykin wrote:
>> On 23.03.2016 23:34, Paolo Bonzini wrote:
>
> s/loose/lose/ in the subject line (lose rhymes with "use" and deals with
> loss; loose rhymes with "goose" and is the opposite of tighten)
>
>
thanks)
John Snow March 30, 2016, 5:35 p.m. UTC | #11
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>
Denis V. Lunev March 30, 2016, 5:38 p.m. UTC | #12
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
John Snow March 30, 2016, 5:44 p.m. UTC | #13
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.

Patch
diff mbox

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;