Patchwork [33/41] virtio-net: port to vmstate

login
register
mail settings
Submitter Juan Quintela
Date Dec. 2, 2009, 12:04 p.m.
Message ID <bcca3343e5ec8fe6c2d011c87d496426f9ec5a5f.1259754427.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/40039/
State New
Headers show

Comments

Juan Quintela - Dec. 2, 2009, 12:04 p.m.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
 1 files changed, 64 insertions(+), 84 deletions(-)
Michael S. Tsirkin - Dec. 2, 2009, 2:58 p.m.
On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
>  1 files changed, 64 insertions(+), 84 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 4434827..3a59449 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
>      virtio_net_flush_tx(n, n->tx_vq);
>  }
> 
> +/* Restore an uint8_t from an uint32_t
> +   This is a Big hack, but it is how the old state did it.
> + */
> +
> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
> +{
> +    uint8_t *v = pv;
> +    *v = qemu_get_be32(f);
> +    return 0;
> +}
> +
> +static void put_unused(QEMUFile *f, void *pv, size_t size)
> +{
> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");

line too long

> +    fprintf(stderr, "Never should be used to write a new state.\n");
> +    exit(0);

I don't understand.  what is this dong? when
is this called? Please supply a comment.
Maybe call assert?

> +}
> +
> +static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
> +    .name = "uint8_from_uint32",
> +    .get  = get_uint8_from_uint32,
> +    .put  = put_unused,
> +};
> +
> +#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)
> +
> +static bool between_4_and_8(void *opaque, int version_id)
> +{
> +    return version_id >= 4 && version_id < 8;
> +}
> +
>  static int virtio_net_post_load(void *opaque, int version_id)
>  {
>      VirtIONet *n = opaque;
> @@ -748,88 +780,37 @@ static int virtio_net_post_load(void *opaque, int version_id)
>      return 0;
>  }
> 
> -static void virtio_net_save(QEMUFile *f, void *opaque)
> -{
> -    VirtIONet *n = opaque;
> -
> -    virtio_save(&n->vdev, f);
> -
> -    qemu_put_buffer(f, n->mac, ETH_ALEN);
> -    qemu_put_be32s(f, &n->tx_timer_active);
> -    qemu_put_be32s(f, &n->mergeable_rx_bufs);
> -    qemu_put_be16s(f, &n->status);
> -    qemu_put_8s(f, &n->promisc);
> -    qemu_put_8s(f, &n->allmulti);
> -    qemu_put_be32s(f, &n->mac_table.in_use);
> -    qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
> -    qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
> -    qemu_put_be32s(f, &n->has_vnet_hdr);
> -    qemu_put_8s(f, &n->mac_table.multi_overflow);
> -    qemu_put_8s(f, &n->mac_table.uni_overflow);
> -    qemu_put_8s(f, &n->alluni);
> -    qemu_put_8s(f, &n->nomulti);
> -    qemu_put_8s(f, &n->nouni);
> -    qemu_put_8s(f, &n->nobcast);
> -    qemu_put_8s(f, &n->has_ufo);
> -}
> -
> -static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
> -{
> -    VirtIONet *n = opaque;
> -
> -    if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
> -        return -EINVAL;
> -
> -    virtio_load(&n->vdev, f);
> -
> -    qemu_get_buffer(f, n->mac, ETH_ALEN);
> -    qemu_get_be32s(f, &n->tx_timer_active);
> -    qemu_get_be32s(f, &n->mergeable_rx_bufs);
> -
> -    if (version_id >= 3)
> -        qemu_get_be16s(f, &n->status);
> -
> -    if (version_id >= 4) {
> -        if (version_id < 8) {
> -            n->promisc = qemu_get_be32(f);
> -            n->allmulti = qemu_get_be32(f);
> -        } else {
> -            qemu_get_8s(f, &n->promisc);
> -            qemu_get_8s(f, &n->allmulti);
> -        }
> -    }
> -
> -    if (version_id >= 5) {
> -        qemu_get_be32s(f, &n->mac_table.in_use);
> -        qemu_get_buffer(f, n->mac_table.macs,
> -                        n->mac_table.in_use * ETH_ALEN);
> -    }
> - 
> -    if (version_id >= 6)
> -        qemu_get_buffer(f, n->vlans, MAX_VLAN >> 3);
> -
> -    if (version_id >= 7) {
> -        qemu_get_be32s(f, &n->has_vnet_hdr);
> -    }
> -
> -    if (version_id >= 9) {
> -        qemu_get_8s(f, &n->mac_table.multi_overflow);
> -        qemu_get_8s(f, &n->mac_table.uni_overflow);
> -    }
> -
> -    if (version_id >= 10) {
> -        qemu_get_8s(f, &n->alluni);
> -        qemu_get_8s(f, &n->nomulti);
> -        qemu_get_8s(f, &n->nouni);
> -        qemu_get_8s(f, &n->nobcast);
> +static const VMStateDescription vmstate_virtio_net = {
> +    .name = "virtio-net",
> +    .version_id = 11,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 2,
> +    .post_load = virtio_net_post_load,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_VIRTIO(vdev, VirtIONet),
> +        VMSTATE_BUFFER(mac, VirtIONet),
> +        VMSTATE_UINT32(tx_timer_active, VirtIONet),
> +        VMSTATE_UINT32(mergeable_rx_bufs, VirtIONet),
> +        VMSTATE_UINT16_V(status, VirtIONet, 3),
> +        VMSTATE_UINT8_HACK_TEST(promisc, VirtIONet, between_4_and_8),
> +        VMSTATE_UINT8_HACK_TEST(allmulti, VirtIONet, between_4_and_8),
> +        VMSTATE_UINT8_V(promisc, VirtIONet, 8),
> +        VMSTATE_UINT8_V(allmulti, VirtIONet, 8),
> +        VMSTATE_UINT32_V(mac_table.in_use, VirtIONet, 5),
> +        VMSTATE_BUFFER_MULTIPLY(mac_table.macs, VirtIONet, 5, NULL, 0,
> +                                mac_table.in_use, ETH_ALEN),
> +        VMSTATE_BUFFER_V(vlans, VirtIONet, 6),
> +        VMSTATE_UINT32_V(has_vnet_hdr, VirtIONet, 7),
> +        VMSTATE_UINT8_V(mac_table.multi_overflow, VirtIONet, 9),
> +        VMSTATE_UINT8_V(mac_table.uni_overflow, VirtIONet, 9),
> +        VMSTATE_UINT8_V(alluni, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nomulti, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nouni, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nobcast, VirtIONet, 10),
> +        VMSTATE_UINT8_V(has_ufo, VirtIONet, 11),
> +        VMSTATE_END_OF_LIST()
>      }
> -
> -    if (version_id >= 11) {
> -        qemu_get_8s(f, &n->has_ufo);
> -    }
> -
> -    return virtio_net_post_load(n, version_id);
> -}
> +};
> 
>  static void virtio_net_cleanup(VLANClientState *vc)
>  {
> @@ -873,8 +854,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
>      n->mergeable_rx_bufs = 0;
>      n->promisc = 1; /* for compatibility */
> 
> -    register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
> -                    virtio_net_save, virtio_net_load, n);
> +    vmstate_register(virtio_net_id++, &vmstate_virtio_net, n);
> 
>      return &n->vdev;
>  }
> @@ -885,7 +865,7 @@ void virtio_net_exit(VirtIODevice *vdev)
> 
>      qemu_purge_queued_packets(n->vc);
> 
> -    unregister_savevm("virtio-net", n);
> +    vmstate_unregister(&vmstate_virtio_net, n);
> 
>      qemu_del_timer(n->tx_timer);
>      qemu_free_timer(n->tx_timer);
> -- 
> 1.6.5.2
Michael S. Tsirkin - Dec. 2, 2009, 6:37 p.m.
On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
>  1 files changed, 64 insertions(+), 84 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 4434827..3a59449 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
>      virtio_net_flush_tx(n, n->tx_vq);
>  }
> 
> +/* Restore an uint8_t from an uint32_t
> +   This is a Big hack, but it is how the old state did it.
> + */

