From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44006) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1am6CL-00045R-FJ for qemu-devel@nongnu.org; Fri, 01 Apr 2016 17:01:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1am6CI-0005Ln-6u for qemu-devel@nongnu.org; Fri, 01 Apr 2016 17:01:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53240) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1am6CH-0005Ld-VB for qemu-devel@nongnu.org; Fri, 01 Apr 2016 17:01:38 -0400 References: <1459521130-3792-1-git-send-email-den@openvz.org> <1459521130-3792-3-git-send-email-den@openvz.org> From: John Snow Message-ID: <56FEE1AF.2090102@redhat.com> Date: Fri, 1 Apr 2016 17:01:35 -0400 MIME-Version: 1.0 In-Reply-To: <1459521130-3792-3-git-send-email-den@openvz.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" , qemu-devel@nongnu.org Cc: Paolo Bonzini , rkagan@virtuozzo.com, Pavel Butsykin On 04/01/2016 10:32 AM, Denis V. Lunev wrote: > From: Pavel Butsykin > > 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 > Reviewed-by: Roman Kagan > Signed-off-by: Denis V. Lunev > --- > 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