qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] virtio-net : Add check for VIRTIO_NET_F_MAC
@ 2021-09-29  6:52 Cindy Lu
  2021-09-29 10:01 ` Michael Tokarev
  0 siblings, 1 reply; 6+ messages in thread
From: Cindy Lu @ 2021-09-29  6:52 UTC (permalink / raw)
  To: lulu, mst, jasowang, qemu-devel; +Cc: qemu-stable

For vdpa device, if the host support VIRTIO_NET_F_MAC
we need to read the mac address from hardware, so need
to check this bit, the logic is
1 if the host support VIRTIO_NET_F_MAC and the mac address
   is correct, qemu will use the mac address in hardware
2.if the host not support , qemu will use the mac from cmdline
3.if the cmdline not provide mac address, qemu will use radam mac
address

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 hw/net/virtio-net.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 55aac06a0a..085daa28b0 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -127,9 +127,9 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     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));
+
+    memset(&netcfg, 0, sizeof(struct virtio_net_config));
     virtio_stw_p(vdev, &netcfg.status, n->status);
     virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
     virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
@@ -159,12 +159,21 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
              * address has been configured 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);
             }
-            memcpy(config, &netcfg, n->config_size);
+            /*
+             * If the host support VIRTIO_NET_F_MAC, That means hardware
+             * will provide the mac address, otherwise we don't need to use it.
+             * use  the mac address from qemu cfg
+             */
+            if (!(virtio_host_has_feature(vdev, VIRTIO_NET_F_MAC))) {
+                memcpy(netcfg.mac, n->mac, ETH_ALEN);
+            }
         }
+        memcpy(config, &netcfg, n->config_size);
     }
 }
 
@@ -3453,11 +3462,22 @@ 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) {
+
         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_MASTER);
+        /*
+         * get the real mac address from hardware or qemu cmline
+         *
+         * If the host support VIRTIO_NET_F_MAC, That means hardware
+         * will provide the mac address, otherwise we don't need to use it.
+         * use  the mac address from qemu cfg
+         */
+        virtio_net_get_config(vdev, (uint8_t *)&netcfg);
+        /*
+         * for vdpa device qemu need to set the real mac back to hardware
+         */
+        vhost_net_set_config(get_vhost_net(nc->peer),  (uint8_t *)&netcfg, 0,
+                             ETH_ALEN, VHOST_SET_CONFIG_TYPE_MASTER);
     }
     QTAILQ_INIT(&n->rsc_chains);
     n->qdev = dev;
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] virtio-net : Add check for VIRTIO_NET_F_MAC
  2021-09-29  6:52 [PATCH] virtio-net : Add check for VIRTIO_NET_F_MAC Cindy Lu
@ 2021-09-29 10:01 ` Michael Tokarev
  2021-09-29 12:08   ` Cindy Lu
  0 siblings, 1 reply; 6+ messages in thread
From: Michael Tokarev @ 2021-09-29 10:01 UTC (permalink / raw)
  To: Cindy Lu, mst, jasowang, qemu-devel; +Cc: qemu-stable

29.09.2021 09:52, Cindy Lu wrote:
> For vdpa device, if the host support VIRTIO_NET_F_MAC
> we need to read the mac address from hardware, so need
> to check this bit, the logic is
> 1 if the host support VIRTIO_NET_F_MAC and the mac address
>     is correct, qemu will use the mac address in hardware
> 2.if the host not support , qemu will use the mac from cmdline

So if hw supports NET_F_MAC, cmdline-provided parameter will
silently be ignored?

s/host not support/host does not support this feature/

> 3.if the cmdline not provide mac address, qemu will use radam mac
> address

s/not/does not/
s/radam/random/

Thanks,

/mjt


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] virtio-net : Add check for VIRTIO_NET_F_MAC
  2021-09-29 10:01 ` Michael Tokarev
@ 2021-09-29 12:08   ` Cindy Lu
  2021-09-29 13:36     ` Michael S. Tsirkin
  0 siblings, 1 reply; 6+ messages in thread
From: Cindy Lu @ 2021-09-29 12:08 UTC (permalink / raw)
  To: mjt; +Cc: Jason Wang, qemu-stable, QEMU Developers, Michael Tsirkin

On Wed, Sep 29, 2021 at 6:07 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 29.09.2021 09:52, Cindy Lu wrote:
> > For vdpa device, if the host support VIRTIO_NET_F_MAC
> > we need to read the mac address from hardware, so need
> > to check this bit, the logic is
> > 1 if the host support VIRTIO_NET_F_MAC and the mac address
> >     is correct, qemu will use the mac address in hardware
> > 2.if the host not support , qemu will use the mac from cmdline
>
> So if hw supports NET_F_MAC, cmdline-provided parameter will
> silently be ignored?
>
yes, this is based on the virtio spec, you can check this document in
5.1.5 Device Initialization
https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html