I do not see why this is such a big hack.
u8 fits in u32. there might be many reasons
to use u32 for savevm (e.g. to be forward
compatible) and still use u8 at runtime
(e.g. to save memory).

IMO we should teach infrastructure to cope
wit this gracefully without littering
code with _hack suffixes.

> +
> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
> +{
> +    uint8_t *v = pv;
> +    *v = qemu_get_be32(f);
> +    return 0;
> +}
> +
> +static void put_unused(QEMUFile *f, void *pv, size_t size)
> +{
> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
> +    fprintf(stderr, "Never should be used to write a new state.\n");
> +    exit(0);
> +}
> +
> +static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
> +    .name = "uint8_from_uint32",
> +    .get  = get_uint8_from_uint32,
> +    .put  = put_unused,
> +};
> +
> +#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)


So a different format for a different version.
Why is this so bad?
And IMO VMSTATE macros really should be able to cope with
version range checks without open-coded
predicates. These are common and we will have more
of these.


> +
> +static bool between_4_and_8(void *opaque, int version_id)
> +{
> +    return version_id >= 4 && version_id < 8;
> +}
> +

This is pretty ugly, isn't it?
How about ..._V_RANGE() macros?

>  static int virtio_net_post_load(void *opaque, int version_id)
>  {
>      VirtIONet *n = opaque;
> @@ -748,88 +780,37 @@ static int virtio_net_post_load(void *opaque, int version_id)
>      return 0;
>  }
> 
> -static void virtio_net_save(QEMUFile *f, void *opaque)
> -{
> -    VirtIONet *n = opaque;
> -
> -    virtio_save(&n->vdev, f);
> -
> -    qemu_put_buffer(f, n->mac, ETH_ALEN);
> -    qemu_put_be32s(f, &n->tx_timer_active);
> -    qemu_put_be32s(f, &n->mergeable_rx_bufs);
> -    qemu_put_be16s(f, &n->status);
> -    qemu_put_8s(f, &n->promisc);
> -    qemu_put_8s(f, &n->allmulti);
> -    qemu_put_be32s(f, &n->mac_table.in_use);
> -    qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
> -    qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
> -    qemu_put_be32s(f, &n->has_vnet_hdr);
> -    qemu_put_8s(f, &n->mac_table.multi_overflow);
> -    qemu_put_8s(f, &n->mac_table.uni_overflow);
> -    qemu_put_8s(f, &n->alluni);
> -    qemu_put_8s(f, &n->nomulti);
> -    qemu_put_8s(f, &n->nouni);
> -    qemu_put_8s(f, &n->nobcast);
> -    qemu_put_8s(f, &n->has_ufo);
> -}
> -
> -static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
> -{
> -    VirtIONet *n = opaque;
> -
> -    if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
> -        return -EINVAL;
> -
> -    virtio_load(&n->vdev, f);
> -
> -    qemu_get_buffer(f, n->mac, ETH_ALEN);
> -    qemu_get_be32s(f, &n->tx_timer_active);
> -    qemu_get_be32s(f, &n->mergeable_rx_bufs);
> -
> -    if (version_id >= 3)
> -        qemu_get_be16s(f, &n->status);
> -
> -    if (version_id >= 4) {
> -        if (version_id < 8) {
> -            n->promisc = qemu_get_be32(f);
> -            n->allmulti = qemu_get_be32(f);
> -        } else {
> -            qemu_get_8s(f, &n->promisc);
> -            qemu_get_8s(f, &n->allmulti);
> -        }
> -    }
> -
> -    if (version_id >= 5) {
> -        qemu_get_be32s(f, &n->mac_table.in_use);
> -        qemu_get_buffer(f, n->mac_table.macs,
> -                        n->mac_table.in_use * ETH_ALEN);
> -    }
> - 
> -    if (version_id >= 6)
> -        qemu_get_buffer(f, n->vlans, MAX_VLAN >> 3);
> -
> -    if (version_id >= 7) {
> -        qemu_get_be32s(f, &n->has_vnet_hdr);
> -    }
> -
> -    if (version_id >= 9) {
> -        qemu_get_8s(f, &n->mac_table.multi_overflow);
> -        qemu_get_8s(f, &n->mac_table.uni_overflow);
> -    }
> -
> -    if (version_id >= 10) {
> -        qemu_get_8s(f, &n->alluni);
> -        qemu_get_8s(f, &n->nomulti);
> -        qemu_get_8s(f, &n->nouni);
> -        qemu_get_8s(f, &n->nobcast);
> +static const VMStateDescription vmstate_virtio_net = {
> +    .name = "virtio-net",
> +    .version_id = 11,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 2,
> +    .post_load = virtio_net_post_load,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_VIRTIO(vdev, VirtIONet),
> +        VMSTATE_BUFFER(mac, VirtIONet),
> +        VMSTATE_UINT32(tx_timer_active, VirtIONet),
> +        VMSTATE_UINT32(mergeable_rx_bufs, VirtIONet),
> +        VMSTATE_UINT16_V(status, VirtIONet, 3),
> +        VMSTATE_UINT8_HACK_TEST(promisc, VirtIONet, between_4_and_8),
> +        VMSTATE_UINT8_HACK_TEST(allmulti, VirtIONet, between_4_and_8),
> +        VMSTATE_UINT8_V(promisc, VirtIONet, 8),
> +        VMSTATE_UINT8_V(allmulti, VirtIONet, 8),
> +        VMSTATE_UINT32_V(mac_table.in_use, VirtIONet, 5),
> +        VMSTATE_BUFFER_MULTIPLY(mac_table.macs, VirtIONet, 5, NULL, 0,
> +                                mac_table.in_use, ETH_ALEN),
> +        VMSTATE_BUFFER_V(vlans, VirtIONet, 6),
> +        VMSTATE_UINT32_V(has_vnet_hdr, VirtIONet, 7),
> +        VMSTATE_UINT8_V(mac_table.multi_overflow, VirtIONet, 9),
> +        VMSTATE_UINT8_V(mac_table.uni_overflow, VirtIONet, 9),
> +        VMSTATE_UINT8_V(alluni, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nomulti, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nouni, VirtIONet, 10),
> +        VMSTATE_UINT8_V(nobcast, VirtIONet, 10),
> +        VMSTATE_UINT8_V(has_ufo, VirtIONet, 11),
> +        VMSTATE_END_OF_LIST()
>      }
> -
> -    if (version_id >= 11) {
> -        qemu_get_8s(f, &n->has_ufo);
> -    }
> -
> -    return virtio_net_post_load(n, version_id);
> -}
> +};
> 
>  static void virtio_net_cleanup(VLANClientState *vc)
>  {
> @@ -873,8 +854,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
>      n->mergeable_rx_bufs = 0;
>      n->promisc = 1; /* for compatibility */
> 
> -    register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
> -                    virtio_net_save, virtio_net_load, n);
> +    vmstate_register(virtio_net_id++, &vmstate_virtio_net, n);
> 
>      return &n->vdev;
>  }
> @@ -885,7 +865,7 @@ void virtio_net_exit(VirtIODevice *vdev)
> 
>      qemu_purge_queued_packets(n->vc);
> 
> -    unregister_savevm("virtio-net", n);
> +    vmstate_unregister(&vmstate_virtio_net, n);
> 
>      qemu_del_timer(n->tx_timer);
>      qemu_free_timer(n->tx_timer);
> -- 
> 1.6.5.2
Juan Quintela - Dec. 2, 2009, 6:38 p.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
>>  1 files changed, 64 insertions(+), 84 deletions(-)
>> 
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index 4434827..3a59449 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
>>      virtio_net_flush_tx(n, n->tx_vq);
>>  }
>> 
>> +/* Restore an uint8_t from an uint32_t
>> +   This is a Big hack, but it is how the old state did it.
>> + */
>> +
>> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    uint8_t *v = pv;
>> +    *v = qemu_get_be32(f);
>> +    return 0;
>> +}
>> +
>> +static void put_unused(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
>
> line too long
>
>> +    fprintf(stderr, "Never should be used to write a new state.\n");
>> +    exit(0);
>
> I don't understand.  what is this dong?

it is used later.
current code is reading an uint32_t value into a int8_t value.  As you
can guess that don't fit (that is the HACK part of it).  That is only
needed for old versions that we are reading (get_* function has real
code).  But we are supposed to never write old versions (*).
Thet that shouldn't happen ever.



>  when
> is this called? Please supply a comment.
> Maybe call assert?
>

assert or exit is ok for me, what does people preffer?

Later, Juan.

(*): My next series will propose to change that and allow to write old
     versions, but that didn't exist when this code was written, and
     there are still no agreement about how/if doing it.
Michael S. Tsirkin - Dec. 2, 2009, 6:40 p.m.
On Wed, Dec 02, 2009 at 07:38:03PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
> >>  1 files changed, 64 insertions(+), 84 deletions(-)
> >> 
> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >> index 4434827..3a59449 100644
> >> --- a/hw/virtio-net.c
> >> +++ b/hw/virtio-net.c
> >> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
> >>      virtio_net_flush_tx(n, n->tx_vq);
> >>  }
> >> 
> >> +/* Restore an uint8_t from an uint32_t
> >> +   This is a Big hack, but it is how the old state did it.
> >> + */
> >> +
> >> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> +    uint8_t *v = pv;
> >> +    *v = qemu_get_be32(f);
> >> +    return 0;
> >> +}
> >> +
> >> +static void put_unused(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
> >
> > line too long
> >
> >> +    fprintf(stderr, "Never should be used to write a new state.\n");
> >> +    exit(0);
> >
> > I don't understand.  what is this dong?
> 
> it is used later.
> current code is reading an uint32_t value into a int8_t value.  As you
> can guess that don't fit (that is the HACK part of it).

I expect it fits in practice.
But you should range check the value and fail migration on error.

>  That is only
> needed for old versions that we are reading (get_* function has real
> code).  But we are supposed to never write old versions (*).
> Thet that shouldn't happen ever.

vmstate guarantees this won't be called?
So just assert(1)? Let's not write a ton of
code that isn't called?


> 
> 
> >  when
> > is this called? Please supply a comment.
> > Maybe call assert?
> >
> 
> assert or exit is ok for me, what does people preffer?
> 
> Later, Juan.
> 
> (*): My next series will propose to change that and allow to write old
>      versions, but that didn't exist when this code was written, and
>      there are still no agreement about how/if doing it.
Juan Quintela - Dec. 2, 2009, 7:07 p.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 07:38:03PM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:

> I expect it fits in practice.
> But you should range check the value and fail migration on error.
>
>>  That is only
>> needed for old versions that we are reading (get_* function has real
>> code).  But we are supposed to never write old versions (*).
>> Thet that shouldn't happen ever.
>
> vmstate guarantees this won't be called?

No, I express it badly.  That function should never be called.  It is a
compatibility type for old versions.  Only get() part should be used.
What should be guaranteed is that you protect that with a test or
something to assure that only the get() part is ever used, not the put()
one.

> So just assert(1)? Let's not write a ton of
> code that isn't called?

options are:
a- put a NULL value in the struct, and if there are ever an error get a
   segmentation fault.
b- put a function that just writes "the impossible happened in <foo>"
   and exit.

I don't know where do you want me to put one assert.

Later, Juan.

>> >  when
>> > is this called? Please supply a comment.
>> > Maybe call assert?
>> >
>> 
>> assert or exit is ok for me, what does people preffer?
>> 
>> Later, Juan.
>> 
>> (*): My next series will propose to change that and allow to write old
>>      versions, but that didn't exist when this code was written, and
>>      there are still no agreement about how/if doing it.
Juan Quintela - Dec. 2, 2009, 7:18 p.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
>>  1 files changed, 64 insertions(+), 84 deletions(-)
>> 
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index 4434827..3a59449 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
>>      virtio_net_flush_tx(n, n->tx_vq);
>>  }
>> 
>> +/* Restore an uint8_t from an uint32_t
>> +   This is a Big hack, but it is how the old state did it.
>> + */
>
> I do not see why this is such a big hack.
> u8 fits in u32. there might be many reasons
> to use u32 for savevm (e.g. to be forward
> compatible) and still use u8 at runtime
> (e.g. to save memory).

