[Qemu-devel] [PATCH 0/4] ppc: loadvm/savevm fixups for macio/DBDMA

[Qemu-devel] [PATCH 0/4] ppc: loadvm/savevm fixups for macio/DBDMA

[Mark Cave-Ayland]

This patchset fixes up macio/DBDMA to allow migration to succeed for Mac
machines while IDE/DMA requests are in-flight, and in conjunction with the
migration fixup patchset, enables me to successfully migrate live VMs
under TCG.

Migration was tested by running through a complete Darwin install whilst
issuing savevm/loadvm command pairs every minute or so in the monitor
with no visible ill-effects, and resulted in a bootable disk image.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Mark Cave-Ayland (4):
  macio: use the existing IDEDMA aiocb to hold the active DMA aiocb
  macio: add dma_active to VMStateDescription
  mac_dbdma: add DBDMA controller state to VMStateDescription
  cuda: add missing fields to VMStateDescription

 hw/ide/macio.c            |   23 ++++++++++++++---------
 hw/misc/macio/cuda.c      |    7 +++++--
 hw/misc/macio/mac_dbdma.c |   40 ++++++++++++++++++++++++++++++++++++----
 hw/ppc/mac.h              |    1 -
 4 files changed, 55 insertions(+), 16 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/4] macio: use the existing IDEDMA aiocb to hold the active DMA aiocb

[Mark Cave-Ayland]

Currently the aiocb is held within MACIOIDEState, however the IDE core code
assumes that the current actvie DMA aiocb is held in aiocb in a few places,
e.g. ide_bus_reset() and ide_reset().

Switch over to using IDEDMA aiocb to store the aiocb for the current active
DMA request so that bus resets and restarts are handled correctly. As a
consequence we can now use ide_set_inactive() rather than handling its
functionality ourselves.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c |   20 ++++++++++++--------
 hw/ppc/mac.h   |    1 -
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 3ee962f..560c071 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -119,8 +119,8 @@ static void pmac_dma_read(BlockBackend *blk,
     MACIO_DPRINTF("--- Block read transfer - sector_num: %" PRIx64 "  "
                   "nsector: %x\n", (offset >> 9), (bytes >> 9));
 
-    m->aiocb = blk_aio_readv(blk, (offset >> 9), &io->iov, (bytes >> 9),
-                             cb, io);
+    s->bus->dma->aiocb = blk_aio_readv(blk, (offset >> 9), &io->iov,
+                             (bytes >> 9), cb, io);
 }
 
 static void pmac_dma_write(BlockBackend *blk,
@@ -204,8 +204,8 @@ static void pmac_dma_write(BlockBackend *blk,
     MACIO_DPRINTF("--- Block write transfer - sector_num: %" PRIx64 "  "
                   "nsector: %x\n", (offset >> 9), (bytes >> 9));
 
-    m->aiocb = blk_aio_writev(blk, (offset >> 9), &io->iov, (bytes >> 9),
-                              cb, io);
+    s->bus->dma->aiocb = blk_aio_writev(blk, (offset >> 9), &io->iov,
+                             (bytes >> 9), cb, io);
 }
 
 static void pmac_dma_trim(BlockBackend *blk,
@@ -231,8 +231,8 @@ static void pmac_dma_trim(BlockBackend *blk,
     s->io_buffer_index += io->len;
     io->len = 0;
 
-    m->aiocb = ide_issue_trim(blk, (offset >> 9), &io->iov, (bytes >> 9),
-                              cb, io);
+    s->bus->dma->aiocb = ide_issue_trim(blk, (offset >> 9), &io->iov,
+                             (bytes >> 9), cb, io);
 }
 
 static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
@@ -291,6 +291,8 @@ done:
     } else {
         block_acct_done(blk_get_stats(s->blk), &s->acct);
     }
+
+    ide_set_inactive(s, false);
     io->dma_end(opaque);
 
     return;
@@ -307,7 +309,6 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 
     if (ret < 0) {
         MACIO_DPRINTF("DMA error: %d\n", ret);
-        m->aiocb = NULL;
         ide_dma_error(s);
         goto done;
     }
@@ -358,6 +359,8 @@ done:
             block_acct_done(blk_get_stats(s->blk), &s->acct);
         }
     }
+
+    ide_set_inactive(s, false);
     io->dma_end(opaque);
 }
 
