Patchwork [v2,2/2] hw/arm_gic_common: Use vmstate struct rather than save/load functions

login
register
mail settings
Submitter Peter Maydell
Date March 18, 2013, 5:47 p.m.
Message ID <1363628865-29390-3-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/228756/
State New
Headers show

Comments

Peter Maydell - March 18, 2013, 5:47 p.m.
Update the GIC save/restore to use vmstate rather than hand-rolled
save/load functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm_gic_common.c |  109 ++++++++++++++++++++-------------------------------
 1 file changed, 42 insertions(+), 67 deletions(-)
Igor Mitsyanko - March 18, 2013, 7:48 p.m.
On 03/18/2013 09:47 PM, Peter Maydell wrote:
>
>> Update the GIC save/restore to use vmstate rather than hand-rolled
>> save/load functions.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/arm_gic_common.c |  109 ++++++++++++++++++++----------**
>> ---------------------
>>   1 file changed, 42 insertions(+), 67 deletions(-)
>>
>> diff --git a/hw/arm_gic_common.c b/hw/arm_gic_common.c
>> index f95bec3..0785a11 100644
>> --- a/hw/arm_gic_common.c
>> +++ b/hw/arm_gic_common.c
>> @@ -20,90 +20,66 @@
>>
>
>


> - GIC_INTERNAL),
>> +        VMSTATE_BUFFER_UNSAFE(last_**active, GICState, 0,
>> +                              GIC_MAXIRQ * NCPU * sizeof(uint16_t)),
>>
>
>
>

I'm not sure about this one, do we have any guarantees that it will always
be tightly packed? What will happen when we will try to migrate VM between
BE and LE hosts?
I successfully tested it on my PC, but I think generally, this macro is OK
to use with byte buffers, but not with an array of arbitrary type. That
said, I couldn't come up with any alternatives yet.
Peter Maydell - March 18, 2013, 8:20 p.m.
On 18 March 2013 19:48, Igor Mitsyanko <i.mitsyanko@gmail.com> wrote:
>> On 03/18/2013 09:47 PM, Peter Maydell wrote:
>>>
>>> +        VMSTATE_BUFFER_UNSAFE(last_active, GICState, 0,
>>> +                              GIC_MAXIRQ * NCPU * sizeof(uint16_t)),

> I'm not sure about this one, do we have any guarantees that it will always
> be tightly packed? What will happen when we will try to migrate VM between
> BE and LE hosts?

Ugh. I think the packing is ok but I hadn't thought about the
endianness issue.

Gerd and I were talking on IRC about 2D arrays. I think we came to
the conclusion that you could provide a new set of vmstate macros
for 2D arrays which basically work just like the existing 1D array
ones except that the typecheck is different.

(vmstate.h is getting hugely repetitive to the point that I'm
really tempted to say we should just autogenerate it. That way
you could define a fairly small set of things (arrays, base types,
safe vs unsafe, etc) and have a script generate the cross product,
rather than the current setup where there is a lot of hand written
repetition and a tendency to gaps in the coverage where nobody's
using them yet.)

-- PMM
Igor Mitsyanko - March 18, 2013, 8:43 p.m.
On Mar 19, 2013 12:21 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>
> On 18 March 2013 19:48, Igor Mitsyanko <i.mitsyanko@gmail.com> wrote:
> >> On 03/18/2013 09:47 PM, Peter Maydell wrote:
> >>>
> >>> +        VMSTATE_BUFFER_UNSAFE(last_active, GICState, 0,
> >>> +                              GIC_MAXIRQ * NCPU * sizeof(uint16_t)),
>
> > I'm not sure about this one, do we have any guarantees that it will
always
> > be tightly packed? What will happen when we will try to migrate VM
between
> > BE and LE hosts?
>
> Ugh. I think the packing is ok but I hadn't thought about the
> endianness issue.
>
> Gerd and I were talking on IRC about 2D arrays. I think we came to
> the conclusion that you could provide a new set of vmstate macros
> for 2D arrays which basically work just like the existing 1D array
> ones except that the typecheck is different.
>
> (vmstate.h is getting hugely repetitive to the point that I'm
> really tempted to say we should just autogenerate it. That way
> you could define a fairly small set of things (arrays, base types,
> safe vs unsafe, etc) and have a script generate the cross product,
> rather than the current setup where there is a lot of hand written
> repetition and a tendency to gaps in the coverage where nobody's
> using them yet.)
>
> -- PMM