Also, this check it only working for vdpa device
> s/host not support/host does not support this feature/
Thanks , will fix this
>
> > 3.if the cmdline not provide mac address, qemu will use radam mac
> > address
>
> s/not/does not/
> s/radam/random/
>
thanks, will fix this
> Thanks,
>
> /mjt
>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] virtio-net : Add check for VIRTIO_NET_F_MAC
  2021-09-29 12:08   ` Cindy Lu
@ 2021-09-29 13:36     ` Michael S. Tsirkin
  2021-09-30  1:35       ` Cindy Lu
  0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2021-09-29 13:36 UTC (permalink / raw)
  To: Cindy Lu; +Cc: Jason Wang, mjt, QEMU Developers, qemu-stable

On Wed, Sep 29, 2021 at 08:08:40PM +0800, Cindy Lu wrote:
> On Wed, Sep 29, 2021 at 6:07 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
> >
> > 29.09.2021 09:52, Cindy Lu wrote:
> > > For vdpa device, if the host support VIRTIO_NET_F_MAC
> > > we need to read the mac address from hardware, so need
> > > to check this bit, the logic is
> > > 1 if the host support VIRTIO_NET_F_MAC and the mac address
> > >     is correct, qemu will use the mac address in hardware
> > > 2.if the host not support , qemu will use the mac from cmdline
> >
> > So if hw supports NET_F_MAC, cmdline-provided parameter will
> > silently be ignored?
> >
> yes, this is based on the virtio spec, you can check this document in
> 5.1.5 Device Initialization
> https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html

Maybe use the hw mac if mac is not provided? If provided
make sure the command line matches the hardware, and fail
otherwise?

> Also, this check it only working for vdpa device
> > s/host not support/host does not support this feature/
> Thanks , will fix this
> >
> > > 3.if the cmdline not provide mac address, qemu will use radam mac
> > > address
> >
> > s/not/does not/
> > s/radam/random/
> >
> thanks, will fix this
> > Thanks,
> >
> > /mjt
> >



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] virtio-net : Add check for VIRTIO_NET_F_MAC
  2021-09-29 13:36     ` Michael S. Tsirkin
@ 2021-09-30  1:35       ` Cindy Lu
  2021-09-30 10:46         ` Michael Tokarev
  0 siblings, 1 reply; 6+ messages in thread
From: Cindy Lu @ 2021-09-30  1:35 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Jason Wang, mjt, QEMU Developers, qemu-stable

On Wed, Sep 29, 2021 at 9:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Sep 29, 2021 at 08:08:40PM +0800, Cindy Lu wrote:
> > On Wed, Sep 29, 2021 at 6:07 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
> > >
> > > 29.09.2021 09:52, Cindy Lu wrote:
> > > > For vdpa device, if the host support VIRTIO_NET_F_MAC
> > > > we need to read the mac address from hardware, so need
> > > > to check this bit, the logic is
> > > > 1 if the host support VIRTIO_NET_F_MAC and the mac address
> > > >     is correct, qemu will use the mac address in hardware
> > > > 2.if the host not support , qemu will use the mac from cmdline
> > >
> > > So if hw supports NET_F_MAC, cmdline-provided parameter will
> > > silently be ignored?
> > >
> > yes, this is based on the virtio spec, you can check this document in
> > 5.1.5 Device Initialization
> > https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
>
> Maybe use the hw mac if mac is not provided? If provided
> make sure the command line matches the hardware, and fail
> otherwise?
>

so here come to the final question. which mac address has the higher priority?
I think the NET_F_MAC bit means the hw mac address > command-line address.
if the hw drivers want to change this. they can simply remove this bit.


> > Also, this check it only working for vdpa device
> > > s/host not support/host does not support this feature/
> > Thanks , will fix this
> > >
> > > > 3.if the cmdline not provide mac address, qemu will use radam mac
> > > > address
> > >
> > > s/not/does not/
> > > s/radam/random/
> > >
> > thanks, will fix this
> > > Thanks,
> > >
> > > /mjt
> > >
>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] virtio-net : Add check for VIRTIO_NET_F_MAC
  2021-09-30  1:35       ` Cindy Lu
@ 2021-09-30 10:46         ` Michael Tokarev
  0 siblings, 0 replies; 6+ messages in thread
From: Michael Tokarev @ 2021-09-30 10:46 UTC (permalink / raw)
  To: Cindy Lu, Michael S. Tsirkin; +Cc: Jason Wang, QEMU Developers, qemu-stable

30.09.2021 04:35, Cindy Lu wrote:
[]
> so here come to the final question. which mac address has the higher priority?
> I think the NET_F_MAC bit means the hw mac address > command-line address.
> if the hw drivers want to change this. they can simply remove this bit.

At the very least, qemu should NEVER silently ignore stuff specified
on the command line. It should either warn the user or error out if
command-line parameter can not be applied for whatever reason.

I don't know the context, what NET_F_MAC is, but the general rule is
this: if the user specified something, it must not be ignored.

/mjt


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-30 10:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-29  6:52 [PATCH] virtio-net : Add check for VIRTIO_NET_F_MAC Cindy Lu
2021-09-29 10:01 ` Michael Tokarev
2021-09-29 12:08   ` Cindy Lu
2021-09-29 13:36     ` Michael S. Tsirkin
2021-09-30  1:35       ` Cindy Lu
2021-09-30 10:46         ` Michael Tokarev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).