[PATCH 1/2] virtio-net: make VirtIONet.vlans an array instead of a pointer

[PATCH 1/2] virtio-net: make VirtIONet.vlans an array instead of a pointer

[Michael Tokarev]

This field is a fixed-size buffer (number of elements is MAX_VLAN,
known at build time).  There's no need to allocate it dynamically,
it can be made an integral part of VirtIONet structure.

This field is the only user of VMSTATE_BUFFER_POINTER_UNSAFE() macro.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/net/virtio-net.c            | 8 +++-----
 include/hw/virtio/virtio-net.h | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 6b5b5dace3..3973dd0e5f 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -931,7 +931,7 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
 
     if (virtio_has_feature(vdev->guest_features ^ features, VIRTIO_NET_F_CTRL_VLAN)) {
         bool vlan = virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN);
-        memset(n->vlans, vlan ? 0 : 0xff, MAX_VLAN >> 3);
+        memset(n->vlans, vlan ? 0 : 0xff, sizeof(n->vlans));
     }
 
     if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) {
@@ -3524,7 +3524,7 @@ static const VMStateDescription vmstate_virtio_net_device = {
          * buffer; hold onto your endiannesses; it's actually used as a bitmap
          * but based on the uint.
          */
-        VMSTATE_BUFFER_POINTER_UNSAFE(vlans, VirtIONet, 0, MAX_VLAN >> 3),
+        VMSTATE_BUFFER(vlans, VirtIONet),
         VMSTATE_WITH_TMP(VirtIONet, struct VirtIONetMigTmp,
                          vmstate_virtio_net_has_vnet),
         VMSTATE_UINT8(mac_table.multi_overflow, VirtIONet),
@@ -3942,8 +3942,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
 
     n->mac_table.macs = g_malloc0(MAC_TABLE_ENTRIES * ETH_ALEN);
 
-    n->vlans = g_malloc0(MAX_VLAN >> 3);
-    memset(n->vlans, 0xff, MAX_VLAN >> 3);
+    memset(n->vlans, 0xff, sizeof(n->vlans));
 
     nc = qemu_get_queue(n->nic);
     nc->rxfilter_notify_enabled = 1;
@@ -3992,7 +3991,6 @@ static void virtio_net_device_unrealize(DeviceState *dev)
     n->netclient_type = NULL;
 
     g_free(n->mac_table.macs);
-    g_free(n->vlans);
 
     if (n->failover) {
         qobject_unref(n->primary_opts);
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 73fdefc0dc..4a0cc34ae6 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -202,7 +202,7 @@ struct VirtIONet {
         uint8_t uni_overflow;
         uint8_t *macs;
     } mac_table;
-    uint32_t *vlans;
+    uint32_t vlans[MAX_VLAN];
     virtio_net_conf net_conf;
     NICConf nic_conf;
     DeviceState *qdev;
-- 
2.47.2

[PATCH 2/2] migration/vmstate: remove VMSTATE_BUFFER_POINTER_UNSAFE macro

[Michael Tokarev]

The only user of this macro was VirtIONet.vlans, which has been
converted to regular VMSTATE_BUFFER.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 include/migration/vmstate.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1ff7bd9ac4..ec0946c2aa 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -702,15 +702,6 @@ extern const VMStateInfo vmstate_info_qlist;
     .offset     = offsetof(_state, _field),                          \
 }
 
-#define VMSTATE_BUFFER_POINTER_UNSAFE(_field, _state, _version, _size) { \
-    .name       = (stringify(_field)),                               \
-    .version_id = (_version),                                        \
-    .size       = (_size),                                           \
-    .info       = &vmstate_info_buffer,                              \
-    .flags      = VMS_BUFFER|VMS_POINTER,                            \
-    .offset     = offsetof(_state, _field),                          \
-}
-
 /* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
  * and execute the vmsd on the temporary.  Note that we're working with
  * the whole of _state here, not a field within it.
-- 
2.47.2

Re: [PATCH 1/2] virtio-net: make VirtIONet.vlans an array instead of a pointer

[Michael Tokarev]

On 02.08.2025 17:21, Michael Tokarev wrote:
> This field is a fixed-size buffer (number of elements is MAX_VLAN,
> known at build time).  There's no need to allocate it dynamically,
> it can be made an integral part of VirtIONet structure.
> 
> This field is the only user of VMSTATE_BUFFER_POINTER_UNSAFE() macro.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>   hw/net/virtio-net.c            | 8 +++-----
>   include/hw/virtio/virtio-net.h | 2 +-
>   2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> @@ -3524,7 +3524,7 @@ static const VMStateDescription vmstate_virtio_net_device = {
>            * buffer; hold onto your endiannesses; it's actually used as a bitmap
>            * but based on the uint.
>            */
> -        VMSTATE_BUFFER_POINTER_UNSAFE(vlans, VirtIONet, 0, MAX_VLAN >> 3),
> +        VMSTATE_BUFFER(vlans, VirtIONet),

This doesn't compile.  And I can't figure out, so far, what's needed
here :)

