On Thu, Feb 01, 2024 at 10:47:56AM +0000, Wentao Jia wrote:
> Hi, Eugenio
>
> Thanks for you comments
>
> It is a dilemma, our team mainly work on smartNIC vDPA, features
> implementation in the QEMU emulated devices is a certain workload for
> us.
In this case the implementation is mostly trivial I have no idea
why argue about it - just do it.
Implementing it in software will make it possible to e.g. test
the driver without the device. Which is of value even to yourself.
IOW - tough, you want a feature in please make it consumable
by implementing a software fallback.
> I have a proposal, clear these features except vhost, it will not affect
> emulated devices, do you agree the change?
>
> partial codes for clear these features
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7a2846fa1c..f4cf8b74da 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -822,6 +822,8 @@ static uint64_t virtio_net_get_features(VirtIODevice
> *vdev, uint64_t features,
> }
>
> if (!get_vhost_net(nc->peer)) {
> + virtio_clear_feature(&features, VIRTIO_F_IN_ORDER);
> + virtio_clear_feature(&features, VIRTIO_F_NOTIFICATION_DATA);
> return features;
> }
>
> Best Regards
> Wentao Jia
>
> -----Original Message-----
> From: Eugenio Perez Martin <epere...@redhat.com>
> Sent: Saturday, January 27, 2024 2:04 AM
> To: Wentao Jia <wentao....@nephogine.com>
> Cc: Rick Zhong <zhaoyong.zh...@nephogine.com>; qemu-devel@nongnu.org;
> m...@redhat.com; Jason Wang <jasow...@redhat.com>; Peter Xu
> <pet...@redhat.com>; Guo Zhi <qtxuning1...@sjtu.edu.cn>; Xinying Yu
> <xinying...@nephogine.com>
> Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and
> VIRTIO_F_NOTIFICATION_DATA feature
>
> On Fri, Jan 26, 2024 at 9:59 AM Wentao Jia <wentao....@nephogine.com> wrote:
> >
> > Hi, Eugenio
> >
> > Thanks for you comments, Our team has made new change about the patch,
> > these features in hw/virtio/virtio.h:DEFINE_VIRTIO_COMMON_FEATURES,
> > they are turned off by default , and can be turned on from at qemu
> > command line Do you have comments about this patch?
> >
>
> If the commandline is set to =on on an emulated device, we're back at square
> one: The guest will try to use these features in the emulator device and the
> kick or the descriptors exchange will fail.
>
> Maybe we can propose their implementation in the emulated devices on Google
> Summer of Code? Would you be interested in mentoring this? I can help with it
> for sure.
>
> On the other hand I'm not sure about the benefits of notification_data for
> emulated devices or even vhost-kernel. My understanding is that the data
> written cannot be passed with the eventfd, so QEMU should fully vmexit to the
> iowrite (which probably is slower in the event of a lot of notifications).
> Unless we can transmit the avail idx, the device must read the avail ring
> anyway.
>
> So the question for MST / Jason is, Is this enough justification to maybe
> fail the initialization of virtio-net-pci devices with backends different
> than vhost-user of vdpa if notification_data=on? Should this be backed by
> profiled data?
>
> In my opinion the emulated device should implement it and be =off by default,
> just for testing the driver implementation. But maybe it can be done on top
> after the early failure?
>
> Thanks!
>
> > Best Regards
> > Wentao Jia
> >
> >
> > VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature are important
> > feature for dpdk vdpa packets transmitting performance, add these
> > features at vhost-user front-end to negotiation with backend.
> >
> > In this patch, these features are turned off by default, turn on the
> > features at qemu command line.
> > ... notification_data=on,in_order=on ...
> >
> > Signed-off-by: Wentao Jia <wentao....@corigine.com>
> > Signed-off-by: Xinying Yu <xinying...@corigine.com>
> > Signed-off-by: Kyle Xu <zhenbing...@corigine.com>
> > Reviewed-by: Shujing Dong <shujing.d...@corigine.com>
> > Reviewed-by: Rick Zhong <zhaoyong.zh...@corigine.com>
> > ---
> > hw/core/machine.c | 2 ++
> > hw/net/vhost_net.c | 2 ++
> > include/hw/virtio/virtio.h | 6 +++++-
> > 3 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c index
> > fb5afdcae4..40489c23a6 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -40,6 +40,7 @@ GlobalProperty hw_compat_8_1[] = {
> > { "ramfb", "x-migrate", "off" },
> > { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" },
> > { "igb", "x-pcie-flr-init", "off" },
> > + { "virtio-device", "notification_data", "off"},
> > };
> > const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
> >
> > @@ -65,6 +66,7 @@ GlobalProperty hw_compat_7_1[] = {
> > { "virtio-rng-pci", "vectors", "0" },
> > { "virtio-rng-pci-transitional", "vectors", "0" },
> > { "virtio-rng-pci-non-transitional", "vectors", "0" },
> > + { "virtio-device", "in_order", "off"},
> > };
> > const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index
> > e8e1661646..211ca859a6 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -76,6 +76,8 @@ static const int user_feature_bits[] = {
> > VIRTIO_F_IOMMU_PLATFORM,
> > VIRTIO_F_RING_PACKED,
> > VIRTIO_F_RING_RESET,
> > + VIRTIO_F_IN_ORDER,
> > + VIRTIO_F_NOTIFICATION_DATA,
> > VIRTIO_NET_F_RSS,
> > VIRTIO_NET_F_HASH_REPORT,
> > VIRTIO_NET_F_GUEST_USO4,
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index c8f72850bc..e6aa10f01b 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -368,7 +368,11 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> > DEFINE_PROP_BIT64("packed", _state, _field, \
> > VIRTIO_F_RING_PACKED, false), \
> > DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> > - VIRTIO_F_RING_RESET, true)
> > + VIRTIO_F_RING_RESET, true), \
> > + DEFINE_PROP_BIT64("in_order", _state, _field, \
> > + VIRTIO_F_IN_ORDER, false), \
> > + DEFINE_PROP_BIT64("notification_data", _state, _field, \
> > + VIRTIO_F_NOTIFICATION_DATA, false)
> >
> > hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n); bool
> > virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
> > --
> > 2.31.1
> >
> > -----Original Message-----
> > From: Rick Zhong <zhaoyong.zh...@nephogine.com>
> > Sent: Friday, January 19, 2024 6:39 PM
> > To: Eugenio Perez Martin <epere...@redhat.com>; Wentao Jia
> > <wentao....@nephogine.com>
> > Cc: qemu-devel@nongnu.org; m...@redhat.com; Jason Wang
> > <jasow...@redhat.com>; Peter Xu <pet...@redhat.com>; Guo Zhi
> > <qtxuning1...@sjtu.edu.cn>
> > Subject: 回复: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> > Hi Eugenio,
> >
> > Thanks for your comments. Very helpful. Wentao and I will discuss and get
> > back to you later.
> >
> > Also welcome for any comments from other guys.
> >
> > Best Regards,
> > Rick Zhong
> >
> > -----邮件原件-----
> > 发件人: Eugenio Perez Martin <epere...@redhat.com>
> > 发送时间: 2024年1月19日 18:26
> > 收件人: Wentao Jia <wentao....@nephogine.com>
> > 抄送: qemu-devel@nongnu.org; m...@redhat.com; Rick Zhong
> > <zhaoyong.zh...@nephogine.com>; Jason Wang <jasow...@redhat.com>;
> > Peter Xu <pet...@redhat.com>; Guo Zhi <qtxuning1...@sjtu.edu.cn>
> > 主题: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> > On Fri, Jan 19, 2024 at 7:42 AM Wentao Jia <wentao....@nephogine.com> wrote:
> > >
> > >
> > > VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature are
> > > important feature for dpdk vdpa packets transmitting performance,
> > > add the 2 features at vhost-user front-end to negotiation with backend.
> > >
> > > Signed-off-by: Kyle Xu <zhenbing...@corigine.com>
> > > Signed-off-by: Wentao Jia <wentao....@corigine.com>
> > > Reviewed-by: Xinying Yu <xinying...@corigine.com>
> > > Reviewed-by: Shujing Dong <shujing.d...@corigine.com>
> > > Reviewed-by: Rick Zhong <zhaoyong.zh...@corigine.com>
> > > ---
> > > hw/core/machine.c | 2 ++
> > > hw/net/vhost_net.c | 2 ++
> > > hw/net/virtio-net.c | 4 ++++
> > > 3 files changed, 8 insertions(+)
> > >
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c index
> > > fb5afdcae4..e620f5e7d0 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -40,6 +40,7 @@ GlobalProperty hw_compat_8_1[] = {
> > > { "ramfb", "x-migrate", "off" },
> > > { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" },
> > > { "igb", "x-pcie-flr-init", "off" },
> > > + { TYPE_VIRTIO_NET, "notification_data", "off"},
> > > };
> >
> > Assuming the default "true" in
> > hw/net/virtio-net.c:virtio_net_properties is valid, this needs to be
> > appended to the array of the QEMU version that introduced the property in
> > the virtio_net_properties array, not the one that imported the macro from
> > the kernel. This allows QEMU to know that old versions have these features
> > disabled although the default set in
> > hw/net/virtio-net.c:virtio_net_properties is true when migrating from / to
> > these versions.
> >
> > You can check that this is added properly by migrating from / to a previous
> > version of QEMU, with the combinations of true and false.
> >
> > You have an example in [1] with blk devices multiqueue. CCing Peter Xu as
> > he knows more than me about this.
> >
> > This is very easy to miss when adding new features. Somebody who knows perl
> > should add a test in checkpath.pl similar to the warning "added, moved or
> > deleted file(s), does MAINTAINERS need updating?" when virtio properties
> > are modified :).
> >
> > > const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
> > >
> > > @@ -65,6 +66,7 @@ GlobalProperty hw_compat_7_1[] = {
> > > { "virtio-rng-pci", "vectors", "0" },
> > > { "virtio-rng-pci-transitional", "vectors", "0" },
> > > { "virtio-rng-pci-non-transitional", "vectors", "0" },
> > > + { TYPE_VIRTIO_NET, "in_order", "off"},
> > > };
> > > const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
> > >
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index
> > > e8e1661646..211ca859a6 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -76,6 +76,8 @@ static const int user_feature_bits[] = {
> > > VIRTIO_F_IOMMU_PLATFORM,
> > > VIRTIO_F_RING_PACKED,
> > > VIRTIO_F_RING_RESET,
> > > + VIRTIO_F_IN_ORDER,
> > > + VIRTIO_F_NOTIFICATION_DATA,
> > > VIRTIO_NET_F_RSS,
> > > VIRTIO_NET_F_HASH_REPORT,
> > > VIRTIO_NET_F_GUEST_USO4,
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index
> > > 7a2846fa1c..dc0a028934 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -3949,6 +3949,10 @@ static Property virtio_net_properties[] = {
> > > VIRTIO_NET_F_GUEST_USO6, true),
> > > DEFINE_PROP_BIT64("host_uso", VirtIONet, host_features,
> > > VIRTIO_NET_F_HOST_USO, true),
> > > + DEFINE_PROP_BIT64("in_order", VirtIONet, host_features,
> > > + VIRTIO_F_IN_ORDER, true),
> > > + DEFINE_PROP_BIT64("notification_data", VirtIONet, host_features,
> > > + VIRTIO_F_NOTIFICATION_DATA, true),
> >
> > This default=true is wrong, and makes emulated devices show these features
> > as available when they're not. You can test it by running qemu with the
> > parameters:
> >
> > -netdev tap,id=hostnet0,vhost=off -device virtio-net-pci,netdev=hostnet0,...
> >
> > The emulated device must support both features before making them tunnables.
> >
> > On the other hand, all kinds of virtio devices can use in_order and
> > notification_data, so they should be in
> > include/hw/virtio/virtio.h:DEFINE_VIRTIO_COMMON_FEATURES. But not all of
> > them benefit from in_order. One example of this is virtio-blk. It is usual
> > that requests are completed out of order by the backend device, so my
> > impression is that in_order will hurt its performance.
> > I've never profiled it though, so I may be wrong :).
> >
> > Long story short: Maybe in_order should be false by default, and enabled
> > just in virtio-net?
> >
> > You can see previous attempts of implementing this feature in qemu in [2].
> > CCing Guo too, as I don't know if he plans to continue this work soon.
> >
> > Please let me know if you need any help with these!
> >
> > Thanks!
> >
> > [1]
> > https://www.qemu.org/docs/master/devel/migration/compatibility.html#ho
> > w-backwards-compatibility-works [2]
> > https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02772.html
> >
> > > DEFINE_PROP_END_OF_LIST(),
> > > };
> > >
> > > --
> > >
> >
>
