[2/3] ide: restart atapi dma by re-evaluating command packet
diff mbox

Message ID 1459521130-3792-3-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev April 1, 2016, 2:32 p.m. UTC
From: Pavel Butsykin <pbutsykin@virtuozzo.com>

ide_atapi_dma_restart() used to just complete the DMA with an error,
under the assumption that there isn't enough information to restart it.

However, as the contents of the ->io_buffer is preserved, it looks safe to
just re-evaluate it and dispatch the ATAPI command again.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 hw/ide/atapi.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

John Snow April 1, 2016, 9:01 p.m. UTC | #1
On 04/01/2016 10:32 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> ide_atapi_dma_restart() used to just complete the DMA with an error,
> under the assumption that there isn't enough information to restart it.
> 
> However, as the contents of the ->io_buffer is preserved, it looks safe to
> just re-evaluate it and dispatch the ATAPI command again.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  hw/ide/atapi.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 dweletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 1fe58ab..acc52cd 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -488,14 +488,13 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors,
>  void ide_atapi_dma_restart(IDEState *s)
>  {
>      /*
> -     * I'm not sure we have enough stored to restart the command
> -     * safely, so give the guest an error it should recover from.
> -     * I'm assuming most guests will try to recover from something
> -     * listed as a medium error on a CD; it seems to work on Linux.
> -     * This would be more of a problem if we did any other type of
> -     * DMA operation.
> +     * At this point we can just re-evaluate the packet command and start over.
> +     * The presence of ->dma_cb callback in the pre_save ensures that the packet
> +     * command has been completely sent and we can safely restart command.
>       */
> -    ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE);
> +    s->unit = s->bus->retry_unit;
> +    s->bus->dma->ops->restart_dma(s->bus->dma);
> +    ide_atapi_cmd(s);
>  }
>  
>  static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index,
> 

Is it at all possible that a previous command may have edited the
s->io_buffer that ide_atapi_cmd() uses for SCSI command dispatch?

Let me try to answer my own question.

Here's my understanding: On state change, ide_restart_bh is invoked
unconditionally. If end_transfer_func is ide_atapi_cmd, we invoke
ide_atapi_dma_restart.

What are the conditions for end_transfer_func being set to ide_atapi_cmd
on state change? well... mostly that any ATAPI command got interrupted
before it finished, which is generally not possible with PIO or
synchronous commands because the AIO flush on savevm or migrate should
clear those requests out.

I *think* the only time we run into this problem is with e.g. PCI HBAs
where the DMA controller is programmed before we kick the HBA with the
start signal... which I *think* means that we have no chance of actually
editing the io_buffer before we attempt to "resume" this command --
because if the command *starts* at all, it should *finish* and the only
time we run into this migration case is if we didn't actually start the
command.

Did you audit this at all? Do I sound crazy or correct?
(I really should document this or clean up our restore/resume code, but
that's not up to you. Just a passing thought...)

Thanks,
--js
Pavel Butsykin April 4, 2016, 10:32 a.m. UTC | #2
On 02.04.2016 00:01, John Snow wrote:
>
>
> On 04/01/2016 10:32 AM, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> ide_atapi_dma_restart() used to just complete the DMA with an error,
>> under the assumption that there isn't enough information to restart it.
>>
>> However, as the contents of the ->io_buffer is preserved, it looks safe to
>> just re-evaluate it and dispatch the ATAPI command again.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> ---
>>   hw/ide/atapi.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 dweletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 1fe58ab..acc52cd 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -488,14 +488,13 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors,
>>   void ide_atapi_dma_restart(IDEState *s)
>>   {
>>       /*
>> -     * I'm not sure we have enough stored to restart the command
>> -     * safely, so give the guest an error it should recover from.
>> -     * I'm assuming most guests will try to recover from something
>> -     * listed as a medium error on a CD; it seems to work on Linux.
>> -     * This would be more of a problem if we did any other type of
>> -     * DMA operation.
>> +     * At this point we can just re-evaluate the packet command and start over.
>> +     * The presence of ->dma_cb callback in the pre_save ensures that the packet
>> +     * command has been completely sent and we can safely restart command.
>>        */
>> -    ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE);
>> +    s->unit = s->bus->retry_unit;
>> +    s->bus->dma->ops->restart_dma(s->bus->dma);
>> +    ide_atapi_cmd(s);
>>   }
>>
>>   static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index,
>>
>
> Is it at all possible that a previous command may have edited the
> s->io_buffer that ide_atapi_cmd() uses for SCSI command dispatch?
>
> Let me try to answer my own question.
>
> Here's my understanding: On state change, ide_restart_bh is invoked
> unconditionally. If end_transfer_func is ide_atapi_cmd, we invoke
> ide_atapi_dma_restart.
>
> What are the conditions for end_transfer_func being set to ide_atapi_cmd
> on state change? well... mostly that any ATAPI command got interrupted
> before it finished, which is generally not possible with PIO or
> synchronous commands because the AIO flush on savevm or migrate should
> clear those requests out.
>
In general, it's impossible for ATAPI commands that don't need to set
the dma_cb for execution after bus mastering..

> I *think* the only time we run into this problem is with e.g. PCI HBAs
> where the DMA controller is programmed before we kick the HBA with the
> start signal... which I *think* means that we have no chance of actually
> editing the io_buffer before we attempt to "resume" this command --
> because if the command *starts* at all, it should *finish* and the only
> time we run into this migration case is if we didn't actually start the
> command.
>
> Did you audit this at all? Do I sound crazy or correct?

:) All looks correct to me! In the form in which there is now the
realization of the DMA ATAPI it should work. Although it doesn't
look so transparent.

> (I really should document this or clean up our restore/resume code, but
> that's not up to you. Just a passing thought...)
>
> Thanks,
> --js
>

Patch
diff mbox

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 1fe58ab..acc52cd 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -488,14 +488,13 @@  static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors,
 void ide_atapi_dma_restart(IDEState *s)
 {
     /*
-     * I'm not sure we have enough stored to restart the command
-     * safely, so give the guest an error it should recover from.
-     * I'm assuming most guests will try to recover from something
-     * listed as a medium error on a CD; it seems to work on Linux.
-     * This would be more of a problem if we did any other type of
-     * DMA operation.
+     * At this point we can just re-evaluate the packet command and start over.
+     * The presence of ->dma_cb callback in the pre_save ensures that the packet
+     * command has been completely sent and we can safely restart command.
      */
-    ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE);
+    s->unit = s->bus->retry_unit;
+    s->bus->dma->ops->restart_dma(s->bus->dma);
+    ide_atapi_cmd(s);
 }
 
 static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index,