@@ -395,8 +398,9 @@ static void pmac_ide_transfer(DBDMA_io *io)
 static void pmac_ide_flush(DBDMA_io *io)
 {
     MACIOIDEState *m = io->opaque;
+    IDEState *s = idebus_active_if(&m->bus);
 
-    if (m->aiocb) {
+    if (s->bus->dma->aiocb) {
         blk_drain_all();
     }
 }
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index e375ed2..ecf7792 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -134,7 +134,6 @@ typedef struct MACIOIDEState {
 
     MemoryRegion mem;
     IDEBus bus;
-    BlockAIOCB *aiocb;
     IDEDMA dma;
     void *dbdma;
     bool dma_active;
-- 
1.7.10.4

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

[Mark Cave-Ayland]

Make sure that we include the value of dma_active in the migration stream.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 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()
     }
 };
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/4] mac_dbdma: add DBDMA controller state to VMStateDescription

[Mark Cave-Ayland]

Make sure that we include the DBDMA controller state in the migration
stream.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/mac_dbdma.c |   40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 5ee8f02..161f49e 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -712,20 +712,52 @@ static const MemoryRegionOps dbdma_ops = {
     },
 };
 
-static const VMStateDescription vmstate_dbdma_channel = {
-    .name = "dbdma_channel",
+static const VMStateDescription vmstate_dbdma_io = {
+    .name = "dbdma_io",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(addr, struct DBDMA_io),
+        VMSTATE_INT32(len, struct DBDMA_io),
+        VMSTATE_INT32(is_last, struct DBDMA_io),
+        VMSTATE_INT32(is_dma_out, struct DBDMA_io),
+        VMSTATE_BOOL(processing, struct DBDMA_io),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_dbdma_cmd = {
+    .name = "dbdma_cmd",
     .version_id = 0,
     .minimum_version_id = 0,
     .fields = (VMStateField[]) {
+        VMSTATE_UINT16(req_count, dbdma_cmd),
+        VMSTATE_UINT16(command, dbdma_cmd),
+        VMSTATE_UINT32(phy_addr, dbdma_cmd),
+        VMSTATE_UINT32(cmd_dep, dbdma_cmd),
+        VMSTATE_UINT16(res_count, dbdma_cmd),
+        VMSTATE_UINT16(xfer_status, dbdma_cmd),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_dbdma_channel = {
+    .name = "dbdma_channel",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(regs, struct DBDMA_channel, DBDMA_REGS),
+        VMSTATE_STRUCT(io, struct DBDMA_channel, 0, vmstate_dbdma_io, DBDMA_io),
+        VMSTATE_STRUCT(current, struct DBDMA_channel, 0, vmstate_dbdma_cmd,
+                       dbdma_cmd),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static const VMStateDescription vmstate_dbdma = {
     .name = "dbdma",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT_ARRAY(channels, DBDMAState, DBDMA_CHANNELS, 1,
                              vmstate_dbdma_channel, DBDMA_channel),
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/4] cuda: add missing fields to VMStateDescription

[Mark Cave-Ayland]

Include some fields missed from the previous VMState conversion to the
migration stream, as well as the new SR_INT delay timer.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 9db4c64..3556852 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -704,15 +704,17 @@ static const VMStateDescription vmstate_cuda_timer = {
 
 static const VMStateDescription vmstate_cuda = {
     .name = "cuda",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(a, CUDAState),
         VMSTATE_UINT8(b, CUDAState),
+        VMSTATE_UINT8(last_b, CUDAState),
         VMSTATE_UINT8(dira, CUDAState),
         VMSTATE_UINT8(dirb, CUDAState),
         VMSTATE_UINT8(sr, CUDAState),
         VMSTATE_UINT8(acr, CUDAState),
+        VMSTATE_UINT8(last_acr, CUDAState),
         VMSTATE_UINT8(pcr, CUDAState),
         VMSTATE_UINT8(ifr, CUDAState),
         VMSTATE_UINT8(ier, CUDAState),
@@ -727,6 +729,7 @@ static const VMStateDescription vmstate_cuda = {
         VMSTATE_STRUCT_ARRAY(timers, CUDAState, 2, 1,
                              vmstate_cuda_timer, CUDATimer),
         VMSTATE_TIMER_PTR(adb_poll_timer, CUDAState),
+        VMSTATE_TIMER_PTR(sr_delay_timer, CUDAState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.10.4

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

[John Snow]

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 <mark.cave-ayland@ilande.co.uk>
> ---
>  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.

--js

Re: [Qemu-devel] [PATCH 1/4] macio: use the existing IDEDMA aiocb to hold the active DMA aiocb

[John Snow]

On 01/06/2016 03:37 PM, Mark Cave-Ayland wrote:
> Currently the aiocb is held within MACIOIDEState, however the IDE core code
> assumes that the current actvie DMA aiocb is held in aiocb in a few places,
> e.g. ide_bus_reset() and ide_reset().
> 
> Switch over to using IDEDMA aiocb to store the aiocb for the current active
> DMA request so that bus resets and restarts are handled correctly. As a
> consequence we can now use ide_set_inactive() rather than handling its
> functionality ourselves.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/ide/macio.c |   20 ++++++++++++--------
>  hw/ppc/mac.h   |    1 -
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 3ee962f..560c071 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -119,8 +119,8 @@ static void pmac_dma_read(BlockBackend *blk,
>      MACIO_DPRINTF("--- Block read transfer - sector_num: %" PRIx64 "  "
>                    "nsector: %x\n", (offset >> 9), (bytes >> 9));
>  
> -    m->aiocb = blk_aio_readv(blk, (offset >> 9), &io->iov, (bytes >> 9),
> -                             cb, io);
> +    s->bus->dma->aiocb = blk_aio_readv(blk, (offset >> 9), &io->iov,
> +                             (bytes >> 9), cb, io);
>  }
>  
>  static void pmac_dma_write(BlockBackend *blk,
> @@ -204,8 +204,8 @@ static void pmac_dma_write(BlockBackend *blk,
>      MACIO_DPRINTF("--- Block write transfer - sector_num: %" PRIx64 "  "
>                    "nsector: %x\n", (offset >> 9), (bytes >> 9));
>  
> -    m->aiocb = blk_aio_writev(blk, (offset >> 9), &io->iov, (bytes >> 9),
> -                              cb, io);
> +    s->bus->dma->aiocb = blk_aio_writev(blk, (offset >> 9), &io->iov,
> +                             (bytes >> 9), cb, io);
>  }
>  
>  static void pmac_dma_trim(BlockBackend *blk,
> @@ -231,8 +231,8 @@ static void pmac_dma_trim(BlockBackend *blk,
>      s->io_buffer_index += io->len;
>      io->len = 0;
>  
> -    m->aiocb = ide_issue_trim(blk, (offset >> 9), &io->iov, (bytes >> 9),
> -                              cb, io);
> +    s->bus->dma->aiocb = ide_issue_trim(blk, (offset >> 9), &io->iov,
> +                             (bytes >> 9), cb, io);
>  }
>  
>  static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
> @@ -291,6 +291,8 @@ done:
>      } else {
>          block_acct_done(blk_get_stats(s->blk), &s->acct);
>      }
> +
> +    ide_set_inactive(s, false);
>      io->dma_end(opaque);
>  
>      return;
> @@ -307,7 +309,6 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>  
>      if (ret < 0) {
>          MACIO_DPRINTF("DMA error: %d\n", ret);
> -        m->aiocb = NULL;
>          ide_dma_error(s);
>          goto done;
>      }
> @@ -358,6 +359,8 @@ done:
>              block_acct_done(blk_get_stats(s->blk), &s->acct);
>          }
>      }
> +
> +    ide_set_inactive(s, false);
>      io->dma_end(opaque);
>  }
>  
> @@ -395,8 +398,9 @@ static void pmac_ide_transfer(DBDMA_io *io)
>  static void pmac_ide_flush(DBDMA_io *io)
>  {
>      MACIOIDEState *m = io->opaque;
> +    IDEState *s = idebus_active_if(&m->bus);
>  
> -    if (m->aiocb) {
> +    if (s->bus->dma->aiocb) {
>          blk_drain_all();
>      }
>  }
> diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
> index e375ed2..ecf7792 100644
> --- a/hw/ppc/mac.h
> +++ b/hw/ppc/mac.h
> @@ -134,7 +134,6 @@ typedef struct MACIOIDEState {
>  
>      MemoryRegion mem;
>      IDEBus bus;
> -    BlockAIOCB *aiocb;
>      IDEDMA dma;
>      void *dbdma;
>      bool dma_active;
> 

Reviewed-by: John Snow <jsnow@redhat.com>

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

[Mark Cave-Ayland]

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 <mark.cave-ayland@ilande.co.uk>
>> ---
>>  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.

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 <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  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 <jsnow@redhat.com>

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

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

[Mark Cave-Ayland]

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 <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>  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 <jsnow@redhat.com>
> 
> 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.

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 <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>>  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 <jsnow@redhat.com>
>>
>> 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

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

[Mark Cave-Ayland]

On 14/01/16 16:51, John Snow wrote:

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

Sure :)

> 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.

I think with the interrupt status/timer issues sorted soon it's worth
maintaining migration from this release onwards, but I can say without
fear having been trying to get migration to work on various PPC/TCG
guests up until now that there is a < 10% of having a working image on
loadvm and without the macio fixes a very good chance of disk corruption
too.


ATB,

Mark.