Patchwork [21/23] Port PCIDevice state to VMState

login
register
mail settings
Submitter Juan Quintela
Date Aug. 20, 2009, 5:42 p.m.
Message ID <20ca5615c8cdc456296698133e3b0dbd5a1f4de7.1250788880.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/31770/
State Superseded
Headers show

Comments

Juan Quintela - Aug. 20, 2009, 5:42 p.m.
This uses a variant of buffer, with extra checks. Also uses the new
support for cheking that a read value is less or equal than a field.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/hw.h  |   12 +++++++++++
 hw/pci.c |   65 +++++++++++++++++++++++++++++++++++++++++---------------------
 2 files changed, 55 insertions(+), 22 deletions(-)
Gerd Hoffmann - Aug. 21, 2009, 8:52 a.m.
On 08/20/09 19:42, Juan Quintela wrote:
> This uses a variant of buffer, with extra checks. Also uses the new
> support for cheking that a read value is less or equal than a field.

Hmm, why you are hopping through all these loops?

I'd just keep the existing pci_device_{save,load} functions.  load 
anyway for backward compatibility.  save also for devices not yet 
converted to vmstate.  I think alot of the complex stuff you are 
building up here isn't needed then, unless I missed something ...

Beside that we'll have to think about how to handle versioning of 
structs.  Do we want vmstate_pci_device (and all others) have its own 
version?  Probably makes sense.  Handling this should go into the 
generic code then and not be hacked into each structure using 
VMSTATE_INT32_LE().

cheers,
   Gerd
Juan Quintela - Aug. 21, 2009, 9:01 a.m.
Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 08/20/09 19:42, Juan Quintela wrote:
>> This uses a variant of buffer, with extra checks. Also uses the new
>> support for cheking that a read value is less or equal than a field.
>
> Hmm, why you are hopping through all these loops?
>
> I'd just keep the existing pci_device_{save,load} functions.  load
> anyway for backward compatibility.  save also for devices not yet
> converted to vmstate.  I think alot of the complex stuff you are
> building up here isn't needed then, unless I missed something ...

In this specific case, I think that sending:
size + number of things

it is safe.  I agree that the INT32_EQUAL is a bad idea. But that is how
the current code is done.

> Beside that we'll have to think about how to handle versioning of
> structs.  Do we want vmstate_pci_device (and all others) have its own
> version?  Probably makes sense.  Handling this should go into the
> generic code then and not be hacked into each structure using
> VMSTATE_INT32_LE().

Proper solution here is to use subsections.
Each time that you call VMSTATE_PCI_DEVICE() it sends a subsection
there.  Then we get the versioning by free.  This is how I am thinking
it is the right way to do it.

virtio stuff: the more than I think about it, the easier way is to just
get rid of the whole mess and do something that is sensible.

Later, Juan.

> cheers,
>   Gerd
Gerd Hoffmann - Aug. 21, 2009, 9:14 a.m.
On 08/21/09 11:01, Juan Quintela wrote:
> I agree that the INT32_EQUAL is a bad idea. But that is how
> the current code is done.

Consider changing the code then ;)

>> Beside that we'll have to think about how to handle versioning of
>> structs.  Do we want vmstate_pci_device (and all others) have its own
>> version?  Probably makes sense.  Handling this should go into the
>> generic code then and not be hacked into each structure using
>> VMSTATE_INT32_LE().
>
> Proper solution here is to use subsections.
> Each time that you call VMSTATE_PCI_DEVICE() it sends a subsection
> there.  Then we get the versioning by free.  This is how I am thinking
> it is the right way to do it.

Yep, something like this.

> virtio stuff: the more than I think about it, the easier way is to just
> get rid of the whole mess and do something that is sensible.

You are talking about VirtIOPCIProxy I guess?
Yea, that is messy ...

cheers,
   Gerd