/mjt

Re: [PATCH 1/2] virtio-net: make VirtIONet.vlans an array instead of a pointer

[Akihiko Odaki]

On 2025/08/02 23:45, Michael Tokarev wrote:
> On 02.08.2025 17:21, Michael Tokarev wrote:
>> This field is a fixed-size buffer (number of elements is MAX_VLAN,
>> known at build time).  There's no need to allocate it dynamically,
>> it can be made an integral part of VirtIONet structure.
>>
>> This field is the only user of VMSTATE_BUFFER_POINTER_UNSAFE() macro.
>>
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>> ---
>>   hw/net/virtio-net.c            | 8 +++-----
>>   include/hw/virtio/virtio-net.h | 2 +-
>>   2 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> @@ -3524,7 +3524,7 @@ static const VMStateDescription 
>> vmstate_virtio_net_device = {
>>            * buffer; hold onto your endiannesses; it's actually used 
>> as a bitmap
>>            * but based on the uint.
>>            */
>> -        VMSTATE_BUFFER_POINTER_UNSAFE(vlans, VirtIONet, 0, MAX_VLAN 
>> >> 3),
>> +        VMSTATE_BUFFER(vlans, VirtIONet),
> 
> This doesn't compile.  And I can't figure out, so far, what's needed
> here :)

The type check is failing because VMSTATE_BUFFER() expects an array of 
uint8_t but it is an array of uint32_t.

Now I get why it used an "UNSAFE" macro. We should usually use 
VMSTATE_UINT32_ARRAY() instead, but we need "BUFFER" for compatibility, 
so it needed the "UNSAFE" variant to disable the type check.

Fortunately there is a variant that is "BUFFER" and "UNSAFE" but not 
"POINTER". So this should be:
VMSTATE_BUFFER_UNSAFE(vlans, VirtIONet, 0,
                       sizeof(typeof_field(VirtIONet, vlans)))

[PATCH v2 1/2] virtio-net: make VirtIONet.vlans an array instead of a pointer

[Michael Tokarev]

This field is a fixed-size buffer (number of elements is MAX_VLAN,
known at build time).  There's no need to allocate it dynamically,
it can be made an integral part of VirtIONet structure.

This field is the only user of VMSTATE_BUFFER_POINTER_UNSAFE() macro.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
v2: use VMSTATE_BUFFER_UNSAFE instead of VMSTATE_BUFFER
    as suggested by Akihiko Odaki.
    Only the first patch is resent, patch 2/2 is the same.

 hw/net/virtio-net.c            | 9 ++++-----
 include/hw/virtio/virtio-net.h | 2 +-
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 6b5b5dace3..1ee3bd131c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -931,7 +931,7 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
 
     if (virtio_has_feature(vdev->guest_features ^ features, VIRTIO_NET_F_CTRL_VLAN)) {
         bool vlan = virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN);
