From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44032) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHe4P-0003qF-Nz for qemu-devel@nongnu.org; Fri, 08 Jan 2016 15:55:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHe4K-0000TK-NK for qemu-devel@nongnu.org; Fri, 08 Jan 2016 15:55:37 -0500 References: <1452112646-15969-1-git-send-email-mark.cave-ayland@ilande.co.uk> <1452112646-15969-3-git-send-email-mark.cave-ayland@ilande.co.uk> <568D7FBC.1000505@redhat.com> <568D8487.6000105@ilande.co.uk> From: John Snow Message-ID: <56902240.4040106@redhat.com> Date: Fri, 8 Jan 2016 15:55:28 -0500 MIME-Version: 1.0 In-Reply-To: <568D8487.6000105@ilande.co.uk> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/4] macio: add dma_active to VMStateDescription List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mark Cave-Ayland , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de Cc: "Dr. David Alan Gilbert" , Stefan Hajnoczi On 01/06/2016 04:17 PM, Mark Cave-Ayland wrote: > On 06/01/16 20:57, John Snow wrote: > >> On 01/06/2016 03:37 PM, Mark Cave-Ayland wrote: >>> Make sure that we include the value of dma_active in the migration stream. >>> >>> Signed-off-by: Mark Cave-Ayland >>> --- >>> hw/ide/macio.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c >>> index 560c071..695d4d2 100644 >>> --- a/hw/ide/macio.c >>> +++ b/hw/ide/macio.c >>> @@ -518,11 +518,12 @@ static const MemoryRegionOps pmac_ide_ops = { >>> >>> static const VMStateDescription vmstate_pmac = { >>> .name = "ide", >>> - .version_id = 3, >>> + .version_id = 4, >>> .minimum_version_id = 0, >>> .fields = (VMStateField[]) { >>> VMSTATE_IDE_BUS(bus, MACIOIDEState), >>> VMSTATE_IDE_DRIVES(bus.ifs, MACIOIDEState), >>> + VMSTATE_BOOL(dma_active, MACIOIDEState), >>> VMSTATE_END_OF_LIST() >>> } >>> }; >>> >> >> Did you wind up ever observing this value to be non-zero when it was >> written to the migration stream? >> >> I really did think that we should be able to assume this was always >> false due to how migration will drain all outstanding AIO, but maybe I >> am mistaken. > > I think this can happen because Darwin/MacOS sets the DBDMA processor > running first *before* the IDE request is issued, compared to pretty > much every other OS which issues the IDE request *first* which then in > turn invokes the DMA engine (which is the general assumption in the QEMU > IDE/DMA APIs). > > So there could be a window where the DBDMA is programmed and active but > migration takes place before the corresponding IDE request has been > issued (which is exactly the situation that this flag handles). > > > ATB, > > Mark. > sadly that seems to be the case. ide_dbdma_start looks like it can yield through DBDMA_kick, so there's time for things to go awry. Acked-by: John Snow I had an off-list discussion with David Gilbert on how the migration fields work here -- this will introduce a hard incompatibility between pre-2.5 and post-2.5, which might be fine since Mac has never really quite worked correctly anyway. If you want to worry about compatibility, David advised me that a conditional subsection might be appropriate: since dma_active is /usually/ false, we can use this as a flag for deciding to migrate it or not: i.e. if it's false, we skip the field and the receiver assumes it's false in post_load, or if we migrate to an older version, it never has to worry about it. If it's true, you get a migration error that says the subsection wasn't found, but you get to try to migrate again -- it's kind of a cheesy way to say that you can't migrate to older versions while the DMA is active. Future versions can accept the true boolean, though. HTH --js