[PATCH v6 0/4] virtio_net: Add the check for vdpa's mac address

[PATCH v6 0/4] virtio_net: Add the check for vdpa's mac address

[Cindy Lu]

When using a VDPA device, it is important to ensure that the MAC address
is correctly set. In this patch series, we add a new parameter to
enable this check.
Only three MAC setup configurations are acceptable; any other will
fail to boot.

The usage is:
....
-netdev type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-0,id=vhost-vdpa0,check-mac=true\
-device virtio-net-pci,netdev=vhost-vdpa0\
....

tested by ConnectX-6 Dx/vdpa_sim device

change in v3
1. add a new parameter to enable the check and keep the old behavior
2. adjust the comment and make it more clear

change in v4
1. change the new parameter's name to check-mac
2. change the comment and make it more clear

change in v5
1.These patches haven't been merged for a while, so I rebased
  them with the latest code and resubmitted

change in v6
1. Address comments
2. Rebase with the latest QEMU

Cindy Lu (4):
  vhost_vdpa : Add a new parameter to enable check mac address
  virtio_net: Add the check for vdpa's mac address
  virtio_net: Add second acceptable configuration for MAC setup
  virtio_net: Add third acceptable configuration for MAC setup.

 hw/net/virtio-net.c | 66 ++++++++++++++++++++++++++++++++++++++++++++-
 include/net/net.h   |  1 +
 net/vhost-vdpa.c    |  4 +++
 qapi/net.json       |  5 ++++
 4 files changed, 75 insertions(+), 1 deletion(-)

-- 
2.45.0

[PATCH v6 1/4] vhost_vdpa : Add a new parameter to enable check mac address

[Cindy Lu]

When using a VDPA device, it's important to ensure that the MAC
address is correctly set.
Add a new parameter in qemu cmdline to enable this check, default value
is false

The usage is:
....
-netdev type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-0,id=vhost-vdpa0,check-mac=true\
-device virtio-net-pci,netdev=vhost-vdpa0\
....

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 include/net/net.h | 1 +
 net/vhost-vdpa.c  | 4 ++++
 qapi/net.json     | 5 +++++
 3 files changed, 10 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index cdd5b109b0..fac1951b6e 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -112,6 +112,7 @@ struct NetClientState {
     bool is_netdev;
     bool do_not_pad; /* do not pad to the minimum ethernet frame length */
     bool is_datapath;
+    bool check_mac;
     QTAILQ_HEAD(, NetFilterState) filters;
 };
 
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 7ca8b46eee..ba1da31741 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -1870,6 +1870,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
                                      iova_range, features, shared, errp);
         if (!ncs[i])
             goto err;
+
+        ncs[i]->check_mac = opts->check_mac;
     }
 
     if (has_cvq) {
@@ -1882,6 +1884,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
                                  errp);
         if (!nc)
             goto err;
+
+        nc->check_mac = opts->check_mac;
     }
 
     return 0;
diff --git a/qapi/net.json b/qapi/net.json
index 310cc4fd19..a5c70d1df8 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -510,6 +510,10 @@
 # @queues: number of queues to be created for multiqueue vhost-vdpa
 #     (default: 1)
 #
+# @check-mac: Enable the check for whether the device's MAC address
+#     and the MAC in QEMU command line are acceptable for booting.
+#     (default: false)
+#
 # @x-svq: Start device with (experimental) shadow virtqueue.  (Since
 #     7.1) (default: false)
 #
@@ -524,6 +528,7 @@
     '*vhostdev':     'str',
     '*vhostfd':      'str',
     '*queues':       'int',
+    '*check-mac':    'bool',
     '*x-svq':        {'type': 'bool', 'features' : [ 'unstable'] } } }
 
 ##
-- 
2.45.0

[PATCH v6 2/4] virtio_net: Add the check for vdpa's mac address

[Cindy Lu]

