On Tue, Apr 8, 2025 at 2:46 PM Jason Wang <jasow...@redhat.com> wrote:
>
> On Tue, Apr 8, 2025 at 2:13 PM Cindy Lu <l...@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 <l...@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
>
> >
>
