[Qemu-devel] [PATCH 0/2] openpic: convert to vmstate

[Qemu-devel] [PATCH 0/2] openpic: convert to vmstate

[Mark Cave-Ayland]

This patchset follows on from my ppc loadvm/savevm work and converts
openpic over to vmstate.

With these patches applied, I can successfully a savevm/loadvm pair
under -M mac99 within OpenBIOS, the same result as with the previous
patchset applied.

Alex: if you're happy with this, I could fixup the previous patch in
my ppc loadvm/savevm patchset and append these two patches to the end
of the series?

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

Mark Cave-Ayland (2):
  openpic: switch IRQQueue queue from inline to bitmap
  openpic: convert to vmstate

 hw/intc/openpic.c |  274 ++++++++++++++++++++++++++---------------------------
 1 file changed, 133 insertions(+), 141 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/2] openpic: switch IRQQueue queue from inline to bitmap

[Mark Cave-Ayland]

This is in preparation for using VMSTATE_BITMAP in a followup vmstate
migration patch.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/intc/openpic.c |   25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 4194cef..2a3144f 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -200,11 +200,13 @@ typedef enum IRQType {
     IRQ_TYPE_FSLSPECIAL,    /* FSL timer/IPI interrupt, edge, no polarity */
 } IRQType;
 
+/* Round up to the nearest 64 IRQs so that the queue length
+ * won't change when moving between 32 and 64 bit hosts.
+ */
+#define IRQQUEUE_SIZE_BITS ((OPENPIC_MAX_IRQ + 63) & ~63)
+
 typedef struct IRQQueue {
-    /* Round up to the nearest 64 IRQs so that the queue length
-     * won't change when moving between 32 and 64 bit hosts.
-     */
-    unsigned long queue[BITS_TO_LONGS((OPENPIC_MAX_IRQ + 63) & ~63)];
+    unsigned long *queue;
     int next;
     int priority;
 } IRQQueue;