Juan Quintela - Aug. 21, 2009, 9:30 a.m.
Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 08/21/09 11:01, Juan Quintela wrote:
>> I agree that the INT32_EQUAL is a bad idea. But that is how
>> the current code is done.
>
> Consider changing the code then ;)
>
>>> Beside that we'll have to think about how to handle versioning of
>>> structs.  Do we want vmstate_pci_device (and all others) have its own
>>> version?  Probably makes sense.  Handling this should go into the
>>> generic code then and not be hacked into each structure using
>>> VMSTATE_INT32_LE().
>>
>> Proper solution here is to use subsections.
>> Each time that you call VMSTATE_PCI_DEVICE() it sends a subsection
>> there.  Then we get the versioning by free.  This is how I am thinking
>> it is the right way to do it.
>
> Yep, something like this.
>
>> virtio stuff: the more than I think about it, the easier way is to just
>> get rid of the whole mess and do something that is sensible.
>
> You are talking about VirtIOPCIProxy I guess?
> Yea, that is messy ...

I am talking about virtio_load() actually. How do you translate this to
a table?

- If at the beggining (and that load_config() reads the config from the
  savevm
- and another if in the middle of a for

int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
    int num, i, ret;

    if (vdev->binding->load_config) {
        ret = vdev->binding->load_config(vdev->binding_opaque, f);
        if (ret)
            return ret;
    }

    qemu_get_8s(f, &vdev->status);
    qemu_get_8s(f, &vdev->isr);
    qemu_get_be16s(f, &vdev->queue_sel);
    qemu_get_be32s(f, &vdev->features);
    vdev->config_len = qemu_get_be32(f);
    qemu_get_buffer(f, vdev->config, vdev->config_len);

    num = qemu_get_be32(f);

    for (i = 0; i < num; i++) {
        vdev->vq[i].vring.num = qemu_get_be32(f);
        vdev->vq[i].pa = qemu_get_be64(f);
        qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);

        if (vdev->vq[i].pa) {
            virtqueue_init(&vdev->vq[i]);
        }
        if (vdev->binding->load_queue) {
            ret = vdev->binding->load_queue(vdev->binding_opaque, i, f);
            if (ret)
                return ret;
        }
    }

    virtio_notify_vector(vdev, VIRTIO_NO_VECTOR);
    return 0;
}

Can we have something saner like:

int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
    int num, i, ret;

    qemu_get_be32s(f, &vdev->config_load_size);
    qemu_get_buffer(f, &vdev->config_load_value, &vdev->config_load_size);

    qemu_get_8s(f, &vdev->status);
    qemu_get_8s(f, &vdev->isr);
    qemu_get_be16s(f, &vdev->queue_sel);
    qemu_get_be32s(f, &vdev->features);
    vdev->config_len = qemu_get_be32(f);
    qemu_get_buffer(f, vdev->config, vdev->config_len);

    num = qemu_get_be32(f);

    for (i = 0; i < num; i++) {
        vdev->vq[i].vring.num = qemu_get_be32(f);
        vdev->vq[i].pa = qemu_get_be64(f);
        qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);

        qemu_get_be32s(f, &vdev->queue_size[i]);
        qemu_get_buffer(f, &vdev->queue_value, &vdev->queue_size[i]);
    }

    /* load of state has ended, time to start configuring things */

    if (vdev->binding->load_config) {
        ret = vdev->binding->load_config(vdev->binding_opaque, f);
        if (ret)
            return ret;
    }

    for (i = 0; i < num; i++) {
        if (vdev->vq[i].pa) {
            virtqueue_init(&vdev->vq[i]);
        if (vdev->binding->load_queue) {
            ret = vdev->binding->load_queue(vdev->binding_opaque, i, f);
            if (ret)
                return ret;
        }
        }
    }

    virtio_notify_vector(vdev, VIRTIO_NO_VECTOR);
    return 0;
}

I.e. we just reserved the space to load the stuff (0 is ok), and after
loading, We initialize things at virtio maintainers pace.  Obviously the
load_queue/config stuff has to be changed to work from a local variable
instead of the savevm stream.

Later, Juan.
Gerd Hoffmann - Aug. 21, 2009, 10:07 a.m.
On 08/21/09 11:30, Juan Quintela wrote:
> I am talking about virtio_load() actually. How do you translate this to
> a table?

Oh yea, that is nasty.

