Re: Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Fri, Sep 23, 2022 at 11:53:22AM +0800, ho...@yusur.tech wrote: > On Thu, 22 Sep 2022 09:34:41 +0800 Jason Wang wrote: > > > >On Thu, Sep 22, 2022 at 1:58 AM Raphael Norwitz > > wrote: > >> > >> If I read your response on the other thread correctly, this change is > intended > >> > >> to prioritize the MAC address exposed by DPDK over the one provided by > the > >> > >> QEMU command line? Sounds reasonable in principle, but I would get > confirmation > >> > >> from vDPA/vhost-net maintainers. > > >I think the best way is to (and it seems easier) > > >1) have the management layer to make sure the mac came from cli > >matches the underlayer vDPA > > Agreed, that's no problem. > > >2) having a sanity check and fail the device initialization if they don't > match > > However, one MAC address for integrity check is from the cli, and the > other > MAC address is from the vDPA device, > How to get it? > > The current situation is if MAC came from cli don't match the underlayer > vDPA, the virtual machine can still start without any hints. > > Thanks > > Let me ask then, why are you supplying the incorrect MAC on the cli? -- MST
Re: Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Fri, Sep 23, 2022 at 12:05:40PM +0800, Jason Wang wrote:
> On Fri, Sep 23, 2022 at 11:55 AM ho...@yusur.tech wrote:
> >
> > On Thu, 22 Sep 2022 09:34:41 +0800 Jason Wang wrote:
> >
> >
> > >On Thu, Sep 22, 2022 at 1:58 AM Raphael Norwitz
> > > wrote:
> > >>
> > >> If I read your response on the other thread correctly, this change is
> > >> intended
> > >>
> > >> to prioritize the MAC address exposed by DPDK over the one provided by
> > >> the
> > >>
> > >> QEMU command line? Sounds reasonable in principle, but I would get
> > >> confirmation
> > >>
> > >> from vDPA/vhost-net maintainers.
> >
> > >I think the best way is to (and it seems easier)
> >
> > >1) have the management layer to make sure the mac came from cli
> > >matches the underlayer vDPA
> >
> > Agreed, that's no problem.
Actually, if we start failing here, won't it break working designs
in which vhost user does not really have a valid mac?
I am not sure we can start changing things so easily.
Let's add a protocol feature?
> > >2) having a sanity check and fail the device initialization if they don't
> > >match
> >
> > However, one MAC address for integrity check is from the cli, and the other
> > MAC address is from the vDPA device,
> > How to get it?
>
> VHOST_USER_GET_CONFIG?
>
> Thanks
>
> >
> > The current situation is if MAC came from cli don't match the underlayer
> > vDPA, the virtual machine can still start without any hints.
> >
> > Thanks
> >
> >
> > >Thanks
> >
> > >>
> > >>
> > >>
> > >> That said the way you’re hacking the vhost-user code breaks a valuable
> > >> check for
> > >>
> > >> bad vhost-user-blk backends. I would suggest finding another
> > >> implementation which
> > >>
> > >> does not regress functionality for other device types.
> > >>
> > >>
> > >>
> > >>
> > >>
> > >> >From: Hao Chen
> > >>
> > >> >
> > >>
> > >> >When use dpdk-vdpa tests vdpa device. You need to specify the mac
> > >> >address to
> > >>
> > >> >start the virtual machine through libvirt or qemu, but now, the libvirt
> > >> >or
> > >>
> > >> >qemu can call dpdk vdpa vendor driver's ops .get_config through
> > >> >vhost_net_get_config
> > >>
> > >> >to get the mac address of the vdpa hardware without manual
> > >> >configuration.
> > >>
> > >> >
> > >>
> > >> >v1->v2:
> > >>
> > >> >Only copy ETH_ALEN data of netcfg for some vdpa device such as
> > >>
> > >> >NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
> > >>
> > >> >We only need the mac address and don't care about the status field.
> > >>
> > >> >
> > >>
> > >> >Signed-off-by: Hao Chen
> > >>
> > >> >---
> > >>
> > >> > hw/block/vhost-user-blk.c | 1 -
> > >>
> > >> > hw/net/virtio-net.c | 7 +++
> > >>
> > >> > hw/virtio/vhost-user.c| 19 ---
> > >>
> > >> > 3 files changed, 7 insertions(+), 20 deletions(-)
> > >>
> > >> >
> > >>
> > >> >diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > >>
> > >> >index 9117222456..5dca4eab09 100644
> > >>
> > >> >--- a/hw/block/vhost-user-blk.c
> > >>
> > >> >+++ b/hw/block/vhost-user-blk.c
> > >>
> > >> >@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
> > >> >Error **errp)
> > >>
> > >> >
> > >>
> > >> > vhost_dev_set_config_notifier(>dev, _ops);
> > >>
> > >> >
> > >>
> > >> >-s->vhost_user.supports_config = true;
> > >>
> > >> > ret = vhost_dev_init(>dev, >vhost_user,
> > >> > VHOST_BACKEND_TYPE_USER, 0,
> > >>
> > >> > errp);
> > >>
> > >> > if (ret < 0) {
> > >>
> > >> >diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > >>
> > >> >index dd0d056fde..90405083b1 100644
> > >>
> > >> >--- a/hw/net/virtio-net.c
> > >>
> > >> >+++ b/hw/net/virtio-net.c
> > >>
> > >> >@@ -166,6 +166,13 @@ static void virtio_net_get_config(VirtIODevice
> > >> >*vdev, uint8_t *config)
> > >>
> > >> > }
> > >>
> > >> > memcpy(config, , n->config_size);
> > >>
> > >> > }
> > >>
> > >> >+} else if (nc->peer && nc->peer->info->type ==
> > >> >NET_CLIENT_DRIVER_VHOST_USER) {
> > >>
> > >> >+ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t
> > >> >*),
> > >>
> > >> >+ n->config_size);
> > >>
> > >> >+if (ret != -1) {
> > >>
> > >> >+ /* Automatically obtain the mac address of the vdpa
> > >> >device
> > >>
> > >> >+* when using the dpdk vdpa */
> > >>
> > >> >+memcpy(config, , ETH_ALEN);
> > >>
> > >> > }
> > >>
> > >> > }
> > >>
> > >> >
> > >>
> > >> >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > >>
> > >> >index bd24741be8..8b01078249 100644
> > >>
> > >> >--- a/hw/virtio/vhost-user.c
> > >>
> > >> >+++ b/hw/virtio/vhost-user.c
> > >>
> > >> >@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct
> > >> >vhost_dev *dev, void *opaque,
> > >>
> > >> > }
> > >>
> > >> >
> > >>
> > >> > if (virtio_has_feature(features,
Re: Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Fri, Sep 23, 2022 at 11:55 AM ho...@yusur.tech wrote:
>
> On Thu, 22 Sep 2022 09:34:41 +0800 Jason Wang wrote:
>
>
> >On Thu, Sep 22, 2022 at 1:58 AM Raphael Norwitz
> > wrote:
> >>
> >> If I read your response on the other thread correctly, this change is
> >> intended
> >>
> >> to prioritize the MAC address exposed by DPDK over the one provided by the
> >>
> >> QEMU command line? Sounds reasonable in principle, but I would get
> >> confirmation
> >>
> >> from vDPA/vhost-net maintainers.
>
> >I think the best way is to (and it seems easier)
>
> >1) have the management layer to make sure the mac came from cli
> >matches the underlayer vDPA
>
> Agreed, that's no problem.
>
> >2) having a sanity check and fail the device initialization if they don't
> >match
>
> However, one MAC address for integrity check is from the cli, and the other
> MAC address is from the vDPA device,
> How to get it?
VHOST_USER_GET_CONFIG?
Thanks
>
> The current situation is if MAC came from cli don't match the underlayer
> vDPA, the virtual machine can still start without any hints.
>
> Thanks
>
>
> >Thanks
>
> >>
> >>
> >>
> >> That said the way you’re hacking the vhost-user code breaks a valuable
> >> check for
> >>
> >> bad vhost-user-blk backends. I would suggest finding another
> >> implementation which
> >>
> >> does not regress functionality for other device types.
> >>
> >>
> >>
> >>
> >>
> >> >From: Hao Chen
> >>
> >> >
> >>
> >> >When use dpdk-vdpa tests vdpa device. You need to specify the mac address
> >> >to
> >>
> >> >start the virtual machine through libvirt or qemu, but now, the libvirt or
> >>
> >> >qemu can call dpdk vdpa vendor driver's ops .get_config through
> >> >vhost_net_get_config
> >>
> >> >to get the mac address of the vdpa hardware without manual configuration.
> >>
> >> >
> >>
> >> >v1->v2:
> >>
> >> >Only copy ETH_ALEN data of netcfg for some vdpa device such as
> >>
> >> >NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
> >>
> >> >We only need the mac address and don't care about the status field.
> >>
> >> >
> >>
> >> >Signed-off-by: Hao Chen
> >>
> >> >---
> >>
> >> > hw/block/vhost-user-blk.c | 1 -
> >>
> >> > hw/net/virtio-net.c | 7 +++
> >>
> >> > hw/virtio/vhost-user.c| 19 ---
> >>
> >> > 3 files changed, 7 insertions(+), 20 deletions(-)
> >>
> >> >
> >>
> >> >diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> >>
> >> >index 9117222456..5dca4eab09 100644
> >>
> >> >--- a/hw/block/vhost-user-blk.c
> >>
> >> >+++ b/hw/block/vhost-user-blk.c
> >>
> >> >@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
> >> >Error **errp)
> >>
> >> >
> >>
> >> > vhost_dev_set_config_notifier(>dev, _ops);
> >>
> >> >
> >>
> >> >-s->vhost_user.supports_config = true;
> >>
> >> > ret = vhost_dev_init(>dev, >vhost_user,
> >> > VHOST_BACKEND_TYPE_USER, 0,
> >>
> >> > errp);
> >>
> >> > if (ret < 0) {
> >>
> >> >diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>
> >> >index dd0d056fde..90405083b1 100644
> >>
> >> >--- a/hw/net/virtio-net.c
> >>
> >> >+++ b/hw/net/virtio-net.c
> >>
> >> >@@ -166,6 +166,13 @@ static void virtio_net_get_config(VirtIODevice
> >> >*vdev, uint8_t *config)
> >>
> >> > }
> >>
> >> > memcpy(config, , n->config_size);
> >>
> >> > }
> >>
> >> >+} else if (nc->peer && nc->peer->info->type ==
> >> >NET_CLIENT_DRIVER_VHOST_USER) {
> >>
> >> >+ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t
> >> >*),
> >>
> >> >+ n->config_size);
> >>
> >> >+if (ret != -1) {
> >>
> >> >+ /* Automatically obtain the mac address of the vdpa device
> >>
> >> >+* when using the dpdk vdpa */
> >>
> >> >+memcpy(config, , ETH_ALEN);
> >>
> >> > }
> >>
> >> > }
> >>
> >> >
> >>
> >> >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >>
> >> >index bd24741be8..8b01078249 100644
> >>
> >> >--- a/hw/virtio/vhost-user.c
> >>
> >> >+++ b/hw/virtio/vhost-user.c
> >>
> >> >@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev
> >> >*dev, void *opaque,
> >>
> >> > }
> >>
> >> >
> >>
> >> > if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> >>
> >> >-bool supports_f_config = vus->supports_config ||
> >>
> >> >-(dev->config_ops &&
> >> >dev->config_ops->vhost_dev_config_notifier);
> >>
> >> > uint64_t protocol_features;
> >>
> >> >
> >>
> >> > dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> >>
> >> >@@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct
> >> >vhost_dev *dev, void *opaque,
> >>
> >> > */
> >>
> >> > protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
> >>
> >> >
> >>
> >> >-if (supports_f_config) {
> >>
> >> >-if (!virtio_has_feature(protocol_features,
> >>
>
Re: Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Thu, 22 Sep 2022 09:34:41 +0800 Jason Wang wrote:
>On Thu, Sep 22, 2022 at 1:58 AM Raphael Norwitz
> wrote:
>>
>> If I read your response on the other thread correctly, this change is
>> intended
>>
>> to prioritize the MAC address exposed by DPDK over the one provided by the
>>
>> QEMU command line? Sounds reasonable in principle, but I would get
>> confirmation
>>
>> from vDPA/vhost-net maintainers.
>I think the best way is to (and it seems easier)
>1) have the management layer to make sure the mac came from cli
>matches the underlayer vDPA
Agreed, that's no problem.
>2) having a sanity check and fail the device initialization if they don't match
However, one MAC address for integrity check is from the cli, and the other MAC
address is from the vDPA device,
How to get it?
The current situation is if MAC came from cli don't match the underlayer vDPA,
the virtual machine can still start without any hints.
Thanks
>Thanks
>>
>>
>>
>> That said the way you’re hacking the vhost-user code breaks a valuable check
>> for
>>
>> bad vhost-user-blk backends. I would suggest finding another implementation
>> which
>>
>> does not regress functionality for other device types.
>>
>>
>>
>>
>>
>> >From: Hao Chen
>>
>> >
>>
>> >When use dpdk-vdpa tests vdpa device. You need to specify the mac address to
>>
>> >start the virtual machine through libvirt or qemu, but now, the libvirt or
>>
>> >qemu can call dpdk vdpa vendor driver's ops .get_config through
>> >vhost_net_get_config
>>
>> >to get the mac address of the vdpa hardware without manual configuration.
>>
>> >
>>
>> >v1->v2:
>>
>> >Only copy ETH_ALEN data of netcfg for some vdpa device such as
>>
>> >NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
>>
>> >We only need the mac address and don't care about the status field.
>>
>> >
>>
>> >Signed-off-by: Hao Chen
>>
>> >---
>>
>> > hw/block/vhost-user-blk.c | 1 -
>>
>> > hw/net/virtio-net.c | 7 +++
>>
>> > hw/virtio/vhost-user.c| 19 ---
>>
>> > 3 files changed, 7 insertions(+), 20 deletions(-)
>>
>> >
>>
>> >diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>
>> >index 9117222456..5dca4eab09 100644
>>
>> >--- a/hw/block/vhost-user-blk.c
>>
>> >+++ b/hw/block/vhost-user-blk.c
>>
>> >@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
>> >Error **errp)
>>
>> >
>>
>> > vhost_dev_set_config_notifier(>dev, _ops);
>>
>> >
>>
>> >-s->vhost_user.supports_config = true;
>>
>> > ret = vhost_dev_init(>dev, >vhost_user, VHOST_BACKEND_TYPE_USER,
>> > 0,
>>
>> > errp);
>>
>> > if (ret < 0) {
>>
>> >diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>
>> >index dd0d056fde..90405083b1 100644
>>
>> >--- a/hw/net/virtio-net.c
>>
>> >+++ b/hw/net/virtio-net.c
>>
>> >@@ -166,6 +166,13 @@ static void virtio_net_get_config(VirtIODevice *vdev,
>> >uint8_t *config)
>>
>> > }
>>
>> > memcpy(config, , n->config_size);
>>
>> > }
>>
>> >+} else if (nc->peer && nc->peer->info->type ==
>> >NET_CLIENT_DRIVER_VHOST_USER) {
>>
>> >+ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t
>> >*),
>>
>> >+ n->config_size);
>>
>> >+if (ret != -1) {
>>
>> >+ /* Automatically obtain the mac address of the vdpa device
>>
>> >+* when using the dpdk vdpa */
>>
>> >+memcpy(config, , ETH_ALEN);
>>
>> > }
>>
>> > }
>>
>> >
>>
>> >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>
>> >index bd24741be8..8b01078249 100644
>>
>> >--- a/hw/virtio/vhost-user.c
>>
>> >+++ b/hw/virtio/vhost-user.c
>>
>> >@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev
>> >*dev, void *opaque,
>>
>> > }
>>
>> >
>>
>> > if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>>
>> >-bool supports_f_config = vus->supports_config ||
>>
>> >-(dev->config_ops &&
>> >dev->config_ops->vhost_dev_config_notifier);
>>
>> > uint64_t protocol_features;
>>
>> >
>>
>> > dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
>>
>> >@@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
>> >*dev, void *opaque,
>>
>> > */
>>
>> > protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
>>
>> >
>>
>> >-if (supports_f_config) {
>>
>> >-if (!virtio_has_feature(protocol_features,
>>
>> >-VHOST_USER_PROTOCOL_F_CONFIG)) {
>>
>> >-error_setg(errp, "vhost-user device expecting "
>>
>> >- "VHOST_USER_PROTOCOL_F_CONFIG but the
>> >vhost-user backend does "
>>
>> >- "not support it.");
>>
>> >-return -EPROTO;
>>
>> >-}
>>
>> >-} else {
>>
>> >-if (virtio_has_feature(protocol_features,
>>
>> >-