but u32 don't fit in u8.  And we read u32 into u8.  I agree that writing
u8 into u32 is not a problem, the problem is the reverse.

> IMO we should teach infrastructure to cope
> wit this gracefully without littering
> code with _hack suffixes.

There is no way to get this one.  vmstate only knows about types.
You want vmstate to know that:
- sometimes writing u32 in u8 is one error
- othertimes writing u32 in u8 is correct, because we should have using
  u8 in the 1st place.

vmstate only has to look at:
type of field and function used to send/receive field. No code analisys
to know how many values are used of that type, etc.

What code is doing here is not valid in general.  It is valid in this
particular case, that is the reason why it is a _hack.

Later, Juan.

>> +
>> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    uint8_t *v = pv;
>> +    *v = qemu_get_be32(f);
>> +    return 0;
>> +}
>> +
>> +static void put_unused(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
>> +    fprintf(stderr, "Never should be used to write a new state.\n");
>> +    exit(0);
>> +}
>> +
>> +static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
>> +    .name = "uint8_from_uint32",
>> +    .get  = get_uint8_from_uint32,
>> +    .put  = put_unused,
>> +};
>> +
>> +#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
>> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)
>
>
> So a different format for a different version.
> Why is this so bad?
> And IMO VMSTATE macros really should be able to cope with
> version range checks without open-coded
> predicates. These are common and we will have more
> of these.