I can recall a qemu-devel discussion over a long-term QOM goals a while
ago.Somebody suggested that in the future we will define devices state
structures using some special macro which will be parsed during
compilation, serializing each member for both QOM introspection and vmstate
migration.
Gerd Hoffmann - March 19, 2013, 7:14 a.m.
Hi,

>> (vmstate.h is getting hugely repetitive to the point that I'm
>> really tempted to say we should just autogenerate it. That way
>> you could define a fairly small set of things (arrays, base types,
>> safe vs unsafe, etc) and have a script generate the cross product,
>> rather than the current setup where there is a lot of hand written
>> repetition and a tendency to gaps in the coverage where nobody's
>> using them yet.)

> I can recall a qemu-devel discussion over a long-term QOM goals a while
> ago.Somebody suggested that in the future we will define devices state
> structures using some special macro which will be parsed during
> compilation, serializing each member for both QOM introspection and vmstate
> migration.

That is where I see the future too.  Michael Roth [ cc'ed ] has this on
his agenda.  We have code generation infrastructure for qapi and it
surely makes sense to reuse that for vmstate.

cheers,
  Gerd
Andreas Färber - March 19, 2013, 10:57 a.m.
Am 18.03.2013 21:43, schrieb Igor Mitsyanko:
> 
> On Mar 19, 2013 12:21 AM, "Peter Maydell" <peter.maydell@linaro.org
> <mailto:peter.maydell@linaro.org>> wrote:
>>
>> On 18 March 2013 19:48, Igor Mitsyanko <i.mitsyanko@gmail.com
> <mailto:i.mitsyanko@gmail.com>> wrote:
>> >> On 03/18/2013 09:47 PM, Peter Maydell wrote:
>> >>>
>> >>> +        VMSTATE_BUFFER_UNSAFE(last_active, GICState, 0,
>> >>> +                              GIC_MAXIRQ * NCPU * sizeof(uint16_t)),
>>
>> > I'm not sure about this one, do we have any guarantees that it will
> always
>> > be tightly packed? What will happen when we will try to migrate VM
> between
>> > BE and LE hosts?
>>
>> Ugh. I think the packing is ok but I hadn't thought about the
>> endianness issue.
>>
>> Gerd and I were talking on IRC about 2D arrays. I think we came to
>> the conclusion that you could provide a new set of vmstate macros
>> for 2D arrays which basically work just like the existing 1D array
>> ones except that the typecheck is different.
>>
>> (vmstate.h is getting hugely repetitive to the point that I'm
>> really tempted to say we should just autogenerate it. That way
>> you could define a fairly small set of things (arrays, base types,
>> safe vs unsafe, etc) and have a script generate the cross product,
>> rather than the current setup where there is a lot of hand written
>> repetition and a tendency to gaps in the coverage where nobody's
>> using them yet.)
>>
>> -- PMM
> 
> I can recall a qemu-devel discussion over a long-term QOM goals a while
> ago.Somebody suggested that in the future we will define devices state
> structures using some special macro which will be parsed during
> compilation, serializing each member for both QOM introspection and
> vmstate migration.

QIDL - CC'ing Michael.

Andreas

Patch

diff --git a/hw/arm_gic_common.c b/hw/arm_gic_common.c
index f95bec3..0785a11 100644
--- a/hw/arm_gic_common.c
+++ b/hw/arm_gic_common.c
@@ -20,90 +20,66 @@ 
 
 #include "hw/arm_gic_internal.h"
 
-static void gic_save(QEMUFile *f, void *opaque)
+static void gic_pre_save(void *opaque)
 {
     GICState *s = (GICState *)opaque;
     ARMGICCommonClass *c = ARM_GIC_COMMON_GET_CLASS(s);
-    int i;
-    int j;
 
     if (c->pre_save) {
         c->pre_save(s);
     }
-
-    qemu_put_be32(f, s->enabled);
-    for (i = 0; i < s->num_cpu; i++) {
-        qemu_put_be32(f, s->cpu_enabled[i]);
-        for (j = 0; j < GIC_INTERNAL; j++) {
-            qemu_put_be32(f, s->priority1[j][i]);
-        }
-        for (j = 0; j < s->num_irq; j++) {
-            qemu_put_be32(f, s->last_active[j][i]);
-        }
-        qemu_put_be32(f, s->priority_mask[i]);
-        qemu_put_be32(f, s->running_irq[i]);
-        qemu_put_be32(f, s->running_priority[i]);
-        qemu_put_be32(f, s->current_pending[i]);
-    }
-    for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
-        qemu_put_be32(f, s->priority2[i]);
-    }
-    for (i = 0; i < s->num_irq; i++) {
-        qemu_put_be32(f, s->irq_target[i]);
-        qemu_put_byte(f, s->irq_state[i].enabled);
-        qemu_put_byte(f, s->irq_state[i].pending);
-        qemu_put_byte(f, s->irq_state[i].active);
-        qemu_put_byte(f, s->irq_state[i].level);
-        qemu_put_byte(f, s->irq_state[i].model);
-        qemu_put_byte(f, s->irq_state[i].trigger);
-    }
 }
 
