On Wed, 7 Aug 2024 at 10:36, Jason Wang <jasow...@redhat.com> wrote:
> On Tue, Aug 6, 2024 at 5:44 PM Cindy Lu <l...@redhat.com> wrote:
> >
> > On Tue, 6 Aug 2024 at 11:07, Jason Wang <jasow...@redhat.com> wrote:
> > >
> > > On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <l...@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 <l...@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?
> > >
> > yes, there is a section 2, will change this code here
> > Thanks
> > cindy
> > > > + */
> > > > +
> > > > + 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
> > >
> > as we discussed before， the MAC address in hardware should have a
> > "higher priority"
> > than the MAC address in qemu cmdline. So I remove the set_config there,
> > the MAC address from the hardware will overwrite the MAC in qemu
> > cmdline. so don't need to set_config to hardware now
>
> Probably, but I meant it needs to be a separate patch.
>
> For example the title said "add ...." but here it's a removal of something.
>
> Thanks
>
> sure will fix this
Thanks
cindy
> > Thanks,
> > cindy
> > > > }
> > > > QTAILQ_INIT(&n->rsc_chains);
> > > > n->qdev = dev;
> > > > --
> > > > 2.45.0
> > > >
> > >
> >
>
>