if they are common, we can have a library of them.  That is not the
problem.  cope with version range checks is not so trivial (at least we
need to add another field to store the in,from).  And there are places
where that is not enough (think of the virtio check to know if it is
pci/msix).

>> +
>> +static bool between_4_and_8(void *opaque, int version_id)
>> +{
>> +    return version_id >= 4 && version_id < 8;
>> +}
>> +
>
> This is pretty ugly, isn't it?
> How about ..._V_RANGE() macros?

I am really thinking about remove the _V() version, and move all to
tests.  reason for that is that there are already too much arguments
that are integers.  If we add more, we end having something like:

VMSTATE_FOO(bar, FOOState, 1, 2, 3, 4)

Once that we are there, moving one integer to the wrong place is too
easy.  In the other hand, it is trivial to have already defined
functions that are:
v1
v2
v3
v1_8
....

And for that, I can typecheck that you are not putting integers in the
place of versions.

technically, there is easy to add _RANGE() macros.  My problem is that
each time that we add another integer parameter, we make the interfaz
more difficult to use.

This proposal has the advantage that it removes the version parameter
and improves typechecking.

Later, Juan.
Michael S. Tsirkin - Dec. 3, 2009, 9:19 a.m.
On Wed, Dec 02, 2009 at 08:18:40PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
> >>  1 files changed, 64 insertions(+), 84 deletions(-)
> >> 
> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >> index 4434827..3a59449 100644
> >> --- a/hw/virtio-net.c
> >> +++ b/hw/virtio-net.c
> >> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
> >>      virtio_net_flush_tx(n, n->tx_vq);
> >>  }
> >> 
> >> +/* Restore an uint8_t from an uint32_t
> >> +   This is a Big hack, but it is how the old state did it.
> >> + */
> >
> > I do not see why this is such a big hack.
> > u8 fits in u32. there might be many reasons
> > to use u32 for savevm (e.g. to be forward
> > compatible) and still use u8 at runtime
> > (e.g. to save memory).
> 
> but u32 don't fit in u8.  And we read u32 into u8.  I agree that writing
> u8 into u32 is not a problem, the problem is the reverse.