@@ -1291,7 +1293,7 @@ static void openpic_save_IRQ_queue(QEMUFile* f, IRQQueue *q)
 {
     unsigned int i;
 
-    for (i = 0; i < ARRAY_SIZE(q->queue); i++) {
+    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
         /* Always put the lower half of a 64-bit long first, in case we
          * restore on a 32-bit host.  The least significant bits correspond
          * to lower IRQ numbers in the bitmap.
@@ -1345,7 +1347,7 @@ static void openpic_load_IRQ_queue(QEMUFile* f, IRQQueue *q)
 {
     unsigned int i;
 
-    for (i = 0; i < ARRAY_SIZE(q->queue); i++) {
+    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
         unsigned long val;
 
         val = qemu_get_be32(f);
@@ -1444,12 +1446,14 @@ static void openpic_reset(DeviceState *d)
         write_IRQreg_idr(opp, i, opp->idr_reset);
     }
     /* Initialise IRQ destinations */
-    for (i = 0; i < MAX_CPU; i++) {
+    for (i = 0; i < opp->nb_cpus; i++) {
         opp->dst[i].ctpr      = 15;
-        memset(&opp->dst[i].raised, 0, sizeof(IRQQueue));
         opp->dst[i].raised.next = -1;
-        memset(&opp->dst[i].servicing, 0, sizeof(IRQQueue));
+        opp->dst[i].raised.priority = 0;
+        bitmap_clear(opp->dst[i].raised.queue, 0, IRQQUEUE_SIZE_BITS);
         opp->dst[i].servicing.next = -1;
+        opp->dst[i].servicing.priority = 0;
+        bitmap_clear(opp->dst[i].servicing.queue, 0, IRQQUEUE_SIZE_BITS);
     }
     /* Initialise timers */
     for (i = 0; i < OPENPIC_MAX_TMR; i++) {
@@ -1629,6 +1633,9 @@ static void openpic_realize(DeviceState *dev, Error **errp)
         for (j = 0; j < OPENPIC_OUTPUT_NB; j++) {
             sysbus_init_irq(d, &opp->dst[i].irqs[j]);
         }
+
+        opp->dst[i].raised.queue = bitmap_new(IRQQUEUE_SIZE_BITS);
+        opp->dst[i].servicing.queue = bitmap_new(IRQQUEUE_SIZE_BITS);
     }
 
     register_savevm(dev, "openpic", 0, 2,
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/2] openpic: convert to vmstate

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/intc/openpic.c |  253 +++++++++++++++++++++++++----------------------------
 1 file changed, 119 insertions(+), 134 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 2a3144f..1ceac97 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -207,6 +207,7 @@ typedef enum IRQType {
 
 typedef struct IRQQueue {
     unsigned long *queue;
+    int32_t queue_size; /* Only used for VMSTATE_BITMAP */
     int next;
     int priority;
 } IRQQueue;
@@ -242,6 +243,15 @@ typedef struct IRQSource {
 #define IDR_EP      0x80000000  /* external pin */
 #define IDR_CI      0x40000000  /* critical interrupt */
 
+typedef struct OpenPICTimer {
+    uint32_t tccr;  /* Global timer current count register */
+    uint32_t tbcr;  /* Global timer base count register */
+} OpenPICTimer;
+
+typedef struct OpenPICMSI {
+    uint32_t msir;   /* Shared Message Signaled Interrupt Register */
+} OpenPICMSI;
+
 typedef struct IRQDest {
     int32_t ctpr; /* CPU current task priority */
     IRQQueue raised;
@@ -290,14 +300,9 @@ typedef struct OpenPICState {
     IRQDest dst[MAX_CPU];
     uint32_t nb_cpus;
     /* Timer registers */
-    struct {
-        uint32_t tccr;  /* Global timer current count register */
-        uint32_t tbcr;  /* Global timer base count register */
-    } timers[OPENPIC_MAX_TMR];
+    OpenPICTimer timers[OPENPIC_MAX_TMR];
     /* Shared MSI registers */
-    struct {
-        uint32_t msir;   /* Shared Message Signaled Interrupt Register */
-    } msi[MAX_MSI];
+    OpenPICMSI msi[MAX_MSI];
     uint32_t max_irq;
     uint32_t irq_ipi0;
     uint32_t irq_tim0;
@@ -1289,130 +1294,6 @@ static const MemoryRegionOps openpic_summary_ops_be = {
     },
 };
 
-static void openpic_save_IRQ_queue(QEMUFile* f, IRQQueue *q)
-{
-    unsigned int i;
-
-    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
-        /* Always put the lower half of a 64-bit long first, in case we
-         * restore on a 32-bit host.  The least significant bits correspond
-         * to lower IRQ numbers in the bitmap.
-         */
-        qemu_put_be32(f, (uint32_t)q->queue[i]);
-#if LONG_MAX > 0x7FFFFFFF
-        qemu_put_be32(f, (uint32_t)(q->queue[i] >> 32));
-#endif
-    }
-
-    qemu_put_sbe32s(f, &q->next);
-    qemu_put_sbe32s(f, &q->priority);
-}
-
-static void openpic_save(QEMUFile* f, void *opaque)
-{
-    OpenPICState *opp = (OpenPICState *)opaque;
-    unsigned int i;
-
-    qemu_put_be32s(f, &opp->gcr);
-    qemu_put_be32s(f, &opp->vir);
-    qemu_put_be32s(f, &opp->pir);
-    qemu_put_be32s(f, &opp->spve);
-    qemu_put_be32s(f, &opp->tfrr);
-
-    qemu_put_be32s(f, &opp->nb_cpus);
-
-    for (i = 0; i < opp->nb_cpus; i++) {
-        qemu_put_sbe32s(f, &opp->dst[i].ctpr);
-        openpic_save_IRQ_queue(f, &opp->dst[i].raised);
-        openpic_save_IRQ_queue(f, &opp->dst[i].servicing);
-        qemu_put_buffer(f, (uint8_t *)&opp->dst[i].outputs_active,
-                        sizeof(opp->dst[i].outputs_active));
-    }
-
-    for (i = 0; i < OPENPIC_MAX_TMR; i++) {
-        qemu_put_be32s(f, &opp->timers[i].tccr);
-        qemu_put_be32s(f, &opp->timers[i].tbcr);
-    }
-
-    for (i = 0; i < opp->max_irq; i++) {
-        qemu_put_be32s(f, &opp->src[i].ivpr);
-        qemu_put_be32s(f, &opp->src[i].idr);
-        qemu_put_be32s(f, &opp->src[i].destmask);
-        qemu_put_sbe32s(f, &opp->src[i].last_cpu);
-        qemu_put_sbe32s(f, &opp->src[i].pending);
-    }
-}
-
-static void openpic_load_IRQ_queue(QEMUFile* f, IRQQueue *q)
-{
-    unsigned int i;
-
-    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
-        unsigned long val;
-
-        val = qemu_get_be32(f);
-#if LONG_MAX > 0x7FFFFFFF
-        val <<= 32;
-        val |= qemu_get_be32(f);
-#endif
-
-        q->queue[i] = val;
-    }
-
-    qemu_get_sbe32s(f, &q->next);
-    qemu_get_sbe32s(f, &q->priority);
-}
-
-static int openpic_load(QEMUFile* f, void *opaque, int version_id)
-{
-    OpenPICState *opp = (OpenPICState *)opaque;
-    unsigned int i, nb_cpus;
-
-    if (version_id != 2) {
-        return -EINVAL;
-    }
-
-    qemu_get_be32s(f, &opp->gcr);
-    qemu_get_be32s(f, &opp->vir);
-    qemu_get_be32s(f, &opp->pir);
-    qemu_get_be32s(f, &opp->spve);
-    qemu_get_be32s(f, &opp->tfrr);
-
-    qemu_get_be32s(f, &nb_cpus);
-    if (opp->nb_cpus != nb_cpus) {
-        return -EINVAL;
-    }
-    assert(nb_cpus > 0 && nb_cpus <= MAX_CPU);
-
-    for (i = 0; i < opp->nb_cpus; i++) {
-        qemu_get_sbe32s(f, &opp->dst[i].ctpr);
-        openpic_load_IRQ_queue(f, &opp->dst[i].raised);
-        openpic_load_IRQ_queue(f, &opp->dst[i].servicing);
-        qemu_get_buffer(f, (uint8_t *)&opp->dst[i].outputs_active,
-                        sizeof(opp->dst[i].outputs_active));
-    }
-
-    for (i = 0; i < OPENPIC_MAX_TMR; i++) {
-        qemu_get_be32s(f, &opp->timers[i].tccr);
-        qemu_get_be32s(f, &opp->timers[i].tbcr);
-    }
-
-    for (i = 0; i < opp->max_irq; i++) {
-        uint32_t val;
-
-        val = qemu_get_be32(f);
-        write_IRQreg_ivpr(opp, i, val);
-        val = qemu_get_be32(f);
-        write_IRQreg_idr(opp, i, val);
-
-        qemu_get_be32s(f, &opp->src[i].destmask);
-        qemu_get_sbe32s(f, &opp->src[i].last_cpu);
-        qemu_get_sbe32s(f, &opp->src[i].pending);
-    }
-
-    return 0;
-}
-
 static void openpic_reset(DeviceState *d)
 {
     OpenPICState *opp = OPENPIC(d);
@@ -1527,6 +1408,110 @@ static void map_list(OpenPICState *opp, const MemReg *list, int *count)
     }
 }
 
+static const VMStateDescription vmstate_openpic_irq_queue = {
+    .name = "openpic_irq_queue",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_BITMAP(queue, IRQQueue, 0, queue_size),
+        VMSTATE_INT32(next, IRQQueue),
+        VMSTATE_INT32(priority, IRQQueue),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_openpic_irqdest = {
+    .name = "openpic_irqdest",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(ctpr, IRQDest),
+        VMSTATE_STRUCT(raised, IRQDest, 0, vmstate_openpic_irq_queue,
+                       IRQQueue),
+        VMSTATE_STRUCT(servicing, IRQDest, 0, vmstate_openpic_irq_queue,
+                       IRQQueue),
+        VMSTATE_UINT32_ARRAY(outputs_active, IRQDest, OPENPIC_OUTPUT_NB),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static int openpic_post_load(void *opaque, int version_id)
+{
+    OpenPICState *opp = (OpenPICState *)opaque;
+    int i;
+
+    /* Update internal ivpr and idr variables */
+    for (i = 0; i < opp->max_irq; i++) {
+        write_IRQreg_idr(opp, i, opp->src[i].idr);
+        write_IRQreg_ivpr(opp, i, opp->src[i].ivpr);
+    }
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_openpic_irqsource = {
+    .name = "openpic_irqsource",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(ivpr, IRQSource),
+        VMSTATE_UINT32(idr, IRQSource),
+        VMSTATE_UINT32(destmask, IRQSource),
+        VMSTATE_INT32(last_cpu, IRQSource),
+        VMSTATE_INT32(pending, IRQSource),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_openpic_timer = {
+    .name = "openpic_timer",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(tccr, OpenPICTimer),
+        VMSTATE_UINT32(tbcr, OpenPICTimer),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_openpic_msi = {
+    .name = "openpic_msi",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(msir, OpenPICMSI),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_openpic = {
+    .name = "openpic",
+    .version_id = 3,
+    .minimum_version_id = 3,
+    .post_load = openpic_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(gcr, OpenPICState),
+        VMSTATE_UINT32(vir, OpenPICState),
+        VMSTATE_UINT32(pir, OpenPICState),
+        VMSTATE_UINT32(spve, OpenPICState),
+        VMSTATE_UINT32(tfrr, OpenPICState),
+        VMSTATE_UINT32(max_irq, OpenPICState),
+        VMSTATE_STRUCT_VARRAY_UINT32(src, OpenPICState, max_irq, 0,
+                                     vmstate_openpic_irqsource, IRQSource),
+        VMSTATE_UINT32(nb_cpus, OpenPICState),
+        VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0,
+                                     vmstate_openpic_irqdest, IRQDest),
+        VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0,
+                             vmstate_openpic_timer, OpenPICTimer),
+        VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0,
+                             vmstate_openpic_msi, OpenPICMSI),
+        VMSTATE_UINT32(irq_ipi0, OpenPICState),
+        VMSTATE_UINT32(irq_tim0, OpenPICState),
+        VMSTATE_UINT32(irq_msi, OpenPICState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void openpic_init(Object *obj)
 {
     OpenPICState *opp = OPENPIC(obj);
@@ -1634,13 +1619,12 @@ static void openpic_realize(DeviceState *dev, Error **errp)
             sysbus_init_irq(d, &opp->dst[i].irqs[j]);
         }
 
+        opp->dst[i].raised.queue_size = IRQQUEUE_SIZE_BITS;
         opp->dst[i].raised.queue = bitmap_new(IRQQUEUE_SIZE_BITS);
+        opp->dst[i].servicing.queue_size = IRQQUEUE_SIZE_BITS;
         opp->dst[i].servicing.queue = bitmap_new(IRQQUEUE_SIZE_BITS);
     }
 
-    register_savevm(dev, "openpic", 0, 2,
-                    openpic_save, openpic_load, opp);
-
     sysbus_init_mmio(d, &opp->mem);
     qdev_init_gpio_in(dev, openpic_set_irq, opp->max_irq);
 }
@@ -1658,6 +1642,7 @@ static void openpic_class_init(ObjectClass *oc, void *data)
     dc->realize = openpic_realize;
     dc->props = openpic_properties;
     dc->reset = openpic_reset;
+    dc->vmsd = &vmstate_openpic;
 }
 
 static const TypeInfo openpic_info = {
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/2] openpic: convert to vmstate

[Juan Quintela]

Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/intc/openpic.c |  253 +++++++++++++++++++++++++----------------------------
>  1 file changed, 119 insertions(+), 134 deletions(-)
>

> +static const VMStateDescription vmstate_openpic_irq_queue = {
> +    .name = "openpic_irq_queue",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BITMAP(queue, IRQQueue, 0, queue_size),

Can I assume that this layout is compatible with the one given by:

-    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
-        /* Always put the lower half of a 64-bit long first, in case we
-         * restore on a 32-bit host.  The least significant bits correspond
-         * to lower IRQ numbers in the bitmap.
-         */
-        qemu_put_be32(f, (uint32_t)q->queue[i]);
-#if LONG_MAX > 0x7FFFFFFF
-        qemu_put_be32(f, (uint32_t)(q->queue[i] >> 32));
-#endif
-    }


> +        VMSTATE_INT32(next, IRQQueue),
> +        VMSTATE_INT32(priority, IRQQueue),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_openpic_irqdest = {
> +    .name = "openpic_irqdest",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(ctpr, IRQDest),
> +        VMSTATE_STRUCT(raised, IRQDest, 0, vmstate_openpic_irq_queue,
> +                       IRQQueue),
> +        VMSTATE_STRUCT(servicing, IRQDest, 0, vmstate_openpic_irq_queue,
> +                       IRQQueue),
> +        VMSTATE_UINT32_ARRAY(outputs_active, IRQDest, OPENPIC_OUTPUT_NB),

This change would make big/little endian migration unhappy. (no, I don't
know if it is more correct the new or the old code, just that they give
different results).

> +        VMSTATE_END_OF_LIST()
> +    }
> +};

> +static const VMStateDescription vmstate_openpic = {
> +    .name = "openpic",
       .version_id = 2,
       .minimum_version_id = 2
> +    .post_load = openpic_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(gcr, OpenPICState),
> +        VMSTATE_UINT32(vir, OpenPICState),
> +        VMSTATE_UINT32(pir, OpenPICState),
> +        VMSTATE_UINT32(spve, OpenPICState),
> +        VMSTATE_UINT32(tfrr, OpenPICState),
> +        VMSTATE_UINT32(max_irq, OpenPICState),

           VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState),

           VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0,
                                        vmstate_openpic_irqdest, IRQDest),

           VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0,
                                vmstate_openpic_timer, OpenPICTimer),

           VMSTATE_STRUCT_VARRAY_UINT32(src, OpenPICState, max_irq, 0,
                                        vmstate_openpic_irqsource, IRQSource),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

If you do this changes, this should get the v2 format working, right?
Notice the two questions that I asked above, if that is true, we can go
from here making the change compatible.

If so, we could go from here about ading the following ones with a
subsection or so.


> +        VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0,
> +                             vmstate_openpic_msi, OpenPICMSI),
> +        VMSTATE_UINT32(irq_ipi0, OpenPICState),
> +        VMSTATE_UINT32(irq_tim0, OpenPICState),
> +        VMSTATE_UINT32(irq_msi, OpenPICState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

If this is only used when MSI is used, and there is a check for that, we
could use a new subsection.  If it always used, we should change format
version to three.

Anyways, spliting this patch in two would make clear what is the
equivalent of the old code and what is new.

What do you think?

Later, Juan.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] openpic: convert to vmstate

[Alexander Graf]

On 09.02.15 13:39, Juan Quintela wrote:
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/intc/openpic.c |  253 +++++++++++++++++++++++++----------------------------
>>  1 file changed, 119 insertions(+), 134 deletions(-)
>>
> 
>> +static const VMStateDescription vmstate_openpic_irq_queue = {
>> +    .name = "openpic_irq_queue",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_BITMAP(queue, IRQQueue, 0, queue_size),
> 
> Can I assume that this layout is compatible with the one given by:
> 
> -    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
> -        /* Always put the lower half of a 64-bit long first, in case we
> -         * restore on a 32-bit host.  The least significant bits correspond
> -         * to lower IRQ numbers in the bitmap.
> -         */
> -        qemu_put_be32(f, (uint32_t)q->queue[i]);
> -#if LONG_MAX > 0x7FFFFFFF
> -        qemu_put_be32(f, (uint32_t)(q->queue[i] >> 32));
> -#endif
> -    }
> 
> 
>> +        VMSTATE_INT32(next, IRQQueue),
>> +        VMSTATE_INT32(priority, IRQQueue),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static const VMStateDescription vmstate_openpic_irqdest = {
>> +    .name = "openpic_irqdest",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_INT32(ctpr, IRQDest),
>> +        VMSTATE_STRUCT(raised, IRQDest, 0, vmstate_openpic_irq_queue,
>> +                       IRQQueue),
>> +        VMSTATE_STRUCT(servicing, IRQDest, 0, vmstate_openpic_irq_queue,
>> +                       IRQQueue),
>> +        VMSTATE_UINT32_ARRAY(outputs_active, IRQDest, OPENPIC_OUTPUT_NB),
> 
> This change would make big/little endian migration unhappy. (no, I don't
> know if it is more correct the new or the old code, just that they give
> different results).
> 
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
> 
>> +static const VMStateDescription vmstate_openpic = {
>> +    .name = "openpic",
>        .version_id = 2,
>        .minimum_version_id = 2
>> +    .post_load = openpic_post_load,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(gcr, OpenPICState),
>> +        VMSTATE_UINT32(vir, OpenPICState),
>> +        VMSTATE_UINT32(pir, OpenPICState),
>> +        VMSTATE_UINT32(spve, OpenPICState),
>> +        VMSTATE_UINT32(tfrr, OpenPICState),
>> +        VMSTATE_UINT32(max_irq, OpenPICState),
> 
>            VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState),
> 
>            VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0,
>                                         vmstate_openpic_irqdest, IRQDest),
> 
>            VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0,
>                                 vmstate_openpic_timer, OpenPICTimer),
> 
>            VMSTATE_STRUCT_VARRAY_UINT32(src, OpenPICState, max_irq, 0,
>                                         vmstate_openpic_irqsource, IRQSource),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
> 
> If you do this changes, this should get the v2 format working, right?
> Notice the two questions that I asked above, if that is true, we can go
> from here making the change compatible.
> 
> If so, we could go from here about ading the following ones with a
> subsection or so.
> 
> 
>> +        VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0,
>> +                             vmstate_openpic_msi, OpenPICMSI),
>> +        VMSTATE_UINT32(irq_ipi0, OpenPICState),
>> +        VMSTATE_UINT32(irq_tim0, OpenPICState),
>> +        VMSTATE_UINT32(irq_msi, OpenPICState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
> 
> If this is only used when MSI is used, and there is a check for that, we
> could use a new subsection.  If it always used, we should change format
> version to three.

Yes, please, just bump the version number and ignore all the legacy
cruft. OpenPIC state saving is broken for years, any old state that we
could potentially save is not usable anyway :)


