From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44528) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YCAhA-0005t7-P7 for qemu-devel@nongnu.org; Fri, 16 Jan 2015 12:28:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YCAh6-0006gI-Ny for qemu-devel@nongnu.org; Fri, 16 Jan 2015 12:28:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46325) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YCAh6-0006gC-GX for qemu-devel@nongnu.org; Fri, 16 Jan 2015 12:28:24 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t0GHSNpW015585 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 16 Jan 2015 12:28:24 -0500 Message-ID: <54B94A36.5060409@redhat.com> Date: Fri, 16 Jan 2015 12:28:22 -0500 From: John Snow MIME-Version: 1.0 References: <1418148909-19870-1-git-send-email-dgilbert@redhat.com> <1418148909-19870-3-git-send-email-dgilbert@redhat.com> In-Reply-To: <1418148909-19870-3-git-send-email-dgilbert@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] atapi migration: Throw recoverable error to avoid recovery List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" , qemu-devel@nongnu.org Cc: pbonzini@redhat.com On 12/09/2014 01:15 PM, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" > > (With the previous atapi_dma flag recovery) > If migration happens between the ATAPI command being written and the > bmdma being started, the DMA is dropped. Eventually the guest times > out and recovers, but that can take many seconds. > (This is rare, on a pingpong reading the CD continuously I hit > this about ~1/30-1/50 migrates) > > I don't think we've got enough state to be able to recover safely > at this point, so I throw a 'medium error, no seek complete' > that I'm assuming guests will try and recover from an apparently > dirty CD. > > OK, it's a hack, the real solution is probably to push a lot of > ATAPI state into the migration stream, but this is a fix that > works with no stream changes. Tested only on Linux (both RHEL5 > (pre-libata) and RHEL7). > > Signed-off-by: Dr. David Alan Gilbert > --- > hw/ide/atapi.c | 17 +++++++++++++++++ > hw/ide/internal.h | 2 ++ > hw/ide/pci.c | 11 +++++++++++ > 3 files changed, 30 insertions(+) > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > index c63b7e5..e17799c 100644 > --- a/hw/ide/atapi.c > +++ b/hw/ide/atapi.c > @@ -394,6 +394,23 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors, > } > } > > + > +/* Called by *_restart_bh when the transfer function points > + * to ide_atapi_cmd > + */ > +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. > + */ > + ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE); > +} > + > static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index, > uint16_t profile) > { > diff --git a/hw/ide/internal.h b/hw/ide/internal.h > index 8a3eca4..8b65285 100644 > --- a/hw/ide/internal.h > +++ b/hw/ide/internal.h > @@ -289,6 +289,7 @@ typedef struct IDEDMAOps IDEDMAOps; > #define ATAPI_INT_REASON_TAG 0xf8 > > /* same constants as bochs */ > +#define ASC_NO_SEEK_COMPLETE 0x02 > #define ASC_ILLEGAL_OPCODE 0x20 > #define ASC_LOGICAL_BLOCK_OOR 0x21 > #define ASC_INV_FIELD_IN_CMD_PACKET 0x24 > @@ -529,6 +530,7 @@ void ide_dma_error(IDEState *s); > > void ide_atapi_cmd_ok(IDEState *s); > void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc); > +void ide_atapi_dma_restart(IDEState *s); > void ide_atapi_io_error(IDEState *s, int ret); > > void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val); > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index bee5ad3..e3f2054 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -235,6 +235,17 @@ static void bmdma_restart_bh(void *opaque) > } > } else if (error_status & IDE_RETRY_FLUSH) { > ide_flush_cache(bmdma_active_if(bm)); > + } else { > + IDEState *s = bmdma_active_if(bm); > + > + /* > + * We've not got any bits to tell us about ATAPI - but > + * we do have the end_transfer_func that tells us what > + * we're trying to do. > + */ > + if (s->end_transfer_func == ide_atapi_cmd) { > + ide_atapi_dma_restart(s); > + } > } > } > > I think this workaround is sensible until we develop a more precise fix. Reviewed-by: John Snow