Re: [Qemu-devel] [PATCH 2/4] macio: add dma_active to VMStateDescription

[John Snow]

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 <address@hidden>
>>> ---
>>>  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 <address@hidden>

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