Alex

> 
> Anyways, spliting this patch in two would make clear what is the
> equivalent of the old code and what is new.
> 
> What do you think?
> 
> Later, Juan.
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] openpic: convert to vmstate

[Mark Cave-Ayland]

On 09/02/15 12:43, Alexander Graf wrote:

Hi Juan,

> On 09.02.15 13:39, Juan Quintela wrote:
>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  hw/intc/openpic.c |  253 +++++++++++++++++++++++++----------------------------
>>>  1 file changed, 119 insertions(+), 134 deletions(-)
>>>
>>
>>> +static const VMStateDescription vmstate_openpic_irq_queue = {
>>> +    .name = "openpic_irq_queue",
>>> +    .version_id = 0,
>>> +    .minimum_version_id = 0,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_BITMAP(queue, IRQQueue, 0, queue_size),
>>
>> Can I assume that this layout is compatible with the one given by:
>>
>> -    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
>> -        /* Always put the lower half of a 64-bit long first, in case we
>> -         * restore on a 32-bit host.  The least significant bits correspond
>> -         * to lower IRQ numbers in the bitmap.
>> -         */
>> -        qemu_put_be32(f, (uint32_t)q->queue[i]);
>> -#if LONG_MAX > 0x7FFFFFFF
>> -        qemu_put_be32(f, (uint32_t)(q->queue[i] >> 32));
>> -#endif
>> -    }