-static int gic_load(QEMUFile *f, void *opaque, int version_id)
+static int gic_post_load(void *opaque, int version_id)
 {
     GICState *s = (GICState *)opaque;
     ARMGICCommonClass *c = ARM_GIC_COMMON_GET_CLASS(s);
-    int i;
-    int j;
-
-    if (version_id != 3) {
-        return -EINVAL;
-    }
-
-    s->enabled = qemu_get_be32(f);
-    for (i = 0; i < s->num_cpu; i++) {
-        s->cpu_enabled[i] = qemu_get_be32(f);
-        for (j = 0; j < GIC_INTERNAL; j++) {
-            s->priority1[j][i] = qemu_get_be32(f);
-        }
-        for (j = 0; j < s->num_irq; j++) {
-            s->last_active[j][i] = qemu_get_be32(f);
-        }
-        s->priority_mask[i] = qemu_get_be32(f);
-        s->running_irq[i] = qemu_get_be32(f);
-        s->running_priority[i] = qemu_get_be32(f);
-        s->current_pending[i] = qemu_get_be32(f);
-    }
-    for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
-        s->priority2[i] = qemu_get_be32(f);
-    }
-    for (i = 0; i < s->num_irq; i++) {
-        s->irq_target[i] = qemu_get_be32(f);
-        s->irq_state[i].enabled = qemu_get_byte(f);
-        s->irq_state[i].pending = qemu_get_byte(f);
-        s->irq_state[i].active = qemu_get_byte(f);
-        s->irq_state[i].level = qemu_get_byte(f);
-        s->irq_state[i].model = qemu_get_byte(f);
-        s->irq_state[i].trigger = qemu_get_byte(f);
-    }
 
     if (c->post_load) {
         c->post_load(s);
     }
-
     return 0;
 }
 
+static const VMStateDescription vmstate_gic_irq_state = {
+    .name = "arm_gic_irq_state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(enabled, gic_irq_state),
+        VMSTATE_UINT8(pending, gic_irq_state),
+        VMSTATE_UINT8(active, gic_irq_state),
+        VMSTATE_UINT8(level, gic_irq_state),
+        VMSTATE_BOOL(model, gic_irq_state),
+        VMSTATE_BOOL(trigger, gic_irq_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_gic = {
+    .name = "arm_gic",
+    .version_id = 4,
+    .minimum_version_id = 4,
+    .pre_save = gic_pre_save,
+    .post_load = gic_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(enabled, GICState),
+        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, NCPU),
+        VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
+                             vmstate_gic_irq_state, gic_irq_state),
+        VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
+        VMSTATE_BUFFER_UNSAFE(priority1, GICState, 0, GIC_INTERNAL * NCPU),
+        VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
+        VMSTATE_BUFFER_UNSAFE(last_active, GICState, 0,
+                              GIC_MAXIRQ * NCPU * sizeof(uint16_t)),
+        VMSTATE_UINT16_ARRAY(priority_mask, GICState, NCPU),
+        VMSTATE_UINT16_ARRAY(running_irq, GICState, NCPU),
+        VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU),
+        VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void arm_gic_common_realize(DeviceState *dev, Error **errp)
 {
     GICState *s = ARM_GIC_COMMON(dev);
@@ -131,8 +107,6 @@  static void arm_gic_common_realize(DeviceState *dev, Error **errp)
                    num_irq);
         return;
     }
-
-    register_savevm(NULL, "arm_gic", -1, 3, gic_save, gic_load, s);
 }
 
 static void arm_gic_common_reset(DeviceState *dev)
@@ -182,6 +156,7 @@  static void arm_gic_common_class_init(ObjectClass *klass, void *data)
     dc->reset = arm_gic_common_reset;
     dc->realize = arm_gic_common_realize;
     dc->props = arm_gic_common_properties;
+    dc->vmsd = &vmstate_gic;
     dc->no_user = 1;
 }