So don't read u32 into u8.
Read data into u32, check that the value fits into u8, fail migration
if it does not, assign value if it does.

> > IMO we should teach infrastructure to cope
> > wit this gracefully without littering
> > code with _hack suffixes.
> 
> There is no way to get this one.  vmstate only knows about types.
> You want vmstate to know that:
> - sometimes writing u32 in u8 is one error
> - othertimes writing u32 in u8 is correct, because we should have using
>   u8 in the 1st place.
> 
> vmstate only has to look at:
> type of field and function used to send/receive field. No code analisys
> to know how many values are used of that type, etc.
> 
> What code is doing here is not valid in general.  It is valid in this
> particular case, that is the reason why it is a _hack.
> 
> Later, Juan.

Assume that you want to make format forward compatible.  If you want to
support large values in the future, you reserve 4 bytes in the format.
This makes more sense than playing with versions, and in my book, this
is a good practice, not a hack.  This should also not force you to store
values at runtime as a 32 bit integers.

> >> +
> >> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> +    uint8_t *v = pv;
> >> +    *v = qemu_get_be32(f);
> >> +    return 0;
> >> +}
> >> +
> >> +static void put_unused(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
> >> +    fprintf(stderr, "Never should be used to write a new state.\n");
> >> +    exit(0);
> >> +}
> >> +
> >> +static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
> >> +    .name = "uint8_from_uint32",
> >> +    .get  = get_uint8_from_uint32,
> >> +    .put  = put_unused,
> >> +};
> >> +
> >> +#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
> >> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)
> >
> >
> > So a different format for a different version.
> > Why is this so bad?
> > And IMO VMSTATE macros really should be able to cope with
> > version range checks without open-coded
> > predicates. These are common and we will have more
> > of these.
> 
> if they are common, we can have a library of them.  That is not the
> problem.  cope with version range checks is not so trivial (at least we
> need to add another field to store the in,from).  And there are places
> where that is not enough (think of the virtio check to know if it is
> pci/msix).
> 
> >> +
> >> +static bool between_4_and_8(void *opaque, int version_id)
> >> +{
> >> +    return version_id >= 4 && version_id < 8;
> >> +}
> >> +
> >
> > This is pretty ugly, isn't it?
> > How about ..._V_RANGE() macros?
> 
> I am really thinking about remove the _V() version, and move all to
> tests.  reason for that is that there are already too much arguments
> that are integers.  If we add more, we end having something like:
> 
> VMSTATE_FOO(bar, FOOState, 1, 2, 3, 4)
> 
> Once that we are there, moving one integer to the wrong place is too
> easy.  In the other hand, it is trivial to have already defined
> functions that are:
> v1
> v2
> v3
> v1_8
> ....
> And for that, I can typecheck that you are not putting integers in the
> place of versions.
> 
> technically, there is easy to add _RANGE() macros.  My problem is that
> each time that we add another integer parameter, we make the interfaz
> more difficult to use.