It won't be over-the-wire migration compatible, however the important
part is that by using VMSTATE_BITMAP (which internally uses longs) then
it should preserve the ability to correctly migrate between a 32-bit and
a 64-bit host as mentioned in the comments.

One minor nit is that VMSTATE_BITMAP uses an in32_t field to specify the
size of the bitmap for each IRQQueue, which in this case is always fixed
because the size of the queue is related to the number of interrupt
pins. As this is likely an edge case, I've just added queue_size to the
IRQQueue struct for the moment even though it is otherwise redundant.

>>> +        VMSTATE_INT32(next, IRQQueue),
>>> +        VMSTATE_INT32(priority, IRQQueue),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>> +static const VMStateDescription vmstate_openpic_irqdest = {
>>> +    .name = "openpic_irqdest",
>>> +    .version_id = 0,
>>> +    .minimum_version_id = 0,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_INT32(ctpr, IRQDest),
>>> +        VMSTATE_STRUCT(raised, IRQDest, 0, vmstate_openpic_irq_queue,
>>> +                       IRQQueue),
>>> +        VMSTATE_STRUCT(servicing, IRQDest, 0, vmstate_openpic_irq_queue,
>>> +                       IRQQueue),
>>> +        VMSTATE_UINT32_ARRAY(outputs_active, IRQDest, OPENPIC_OUTPUT_NB),
>>
>> This change would make big/little endian migration unhappy. (no, I don't
>> know if it is more correct the new or the old code, just that they give
>> different results).

According to the comment in the struct definition, outputs_active is
described as "Count of IRQ sources asserting on non-INT outputs" so this
should be fine transferring between endians moving forwards. I'm not
exactly sure why the old code used the get_buffer/put_buffer routines
for this in the first place, but I'm quite new to this particular
section of code.

>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>
>>> +static const VMStateDescription vmstate_openpic = {
>>> +    .name = "openpic",
>>        .version_id = 2,
>>        .minimum_version_id = 2
>>> +    .post_load = openpic_post_load,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_UINT32(gcr, OpenPICState),
>>> +        VMSTATE_UINT32(vir, OpenPICState),
>>> +        VMSTATE_UINT32(pir, OpenPICState),
>>> +        VMSTATE_UINT32(spve, OpenPICState),
>>> +        VMSTATE_UINT32(tfrr, OpenPICState),
>>> +        VMSTATE_UINT32(max_irq, OpenPICState),
>>
>>            VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState),
>>
>>            VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0,
>>                                         vmstate_openpic_irqdest, IRQDest),
>>
>>            VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0,
>>                                 vmstate_openpic_timer, OpenPICTimer),
>>
>>            VMSTATE_STRUCT_VARRAY_UINT32(src, OpenPICState, max_irq, 0,
>>                                         vmstate_openpic_irqsource, IRQSource),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>
>> If you do this changes, this should get the v2 format working, right?
>> Notice the two questions that I asked above, if that is true, we can go
>> from here making the change compatible.
>>
>> If so, we could go from here about ading the following ones with a
>> subsection or so.
>>
>>
>>> +        VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0,
>>> +                             vmstate_openpic_msi, OpenPICMSI),
>>> +        VMSTATE_UINT32(irq_ipi0, OpenPICState),
>>> +        VMSTATE_UINT32(irq_tim0, OpenPICState),
>>> +        VMSTATE_UINT32(irq_msi, OpenPICState),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>
>> If this is only used when MSI is used, and there is a check for that, we
>> could use a new subsection.  If it always used, we should change format
>> version to three.
> 
> Yes, please, just bump the version number and ignore all the legacy
> cruft. OpenPIC state saving is broken for years, any old state that we
> could potentially save is not usable anyway :)

Ha! I did actually do this but I accidentally messed up a rebase just
before sending out the patchset so it got lost. But yes, I agree a
version bump to 3 should be sufficient here.

Just to put this in context: before the PPC loadvm/savevm patchset was
posted to the list, I was bisecting down to around the 0.10-0.11
timeframe to figure out where things broke, and even then the Mac
machines wouldn't run correctly after loadvm. So I agree with Alex that
any snapshots this old would be useless anyway.

Based upon this, are there any other changes you would like before a respin?


ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] openpic: convert to vmstate

[Juan Quintela]

Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> On 09/02/15 12:43, Alexander Graf wrote:

> Ha! I did actually do this but I accidentally messed up a rebase just
> before sending out the patchset so it got lost. But yes, I agree a
> version bump to 3 should be sufficient here.
>
> Just to put this in context: before the PPC loadvm/savevm patchset was
> posted to the list, I was bisecting down to around the 0.10-0.11
> timeframe to figure out where things broke, and even then the Mac
> machines wouldn't run correctly after loadvm. So I agree with Alex that
> any snapshots this old would be useless anyway.
>
> Based upon this, are there any other changes you would like before a respin?

I didn't knew that it was completely broken (TM).

As said in a different email, I think that using the _EQUAL() macro for
nb_cpus is a good idea.  For the rest, I agree with it.