When using a VDPA device, it is important to ensure that the MAC
address is correctly set. The MAC address in the hardware should
match the MAC address from the QEMU command line. This is a recommended
configuration and will allow the system to boot.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 hw/net/virtio-net.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 340c6b6422..94ee21d1fc 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3751,12 +3751,43 @@ static bool failover_hide_primary_device(DeviceListener *listener,
     /* failover_primary_hidden is set during feature negotiation */
     return qatomic_read(&n->failover_primary_hidden);
 }
+static bool virtio_net_check_vdpa_mac(NetClientState *nc, VirtIONet *n,
+                                      MACAddr *cmdline_mac, Error **errp)
+{
+    struct virtio_net_config hwcfg = {};
+    static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
+
+    vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&hwcfg, ETH_ALEN);
+
+    /* For VDPA device following situations are acceptable: */
 
+    if (memcmp(&hwcfg.mac, &zero, sizeof(MACAddr)) != 0) {
+        /*
+         * 1. The hardware MAC address is the same as the QEMU command line MAC
+         *   address, and both of them are not 0.
+         */
+        if ((memcmp(&hwcfg.mac, cmdline_mac, sizeof(MACAddr)) == 0)) {
+            return true;
+        }
+    }
+    error_setg(errp,
+               "vDPA device's MAC address %02x:%02x:%02x:%02x:%02x:%02x "
+               "is not the same as the QEMU command line MAC address "
+               "%02x:%02x:%02x:%02x:%02x:%02x,"
+               "Initialization failed.",
+               hwcfg.mac[0], hwcfg.mac[1], hwcfg.mac[2], hwcfg.mac[3],
+               hwcfg.mac[4], hwcfg.mac[5], cmdline_mac->a[0], cmdline_mac->a[1],
+               cmdline_mac->a[2], cmdline_mac->a[3], cmdline_mac->a[4],
+               cmdline_mac->a[5]);
+
+    return false;
+}
 static void virtio_net_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIONet *n = VIRTIO_NET(dev);
     NetClientState *nc;
+    MACAddr macaddr_cmdline;
     int i;
 
     if (n->net_conf.mtu) {
@@ -3864,6 +3895,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
     virtio_net_add_queue(n, 0);
 
     n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
+    memcpy(&macaddr_cmdline, &n->nic_conf.macaddr, sizeof(n->mac));
     qemu_macaddr_default_if_unset(&n->nic_conf.macaddr);
     memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac));
     n->status = VIRTIO_NET_S_LINK_UP;
@@ -3910,7 +3942,13 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
     nc = qemu_get_queue(n->nic);
     nc->rxfilter_notify_enabled = 1;
 