-        memset(n->vlans, vlan ? 0 : 0xff, MAX_VLAN >> 3);
+        memset(n->vlans, vlan ? 0 : 0xff, sizeof(n->vlans));
     }
 
     if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) {
@@ -3524,7 +3524,8 @@ static const VMStateDescription vmstate_virtio_net_device = {
          * buffer; hold onto your endiannesses; it's actually used as a bitmap
          * but based on the uint.
          */
-        VMSTATE_BUFFER_POINTER_UNSAFE(vlans, VirtIONet, 0, MAX_VLAN >> 3),
+        VMSTATE_BUFFER_UNSAFE(vlans, VirtIONet, 0,
+                              sizeof(typeof_field(VirtIONet, vlans))),
         VMSTATE_WITH_TMP(VirtIONet, struct VirtIONetMigTmp,
                          vmstate_virtio_net_has_vnet),
         VMSTATE_UINT8(mac_table.multi_overflow, VirtIONet),
@@ -3942,8 +3943,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
 
     n->mac_table.macs = g_malloc0(MAC_TABLE_ENTRIES * ETH_ALEN);
 
-    n->vlans = g_malloc0(MAX_VLAN >> 3);
-    memset(n->vlans, 0xff, MAX_VLAN >> 3);
+    memset(n->vlans, 0xff, sizeof(n->vlans));
 
     nc = qemu_get_queue(n->nic);
     nc->rxfilter_notify_enabled = 1;
@@ -3992,7 +3992,6 @@ static void virtio_net_device_unrealize(DeviceState *dev)
     n->netclient_type = NULL;
 
     g_free(n->mac_table.macs);
-    g_free(n->vlans);
 
     if (n->failover) {
         qobject_unref(n->primary_opts);
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 73fdefc0dc..4a0cc34ae6 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -202,7 +202,7 @@ struct VirtIONet {
         uint8_t uni_overflow;
         uint8_t *macs;
     } mac_table;
-    uint32_t *vlans;
+    uint32_t vlans[MAX_VLAN];
     virtio_net_conf net_conf;
     NICConf nic_conf;
     DeviceState *qdev;
-- 
2.47.2

Re: [PATCH v2 1/2] virtio-net: make VirtIONet.vlans an array instead of a pointer

[Akihiko Odaki]

On 2025/08/03 17:54, Michael Tokarev wrote:
> This field is a fixed-size buffer (number of elements is MAX_VLAN,
> known at build time).  There's no need to allocate it dynamically,
> it can be made an integral part of VirtIONet structure.
> 
> This field is the only user of VMSTATE_BUFFER_POINTER_UNSAFE() macro.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

Re: [PATCH 2/2] migration/vmstate: remove VMSTATE_BUFFER_POINTER_UNSAFE macro

[Akihiko Odaki]

On 2025/08/02 23:21, Michael Tokarev wrote:
> The only user of this macro was VirtIONet.vlans, which has been
> converted to regular VMSTATE_BUFFER.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

Nice cleanup.

Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

Re: [PATCH v2 1/2] virtio-net: make VirtIONet.vlans an array instead of a pointer

[Lei Yang]

Tested this patch with virtio-net regression tests, everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>

On Sun, Aug 3, 2025 at 5:51 PM Akihiko Odaki
<odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
>
> On 2025/08/03 17:54, Michael Tokarev wrote:
> > This field is a fixed-size buffer (number of elements is MAX_VLAN,
> > known at build time).  There's no need to allocate it dynamically,
> > it can be made an integral part of VirtIONet structure.
> >
> > This field is the only user of VMSTATE_BUFFER_POINTER_UNSAFE() macro.
> >
> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>
> Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>

Re: [PATCH 2/2] migration/vmstate: remove VMSTATE_BUFFER_POINTER_UNSAFE macro

[Philippe Mathieu-Daudé]

On 2/8/25 16:21, Michael Tokarev wrote:
> The only user of this macro was VirtIONet.vlans, which has been
> converted to regular VMSTATE_BUFFER.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>   include/migration/vmstate.h | 9 ---------
>   1 file changed, 9 deletions(-)

Yay!

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/2] virtio-net: make VirtIONet.vlans an array instead of a pointer

[Philippe Mathieu-Daudé]

On 3/8/25 09:38, Akihiko Odaki wrote:
> On 2025/08/02 23:45, Michael Tokarev wrote:
>> On 02.08.2025 17:21, Michael Tokarev wrote:
>>> This field is a fixed-size buffer (number of elements is MAX_VLAN,
>>> known at build time).  There's no need to allocate it dynamically,
>>> it can be made an integral part of VirtIONet structure.
>>>
>>> This field is the only user of VMSTATE_BUFFER_POINTER_UNSAFE() macro.
>>>
>>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>>> ---
>>>   hw/net/virtio-net.c            | 8 +++-----
>>>   include/hw/virtio/virtio-net.h | 2 +-
>>>   2 files changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> @@ -3524,7 +3524,7 @@ static const VMStateDescription 
>>> vmstate_virtio_net_device = {
>>>            * buffer; hold onto your endiannesses; it's actually used 
>>> as a bitmap
>>>            * but based on the uint.
>>>            */
>>> -        VMSTATE_BUFFER_POINTER_UNSAFE(vlans, VirtIONet, 0, MAX_VLAN 
>>> >> 3),
>>> +        VMSTATE_BUFFER(vlans, VirtIONet),
>>
>> This doesn't compile.  And I can't figure out, so far, what's needed
>> here :)
> 
> The type check is failing because VMSTATE_BUFFER() expects an array of 
> uint8_t but it is an array of uint32_t.
> 
> Now I get why it used an "UNSAFE" macro. We should usually use 
> VMSTATE_UINT32_ARRAY() instead, but we need "BUFFER" for compatibility, 
> so it needed the "UNSAFE" variant to disable the type check.
> 
> Fortunately there is a variant that is "BUFFER" and "UNSAFE" but not 
> "POINTER". So this should be:
> VMSTATE_BUFFER_UNSAFE(vlans, VirtIONet, 0,
>                        sizeof(typeof_field(VirtIONet, vlans)))

Nice.

Re: [PATCH v2 1/2] virtio-net: make VirtIONet.vlans an array instead of a pointer

[Philippe Mathieu-Daudé]

On 3/8/25 10:54, Michael Tokarev wrote:
> This field is a fixed-size buffer (number of elements is MAX_VLAN,
> known at build time).  There's no need to allocate it dynamically,
> it can be made an integral part of VirtIONet structure.
> 
> This field is the only user of VMSTATE_BUFFER_POINTER_UNSAFE() macro.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
> v2: use VMSTATE_BUFFER_UNSAFE instead of VMSTATE_BUFFER
>      as suggested by Akihiko Odaki.
>      Only the first patch is resent, patch 2/2 is the same.
> 
>   hw/net/virtio-net.c            | 9 ++++-----
>   include/hw/virtio/virtio-net.h | 2 +-
>   2 files changed, 5 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/2] migration/vmstate: remove VMSTATE_BUFFER_POINTER_UNSAFE macro

[Peter Xu]

On Sat, Aug 02, 2025 at 05:21:09PM +0300, Michael Tokarev wrote:
> The only user of this macro was VirtIONet.vlans, which has been
> converted to regular VMSTATE_BUFFER.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 2/2] migration/vmstate: remove VMSTATE_BUFFER_POINTER_UNSAFE macro

[Peter Xu]

On Sat, Aug 02, 2025 at 05:21:09PM +0300, Michael Tokarev wrote:
> The only user of this macro was VirtIONet.vlans, which has been
> converted to regular VMSTATE_BUFFER.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  include/migration/vmstate.h | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1ff7bd9ac4..ec0946c2aa 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -702,15 +702,6 @@ extern const VMStateInfo vmstate_info_qlist;
>      .offset     = offsetof(_state, _field),                          \
>  }
>  
> -#define VMSTATE_BUFFER_POINTER_UNSAFE(_field, _state, _version, _size) { \
> -    .name       = (stringify(_field)),                               \
> -    .version_id = (_version),                                        \
> -    .size       = (_size),                                           \
> -    .info       = &vmstate_info_buffer,                              \
> -    .flags      = VMS_BUFFER|VMS_POINTER,                            \
> -    .offset     = offsetof(_state, _field),                          \
> -}
> -
>  /* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
>   * and execute the vmsd on the temporary.  Note that we're working with
>   * the whole of _state here, not a field within it.
> -- 
> 2.47.2
> 

I'm scanning over lost patches on the list.  I suspect this is lost when
you sent v2 replying to v1.

Wanna repost with a full v2 to catch Michael's attention once more?  I also
always would suggest a cover letter..

Thanks,

-- 
Peter Xu

Re: [PATCH 2/2] migration/vmstate: remove VMSTATE_BUFFER_POINTER_UNSAFE macro

[Michael Tokarev]

On 9/29/25 18:57, Peter Xu wrote:
> On Sat, Aug 02, 2025 at 05:21:09PM +0300, Michael Tokarev wrote:
>> The only user of this macro was VirtIONet.vlans, which has been
>> converted to regular VMSTATE_BUFFER.
..
> I'm scanning over lost patches on the list.  I suspect this is lost when
> you sent v2 replying to v1.

This hasn't been lost due to v2 vs v1, - the reason was that the first
patch in the series was trivial, while the second (this one) is a bit
more than trivial, - so the first patch went through qemu-trivial, and
the second went nowhere :)

> Wanna repost with a full v2 to catch Michael's attention once more?  I also
> always would suggest a cover letter..
Yeah, re-sending it is ok.  Dunno what's the reason for the cover letter
though, for a single patch.

Thank you for your attention - I'd lost it myself without you ;)

/mjt

Re: [PATCH 2/2] migration/vmstate: remove VMSTATE_BUFFER_POINTER_UNSAFE macro

[Peter Xu]

On Wed, Oct 08, 2025 at 11:10:19AM +0300, Michael Tokarev wrote:
> On 9/29/25 18:57, Peter Xu wrote:
> > On Sat, Aug 02, 2025 at 05:21:09PM +0300, Michael Tokarev wrote:
> > > The only user of this macro was VirtIONet.vlans, which has been
> > > converted to regular VMSTATE_BUFFER.
> ..
> > I'm scanning over lost patches on the list.  I suspect this is lost when
> > you sent v2 replying to v1.
> 
> This hasn't been lost due to v2 vs v1, - the reason was that the first
> patch in the series was trivial, while the second (this one) is a bit
> more than trivial, - so the first patch went through qemu-trivial, and
> the second went nowhere :)
> 
> > Wanna repost with a full v2 to catch Michael's attention once more?  I also
> > always would suggest a cover letter..
> Yeah, re-sending it is ok.  Dunno what's the reason for the cover letter
> though, for a single patch.
> 
> Thank you for your attention - I'd lost it myself without you ;)

We might have lost each other somehow.. :)

I am looking at this series that contains patch 1+2 on v1:

https://lore.kernel.org/all/20250802142115.41638-1-mjt@tls.msk.ru/

v2 of patch 1 was sent in a reply here:

https://lore.kernel.org/all/20250803085443.318611-1-mjt@tls.msk.ru/

As of today's git master branch (commit 37ad0e48e9), I still didn't see
patch 1 (v1 or v2) lands..

Thanks,

-- 
Peter Xu