Later, Juan.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] openpic: convert to vmstate

[Mark Cave-Ayland]

On 09/02/15 13:50, Juan Quintela wrote:

> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>> On 09/02/15 12:43, Alexander Graf wrote:
> 
>> Ha! I did actually do this but I accidentally messed up a rebase just
>> before sending out the patchset so it got lost. But yes, I agree a
>> version bump to 3 should be sufficient here.
>>
>> Just to put this in context: before the PPC loadvm/savevm patchset was
>> posted to the list, I was bisecting down to around the 0.10-0.11
>> timeframe to figure out where things broke, and even then the Mac
>> machines wouldn't run correctly after loadvm. So I agree with Alex that
>> any snapshots this old would be useless anyway.
>>
>> Based upon this, are there any other changes you would like before a respin?
> 
> I didn't knew that it was completely broken (TM).
> 
> As said in a different email, I think that using the _EQUAL() macro for
> nb_cpus is a good idea.  For the rest, I agree with it.

Great, thanks! I'll make the changes, add your Reviewed-by and then
re-spin out the complete patchset for Alex.


ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] openpic: convert to vmstate

[Juan Quintela]

Alexander Graf <agraf@suse.de> wrote:


> Yes, please, just bump the version number and ignore all the legacy
> cruft. OpenPIC state saving is broken for years, any old state that we
> could potentially save is not usable anyway :)