I agree this is a problem.  Another option is to use structures
initializing any extra fields outside the macro.

	{
		.min_version = 1,
		.max_version = 2,
		VMSTATE_FOO(bar, FOOState),
	},

> 
> This proposal has the advantage that it removes the version parameter
> and improves typechecking.
> 
> Later, Juan.
Juan Quintela - Dec. 3, 2009, 12:01 p.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:

...

>> but u32 don't fit in u8.  And we read u32 into u8.  I agree that writing
>> u8 into u32 is not a problem, the problem is the reverse.
>
> So don't read u32 into u8.
> Read data into u32, check that the value fits into u8, fail migration
> if it does not, assign value if it does.

Exactly what the code does.  That is one of the definitions of "hack" :)

>> > IMO we should teach infrastructure to cope
>> > wit this gracefully without littering
>> > code with _hack suffixes.
>> 
>> There is no way to get this one.  vmstate only knows about types.
>> You want vmstate to know that:
>> - sometimes writing u32 in u8 is one error
>> - othertimes writing u32 in u8 is correct, because we should have using
>>   u8 in the 1st place.
>> 
>> vmstate only has to look at:
>> type of field and function used to send/receive field. No code analisys
>> to know how many values are used of that type, etc.
>> 
>> What code is doing here is not valid in general.  It is valid in this
>> particular case, that is the reason why it is a _hack.
>> 
>> Later, Juan.
>
> Assume that you want to make format forward compatible.  If you want to
> support large values in the future, you reserve 4 bytes in the format.
> This makes more sense than playing with versions, and in my book, this
> is a good practice, not a hack.  This should also not force you to store
> values at runtime as a 32 bit integers.

