* [PATCH 2/3] virtio_net: Add the check for vdpa mac address
2024-08-06 0:58 [PATCH 1/3] virtio_net: Add the check for vdpa's mac address Cindy Lu
@ 2024-08-06 0:58 ` Cindy Lu
2024-08-06 3:08 ` Jason Wang
2024-08-06 0:58 ` [PATCH 3/3] virtio_net: remove the unnecessary check in get_config Cindy Lu
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Cindy Lu @ 2024-08-06 0:58 UTC (permalink / raw)
To: lulu, mst, jasowang, qemu-devel
When using a VDPA device, this is another acceptable situations
The hardware MAC address is not 0, and the MAC address in the QEMU command line is 0.
This is also acceptable
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
hw/net/virtio-net.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7f51bd0dd3..c144ae2e78 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3592,11 +3592,22 @@ static bool virtio_net_check_vdpa_mac(NetClientState *nc, VirtIONet *n, MACAddr
* 1.The hardware MAC address is the same as the QEMU command line MAC
* address, and both of them are not 0.
*/
-
+ /*
+ * 2.The hardware MAC address is not 0,
+ * and the MAC address in the QEMU command line is 0.
+ * In this situation, the hardware MAC address will overwrite
+ * the QEMU command line address.
+ */
if (memcmp(&hwcfg.mac, &zero, sizeof(MACAddr)) != 0) {
if ((memcmp(&hwcfg.mac, cmdline_mac, sizeof(MACAddr)) == 0)) {
return true;
}
+ if (memcmp(cmdline_mac, &zero, sizeof(MACAddr)) == 0) {
+ /* overwrite the mac address with hardware address*/
+ memcpy(&n->mac[0], &hwcfg.mac, sizeof(n->mac));
+ memcpy(&n->nic_conf.macaddr, &hwcfg.mac, sizeof(n->mac));
+ return true;
+ }
}
error_setg(errp, "vDPA device's mac != the mac address from qemu cmdline"
"Please check the the vdpa device's setting.");
--
2.45.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] virtio_net: Add the check for vdpa mac address
2024-08-06 0:58 ` [PATCH 2/3] virtio_net: Add the check for vdpa " Cindy Lu
@ 2024-08-06 3:08 ` Jason Wang
0 siblings, 0 replies; 14+ messages in thread
From: Jason Wang @ 2024-08-06 3:08 UTC (permalink / raw)
To: Cindy Lu; +Cc: mst, qemu-devel, Jonathon Jongsma
On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <lulu@redhat.com> wrote:
>
> When using a VDPA device, this is another acceptable situations
> The hardware MAC address is not 0, and the MAC address in the QEMU command line is 0.
> This is also acceptable
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
> hw/net/virtio-net.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7f51bd0dd3..c144ae2e78 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3592,11 +3592,22 @@ static bool virtio_net_check_vdpa_mac(NetClientState *nc, VirtIONet *n, MACAddr
> * 1.The hardware MAC address is the same as the QEMU command line MAC
> * address, and both of them are not 0.
> */
> -
> + /*
> + * 2.The hardware MAC address is not 0,
> + * and the MAC address in the QEMU command line is 0.
> + * In this situation, the hardware MAC address will overwrite
> + * the QEMU command line address.
This seems to break libvirt assumption?
Adding Jonathon.
> + */
> if (memcmp(&hwcfg.mac, &zero, sizeof(MACAddr)) != 0) {
> if ((memcmp(&hwcfg.mac, cmdline_mac, sizeof(MACAddr)) == 0)) {
> return true;
> }
> + if (memcmp(cmdline_mac, &zero, sizeof(MACAddr)) == 0) {
> + /* overwrite the mac address with hardware address*/
> + memcpy(&n->mac[0], &hwcfg.mac, sizeof(n->mac));
> + memcpy(&n->nic_conf.macaddr, &hwcfg.mac, sizeof(n->mac));
> + return true;
> + }
> }
> error_setg(errp, "vDPA device's mac != the mac address from qemu cmdline"
> "Please check the the vdpa device's setting.");
> --
> 2.45.0
>
Thanks
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] virtio_net: remove the unnecessary check in get_config
2024-08-06 0:58 [PATCH 1/3] virtio_net: Add the check for vdpa's mac address Cindy Lu
2024-08-06 0:58 ` [PATCH 2/3] virtio_net: Add the check for vdpa " Cindy Lu
@ 2024-08-06 0:58 ` Cindy Lu
2024-08-06 3:09 ` Jason Wang
2024-08-06 3:06 ` [PATCH 1/3] virtio_net: Add the check for vdpa's mac address Jason Wang
2024-08-06 13:30 ` Michael S. Tsirkin
3 siblings, 1 reply; 14+ messages in thread
From: Cindy Lu @ 2024-08-06 0:58 UTC (permalink / raw)
To: lulu, mst, jasowang, qemu-devel
The vdpa device with MAC address 0 should not boot.
So remove the check here
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
hw/net/virtio-net.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index c144ae2e78..8a7c743ad3 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -142,7 +142,6 @@ 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));
@@ -170,18 +169,6 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
if (ret == -1) {
return;
}
-
- /*
- * Some NIC/kernel combinations present 0 as the mac address. As that
- * is not a legal address, try to proceed with the address from the
- * QEMU command line in the hope that the 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);
- }
-
netcfg.status |= virtio_tswap16(vdev,
n->status & VIRTIO_NET_S_ANNOUNCE);
memcpy(config, &netcfg, n->config_size);
--
2.45.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] virtio_net: remove the unnecessary check in get_config
2024-08-06 0:58 ` [PATCH 3/3] virtio_net: remove the unnecessary check in get_config Cindy Lu
@ 2024-08-06 3:09 ` Jason Wang
2024-08-06 9:47 ` Cindy Lu
0 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2024-08-06 3:09 UTC (permalink / raw)
To: Cindy Lu; +Cc: mst, qemu-devel
On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <lulu@redhat.com> wrote:
>
> The vdpa device with MAC address 0 should not boot.
> So remove the check here
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
Please describe what issues you've seen, and how it is fixed by this commit.
Thanks
> ---
> hw/net/virtio-net.c | 13 -------------
> 1 file changed, 13 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index c144ae2e78..8a7c743ad3 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -142,7 +142,6 @@ 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));
> @@ -170,18 +169,6 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> if (ret == -1) {
> return;
> }
> -
> - /*
> - * Some NIC/kernel combinations present 0 as the mac address. As that
> - * is not a legal address, try to proceed with the address from the
> - * QEMU command line in the hope that the 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);
> - }
> -
> netcfg.status |= virtio_tswap16(vdev,
> n->status & VIRTIO_NET_S_ANNOUNCE);
> memcpy(config, &netcfg, n->config_size);
> --
> 2.45.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] virtio_net: remove the unnecessary check in get_config
2024-08-06 3:09 ` Jason Wang
@ 2024-08-06 9:47 ` Cindy Lu
0 siblings, 0 replies; 14+ messages in thread
From: Cindy Lu @ 2024-08-06 9:47 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, qemu-devel
On Tue, 6 Aug 2024 at 11:09, Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <lulu@redhat.com> wrote:
> >
> > The vdpa device with MAC address 0 should not boot.
> > So remove the check here
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
>
> Please describe what issues you've seen, and how it is fixed by this commit.
>
> Thanks
>
this is a fix after the previous one. In the previous fix, the device
with MAC address 0 would not allow booting, so there is no need to
check if the MAC address is 0 here.
I will re-organize the whole patch and make it more clear
Thanks
Cindy
> > ---
> > hw/net/virtio-net.c | 13 -------------
> > 1 file changed, 13 deletions(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index c144ae2e78..8a7c743ad3 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -142,7 +142,6 @@ 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));
> > @@ -170,18 +169,6 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> > if (ret == -1) {
> > return;
> > }
> > -
> > - /*
> > - * Some NIC/kernel combinations present 0 as the mac address. As that
> > - * is not a legal address, try to proceed with the address from the
> > - * QEMU command line in the hope that the 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);
> > - }
> > -
> > netcfg.status |= virtio_tswap16(vdev,
> > n->status & VIRTIO_NET_S_ANNOUNCE);
> > memcpy(config, &netcfg, n->config_size);
> > --
> > 2.45.0
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] virtio_net: Add the check for vdpa's mac address
2024-08-06 0:58 [PATCH 1/3] virtio_net: Add the check for vdpa's mac address Cindy Lu
2024-08-06 0:58 ` [PATCH 2/3] virtio_net: Add the check for vdpa " Cindy Lu
2024-08-06 0:58 ` [PATCH 3/3] virtio_net: remove the unnecessary check in get_config Cindy Lu
@ 2024-08-06 3:06 ` Jason Wang
2024-08-06 9:43 ` Cindy Lu
2024-08-06 13:30 ` Michael S. Tsirkin
3 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2024-08-06 3:06 UTC (permalink / raw)
To: Cindy Lu; +Cc: mst, qemu-devel
On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <lulu@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 <lulu@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?
> + */
> +
> + 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
> }
> QTAILQ_INIT(&n->rsc_chains);
> n->qdev = dev;
> --
> 2.45.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] virtio_net: Add the check for vdpa's mac address
2024-08-06 3:06 ` [PATCH 1/3] virtio_net: Add the check for vdpa's mac address Jason Wang
@ 2024-08-06 9:43 ` Cindy Lu
2024-08-06 11:12 ` Yan Vugenfirer
2024-08-07 2:35 ` Jason Wang
0 siblings, 2 replies; 14+ messages in thread
From: Cindy Lu @ 2024-08-06 9:43 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, qemu-devel
On Tue, 6 Aug 2024 at 11:07, Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <lulu@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 <lulu@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
Thanks,
cindy
> > }
> > QTAILQ_INIT(&n->rsc_chains);
> > n->qdev = dev;
> > --
> > 2.45.0
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] virtio_net: Add the check for vdpa's mac address
2024-08-06 9:43 ` Cindy Lu
@ 2024-08-06 11:12 ` Yan Vugenfirer
2024-08-09 9:14 ` Cindy Lu
2024-08-07 2:35 ` Jason Wang
1 sibling, 1 reply; 14+ messages in thread
From: Yan Vugenfirer @ 2024-08-06 11:12 UTC (permalink / raw)
To: Cindy Lu; +Cc: Jason Wang, mst, qemu-devel
Do we check that the MAC from the command line or HW was formed
correctly and doesn't include multicast bit?
Best regards,
Yan.
On Tue, Aug 6, 2024 at 12:45 PM Cindy Lu <lulu@redhat.com> wrote:
>
> On Tue, 6 Aug 2024 at 11:07, Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <lulu@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 <lulu@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
> Thanks,
> cindy
> > > }
> > > QTAILQ_INIT(&n->rsc_chains);
> > > n->qdev = dev;
> > > --
> > > 2.45.0
> > >
> >
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] virtio_net: Add the check for vdpa's mac address
2024-08-06 11:12 ` Yan Vugenfirer
@ 2024-08-09 9:14 ` Cindy Lu
0 siblings, 0 replies; 14+ messages in thread
From: Cindy Lu @ 2024-08-09 9:14 UTC (permalink / raw)
To: Yan Vugenfirer; +Cc: Jason Wang, mst, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5112 bytes --]
On Tue, 6 Aug 2024 at 19:13, Yan Vugenfirer <yvugenfi@redhat.com> wrote:
> Do we check that the MAC from the command line or HW was formed
> correctly and doesn't include multicast bit?
>
> Best regards,
> Yan.
>
> I didn't include a check for this, but it seems the vhost also doesn't
have this kind of verification. I will double check this
Thanks
Cindy
>
> On Tue, Aug 6, 2024 at 12:45 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > On Tue, 6 Aug 2024 at 11:07, Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <lulu@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 <lulu@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
> > Thanks,
> > cindy
> > > > }
> > > > QTAILQ_INIT(&n->rsc_chains);
> > > > n->qdev = dev;
> > > > --
> > > > 2.45.0
> > > >
> > >
> >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 7259 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] virtio_net: Add the check for vdpa's mac address
2024-08-06 9:43 ` Cindy Lu
2024-08-06 11:12 ` Yan Vugenfirer
@ 2024-08-07 2:35 ` Jason Wang
2024-08-09 9:14 ` Cindy Lu
1 sibling, 1 reply; 14+ messages in thread
From: Jason Wang @ 2024-08-07 2:35 UTC (permalink / raw)
To: Cindy Lu; +Cc: mst, qemu-devel
On Tue, Aug 6, 2024 at 5:44 PM Cindy Lu <lulu@redhat.com> wrote:
>
> On Tue, 6 Aug 2024 at 11:07, Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <lulu@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 <lulu@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
> Thanks,
> cindy
> > > }
> > > QTAILQ_INIT(&n->rsc_chains);
> > > n->qdev = dev;
> > > --
> > > 2.45.0
> > >
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] virtio_net: Add the check for vdpa's mac address
2024-08-07 2:35 ` Jason Wang
@ 2024-08-09 9:14 ` Cindy Lu
0 siblings, 0 replies; 14+ messages in thread
From: Cindy Lu @ 2024-08-09 9:14 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5002 bytes --]
On Wed, 7 Aug 2024 at 10:36, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Aug 6, 2024 at 5:44 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > On Tue, 6 Aug 2024 at 11:07, Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Tue, Aug 6, 2024 at 8:58 AM Cindy Lu <lulu@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 <lulu@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
> > > >
> > >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 7146 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] virtio_net: Add the check for vdpa's mac address
2024-08-06 0:58 [PATCH 1/3] virtio_net: Add the check for vdpa's mac address Cindy Lu
` (2 preceding siblings ...)
2024-08-06 3:06 ` [PATCH 1/3] virtio_net: Add the check for vdpa's mac address Jason Wang
@ 2024-08-06 13:30 ` Michael S. Tsirkin
2024-08-09 9:15 ` Cindy Lu
3 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2024-08-06 13:30 UTC (permalink / raw)
To: Cindy Lu; +Cc: jasowang, qemu-devel
On Tue, Aug 06, 2024 at 08:58:01AM +0800, Cindy Lu 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 <lulu@redhat.com>
Always post threads with a cover letter please.
> ---
> 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.
> + */
> +
> + 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.");
>
> + 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;
> + }
> }
> QTAILQ_INIT(&n->rsc_chains);
> n->qdev = dev;
> --
> 2.45.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] virtio_net: Add the check for vdpa's mac address
2024-08-06 13:30 ` Michael S. Tsirkin
@ 2024-08-09 9:15 ` Cindy Lu
0 siblings, 0 replies; 14+ messages in thread
From: Cindy Lu @ 2024-08-09 9:15 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: jasowang, qemu-devel
On Tue, 6 Aug 2024 at 21:30, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Aug 06, 2024 at 08:58:01AM +0800, Cindy Lu 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 <lulu@redhat.com>
>
> Always post threads with a cover letter please.
>
Sure,will fix this
thanks
cindy
>
> > ---
> > 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.
> > + */
> > +
> > + 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.");
> >
> > + 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;
> > + }
> > }
> > QTAILQ_INIT(&n->rsc_chains);
> > n->qdev = dev;
> > --
> > 2.45.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread