[v4,23/47] ivshmem: migrate with VMStateDescription

From: Marc-André Lureau <marcandre.lureau@redhat.com>

load_state_old() is used to keep compatibility with version 0.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/misc/ivshmem.c | 141 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 88 insertions(+), 53 deletions(-)

On 24.09.2015 13:37, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> load_state_old() is used to keep compatibility with version 0.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/misc/ivshmem.c | 141 ++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 88 insertions(+), 53 deletions(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 8207a98..54f7093 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -651,56 +651,6 @@ static int ivshmem_setup_msi(IVShmemState * s)
>      return 0;
>  }
>  
> -static void ivshmem_save(QEMUFile* f, void *opaque)
> -{
> -    IVShmemState *proxy = opaque;
> -    PCIDevice *pci_dev = PCI_DEVICE(proxy);
> -
> -    IVSHMEM_DPRINTF("ivshmem_save\n");
> -    pci_device_save(pci_dev, f);
> -
> -    if (ivshmem_has_feature(proxy, IVSHMEM_MSI)) {
> -        msix_save(pci_dev, f);
> -    } else {
> -        qemu_put_be32(f, proxy->intrstatus);
> -        qemu_put_be32(f, proxy->intrmask);
> -    }
> -
> -}
> -
> -static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
> -{
> -    IVSHMEM_DPRINTF("ivshmem_load\n");
> -
> -    IVShmemState *proxy = opaque;
> -    PCIDevice *pci_dev = PCI_DEVICE(proxy);
> -    int ret;
> -
> -    if (version_id > 0) {
> -        return -EINVAL;
> -    }
> -
> -    if (proxy->role_val == IVSHMEM_PEER) {
> -        error_report("'peer' devices are not migratable");
> -        return -EINVAL;
> -    }
> -
> -    ret = pci_device_load(pci_dev, f);
> -    if (ret) {
> -        return ret;
> -    }
> -
> -    if (ivshmem_has_feature(proxy, IVSHMEM_MSI)) {
> -        msix_load(pci_dev, f);
> -	ivshmem_use_msix(proxy);
> -    } else {
> -        proxy->intrstatus = qemu_get_be32(f);
> -        proxy->intrmask = qemu_get_be32(f);
> -    }
> -
> -    return 0;
> -}
> -
>  static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
>                                   uint32_t val, int len)
>  {
> @@ -726,8 +676,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
>      }
>  
>      fifo8_create(&s->incoming_fifo, sizeof(long));
> -    register_savevm(DEVICE(dev), "ivshmem", 0, 0, ivshmem_save, ivshmem_load,
> -                                                                        dev);
> +
>      /* IRQFD requires MSI */
>      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD) &&
>          !ivshmem_has_feature(s, IVSHMEM_MSI)) {
> @@ -853,10 +802,95 @@ static void pci_ivshmem_exit(PCIDevice *dev)
>  
>      memory_region_del_subregion(&s->bar, &s->ivshmem);
>      vmstate_unregister_ram(&s->ivshmem, DEVICE(dev));
> -    unregister_savevm(DEVICE(dev), "ivshmem", s);
>      fifo8_destroy(&s->incoming_fifo);
>  }
>  
> +static bool test_msix(void *opaque, int version_id)
> +{
> +    IVShmemState *s = opaque;
> +
> +    return ivshmem_has_feature(s, IVSHMEM_MSI);
> +}
> +
> +static bool test_no_msix(void *opaque, int version_id)
> +{
> +    return !test_msix(opaque, version_id);
> +}
> +
> +static int ivshmem_pre_load(void *opaque)
> +{
> +    IVShmemState *s = opaque;
> +
> +    if (s->role_val == IVSHMEM_PEER) {
> +        error_report("'peer' devices are not migratable");
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int ivshmem_post_load(void *opaque, int version_id)
> +{
> +    IVShmemState *s = opaque;
> +
> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> +        ivshmem_use_msix(s);
> +    }
> +
> +    return 0;
> +}
> +
> +static int ivshmem_load_old(QEMUFile *f, void *opaque, int version_id)
> +{
> +    IVShmemState *s = opaque;
> +    PCIDevice *pdev = PCI_DEVICE(s);
> +    int ret;
> +
> +    IVSHMEM_DPRINTF("ivshmem_load_old\n");
> +
> +    if (version_id != 0) {
> +        return -EINVAL;
> +    }
> +
> +    if (s->role_val == IVSHMEM_PEER) {
> +        error_report("'peer' devices are not migratable");
> +        return -EINVAL;
> +    }
> +
> +    ret = pci_device_load(pdev, f);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> +        msix_load(pdev, f);
> +        ivshmem_use_msix(s);
> +    } else {
> +        s->intrstatus = qemu_get_be32(f);
> +        s->intrmask = qemu_get_be32(f);
> +    }
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription ivshmem_vmsd = {
> +    .name = "ivshmem",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .pre_load = ivshmem_pre_load,
> +    .post_load = ivshmem_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_DEVICE(parent_obj, IVShmemState),
> +
> +        VMSTATE_MSIX_TEST(parent_obj, IVShmemState, test_msix),
> +        VMSTATE_UINT32_TEST(intrstatus, IVShmemState, test_no_msix),
> +        VMSTATE_UINT32_TEST(intrmask, IVShmemState, test_no_msix),
> +
> +        VMSTATE_END_OF_LIST()
> +    },
> +    .load_state_old = ivshmem_load_old

Should you not have a minimum_version_id_old if you want to support load_state_old?
Granted, it should be =0, so it probably works anyway..

> +};
> +
>  static Property ivshmem_properties[] = {
>      DEFINE_PROP_CHR("chardev", IVShmemState, server_chr),
>      DEFINE_PROP_STRING("size", IVShmemState, sizearg),
> @@ -882,6 +916,7 @@ static void ivshmem_class_init(ObjectClass *klass, void *data)
>      k->class_id = PCI_CLASS_MEMORY_RAM;
>      dc->reset = ivshmem_reset;
>      dc->props = ivshmem_properties;
> +    dc->vmsd = &ivshmem_vmsd;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
>  
>

----- Original Message -----
> On 24.09.2015 13:37, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > load_state_old() is used to keep compatibility with version 0.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/misc/ivshmem.c | 141
> >  ++++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 88 insertions(+), 53 deletions(-)
> > 
> > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> > index 8207a98..54f7093 100644
> > --- a/hw/misc/ivshmem.c
> > +++ b/hw/misc/ivshmem.c
> > @@ -651,56 +651,6 @@ static int ivshmem_setup_msi(IVShmemState * s)
> >      return 0;
> >  }
> >  
> > -static void ivshmem_save(QEMUFile* f, void *opaque)
> > -{
> > -    IVShmemState *proxy = opaque;
> > -    PCIDevice *pci_dev = PCI_DEVICE(proxy);
> > -
> > -    IVSHMEM_DPRINTF("ivshmem_save\n");
> > -    pci_device_save(pci_dev, f);
> > -
> > -    if (ivshmem_has_feature(proxy, IVSHMEM_MSI)) {
> > -        msix_save(pci_dev, f);
> > -    } else {
> > -        qemu_put_be32(f, proxy->intrstatus);
> > -        qemu_put_be32(f, proxy->intrmask);
> > -    }
> > -
> > -}
> > -
> > -static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
> > -{
> > -    IVSHMEM_DPRINTF("ivshmem_load\n");
> > -
> > -    IVShmemState *proxy = opaque;
> > -    PCIDevice *pci_dev = PCI_DEVICE(proxy);
> > -    int ret;
> > -
> > -    if (version_id > 0) {
> > -        return -EINVAL;
> > -    }
> > -
> > -    if (proxy->role_val == IVSHMEM_PEER) {
> > -        error_report("'peer' devices are not migratable");
> > -        return -EINVAL;
> > -    }
> > -
> > -    ret = pci_device_load(pci_dev, f);
> > -    if (ret) {
> > -        return ret;
> > -    }
> > -
> > -    if (ivshmem_has_feature(proxy, IVSHMEM_MSI)) {
> > -        msix_load(pci_dev, f);
> > -	ivshmem_use_msix(proxy);
> > -    } else {
> > -        proxy->intrstatus = qemu_get_be32(f);
> > -        proxy->intrmask = qemu_get_be32(f);
> > -    }
> > -
> > -    return 0;
> > -}
> > -
> >  static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
> >                                   uint32_t val, int len)
> >  {
> > @@ -726,8 +676,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error
> > **errp)
> >      }
> >  
> >      fifo8_create(&s->incoming_fifo, sizeof(long));
> > -    register_savevm(DEVICE(dev), "ivshmem", 0, 0, ivshmem_save,
> > ivshmem_load,
> > -
> > dev);
> > +
> >      /* IRQFD requires MSI */
> >      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD) &&
> >          !ivshmem_has_feature(s, IVSHMEM_MSI)) {
> > @@ -853,10 +802,95 @@ static void pci_ivshmem_exit(PCIDevice *dev)
> >  
> >      memory_region_del_subregion(&s->bar, &s->ivshmem);
> >      vmstate_unregister_ram(&s->ivshmem, DEVICE(dev));
> > -    unregister_savevm(DEVICE(dev), "ivshmem", s);
> >      fifo8_destroy(&s->incoming_fifo);
> >  }
> >  
> > +static bool test_msix(void *opaque, int version_id)
> > +{
> > +    IVShmemState *s = opaque;
> > +
> > +    return ivshmem_has_feature(s, IVSHMEM_MSI);
> > +}
> > +
> > +static bool test_no_msix(void *opaque, int version_id)
> > +{
> > +    return !test_msix(opaque, version_id);
> > +}
> > +
> > +static int ivshmem_pre_load(void *opaque)
> > +{
> > +    IVShmemState *s = opaque;
> > +
> > +    if (s->role_val == IVSHMEM_PEER) {
> > +        error_report("'peer' devices are not migratable");
> > +        return -EINVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int ivshmem_post_load(void *opaque, int version_id)
> > +{
> > +    IVShmemState *s = opaque;
> > +
> > +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> > +        ivshmem_use_msix(s);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int ivshmem_load_old(QEMUFile *f, void *opaque, int version_id)
> > +{
> > +    IVShmemState *s = opaque;
> > +    PCIDevice *pdev = PCI_DEVICE(s);
> > +    int ret;
> > +
> > +    IVSHMEM_DPRINTF("ivshmem_load_old\n");
> > +
> > +    if (version_id != 0) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    if (s->role_val == IVSHMEM_PEER) {
> > +        error_report("'peer' devices are not migratable");
> > +        return -EINVAL;
> > +    }
> > +
> > +    ret = pci_device_load(pdev, f);
> > +    if (ret) {
> > +        return ret;
> > +    }
> > +
> > +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> > +        msix_load(pdev, f);
> > +        ivshmem_use_msix(s);
> > +    } else {
> > +        s->intrstatus = qemu_get_be32(f);
> > +        s->intrmask = qemu_get_be32(f);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static const VMStateDescription ivshmem_vmsd = {
> > +    .name = "ivshmem",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .pre_load = ivshmem_pre_load,
> > +    .post_load = ivshmem_post_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_PCI_DEVICE(parent_obj, IVShmemState),
> > +
> > +        VMSTATE_MSIX_TEST(parent_obj, IVShmemState, test_msix),
> > +        VMSTATE_UINT32_TEST(intrstatus, IVShmemState, test_no_msix),
> > +        VMSTATE_UINT32_TEST(intrmask, IVShmemState, test_no_msix),
> > +
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +    .load_state_old = ivshmem_load_old
> 
> Should you not have a minimum_version_id_old if you want to support
> load_state_old?
> Granted, it should be =0, so it probably works anyway..

yes, I can make this more explicit though, agree.

is the rest of the patch getting your reviewed-by?

> 
> > +};
> > +
> >  static Property ivshmem_properties[] = {
> >      DEFINE_PROP_CHR("chardev", IVShmemState, server_chr),
> >      DEFINE_PROP_STRING("size", IVShmemState, sizearg),
> > @@ -882,6 +916,7 @@ static void ivshmem_class_init(ObjectClass *klass, void
> > *data)
> >      k->class_id = PCI_CLASS_MEMORY_RAM;
> >      dc->reset = ivshmem_reset;
> >      dc->props = ivshmem_properties;
> > +    dc->vmsd = &ivshmem_vmsd;
> >      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> >  }
> >  
> > 
> 
> 
> --
> Claudio Fontana
> Server Virtualization Architect
> Huawei Technologies Duesseldorf GmbH
> Riesstraße 25 - 80992 München
> 
>

On 29.09.2015 15:39, Marc-André Lureau wrote:
> 
> 
> ----- Original Message -----
>> On 24.09.2015 13:37, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> load_state_old() is used to keep compatibility with version 0.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  hw/misc/ivshmem.c | 141
>>>  ++++++++++++++++++++++++++++++++++--------------------
>>>  1 file changed, 88 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>>> index 8207a98..54f7093 100644
>>> --- a/hw/misc/ivshmem.c
>>> +++ b/hw/misc/ivshmem.c
>>> @@ -651,56 +651,6 @@ static int ivshmem_setup_msi(IVShmemState * s)
>>>      return 0;
>>>  }
>>>  
>>> -static void ivshmem_save(QEMUFile* f, void *opaque)
>>> -{
>>> -    IVShmemState *proxy = opaque;
>>> -    PCIDevice *pci_dev = PCI_DEVICE(proxy);
>>> -
>>> -    IVSHMEM_DPRINTF("ivshmem_save\n");
>>> -    pci_device_save(pci_dev, f);
>>> -
>>> -    if (ivshmem_has_feature(proxy, IVSHMEM_MSI)) {
>>> -        msix_save(pci_dev, f);
>>> -    } else {
>>> -        qemu_put_be32(f, proxy->intrstatus);
>>> -        qemu_put_be32(f, proxy->intrmask);
>>> -    }
>>> -
>>> -}
>>> -
>>> -static int ivshmem_load(QEMUFile* f, void *opaque, int version_id)
>>> -{
>>> -    IVSHMEM_DPRINTF("ivshmem_load\n");
>>> -
>>> -    IVShmemState *proxy = opaque;
>>> -    PCIDevice *pci_dev = PCI_DEVICE(proxy);
>>> -    int ret;
>>> -
>>> -    if (version_id > 0) {
>>> -        return -EINVAL;
>>> -    }
>>> -
>>> -    if (proxy->role_val == IVSHMEM_PEER) {
>>> -        error_report("'peer' devices are not migratable");
>>> -        return -EINVAL;
>>> -    }
>>> -
>>> -    ret = pci_device_load(pci_dev, f);
>>> -    if (ret) {
>>> -        return ret;
>>> -    }
>>> -
>>> -    if (ivshmem_has_feature(proxy, IVSHMEM_MSI)) {
>>> -        msix_load(pci_dev, f);
>>> -	ivshmem_use_msix(proxy);
>>> -    } else {
>>> -        proxy->intrstatus = qemu_get_be32(f);
>>> -        proxy->intrmask = qemu_get_be32(f);
>>> -    }
>>> -
>>> -    return 0;
>>> -}
>>> -
>>>  static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
>>>                                   uint32_t val, int len)
>>>  {
>>> @@ -726,8 +676,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error
>>> **errp)
>>>      }
>>>  
>>>      fifo8_create(&s->incoming_fifo, sizeof(long));
>>> -    register_savevm(DEVICE(dev), "ivshmem", 0, 0, ivshmem_save,
>>> ivshmem_load,
>>> -
>>> dev);
>>> +
>>>      /* IRQFD requires MSI */
>>>      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD) &&
>>>          !ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>> @@ -853,10 +802,95 @@ static void pci_ivshmem_exit(PCIDevice *dev)
>>>  
>>>      memory_region_del_subregion(&s->bar, &s->ivshmem);
>>>      vmstate_unregister_ram(&s->ivshmem, DEVICE(dev));
>>> -    unregister_savevm(DEVICE(dev), "ivshmem", s);
>>>      fifo8_destroy(&s->incoming_fifo);
>>>  }
>>>  
>>> +static bool test_msix(void *opaque, int version_id)
>>> +{
>>> +    IVShmemState *s = opaque;
>>> +
>>> +    return ivshmem_has_feature(s, IVSHMEM_MSI);
>>> +}
>>> +
>>> +static bool test_no_msix(void *opaque, int version_id)
>>> +{
>>> +    return !test_msix(opaque, version_id);
>>> +}
>>> +
>>> +static int ivshmem_pre_load(void *opaque)
>>> +{
>>> +    IVShmemState *s = opaque;
>>> +
>>> +    if (s->role_val == IVSHMEM_PEER) {
>>> +        error_report("'peer' devices are not migratable");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ivshmem_post_load(void *opaque, int version_id)
>>> +{
>>> +    IVShmemState *s = opaque;
>>> +
>>> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>> +        ivshmem_use_msix(s);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ivshmem_load_old(QEMUFile *f, void *opaque, int version_id)
>>> +{
>>> +    IVShmemState *s = opaque;
>>> +    PCIDevice *pdev = PCI_DEVICE(s);
>>> +    int ret;
>>> +
>>> +    IVSHMEM_DPRINTF("ivshmem_load_old\n");
>>> +
>>> +    if (version_id != 0) {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (s->role_val == IVSHMEM_PEER) {
>>> +        error_report("'peer' devices are not migratable");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    ret = pci_device_load(pdev, f);
>>> +    if (ret) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>> +        msix_load(pdev, f);
>>> +        ivshmem_use_msix(s);
>>> +    } else {
>>> +        s->intrstatus = qemu_get_be32(f);
>>> +        s->intrmask = qemu_get_be32(f);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const VMStateDescription ivshmem_vmsd = {
>>> +    .name = "ivshmem",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .pre_load = ivshmem_pre_load,
>>> +    .post_load = ivshmem_post_load,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_PCI_DEVICE(parent_obj, IVShmemState),
>>> +
>>> +        VMSTATE_MSIX_TEST(parent_obj, IVShmemState, test_msix),
>>> +        VMSTATE_UINT32_TEST(intrstatus, IVShmemState, test_no_msix),
>>> +        VMSTATE_UINT32_TEST(intrmask, IVShmemState, test_no_msix),
>>> +
>>> +        VMSTATE_END_OF_LIST()
>>> +    },
>>> +    .load_state_old = ivshmem_load_old
>>
>> Should you not have a minimum_version_id_old if you want to support
>> load_state_old?
>> Granted, it should be =0, so it probably works anyway..
> 
> yes, I can make this more explicit though, agree.
> 
> is the rest of the patch getting your reviewed-by?

I would not have even bothered probably with the load_old, but I have nothing against adding it.

Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>

> 
>>
>>> +};
>>> +
>>>  static Property ivshmem_properties[] = {
>>>      DEFINE_PROP_CHR("chardev", IVShmemState, server_chr),
>>>      DEFINE_PROP_STRING("size", IVShmemState, sizearg),
>>> @@ -882,6 +916,7 @@ static void ivshmem_class_init(ObjectClass *klass, void
>>> *data)
>>>      k->class_id = PCI_CLASS_MEMORY_RAM;
>>>      dc->reset = ivshmem_reset;
>>>      dc->props = ivshmem_properties;
>>> +    dc->vmsd = &ivshmem_vmsd;
>>>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>>  }
>>>  
>>>