| Message ID | 20240709022707.579474-1-lulu@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] virtio-net: check the mac address for vdpa device | expand |
On Tue, Jul 9, 2024 at 10:27 AM Cindy Lu <lulu@redhat.com> wrote: > > When using VDPA device, we should verify whether the MAC address in the > hardware matches the MAC address from the QEMU command line. If not, > we will need to update the related information. > > Signed-off-by: Cindy Lu <lulu@redhat.com> This seems to be a workaround, for example it means the mac address set from the qemu command line has the higher priority compared to the one that is provisioned by the host? At least we need to have a warning here? Thanks > --- > hw/net/virtio-net.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 9c7e85caea..db04331b82 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -3739,10 +3739,17 @@ 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); > + struct virtio_net_config netcfg = {}; > + static const MACAddr zero = {.a = {0, 0, 0, 0, 0, 0}}; > + vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > + ETH_ALEN); > + if ((memcmp(&netcfg.mac, &n->nic_conf.macaddr, sizeof(MACAddr)) != 0) && > + memcmp(&netcfg.mac, &zero, sizeof(zero) != 0)) { > + memcpy(&n->nic_conf.macaddr, &netcfg.mac, sizeof(MACAddr)); > + memcpy(&n->mac[0], &netcfg.mac, sizeof(MACAddr)); > + } > + vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, 0, > + ETH_ALEN, VHOST_SET_CONFIG_TYPE_FRONTEND); > } > QTAILQ_INIT(&n->rsc_chains); > n->qdev = dev; > -- > 2.45.0 >
On Tue, 9 Jul 2024 at 10:33, Jason Wang <jasowang@redhat.com> wrote: > > On Tue, Jul 9, 2024 at 10:27 AM Cindy Lu <lulu@redhat.com> wrote: > > > > When using VDPA device, we should verify whether the MAC address in the > > hardware matches the MAC address from the QEMU command line. If not, > > we will need to update the related information. > > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > This seems to be a workaround, for example it means the mac address > set from the qemu command line has the higher priority compared to the > one that is provisioned by the host? > > At least we need to have a warning here? > In this design, the MAC address from the host will take higher priority over the MAC address from the command line. I'm not sure if this is acceptable? sure, will add some warning here > Thanks > > > --- > > hw/net/virtio-net.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index 9c7e85caea..db04331b82 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -3739,10 +3739,17 @@ 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); > > + struct virtio_net_config netcfg = {}; > > + static const MACAddr zero = {.a = {0, 0, 0, 0, 0, 0}}; > > + vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > > + ETH_ALEN); > > + if ((memcmp(&netcfg.mac, &n->nic_conf.macaddr, sizeof(MACAddr)) != 0) && > > + memcmp(&netcfg.mac, &zero, sizeof(zero) != 0)) { > > + memcpy(&n->nic_conf.macaddr, &netcfg.mac, sizeof(MACAddr)); > > + memcpy(&n->mac[0], &netcfg.mac, sizeof(MACAddr)); > > + } > > + vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, 0, > > + ETH_ALEN, VHOST_SET_CONFIG_TYPE_FRONTEND); > > } > > QTAILQ_INIT(&n->rsc_chains); > > n->qdev = dev; > > -- > > 2.45.0 > > >
On Tue, Jul 9, 2024 at 10:41 AM Cindy Lu <lulu@redhat.com> wrote: > > On Tue, 9 Jul 2024 at 10:33, Jason Wang <jasowang@redhat.com> wrote: > > > > On Tue, Jul 9, 2024 at 10:27 AM Cindy Lu <lulu@redhat.com> wrote: > > > > > > When using VDPA device, we should verify whether the MAC address in the > > > hardware matches the MAC address from the QEMU command line. If not, > > > we will need to update the related information. > > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > > > This seems to be a workaround, for example it means the mac address > > set from the qemu command line has the higher priority compared to the > > one that is provisioned by the host? > > > > At least we need to have a warning here? > > > In this design, the MAC address from the host will take higher > priority over the MAC address from the command line. I'm not sure if > this is acceptable? That's fine but it seems not what I read for this patch? As you try to set config, or anything I missed here? Thanks > sure, will add some warning here > > Thanks > > > > > --- > > > hw/net/virtio-net.c | 15 +++++++++++---- > > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > > index 9c7e85caea..db04331b82 100644 > > > --- a/hw/net/virtio-net.c > > > +++ b/hw/net/virtio-net.c > > > @@ -3739,10 +3739,17 @@ 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); > > > + struct virtio_net_config netcfg = {}; > > > + static const MACAddr zero = {.a = {0, 0, 0, 0, 0, 0}}; > > > + vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > > > + ETH_ALEN); > > > + if ((memcmp(&netcfg.mac, &n->nic_conf.macaddr, sizeof(MACAddr)) != 0) && > > > + memcmp(&netcfg.mac, &zero, sizeof(zero) != 0)) { > > > + memcpy(&n->nic_conf.macaddr, &netcfg.mac, sizeof(MACAddr)); > > > + memcpy(&n->mac[0], &netcfg.mac, sizeof(MACAddr)); > > > + } > > > + vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, 0, > > > + ETH_ALEN, VHOST_SET_CONFIG_TYPE_FRONTEND); > > > } > > > QTAILQ_INIT(&n->rsc_chains); > > > n->qdev = dev; > > > -- > > > 2.45.0 > > > > > >
On Tue, 9 Jul 2024 at 10:47, Jason Wang <jasowang@redhat.com> wrote: > > On Tue, Jul 9, 2024 at 10:41 AM Cindy Lu <lulu@redhat.com> wrote: > > > > On Tue, 9 Jul 2024 at 10:33, Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Tue, Jul 9, 2024 at 10:27 AM Cindy Lu <lulu@redhat.com> wrote: > > > > > > > > When using VDPA device, we should verify whether the MAC address in the > > > > hardware matches the MAC address from the QEMU command line. If not, > > > > we will need to update the related information. > > > > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > > > > > This seems to be a workaround, for example it means the mac address > > > set from the qemu command line has the higher priority compared to the > > > one that is provisioned by the host? > > > > > > At least we need to have a warning here? > > > > > In this design, the MAC address from the host will take higher > > priority over the MAC address from the command line. I'm not sure if > > this is acceptable? > > That's fine but it seems not what I read for this patch? > > As you try to set config, or anything I missed here? > > Thanks > yes I use vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, ETH_ALEN); to get the mac from hardware and copy it to n->nic_conf.macaddr and n->mac memcpy(&n->nic_conf.macaddr, &netcfg.mac, sizeof(MACAddr)); memcpy(&n->mac[0], &netcfg.mac, sizeof(MACAddr)); qemu will use this information for the following steps Thanks cindy > > sure, will add some warning here > > > Thanks > > > > > > > --- > > > > hw/net/virtio-net.c | 15 +++++++++++---- > > > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > > > index 9c7e85caea..db04331b82 100644 > > > > --- a/hw/net/virtio-net.c > > > > +++ b/hw/net/virtio-net.c > > > > @@ -3739,10 +3739,17 @@ 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); > > > > + struct virtio_net_config netcfg = {}; > > > > + static const MACAddr zero = {.a = {0, 0, 0, 0, 0, 0}}; > > > > + vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > > > > + ETH_ALEN); > > > > + if ((memcmp(&netcfg.mac, &n->nic_conf.macaddr, sizeof(MACAddr)) != 0) && > > > > + memcmp(&netcfg.mac, &zero, sizeof(zero) != 0)) { > > > > + memcpy(&n->nic_conf.macaddr, &netcfg.mac, sizeof(MACAddr)); > > > > + memcpy(&n->mac[0], &netcfg.mac, sizeof(MACAddr)); > > > > + } > > > > + vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, 0, > > > > + ETH_ALEN, VHOST_SET_CONFIG_TYPE_FRONTEND); > > > > } > > > > QTAILQ_INIT(&n->rsc_chains); > > > > n->qdev = dev; > > > > -- > > > > 2.45.0 > > > > > > > > > >
On Tue, Jul 9, 2024 at 10:56 AM Cindy Lu <lulu@redhat.com> wrote: > > On Tue, 9 Jul 2024 at 10:47, Jason Wang <jasowang@redhat.com> wrote: > > > > On Tue, Jul 9, 2024 at 10:41 AM Cindy Lu <lulu@redhat.com> wrote: > > > > > > On Tue, 9 Jul 2024 at 10:33, Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Tue, Jul 9, 2024 at 10:27 AM Cindy Lu <lulu@redhat.com> wrote: > > > > > > > > > > When using VDPA device, we should verify whether the MAC address in the > > > > > hardware matches the MAC address from the QEMU command line. If not, > > > > > we will need to update the related information. > > > > > > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > > > > > > > This seems to be a workaround, for example it means the mac address > > > > set from the qemu command line has the higher priority compared to the > > > > one that is provisioned by the host? > > > > > > > > At least we need to have a warning here? > > > > > > > In this design, the MAC address from the host will take higher > > > priority over the MAC address from the command line. I'm not sure if > > > this is acceptable? > > > > That's fine but it seems not what I read for this patch? > > > > As you try to set config, or anything I missed here? > > > > Thanks > > > yes I use vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > ETH_ALEN); to get the mac from hardware and copy it > to n->nic_conf.macaddr and n->mac > memcpy(&n->nic_conf.macaddr, &netcfg.mac, sizeof(MACAddr)); > memcpy(&n->mac[0], &netcfg.mac, sizeof(MACAddr)); > qemu will use this information for the following steps I think we need to explain: 1) why do we need a get_config in realization, or what happens if we don't do that. (AFAIK we will call get_config when a guest is trying to read the config space). 2) why do we need a set_config, what happens if we don't do that. Thanks > > Thanks > cindy > > > > sure, will add some warning here > > > > Thanks > > > > > > > > > --- > > > > > hw/net/virtio-net.c | 15 +++++++++++---- > > > > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > > > > index 9c7e85caea..db04331b82 100644 > > > > > --- a/hw/net/virtio-net.c > > > > > +++ b/hw/net/virtio-net.c > > > > > @@ -3739,10 +3739,17 @@ 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); > > > > > + struct virtio_net_config netcfg = {}; > > > > > + static const MACAddr zero = {.a = {0, 0, 0, 0, 0, 0}}; > > > > > + vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > > > > > + ETH_ALEN); > > > > > + if ((memcmp(&netcfg.mac, &n->nic_conf.macaddr, sizeof(MACAddr)) != 0) && > > > > > + memcmp(&netcfg.mac, &zero, sizeof(zero) != 0)) { > > > > > + memcpy(&n->nic_conf.macaddr, &netcfg.mac, sizeof(MACAddr)); > > > > > + memcpy(&n->mac[0], &netcfg.mac, sizeof(MACAddr)); > > > > > + } > > > > > + vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, 0, > > > > > + ETH_ALEN, VHOST_SET_CONFIG_TYPE_FRONTEND); > > > > > } > > > > > QTAILQ_INIT(&n->rsc_chains); > > > > > n->qdev = dev; > > > > > -- > > > > > 2.45.0 > > > > > > > > > > > > > > >
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 9c7e85caea..db04331b82 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -3739,10 +3739,17 @@ 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); + struct virtio_net_config netcfg = {}; + static const MACAddr zero = {.a = {0, 0, 0, 0, 0, 0}}; + vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, + ETH_ALEN); + if ((memcmp(&netcfg.mac, &n->nic_conf.macaddr, sizeof(MACAddr)) != 0) && + memcmp(&netcfg.mac, &zero, sizeof(zero) != 0)) { + memcpy(&n->nic_conf.macaddr, &netcfg.mac, sizeof(MACAddr)); + memcpy(&n->mac[0], &netcfg.mac, sizeof(MACAddr)); + } + vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, 0, + ETH_ALEN, VHOST_SET_CONFIG_TYPE_FRONTEND); } QTAILQ_INIT(&n->rsc_chains); n->qdev = dev;
When using VDPA device, we should verify whether the MAC address in the hardware matches the MAC address from the QEMU command line. If not, we will need to update the related information. Signed-off-by: Cindy Lu <lulu@redhat.com> --- hw/net/virtio-net.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)