From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53203) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1an1op-0004HF-D3 for qemu-devel@nongnu.org; Mon, 04 Apr 2016 06:33:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1an1ok-0003Wl-Cv for qemu-devel@nongnu.org; Mon, 04 Apr 2016 06:33:15 -0400 Received: from mail-am1on0147.outbound.protection.outlook.com ([157.56.112.147]:26982 helo=emea01-am1-obe.outbound.protection.outlook.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1an1oj-0003Wd-Nt for qemu-devel@nongnu.org; Mon, 04 Apr 2016 06:33:10 -0400 References: <1459521130-3792-1-git-send-email-den@openvz.org> <1459521130-3792-3-git-send-email-den@openvz.org> <56FEE1AF.2090102@redhat.com> From: Pavel Butsykin Message-ID: <570242B5.3080103@virtuozzo.com> Date: Mon, 4 Apr 2016 13:32:21 +0300 MIME-Version: 1.0 In-Reply-To: <56FEE1AF.2090102@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed 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: John Snow , "Denis V. Lunev" , qemu-devel@nongnu.org Cc: Paolo Bonzini , rkagan@virtuozzo.com On 02.04.2016 00:01, John Snow wrote: > > > 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. > 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 >