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

[PATCH v2 0/3] 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.
There are only three acceptable situations for MAC setup; any other
configuration will fail to boot.

tested by ConnectX-6 Dx device

Cindy Lu (3):
  virtio_net: Add the check for vdpa's mac address
  virtio_net: Add the 2rd acceptable situation for Mac setup.
  virtio_net: Add the 3rd acceptable situation for Mac setup.

 hw/net/virtio-net.c | 63 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 4 deletions(-)

-- 
2.45.0

[PATCH v2 1/3] 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, 36 insertions(+), 4 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index fb84d142ee..7aa7308244 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3589,12 +3589,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 %02x:%02x:%02x:%02x:%02x:%02x"
+               "not same with the cmdline's mac %02x:%02x:%02x:%02x:%02x:%02x,"
+               "Please check.",
+               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) {
@@ -3702,6 +3733,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;
@@ -3749,10 +3781,10 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
     nc->rxfilter_notify_enabled = 1;
 
    if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
-        struct virtio_net_config netcfg = {};
-        memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
-        vhost_net_set_config(get_vhost_net(nc->peer),
-            (uint8_t *)&netcfg, 0, ETH_ALEN, VHOST_SET_CONFIG_TYPE_FRONTEND);
+        if (!virtio_net_check_vdpa_mac(nc, n, &macaddr_cmdline, errp)) {
+            virtio_cleanup(vdev);
+            return;
+       }
     }
     QTAILQ_INIT(&n->rsc_chains);
     n->qdev = dev;
-- 
2.45.0

Re: [PATCH v2 1/3] virtio_net: Add the check for vdpa's mac address

[Jason Wang]

On Mon, Sep 30, 2024 at 12:01 AM 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.

Would there be a case that the system can boot even without the
suggested setting here? Can we even know about that?

I'm asking since if there's one, we break them. At least we can have a
new parameter for strict checking.

Thanks

[PATCH v2 2/3] virtio_net: Add the 2rd acceptable situation for Mac setup.

[Cindy Lu]

When using a VDPA device, the following situations are
also acceptable: the hardware MAC address is not 0,
and the MAC address in the QEMU command line is 0.

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 7aa7308244..5c610d8078 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3607,6 +3607,18 @@ 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, the hardware MAC address will overwrite
+         *  the QEMU command line address.
+         */
+        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,
-- 
2.45.0

Re: [PATCH v2 2/3] virtio_net: Add the 2rd acceptable situation for Mac setup.

[Jason Wang]

On Mon, Sep 30, 2024 at 12:01 AM Cindy Lu <lulu@redhat.com> wrote:
>
> When using a VDPA device, the following situations are
> also acceptable: the hardware MAC address is not 0,
> and the MAC address in the QEMU command line is 0.
>
> 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 7aa7308244..5c610d8078 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3607,6 +3607,18 @@ 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, the hardware MAC address will overwrite
> +         *  the QEMU command line address.

Please explain how such overwrite works. For example, "Function X and
Y will let the hardware mac address overwrite the cli mac"

Thanks

> +         */
> +        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,
> --
> 2.45.0
>

Re: [PATCH v2 2/3] virtio_net: Add the 2rd acceptable situation for Mac setup.

[Cindy Lu]

On Wed, 9 Oct 2024 at 16:30, Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Sep 30, 2024 at 12:01 AM Cindy Lu <lulu@redhat.com> wrote:
> >
> > When using a VDPA device, the following situations are
> > also acceptable: the hardware MAC address is not 0,
> > and the MAC address in the QEMU command line is 0.
> >
> > 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 7aa7308244..5c610d8078 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -3607,6 +3607,18 @@ 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, the hardware MAC address will overwrite
> > +         *  the QEMU command line address.
>
> Please explain how such overwrite works. For example, "Function X and
> Y will let the hardware mac address overwrite the cli mac"
>
sure, will add these
thanks
cindy
> Thanks
>
> > +         */
> > +        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,
> > --
> > 2.45.0
> >
>

[PATCH v2 3/3] virtio_net: Add the 3rd acceptable situation for Mac setup.

[Cindy Lu]

While the hardware MAC address is 0 and the MAC address in
the QEMU command line is also 0, this configuration is
acceptable.

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

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 5c610d8078..668fbed9f2 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3620,6 +3620,17 @@ 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 will use random mac address
+     */
+    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 %02x:%02x:%02x:%02x:%02x:%02x"
-- 
2.45.0

Re: [PATCH v2 3/3] virtio_net: Add the 3rd acceptable situation for Mac setup.

[Jason Wang]

On Mon, Sep 30, 2024 at 12:01 AM Cindy Lu <lulu@redhat.com> wrote:
>
> While the hardware MAC address is 0 and the MAC address in
> the QEMU command line is also 0, this configuration is
> acceptable.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  hw/net/virtio-net.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 5c610d8078..668fbed9f2 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3620,6 +3620,17 @@ 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 will use random mac address

And explain how the device knows such a random mac address or it's
just for a best effort try.

Thanks

> +     */
> +    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 %02x:%02x:%02x:%02x:%02x:%02x"
> --
> 2.45.0
>

Re: [PATCH v2 3/3] virtio_net: Add the 3rd acceptable situation for Mac setup.

[Cindy Lu]

On Wed, 9 Oct 2024 at 16:30, Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Sep 30, 2024 at 12:01 AM Cindy Lu <lulu@redhat.com> wrote:
> >
> > While the hardware MAC address is 0 and the MAC address in
> > the QEMU command line is also 0, this configuration is
> > acceptable.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  hw/net/virtio-net.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 5c610d8078..668fbed9f2 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -3620,6 +3620,17 @@ 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 will use random mac address
>
> And explain how the device knows such a random mac address or it's
> just for a best effort try.
>
> Thanks
>
sure, will add these
Thanks
cindy
> > +     */
> > +    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 %02x:%02x:%02x:%02x:%02x:%02x"
> > --
> > 2.45.0
> >
>