-   if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+    if (nc->peer && (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA)) {
+        if (nc->peer->check_mac) {
+            if (!virtio_net_check_vdpa_mac(nc, n, &macaddr_cmdline, errp)) {
+                virtio_cleanup(vdev);
+                return;
+            }
+        }
         struct virtio_net_config netcfg = {};
         memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
         vhost_net_set_config(get_vhost_net(nc->peer),
-- 
2.45.0

[PATCH v6 3/4] virtio_net: Add second acceptable configuration for MAC setup

[Cindy Lu]

For VDPA devices, Allow configurations where the hardware MAC address
is non-zero while the MAC address in the QEMU command line is zero.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 hw/net/virtio-net.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 94ee21d1fc..45b63eb9de 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3769,6 +3769,20 @@ static bool virtio_net_check_vdpa_mac(NetClientState *nc, VirtIONet *n,
         if ((memcmp(&hwcfg.mac, cmdline_mac, sizeof(MACAddr)) == 0)) {
             return true;
         }
+        /*
+         * 2. The hardware MAC address is NOT 0 and the MAC address in
+         *  the QEMU command line is 0.
+         *  In this situation, we use the hardware MAC address overwrite
+         *  the QEMU command line address saved in VirtIONet->mac[0].
+         *  In the following process, QEMU will use this MAC address
+         *  in VirtIONet to complete the initialization.
+         */
+        if (memcmp(cmdline_mac, &zero, sizeof(MACAddr)) == 0) {
+            /* overwrite the mac address with hardware address */
+            memcpy(&n->mac[0], &hwcfg.mac, sizeof(n->mac));
+            memcpy(&n->nic_conf.macaddr, &hwcfg.mac, sizeof(n->mac));
+            return true;
+        }
     }
     error_setg(errp,
                "vDPA device's MAC address %02x:%02x:%02x:%02x:%02x:%02x "
-- 
2.45.0

[PATCH v6 4/4] virtio_net: Add third acceptable configuration for MAC setup.

[Cindy Lu]

For VDPA devices, Allow configurations where both the hardware MAC address
and QEMU command line MAC address are zero.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 hw/net/virtio-net.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 45b63eb9de..6c6bd116f7 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3784,6 +3784,18 @@ static bool virtio_net_check_vdpa_mac(NetClientState *nc, VirtIONet *n,
             return true;
         }
     }
+    /*
+     * 3. The hardware MAC address is 0,
+     *  and the MAC address in the QEMU command line is also 0.
+     *  In this situation, QEMU generates a random MAC address and
+     *  uses CVQ/set_config to assign it to the device.
+     */
+    if ((memcmp(&hwcfg.mac, &zero, sizeof(MACAddr)) == 0) &&
+        (memcmp(cmdline_mac, &zero, sizeof(MACAddr)) == 0)) {
+        memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac));
+
+        return true;
+    }
     error_setg(errp,
                "vDPA device's MAC address %02x:%02x:%02x:%02x:%02x:%02x "
                "is not the same as the QEMU command line MAC address "
-- 
2.45.0

Re: [PATCH v6 1/4] vhost_vdpa : Add a new parameter to enable check mac address

[Jason Wang]

On Tue, Apr 8, 2025 at 2:13 PM Cindy Lu <lulu@redhat.com> wrote:
>
> When using a VDPA device, it's important to ensure that the MAC
> address is correctly set.
> Add a new parameter in qemu cmdline to enable this check, default value
> is false
>
> The usage is:
> ....
> -netdev type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-0,id=vhost-vdpa0,check-mac=true\
> -device virtio-net-pci,netdev=vhost-vdpa0\
> ....
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  include/net/net.h | 1 +
>  net/vhost-vdpa.c  | 4 ++++
>  qapi/net.json     | 5 +++++
>  3 files changed, 10 insertions(+)
>
> diff --git a/include/net/net.h b/include/net/net.h
> index cdd5b109b0..fac1951b6e 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -112,6 +112,7 @@ struct NetClientState {
>      bool is_netdev;
>      bool do_not_pad; /* do not pad to the minimum ethernet frame length */
>      bool is_datapath;
> +    bool check_mac;
>      QTAILQ_HEAD(, NetFilterState) filters;
>  };
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 7ca8b46eee..ba1da31741 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -1870,6 +1870,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>                                       iova_range, features, shared, errp);
>          if (!ncs[i])
>              goto err;
> +
> +        ncs[i]->check_mac = opts->check_mac;
>      }
>
>      if (has_cvq) {
> @@ -1882,6 +1884,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>                                   errp);
>          if (!nc)
>              goto err;
> +
> +        nc->check_mac = opts->check_mac;

Unless we need to iterate all the ncs during the startup, I think it
would be sufficient to store it in nc[0]?

>      }
>
>      return 0;
> diff --git a/qapi/net.json b/qapi/net.json
> index 310cc4fd19..a5c70d1df8 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -510,6 +510,10 @@
>  # @queues: number of queues to be created for multiqueue vhost-vdpa
>  #     (default: 1)
>  #
> +# @check-mac: Enable the check for whether the device's MAC address
> +#     and the MAC in QEMU command line are acceptable for booting.
> +#     (default: false)
> +#
>  # @x-svq: Start device with (experimental) shadow virtqueue.  (Since
>  #     7.1) (default: false)
>  #
> @@ -524,6 +528,7 @@
>      '*vhostdev':     'str',
>      '*vhostfd':      'str',
>      '*queues':       'int',
> +    '*check-mac':    'bool',