I didn't knew that bit O:-)

Then you get a;

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] openpic: convert to vmstate

[Juan Quintela]

Alexander Graf <agraf@suse.de> wrote:
> On 09.02.15 13:39, Juan Quintela wrote:
>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  hw/intc/openpic.c |  253 +++++++++++++++++++++++++----------------------------
>>            VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState),

This change is a good idea anyways.

Later, Juan.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] openpic: convert to vmstate

[Alexander Graf]

On 06.02.15 15:36, Mark Cave-Ayland wrote:
> This patchset follows on from my ppc loadvm/savevm work and converts
> openpic over to vmstate.
> 
> With these patches applied, I can successfully a savevm/loadvm pair
> under -M mac99 within OpenBIOS, the same result as with the previous
> patchset applied.
> 
> Alex: if you're happy with this, I could fixup the previous patch in
> my ppc loadvm/savevm patchset and append these two patches to the end
> of the series?

Sounds perfect!

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] openpic: convert to vmstate

[Mark Cave-Ayland]

On 06/02/15 14:51, Alexander Graf wrote:

> On 06.02.15 15:36, Mark Cave-Ayland wrote:
>> This patchset follows on from my ppc loadvm/savevm work and converts
>> openpic over to vmstate.
>>
>> With these patches applied, I can successfully a savevm/loadvm pair
>> under -M mac99 within OpenBIOS, the same result as with the previous
>> patchset applied.
>>
>> Alex: if you're happy with this, I could fixup the previous patch in
>> my ppc loadvm/savevm patchset and append these two patches to the end
>> of the series?
> 
> Sounds perfect!
> 
> Reviewed-by: Alexander Graf <agraf@suse.de>

Great! Assuming Juan and Amit have no further comments, I'll repost the
augmented series in a couple of days.


ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/2] openpic: convert to vmstate

[Amit Shah]

On (Fri) 06 Feb 2015 [15:41:38], Mark Cave-Ayland wrote:
> On 06/02/15 14:51, Alexander Graf wrote:
> 
> > On 06.02.15 15:36, Mark Cave-Ayland wrote:
> >> This patchset follows on from my ppc loadvm/savevm work and converts
> >> openpic over to vmstate.
> >>
> >> With these patches applied, I can successfully a savevm/loadvm pair
> >> under -M mac99 within OpenBIOS, the same result as with the previous
> >> patchset applied.
> >>
> >> Alex: if you're happy with this, I could fixup the previous patch in
> >> my ppc loadvm/savevm patchset and append these two patches to the end
> >> of the series?
> > 
> > Sounds perfect!
> > 
> > Reviewed-by: Alexander Graf <agraf@suse.de>
> 
> Great! Assuming Juan and Amit have no further comments, I'll repost the
> augmented series in a couple of days.

My only comment was that the version number should be bumped else
we're not compatible; but that was already covered.



		Amit