| Message ID | 20210929065215.21549-1-lulu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-net : Add check for VIRTIO_NET_F_MAC | expand |
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
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 >
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 > >
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 > > > >
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
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;
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(-)