| Message ID | 20210225165506.18321-2-lulu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-net: Add check for mac address while peer is vdpa | expand |
On Fri, Feb 26, 2021 at 12:55:06AM +0800, Cindy Lu wrote: > While peer is vdpa, sometime qemu get an all zero mac address from the hardware, > This is not a legal value. Add the check for this.if we get an zero mac address. > qemu will use the default mac address or the mac address from qemu cmdline > > Signed-off-by: Cindy Lu <lulu@redhat.com> I guess I will have to rewrite the comments and commit log :( It is all saying what does the patch do. We want it to rather give motivation. Sean could you please comment on whether this patch fixes your config? > --- > hw/net/virtio-net.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 9179013ac4..8f36ca5066 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > VirtIONet *n = VIRTIO_NET(vdev); > struct virtio_net_config netcfg; > NetClientState *nc = qemu_get_queue(n->nic); > + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > > int ret = 0; > memset(&netcfg, 0 , sizeof(struct virtio_net_config)); > @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > n->config_size); > if (ret != -1) { > + /* > + * Here is a work around, the 0 mac address is not a legal value. > + * if we got this from hardware, qemu will use the mac address > + * saved in VirtIONet->mac. > + */ > + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) { > + info_report("Get an all zero mac address from hardware"); > + memcpy(netcfg.mac, n->mac, ETH_ALEN); > + } > memcpy(config, &netcfg, n->config_size); > } > } > -- > 2.21.3
On Thu, Feb 25, 2021 at 02:14:39PM -0500, Michael S. Tsirkin wrote: > On Fri, Feb 26, 2021 at 12:55:06AM +0800, Cindy Lu wrote: > > While peer is vdpa, sometime qemu get an all zero mac address from the hardware, > > This is not a legal value. Add the check for this.if we get an zero mac address. > > qemu will use the default mac address or the mac address from qemu cmdline > > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > I guess I will have to rewrite the comments and commit log :( > > It is all saying what does the patch do. We want it to rather > give motivation. > > Sean could you please comment on whether this patch fixes your > config? ping. if I'm to try and merge this work around it's critical that someone with access to hardware confirm it actually works. > > --- > > hw/net/virtio-net.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index 9179013ac4..8f36ca5066 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > > VirtIONet *n = VIRTIO_NET(vdev); > > struct virtio_net_config netcfg; > > NetClientState *nc = qemu_get_queue(n->nic); > > + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > > > > int ret = 0; > > memset(&netcfg, 0 , sizeof(struct virtio_net_config)); > > @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > > ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > > n->config_size); > > if (ret != -1) { > > + /* > > + * Here is a work around, the 0 mac address is not a legal value. > > + * if we got this from hardware, qemu will use the mac address > > + * saved in VirtIONet->mac. > > + */ > > + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) { > > + info_report("Get an all zero mac address from hardware"); > > + memcpy(netcfg.mac, n->mac, ETH_ALEN); > > + } > > memcpy(config, &netcfg, n->config_size); > > } > > } > > -- > > 2.21.3
On Mon, Mar 1, 2021 at 4:40 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Feb 25, 2021 at 02:14:39PM -0500, Michael S. Tsirkin wrote: > > On Fri, Feb 26, 2021 at 12:55:06AM +0800, Cindy Lu wrote: > > > While peer is vdpa, sometime qemu get an all zero mac address from the hardware, > > > This is not a legal value. Add the check for this.if we get an zero mac address. > > > qemu will use the default mac address or the mac address from qemu cmdline > > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > > > I guess I will have to rewrite the comments and commit log :( > > > > It is all saying what does the patch do. We want it to rather > > give motivation. > > > > Sean could you please comment on whether this patch fixes your > > config? > > ping. if I'm to try and merge this work around it's critical > that someone with access to hardware confirm it actually works. > Hi Michael, I have tested this patch in qemu+vdpa+mlx environment. it's working fine. > > > > --- > > > hw/net/virtio-net.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > > index 9179013ac4..8f36ca5066 100644 > > > --- a/hw/net/virtio-net.c > > > +++ b/hw/net/virtio-net.c > > > @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > > > VirtIONet *n = VIRTIO_NET(vdev); > > > struct virtio_net_config netcfg; > > > NetClientState *nc = qemu_get_queue(n->nic); > > > + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > > > > > > int ret = 0; > > > memset(&netcfg, 0 , sizeof(struct virtio_net_config)); > > > @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > > > ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > > > n->config_size); > > > if (ret != -1) { > > > + /* > > > + * Here is a work around, the 0 mac address is not a legal value. > > > + * if we got this from hardware, qemu will use the mac address > > > + * saved in VirtIONet->mac. > > > + */ > > > + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) { > > > + info_report("Get an all zero mac address from hardware"); > > > + memcpy(netcfg.mac, n->mac, ETH_ALEN); > > > + } > > > memcpy(config, &netcfg, n->config_size); > > > } > > > } > > > -- > > > 2.21.3 >
On 3/1/21 2:36 AM, Cindy Lu wrote: > On Mon, Mar 1, 2021 at 4:40 AM Michael S. Tsirkin <mst@redhat.com> wrote: >> >> On Thu, Feb 25, 2021 at 02:14:39PM -0500, Michael S. Tsirkin wrote: >>> On Fri, Feb 26, 2021 at 12:55:06AM +0800, Cindy Lu wrote: >>>> While peer is vdpa, sometime qemu get an all zero mac address from the hardware, >>>> This is not a legal value. Add the check for this.if we get an zero mac address. >>>> qemu will use the default mac address or the mac address from qemu cmdline >>>> >>>> Signed-off-by: Cindy Lu <lulu@redhat.com> >>> >>> I guess I will have to rewrite the comments and commit log :( >>> >>> It is all saying what does the patch do. We want it to rather >>> give motivation. >>> >>> Sean could you please comment on whether this patch fixes your >>> config? >> >> ping. if I'm to try and merge this work around it's critical >> that someone with access to hardware confirm it actually works. >> > Hi Michael, I have tested this patch in qemu+vdpa+mlx environment. > it's working fine. I have also verified it. For the record, I'll expand: Libvirt definition contains: <interface type='vdpa'> <mac address='52:54:00:8e:a4:12'/> <source dev='/dev/vhost-vdpa-0'/> <model type='virtio'/> <alias name='net1'/> <address type='pci' domain='0x0000' bus='0x06' slot='0x00' function='0x0'/> </interface> Qemu command line contains: -add-fd set=3,fd=42,opaque=/dev/vhost-vdpa-0 \ -netdev vhost-vdpa,vhostdev=/dev/fdset/3,id=hostnet1 \ -device virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:8e:a4:12,bus=pci.4,addr=0x0 \ The vdpa device is created by mlx_vdpa (Connect-X6 DX) on switchdev mode. The original problem was: The mac address in struct virtio_net_config in the vdpa device is zero because it has not been configured. This does not have implications in the traffic steering since the card will rely on the eswitch flow configuration to perform smac/dmac filtering (in my case, this is done through ovs + tcflower hw offload). Since we anyhow have to rely on the mac address having been configured externally on the eswitch, we can also trust that whoever applied such flow configuration also gave us the correct mac address in the cmd line. Therefore, exposing such address to the guest seems like a sane way to tell it what mac address to use. Until the vdpa framework / iproute2 tool support configuring the virtio_net_config struct and such mechanism is used externally to align the virtio device's mac with the one configured in the eswitch, this patch allows the guest to have connectivity. >> >>>> --- >>>> hw/net/virtio-net.c | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>>> index 9179013ac4..8f36ca5066 100644 >>>> --- a/hw/net/virtio-net.c >>>> +++ b/hw/net/virtio-net.c >>>> @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) >>>> VirtIONet *n = VIRTIO_NET(vdev); >>>> struct virtio_net_config netcfg; >>>> NetClientState *nc = qemu_get_queue(n->nic); >>>> + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; >>>> >>>> int ret = 0; >>>> memset(&netcfg, 0 , sizeof(struct virtio_net_config)); >>>> @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) >>>> ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, >>>> n->config_size); >>>> if (ret != -1) { >>>> + /* >>>> + * Here is a work around, the 0 mac address is not a legal value. >>>> + * if we got this from hardware, qemu will use the mac address >>>> + * saved in VirtIONet->mac. >>>> + */ >>>> + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) { >>>> + info_report("Get an all zero mac address from hardware"); s/Get/Got/? >>>> + memcpy(netcfg.mac, n->mac, ETH_ALEN); >>>> + } >>>> memcpy(config, &netcfg, n->config_size); >>>> } >>>> } >>>> -- >>>> 2.21.3 >> > FWIW: Tested-by: Adrián Moreno <amorenoz@redhat.com>
On Thu, 2021-02-25 at 14:14 -0500, Michael S. Tsirkin wrote: > On Fri, Feb 26, 2021 at 12:55:06AM +0800, Cindy Lu wrote: > > While peer is vdpa, sometime qemu get an all zero mac address from the hardware, > > This is not a legal value. Add the check for this.if we get an zero mac address. > > qemu will use the default mac address or the mac address from qemu cmdline > > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > I guess I will have to rewrite the comments and commit log :( > > It is all saying what does the patch do. We want it to rather > give motivation. > > Sean could you please comment on whether this patch fixes your > config? sorry this came in just as i was finsihing up for the day and firday was a company day. i just applied the patch on my dev env. when i do the mac is set to the value i set on the qemu command line. i am able to bring up the interface in the guest and i can successfully ping the host so setting only the mac seams to be sufficent to allow the vdpa interface to be used in a guest. i tested using an unmodifed alpine linux image for the guest and appied the patch to qemu commit e90ef02389dc8b57eaea22b290244609d720a8bf which was the had of master when i cloned qemu a few days ago. regards sean > > > --- > > hw/net/virtio-net.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index 9179013ac4..8f36ca5066 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > > VirtIONet *n = VIRTIO_NET(vdev); > > struct virtio_net_config netcfg; > > NetClientState *nc = qemu_get_queue(n->nic); > > + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > > > > > > > > > > int ret = 0; > > memset(&netcfg, 0 , sizeof(struct virtio_net_config)); > > @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > > ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > > n->config_size); > > if (ret != -1) { > > + /* > > + * Here is a work around, the 0 mac address is not a legal value. > > + * if we got this from hardware, qemu will use the mac address > > + * saved in VirtIONet->mac. > > + */ > > + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) { > > + info_report("Get an all zero mac address from hardware"); > > + memcpy(netcfg.mac, n->mac, ETH_ALEN); > > + } > > memcpy(config, &netcfg, n->config_size); > > } > > } > > -- > > 2.21.3 >
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 9179013ac4..8f36ca5066 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) VirtIONet *n = VIRTIO_NET(vdev); struct virtio_net_config netcfg; NetClientState *nc = qemu_get_queue(n->nic); + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; int ret = 0; memset(&netcfg, 0 , sizeof(struct virtio_net_config)); @@ -151,6 +152,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, n->config_size); if (ret != -1) { + /* + * Here is a work around, the 0 mac address is not a legal value. + * if we got this from hardware, qemu will use the mac address + * saved in VirtIONet->mac. + */ + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) == 0) { + info_report("Get an all zero mac address from hardware"); + memcpy(netcfg.mac, n->mac, ETH_ALEN); + } memcpy(config, &netcfg, n->config_size); } }
While peer is vdpa, sometime qemu get an all zero mac address from the hardware, This is not a legal value. Add the check for this.if we get an zero mac address. qemu will use the default mac address or the mac address from qemu cmdline Signed-off-by: Cindy Lu <lulu@redhat.com> --- hw/net/virtio-net.c | 10 ++++++++++ 1 file changed, 10 insertions(+)