On 04/01/2016 10:32 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsy...@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 <pbutsy...@virtuozzo.com>
> Reviewed-by: Roman Kagan <rka...@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <d...@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