If you want 32bits, use 32bits.  No tricks, no games, all right.  If you
want to read 8bit into 32 bits, that is doable.  If you want to store
32bits into 8bit -> unsafe in general.  If you want to do that, you get
a big RED sing, you can call it "UNSAFE", "HACK", whatever.
Typechecking is lost.

>> Once that we are there, moving one integer to the wrong place is too
>> easy.  In the other hand, it is trivial to have already defined
>> functions that are:
>> v1
>> v2
>> v3
>> v1_8
>> ....
>> And for that, I can typecheck that you are not putting integers in the
>> place of versions.
>> 
>> technically, there is easy to add _RANGE() macros.  My problem is that
>> each time that we add another integer parameter, we make the interfaz
>> more difficult to use.
>
> I agree this is a problem.  Another option is to use structures
> initializing any extra fields outside the macro.
>
> 	{
> 		.min_version = 1,
> 		.max_version = 2,
> 		VMSTATE_FOO(bar, FOOState),
> 	},

That is another option.  I would preffer the test functions, because as
we need them anyways, that makes things easier.  vmstate_field was
pretty small at the beggining, now it is starting to bloat.

Later, Juan.

Patch

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 4434827..3a59449 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -703,6 +703,38 @@  static void virtio_net_tx_timer(void *opaque)
     virtio_net_flush_tx(n, n->tx_vq);
 }

+/* Restore an uint8_t from an uint32_t
+   This is a Big hack, but it is how the old state did it.
+ */
+
+static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
+{
+    uint8_t *v = pv;
+    *v = qemu_get_be32(f);
+    return 0;
+}
+
+static void put_unused(QEMUFile *f, void *pv, size_t size)
+{
+    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
+    fprintf(stderr, "Never should be used to write a new state.\n");
+    exit(0);
+}
+
+static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
+    .name = "uint8_from_uint32",
+    .get  = get_uint8_from_uint32,
+    .put  = put_unused,
+};
+
+#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
+    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)
+
+static bool between_4_and_8(void *opaque, int version_id)
+{
+    return version_id >= 4 && version_id < 8;
+}
+
 static int virtio_net_post_load(void *opaque, int version_id)
 {
     VirtIONet *n = opaque;
@@ -748,88 +780,37 @@  static int virtio_net_post_load(void *opaque, int version_id)
     return 0;
 }

