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

[John Snow]

On 01/11/2016 06:41 PM, Mark Cave-Ayland wrote:
> On 08/01/16 20:55, John Snow wrote:
> 
>> 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.
> 
> I'm not too worried about this since before my patchset last year then
> none of the Mac machines could be migrated since version ~0.10, and even
> then, only when there was no outstanding disk activity (e.g. just within
> OpenBIOS).
> 
> As there are also issues with the CPU interrupt status under TCG (see my
> related patchset) then the chance of getting a successful migration
> before now is amazingly small. Alex, what do you think?
> 
> 
> ATB,
> 
> Mark.
> 

We can revisit this once the solution for the CPU interrupt status is
nailed down :)

Broadly, you are right that this board has been pretty broken for a long
time, but it appears to be at least semi-functional in 2.5, so it might
be time to start maintaining some migration compatibility -- even if
it's just in the form of nicer error messages instead of stuff exploding.

--js