| Message ID | 20ca5615c8cdc456296698133e3b0dbd5a1f4de7.1250788880.git.quintela@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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.
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
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;
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(-)