To make this more useful, we probably need to make it true by default
and do the compatibility work. Btw, while at it, do we need to check
MTU as well?

Thanks

>      '*x-svq':        {'type': 'bool', 'features' : [ 'unstable'] } } }
>
>  ##
> --
> 2.45.0
>

Re: [PATCH v6 2/4] virtio_net: Add the check for vdpa's mac address

[Jason Wang]

On Tue, Apr 8, 2025 at 2:13 PM Cindy Lu <lulu@redhat.com> wrote:
>
> When using a VDPA device, it is important to ensure that the MAC
> address is correctly set. The MAC address in the hardware should
> match the MAC address from the QEMU command line. This is a recommended
> configuration and will allow the system to boot.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  hw/net/virtio-net.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 340c6b6422..94ee21d1fc 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3751,12 +3751,43 @@ static bool failover_hide_primary_device(DeviceListener *listener,
>      /* failover_primary_hidden is set during feature negotiation */
>      return qatomic_read(&n->failover_primary_hidden);
>  }
> +static bool virtio_net_check_vdpa_mac(NetClientState *nc, VirtIONet *n,
> +                                      MACAddr *cmdline_mac, Error **errp)
> +{
> +    struct virtio_net_config hwcfg = {};
> +    static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
> +
> +    vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&hwcfg, ETH_ALEN);
> +
> +    /* For VDPA device following situations are acceptable: */
>

Let's drop the comment here and below as the code explains themselves.

> +    if (memcmp(&hwcfg.mac, &zero, sizeof(MACAddr)) != 0) {
> +        /*
> +         * 1. The hardware MAC address is the same as the QEMU command line MAC
> +         *   address, and both of them are not 0.
> +         */
> +        if ((memcmp(&hwcfg.mac, cmdline_mac, sizeof(MACAddr)) == 0)) {
> +            return true;
> +        }
> +    }
> +    error_setg(errp,
> +               "vDPA device's MAC address %02x:%02x:%02x:%02x:%02x:%02x "
> +               "is not the same as the QEMU command line MAC address "
> +               "%02x:%02x:%02x:%02x:%02x:%02x,"
> +               "Initialization failed.",
> +               hwcfg.mac[0], hwcfg.mac[1], hwcfg.mac[2], hwcfg.mac[3],
> +               hwcfg.mac[4], hwcfg.mac[5], cmdline_mac->a[0], cmdline_mac->a[1],
> +               cmdline_mac->a[2], cmdline_mac->a[3], cmdline_mac->a[4],
> +               cmdline_mac->a[5]);

I would move this to the caller to be consistent with other errors.

