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

[Jason Wang]

On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <lulu@redhat.com> wrote:
>
> When using a VDPA device, it is important to ensure that
> the MAC address in the hardware matches the MAC address
> from the QEMU command line.
> This will allow the device to boot.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  hw/net/virtio-net.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9c7e85caea..7f51bd0dd3 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3579,12 +3579,36 @@ 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: Only two situations are acceptable:
> +     * 1.The hardware MAC address is the same as the QEMU command line MAC
> +     *   address, and both of them are not 0.

I guess there should be a bullet 2?

> +     */
> +
> +       if (memcmp(&hwcfg.mac, &zero, sizeof(MACAddr)) != 0) {
> +               if ((memcmp(&hwcfg.mac, cmdline_mac, sizeof(MACAddr)) == 0)) {
> +                       return true;
> +               }
> +       }
> +       error_setg(errp, "vDPA device's mac != the mac address from qemu 
> cmdline"
> +                        "Please check the the vdpa device's setting.");

For error messages I think it's better to use english instead of "!="
to describe the issue.

>
> +       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) {
> @@ -3692,6 +3716,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;
> @@ -3739,10 +3764,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;
> +          }

Any reason we remove vhost_net_set_config() here? It is not described
in the commit or does it belong to another patch?

Thanks

>      }
>      QTAILQ_INIT(&n->rsc_chains);
>      n->qdev = dev;
> --
> 2.45.0
>