-static void virtio_net_save(QEMUFile *f, void *opaque)
-{
-    VirtIONet *n = opaque;
-
-    virtio_save(&n->vdev, f);
-
-    qemu_put_buffer(f, n->mac, ETH_ALEN);
-    qemu_put_be32s(f, &n->tx_timer_active);
-    qemu_put_be32s(f, &n->mergeable_rx_bufs);
-    qemu_put_be16s(f, &n->status);
-    qemu_put_8s(f, &n->promisc);
-    qemu_put_8s(f, &n->allmulti);
-    qemu_put_be32s(f, &n->mac_table.in_use);
-    qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN);
-    qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3);
-    qemu_put_be32s(f, &n->has_vnet_hdr);
-    qemu_put_8s(f, &n->mac_table.multi_overflow);
-    qemu_put_8s(f, &n->mac_table.uni_overflow);
-    qemu_put_8s(f, &n->alluni);
-    qemu_put_8s(f, &n->nomulti);
-    qemu_put_8s(f, &n->nouni);
-    qemu_put_8s(f, &n->nobcast);
-    qemu_put_8s(f, &n->has_ufo);
-}
-
-static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
-{
-    VirtIONet *n = opaque;
-
-    if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
-        return -EINVAL;
-
-    virtio_load(&n->vdev, f);
-
-    qemu_get_buffer(f, n->mac, ETH_ALEN);
-    qemu_get_be32s(f, &n->tx_timer_active);
-    qemu_get_be32s(f, &n->mergeable_rx_bufs);
-
-    if (version_id >= 3)
-        qemu_get_be16s(f, &n->status);
-
-    if (version_id >= 4) {
-        if (version_id < 8) {
-            n->promisc = qemu_get_be32(f);
-            n->allmulti = qemu_get_be32(f);
-        } else {
-            qemu_get_8s(f, &n->promisc);
-            qemu_get_8s(f, &n->allmulti);
-        }
-    }
-
-    if (version_id >= 5) {
-        qemu_get_be32s(f, &n->mac_table.in_use);
-        qemu_get_buffer(f, n->mac_table.macs,
-                        n->mac_table.in_use * ETH_ALEN);
-    }
- 
-    if (version_id >= 6)
-        qemu_get_buffer(f, n->vlans, MAX_VLAN >> 3);
-
-    if (version_id >= 7) {
-        qemu_get_be32s(f, &n->has_vnet_hdr);
-    }
-
-    if (version_id >= 9) {
-        qemu_get_8s(f, &n->mac_table.multi_overflow);
-        qemu_get_8s(f, &n->mac_table.uni_overflow);
-    }
-
-    if (version_id >= 10) {
-        qemu_get_8s(f, &n->alluni);
-        qemu_get_8s(f, &n->nomulti);
-        qemu_get_8s(f, &n->nouni);
-        qemu_get_8s(f, &n->nobcast);
+static const VMStateDescription vmstate_virtio_net = {
+    .name = "virtio-net",
+    .version_id = 11,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
+    .post_load = virtio_net_post_load,
+    .fields      = (VMStateField []) {
+        VMSTATE_VIRTIO(vdev, VirtIONet),
+        VMSTATE_BUFFER(mac, VirtIONet),
+        VMSTATE_UINT32(tx_timer_active, VirtIONet),
+        VMSTATE_UINT32(mergeable_rx_bufs, VirtIONet),
+        VMSTATE_UINT16_V(status, VirtIONet, 3),
+        VMSTATE_UINT8_HACK_TEST(promisc, VirtIONet, between_4_and_8),
+        VMSTATE_UINT8_HACK_TEST(allmulti, VirtIONet, between_4_and_8),
+        VMSTATE_UINT8_V(promisc, VirtIONet, 8),
+        VMSTATE_UINT8_V(allmulti, VirtIONet, 8),
+        VMSTATE_UINT32_V(mac_table.in_use, VirtIONet, 5),
+        VMSTATE_BUFFER_MULTIPLY(mac_table.macs, VirtIONet, 5, NULL, 0,
+                                mac_table.in_use, ETH_ALEN),
+        VMSTATE_BUFFER_V(vlans, VirtIONet, 6),
+        VMSTATE_UINT32_V(has_vnet_hdr, VirtIONet, 7),
+        VMSTATE_UINT8_V(mac_table.multi_overflow, VirtIONet, 9),
+        VMSTATE_UINT8_V(mac_table.uni_overflow, VirtIONet, 9),
+        VMSTATE_UINT8_V(alluni, VirtIONet, 10),
+        VMSTATE_UINT8_V(nomulti, VirtIONet, 10),
+        VMSTATE_UINT8_V(nouni, VirtIONet, 10),
+        VMSTATE_UINT8_V(nobcast, VirtIONet, 10),
+        VMSTATE_UINT8_V(has_ufo, VirtIONet, 11),
+        VMSTATE_END_OF_LIST()
     }
-
-    if (version_id >= 11) {
-        qemu_get_8s(f, &n->has_ufo);
-    }
-
-    return virtio_net_post_load(n, version_id);
-}
+};

 static void virtio_net_cleanup(VLANClientState *vc)
 {
@@ -873,8 +854,7 @@  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
     n->mergeable_rx_bufs = 0;
     n->promisc = 1; /* for compatibility */

-    register_savevm("virtio-net", virtio_net_id++, VIRTIO_NET_VM_VERSION,
-                    virtio_net_save, virtio_net_load, n);
+    vmstate_register(virtio_net_id++, &vmstate_virtio_net, n);

     return &n->vdev;
 }
@@ -885,7 +865,7 @@  void virtio_net_exit(VirtIODevice *vdev)

     qemu_purge_queued_packets(n->vc);

-    unregister_savevm("virtio-net", n);
+    vmstate_unregister(&vmstate_virtio_net, n);

     qemu_del_timer(n->tx_timer);
     qemu_free_timer(n->tx_timer);