> +
> +    return false;
> +}
>  static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIONet *n = VIRTIO_NET(dev);
>      NetClientState *nc;
> +    MACAddr macaddr_cmdline;
>      int i;
>
>      if (n->net_conf.mtu) {
> @@ -3864,6 +3895,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>      virtio_net_add_queue(n, 0);
>
>      n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
> +    memcpy(&macaddr_cmdline, &n->nic_conf.macaddr, sizeof(n->mac));

Can we avoid this memcpy?

>      qemu_macaddr_default_if_unset(&n->nic_conf.macaddr);
>      memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac));
>      n->status = VIRTIO_NET_S_LINK_UP;
> @@ -3910,7 +3942,13 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>      nc = qemu_get_queue(n->nic);
>      nc->rxfilter_notify_enabled = 1;
>
> -   if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> +    if (nc->peer && (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA)) {
> +        if (nc->peer->check_mac) {
> +            if (!virtio_net_check_vdpa_mac(nc, n, &macaddr_cmdline, errp)) {
> +                virtio_cleanup(vdev);
> +                return;
> +            }
> +        }
>          struct virtio_net_config netcfg = {};
>          memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
>          vhost_net_set_config(get_vhost_net(nc->peer),
> --
> 2.45.0

Thanks

>

Re: [PATCH v6 3/4] virtio_net: Add second acceptable configuration for MAC setup

[Jason Wang]

On Tue, Apr 8, 2025 at 2:13 PM Cindy Lu <lulu@redhat.com> wrote:
>
> For VDPA devices, Allow configurations where the hardware MAC address
> is non-zero while the MAC address in the QEMU command line is zero.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  hw/net/virtio-net.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 94ee21d1fc..45b63eb9de 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3769,6 +3769,20 @@ static bool virtio_net_check_vdpa_mac(NetClientState *nc, VirtIONet *n,
>          if ((memcmp(&hwcfg.mac, cmdline_mac, sizeof(MACAddr)) == 0)) {
>              return true;
>          }
> +        /*
> +         * 2. The hardware MAC address is NOT 0 and the MAC address in
> +         *  the QEMU command line is 0.
> +         *  In this situation, we use the hardware MAC address overwrite
> +         *  the QEMU command line address saved in VirtIONet->mac[0].
> +         *  In the following process, QEMU will use this MAC address
> +         *  in VirtIONet to complete the initialization.
> +         */

Let's describe how this can interact with the trick done in
virtio_net_get_config().

> +        if (memcmp(cmdline_mac, &zero, sizeof(MACAddr)) == 0) {
> +            /* overwrite the mac address with hardware address */
> +            memcpy(&n->mac[0], &hwcfg.mac, sizeof(n->mac));
> +            memcpy(&n->nic_conf.macaddr, &hwcfg.mac, sizeof(n->mac));
> +            return true;
> +        }
>      }
>      error_setg(errp,
>                 "vDPA device's MAC address %02x:%02x:%02x:%02x:%02x:%02x "
> --
> 2.45.0
>

Thanks

Re: [PATCH v6 4/4] virtio_net: Add third acceptable configuration for MAC setup.

[Jason Wang]

On Tue, Apr 8, 2025 at 2:14 PM Cindy Lu <lulu@redhat.com> wrote:
>
> For VDPA devices, Allow configurations where both the hardware MAC address
> and QEMU command line MAC address are zero.
>

Let's explain why this can work.

Thanks

Re: [PATCH v6 4/4] virtio_net: Add third acceptable configuration for MAC setup.

[Cindy Lu]

On Tue, Apr 8, 2025 at 2:48 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Apr 8, 2025 at 2:14 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > For VDPA devices, Allow configurations where both the hardware MAC address
> > and QEMU command line MAC address are zero.
> >
>
> Let's explain why this can work.
>
> Thanks
>
Sure, will do
Thanks
cindy

Re: [PATCH v6 3/4] virtio_net: Add second acceptable configuration for MAC setup

[Cindy Lu]

On Tue, Apr 8, 2025 at 2:48 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Apr 8, 2025 at 2:13 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > For VDPA devices, Allow configurations where the hardware MAC address
> > is non-zero while the MAC address in the QEMU command line is zero.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  hw/net/virtio-net.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 94ee21d1fc..45b63eb9de 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -3769,6 +3769,20 @@ static bool virtio_net_check_vdpa_mac(NetClientState *nc, VirtIONet *n,
> >          if ((memcmp(&hwcfg.mac, cmdline_mac, sizeof(MACAddr)) == 0)) {
> >              return true;
> >          }
> > +        /*
> > +         * 2. The hardware MAC address is NOT 0 and the MAC address in
> > +         *  the QEMU command line is 0.
> > +         *  In this situation, we use the hardware MAC address overwrite
> > +         *  the QEMU command line address saved in VirtIONet->mac[0].
> > +         *  In the following process, QEMU will use this MAC address
> > +         *  in VirtIONet to complete the initialization.
> > +         */
>
> Let's describe how this can interact with the trick done in
> virtio_net_get_config().
>
Sure, will do
Thanks
cindy
> > +        if (memcmp(cmdline_mac, &zero, sizeof(MACAddr)) == 0) {
> > +            /* overwrite the mac address with hardware address */
> > +            memcpy(&n->mac[0], &hwcfg.mac, sizeof(n->mac));
> > +            memcpy(&n->nic_conf.macaddr, &hwcfg.mac, sizeof(n->mac));
> > +            return true;
> > +        }
> >      }
> >      error_setg(errp,
> >                 "vDPA device's MAC address %02x:%02x:%02x:%02x:%02x:%02x "
> > --
> > 2.45.0
> >
>
> Thanks
>

Re: [PATCH v6 2/4] virtio_net: Add the check for vdpa's mac address

[Cindy Lu]

On Tue, Apr 8, 2025 at 2:46 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Apr 8, 2025 at 2:13 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > When using a VDPA device, it is important to ensure that the MAC
> > address is correctly set. The MAC address in the hardware should
> > match the MAC address from the QEMU command line. This is a recommended
> > configuration and will allow the system to boot.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  hw/net/virtio-net.c | 40 +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 340c6b6422..94ee21d1fc 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -3751,12 +3751,43 @@ static bool failover_hide_primary_device(DeviceListener *listener,
> >      /* failover_primary_hidden is set during feature negotiation */
> >      return qatomic_read(&n->failover_primary_hidden);
> >  }
> > +static bool virtio_net_check_vdpa_mac(NetClientState *nc, VirtIONet *n,
> > +                                      MACAddr *cmdline_mac, Error **errp)
> > +{
> > +    struct virtio_net_config hwcfg = {};
> > +    static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
> > +
> > +    vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&hwcfg, ETH_ALEN);
> > +
> > +    /* For VDPA device following situations are acceptable: */
> >
>
> Let's drop the comment here and below as the code explains themselves.
>
sure, will do
> > +    if (memcmp(&hwcfg.mac, &zero, sizeof(MACAddr)) != 0) {
> > +        /*
> > +         * 1. The hardware MAC address is the same as the QEMU command line MAC
> > +         *   address, and both of them are not 0.
> > +         */
> > +        if ((memcmp(&hwcfg.mac, cmdline_mac, sizeof(MACAddr)) == 0)) {
> > +            return true;
> > +        }
> > +    }
> > +    error_setg(errp,
> > +               "vDPA device's MAC address %02x:%02x:%02x:%02x:%02x:%02x "
> > +               "is not the same as the QEMU command line MAC address "
> > +               "%02x:%02x:%02x:%02x:%02x:%02x,"
> > +               "Initialization failed.",
> > +               hwcfg.mac[0], hwcfg.mac[1], hwcfg.mac[2], hwcfg.mac[3],
> > +               hwcfg.mac[4], hwcfg.mac[5], cmdline_mac->a[0], cmdline_mac->a[1],
> > +               cmdline_mac->a[2], cmdline_mac->a[3], cmdline_mac->a[4],
> > +               cmdline_mac->a[5]);
>
> I would move this to the caller to be consistent with other errors.
>
sure, will do

> > +
> > +    return false;
> > +}
> >  static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      VirtIONet *n = VIRTIO_NET(dev);
> >      NetClientState *nc;
> > +    MACAddr macaddr_cmdline;
> >      int i;
> >
> >      if (n->net_conf.mtu) {
> > @@ -3864,6 +3895,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> >      virtio_net_add_queue(n, 0);
> >
> >      n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
> > +    memcpy(&macaddr_cmdline, &n->nic_conf.macaddr, sizeof(n->mac));
>
> Can we avoid this memcpy?
>
n->nic_conf.macaddr will be modified in the
qemu_macaddr_default_if_unset(&n->nic_conf.macaddr) function.If the
MAC is 0, it will be overwritten,
so if we don't use memcpy, we still need another way to track if the
MAC has changed
I think using memcpy here would make the code cleaner and more
straightforward. Maybe we can keep it
thanks
Cindy

> >      qemu_macaddr_default_if_unset(&n->nic_conf.macaddr);
> >      memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac));
> >      n->status = VIRTIO_NET_S_LINK_UP;
> > @@ -3910,7 +3942,13 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> >      nc = qemu_get_queue(n->nic);
> >      nc->rxfilter_notify_enabled = 1;
> >
> > -   if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > +    if (nc->peer && (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA)) {
> > +        if (nc->peer->check_mac) {
> > +            if (!virtio_net_check_vdpa_mac(nc, n, &macaddr_cmdline, errp)) {
> > +                virtio_cleanup(vdev);
> > +                return;
> > +            }
> > +        }
> >          struct virtio_net_config netcfg = {};
> >          memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
> >          vhost_net_set_config(get_vhost_net(nc->peer),
> > --
> > 2.45.0
>
> Thanks
>
> >
>

Re: [PATCH v6 1/4] vhost_vdpa : Add a new parameter to enable check mac address

[Cindy Lu]

On Tue, Apr 8, 2025 at 2:38 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Apr 8, 2025 at 2:13 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > When using a VDPA device, it's important to ensure that the MAC
> > address is correctly set.
> > Add a new parameter in qemu cmdline to enable this check, default value
> > is false
> >
> > The usage is:
> > ....
> > -netdev type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-0,id=vhost-vdpa0,check-mac=true\
> > -device virtio-net-pci,netdev=vhost-vdpa0\
> > ....
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  include/net/net.h | 1 +
> >  net/vhost-vdpa.c  | 4 ++++
> >  qapi/net.json     | 5 +++++
> >  3 files changed, 10 insertions(+)
> >
> > diff --git a/include/net/net.h b/include/net/net.h
> > index cdd5b109b0..fac1951b6e 100644
> > --- a/include/net/net.h
> > +++ b/include/net/net.h
> > @@ -112,6 +112,7 @@ struct NetClientState {
> >      bool is_netdev;
> >      bool do_not_pad; /* do not pad to the minimum ethernet frame length */
> >      bool is_datapath;
> > +    bool check_mac;
> >      QTAILQ_HEAD(, NetFilterState) filters;
> >  };
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 7ca8b46eee..ba1da31741 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -1870,6 +1870,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> >                                       iova_range, features, shared, errp);
> >          if (!ncs[i])
> >              goto err;
> > +
> > +        ncs[i]->check_mac = opts->check_mac;
> >      }
> >
> >      if (has_cvq) {
> > @@ -1882,6 +1884,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> >                                   errp);
> >          if (!nc)
> >              goto err;
> > +
> > +        nc->check_mac = opts->check_mac;
>
> Unless we need to iterate all the ncs during the startup, I think it
> would be sufficient to store it in nc[0]?
>
sure, will fix this
> >      }
> >
> >      return 0;
> > diff --git a/qapi/net.json b/qapi/net.json
> > index 310cc4fd19..a5c70d1df8 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -510,6 +510,10 @@
> >  # @queues: number of queues to be created for multiqueue vhost-vdpa
> >  #     (default: 1)
> >  #
> > +# @check-mac: Enable the check for whether the device's MAC address
> > +#     and the MAC in QEMU command line are acceptable for booting.
> > +#     (default: false)
> > +#
> >  # @x-svq: Start device with (experimental) shadow virtqueue.  (Since
> >  #     7.1) (default: false)
> >  #
> > @@ -524,6 +528,7 @@
> >      '*vhostdev':     'str',
> >      '*vhostfd':      'str',
> >      '*queues':       'int',
> > +    '*check-mac':    'bool',
>
> To make this more useful, we probably need to make it true by default
> and do the compatibility work. Btw, while at it, do we need to check
> MTU as well?
>
> Thanks
>
Sure, will change this,
but there is another issue, the kernel and vdpa tool don't support MTU yet.
Should we add an MTU check later?
Thanks

cindy

> >      '*x-svq':        {'type': 'bool', 'features' : [ 'unstable'] } } }
> >
> >  ##
> > --
> > 2.45.0
> >
>