Message-ID: <CACLfguXNyp1iM+FnxVTrLRntcNxhHpfciE=z6nhhBtWYRFSy9w@mail.gmail.com>
Date: Tue, 16 Jul 2024 14:08:56 +0800
From: Cindy Lu <lulu@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: dtatulea@...dia.com, mst@...hat.com, parav@...dia.com, 
	netdev@...r.kernel.org, qemu-devel@...gnu.org
Subject: Re: [RFC v2] virtio-net: check the mac address for vdpa device

On Tue, 16 Jul 2024 at 13:37, Jason Wang <jasowang@...hat.com> wrote:
>
> On Tue, Jul 16, 2024 at 9:14 AM Cindy Lu <lulu@...hat.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.
> >
> > There are only two acceptable situations:
> > 1. The hardware MAC address is the same as the MAC address specified in the QEMU
> > command line, and both MAC addresses are not 0.
> > 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 this patch tries to do the above two, I'd suggest splitting it into
> two patches.
>
This code is very simple. So I have put these two into one function.
thanks
cindy
> >
> > Signed-off-by: Cindy Lu <lulu@...hat.com>
> > ---
> >  hw/net/virtio-net.c | 43 +++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 9c7e85caea..8f79785f59 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -178,8 +178,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> >           * correctly elsewhere - just not reported by the device.
> >           */
> >          if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) {
> > -            info_report("Zero hardware mac address detected. Ignoring.");
> > -            memcpy(netcfg.mac, n->mac, ETH_ALEN);
> > +          error_report("Zero hardware mac address detected in vdpa device. "
> > +                       "please check the vdpa device!");
>
> I had two questions:
>
> 1) any reason to do this check while the guest is running?
> 2) I think we need a workaround for this, unless I miss something.
>
this is a code change to adjust the new fix. If the mac address is 0
the guest should fail
to load. Maybe I can just assert fail here?
Thanks
cindy
> >          }
> >
> >          netcfg.status |= virtio_tswap16(vdev,
> > @@ -3579,12 +3579,42 @@ 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.
> > +   * 2.The hardware MAC address is NOT 0, and the QEMU command line MAC address
> > +   *   is 0.
>
> Did you mean -device virtio-net-pci,macaddr=0 ? Or you mean mac
> address is not specified in the qemu command line?
>
yes, what I mean is mac address not specified, sorry for the confusion,
I will rewrite this part
Thanks
cindy

> > In this situation, the hardware MAC address will overwrite the QEMU
> > +   *   command line address.
> > +   */
> > +
> > +  if (memcmp(&hwcfg.mac, &zero, sizeof(MACAddr)) != 0) {
> > +    if ((memcmp(&hwcfg.mac, cmdline_mac, sizeof(MACAddr)) == 0) ||
> > +        (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 hardware mac != the mac address from "
> > +                   "qemu cmdline, please check the the vdpa device's setting.");
> > +
> > +  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 +3722,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 +3770,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
> >
>
> Thanks
>

Powered by blists - more mailing lists