diff mbox series

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

Message ID 20250408061327.2590372-3-lulu@redhat.com (mailing list archive)
State New
Headers show
Series virtio_net: Add the check for vdpa's mac address | expand

Commit Message

Cindy Lu April 8, 2025, 6:12 a.m. UTC 
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(-)

Comments

Jason Wang April 8, 2025, 6:45 a.m. UTC | #1 
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

>
Cindy Lu April 9, 2025, 7:04 a.m. UTC | #2 
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
>
> >
>
diff mbox series

Patch

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),