> int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> {
>      int num, i, ret;
>
>      if (vdev->binding->load_config) {
>          ret = vdev->binding->load_config(vdev->binding_opaque, f);
>          if (ret)
>              return ret;
>      }

Hmm.  Move register_savevm from virtio.c to virtio_pci.c?

Then have virtio_load_pci() which basically calls pci_device_save() + 
virtio_common_save() and kill this binding indirection.

Note: There are non-pci virtio bindings somewhere in qemu.

cheers,
   Gerd

Patch

diff --git a/hw/hw.h b/hw/hw.h
index c574b47..c77b1be 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -401,6 +401,18 @@  extern const VMStateInfo vmstate_info_buffer;
         + type_check_array(uint8_t,typeof_field(_state, _field),sizeof(typeof_field(_state,_field))) \
 }

+extern const VMStateDescription vmstate_pci_device;
+
+#define VMSTATE_PCI_DEVICE(_field, _state) {                         \
+    .name       = (stringify(_field)),                               \
+    .version_id = 2,                                                 \
+    .size       = sizeof(PCIDevice),                                 \
+    .vmsd       = &vmstate_pci_device,                               \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = offsetof(_state, _field)                           \
+            + type_check(PCIDevice,typeof_field(_state, _field))     \
+}
+
 /* _f : field name
    _f_n : num of elements field_name
    _n : num of elements
diff --git a/hw/pci.c b/hw/pci.c
index 7644dee..2266e23 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -140,39 +140,60 @@  int pci_bus_num(PCIBus *s)
     return s->bus_num;
 }

-void pci_device_save(PCIDevice *s, QEMUFile *f)
-{
-    int i;
-
-    qemu_put_be32(f, s->version_id); /* PCI device version */
-    qemu_put_buffer(f, s->config, 256);
-    for (i = 0; i < 4; i++)
-        qemu_put_be32(f, s->irq_state[i]);
-}
-
-int pci_device_load(PCIDevice *s, QEMUFile *f)
+static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
 {
-    uint8_t config[PCI_CONFIG_SPACE_SIZE];
-    uint32_t version_id;
+    PCIDevice *s = container_of(pv, PCIDevice, config);
+    uint8_t config[size];
     int i;

-    version_id = qemu_get_be32(f);
-    if (version_id > 2)
-        return -EINVAL;
-    qemu_get_buffer(f, config, sizeof config);
-    for (i = 0; i < sizeof config; ++i)
+    qemu_get_buffer(f, config, size);
+    for (i = 0; i < size; ++i)
         if ((config[i] ^ s->config[i]) & s->cmask[i] & ~s->wmask[i])
             return -EINVAL;
-    memcpy(s->config, config, sizeof config);
+    memcpy(s->config, config, size);

     pci_update_mappings(s);

-    if (version_id >= 2)
-        for (i = 0; i < 4; i ++)
-            s->irq_state[i] = qemu_get_be32(f);
     return 0;
 }

+/* just put buffer */
+static void put_pci_config_device(QEMUFile *f, const void *pv, size_t size)
+{
+    const uint8_t *v = pv;
+    qemu_put_buffer(f, v, size);
+}
+
+static VMStateInfo vmstate_info_pci_config = {
+    .name = "pci config",
+    .get  = get_pci_config_device,
+    .put  = put_pci_config_device,
+};
+
+const VMStateDescription vmstate_pci_device = {
+    .name = "PCIDevice",
+    .version_id = 2,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_INT32_LE(version_id, PCIDevice),
+        VMSTATE_SINGLE(config, PCIDevice, 0, vmstate_info_pci_config,
+                       typeof_field(PCIDevice,config)),
+        VMSTATE_INT32_ARRAY_V(irq_state, PCIDevice, 4, 2),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+void pci_device_save(PCIDevice *s, QEMUFile *f)
+{
+    vmstate_save_state(f, &vmstate_pci_device, s);
+}
+
+int pci_device_load(PCIDevice *s, QEMUFile *f)
+{
+    return vmstate_load_state(f, &vmstate_pci_device, s, s->version_id);
+}
+
 static int pci_set_default_subsystem_id(PCIDevice *pci_dev)
 {
     uint16_t *id;