[PATCH] vhost-user-fs: fix features handling

[PATCH] vhost-user-fs: fix features handling

[Anton Kuchin]

Make virtio-fs take into account server capabilities.

Just returning requested features assumes they all of then are implemented
by server and results in setting unsupported configuration if some of them
are absent.

Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/virtio/vhost-user-fs.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index ac4fc34b36..6cf983ba0e 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -24,6 +24,14 @@
 #include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
 
+static const int user_feature_bits[] = {
+    VIRTIO_F_VERSION_1,
+    VIRTIO_RING_F_INDIRECT_DESC,
+    VIRTIO_RING_F_EVENT_IDX,
+    VIRTIO_F_NOTIFY_ON_EMPTY,
+    VHOST_INVALID_FEATURE_BIT
+};
+
 static void vuf_get_config(VirtIODevice *vdev, uint8_t *config)
 {
     VHostUserFS *fs = VHOST_USER_FS(vdev);
@@ -129,11 +137,12 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
 }
 
 static uint64_t vuf_get_features(VirtIODevice *vdev,
-                                      uint64_t requested_features,
-                                      Error **errp)
+                                 uint64_t features,
+                                 Error **errp)
 {
-    /* No feature bits used yet */
-    return requested_features;
+    VHostUserFS *fs = VHOST_USER_FS(vdev);
+
+    return vhost_get_features(&fs->vhost_dev, user_feature_bits, features);
 }
 
 static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)
-- 
2.25.1

Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling

[Vivek Goyal]

On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
> Make virtio-fs take into account server capabilities.
> 
> Just returning requested features assumes they all of then are implemented
> by server and results in setting unsupported configuration if some of them
> are absent.
> 
> Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>

[CC stefan and qemu-devel.]

Can you give more details of what problem exactly you are facing. Or
this fix is about avoiding a future problem where device can refuse
to support a feature qemu is requesting for.

IIUC, this patch is preparing a list of features vhost-user-fs device
can support. Then it calls vhost_get_features() which makes sure that
all these features are support by real vhost-user device (hdev->features).
If not, then corresponding feature is reset and remaining features
are returned to caller.

This feature negotion bit is called in so many places that I am kind of
lost that who should be doing what. Will leave it to Stefan who
understands it much better.


> ---
>  hw/virtio/vhost-user-fs.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index ac4fc34b36..6cf983ba0e 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -24,6 +24,14 @@
>  #include "monitor/monitor.h"
>  #include "sysemu/sysemu.h"
>  
> +static const int user_feature_bits[] = {
> +    VIRTIO_F_VERSION_1,
> +    VIRTIO_RING_F_INDIRECT_DESC,
> +    VIRTIO_RING_F_EVENT_IDX,
> +    VIRTIO_F_NOTIFY_ON_EMPTY,
> +    VHOST_INVALID_FEATURE_BIT
> +};
> +
>  static void vuf_get_config(VirtIODevice *vdev, uint8_t *config)
>  {
>      VHostUserFS *fs = VHOST_USER_FS(vdev);
> @@ -129,11 +137,12 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>  }
>  
>  static uint64_t vuf_get_features(VirtIODevice *vdev,
> -                                      uint64_t requested_features,
> -                                      Error **errp)
> +                                 uint64_t features,

Will it make sense to keep the name requested_features. This kind of
makes it clear that caller is requesting these features.

I feel there should be few lines of comments also to make it clear
what this function is actually doing.

Vivek

> +                                 Error **errp)
>  {
> -    /* No feature bits used yet */
> -    return requested_features;
> +    VHostUserFS *fs = VHOST_USER_FS(vdev);
> +
> +    return vhost_get_features(&fs->vhost_dev, user_feature_bits, features);
>  }
>  
>  static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling

[Anton Kuchin]

On 09/04/2021 18:56, Vivek Goyal wrote:
> On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
>> Make virtio-fs take into account server capabilities.
>>
>> Just returning requested features assumes they all of then are implemented
>> by server and results in setting unsupported configuration if some of them
>> are absent.
>>
>> Signed-off-by: Anton Kuchin<antonkuchin@yandex-team.ru>
> [CC stefan and qemu-devel.]
>
> Can you give more details of what problem exactly you are facing. Or
> this fix is about avoiding a future problem where device can refuse
> to support a feature qemu is requesting for.

This fixes existing problem that qemu ignores features (un)supported by 
backend and this works fine only if backend features match features of 
qemu. Otherwise qemu incorrectly assumes that backend suports all of 
them and calls vhost_set_features() not taking into account result of 
previous vhost_get_features() call. This breaks protocol and can crash 
server or cause incorrect behavior.

This is why I hope it to be accepted in time for 6.0 release.

> IIUC, this patch is preparing a list of features vhost-user-fs device
> can support. Then it calls vhost_get_features() which makes sure that
> all these features are support by real vhost-user device (hdev->features).
> If not, then corresponding feature is reset and remaining features
> are returned to caller.
When this callback is executed in virtio_bus_device_plugged() list of 
features that vhost-backend supports has been already obtained earlier 
by vhost_user_get_features() in vuf_device_realize() and stored in 
hdev->features. vuf_get_features() should return bitmask of features 
that are common for vhost backend (hdev->features) and frontend 
(vdev->host_features). But instead it just returns host features.
> This feature negotion bit is called in so many places that I am kind of
> lost that who should be doing what. Will leave it to Stefan who
> understands it much better.
>
>
>> ---
>>   hw/virtio/vhost-user-fs.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
>> index ac4fc34b36..6cf983ba0e 100644
>> --- a/hw/virtio/vhost-user-fs.c
>> +++ b/hw/virtio/vhost-user-fs.c
>> @@ -24,6 +24,14 @@
>>   #include "monitor/monitor.h"
>>   #include "sysemu/sysemu.h"
>>   
>> +static const int user_feature_bits[] = {
>> +    VIRTIO_F_VERSION_1,
>> +    VIRTIO_RING_F_INDIRECT_DESC,
>> +    VIRTIO_RING_F_EVENT_IDX,
>> +    VIRTIO_F_NOTIFY_ON_EMPTY,
>> +    VHOST_INVALID_FEATURE_BIT
>> +};
>> +
>>   static void vuf_get_config(VirtIODevice *vdev, uint8_t *config)
>>   {
>>       VHostUserFS *fs = VHOST_USER_FS(vdev);
>> @@ -129,11 +137,12 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
>>   }
>>   
>>   static uint64_t vuf_get_features(VirtIODevice *vdev,
>> -                                      uint64_t requested_features,
>> -                                      Error **errp)
>> +                                 uint64_t features,
> Will it make sense to keep the name requested_features. This kind of
> makes it clear that caller is requesting these features.
>
> I feel there should be few lines of comments also to make it clear
> what this function is actually doing.

This fix was inspired by similar functions in 
hw/scsi/vhost-scsi-common.c, hw/virtio/vhost-user-vsock.c, 
hw/block/vhost-user-blk.c and hw/net/vhost_net.c and I borrowed naming 
from them just to be consistent.

IMO this looks like a good place for refactoring because we have more 
and more vhost-user devices that duplicate that code, but it can be done 
after the bug is fixed.

> Vivek
>
>> +                                 Error **errp)
>>   {
>> -    /* No feature bits used yet */
>> -    return requested_features;
>> +    VHostUserFS *fs = VHOST_USER_FS(vdev);
>> +
>> +    return vhost_get_features(&fs->vhost_dev, user_feature_bits, features);
>>   }
>>   
>>   static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>> -- 
>> 2.25.1
>>
>>
>> _______________________________________________
>> Virtio-fs mailing list
>> Virtio-fs@redhat.com
>> https://listman.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling

[Vivek Goyal]

On Sun, Apr 11, 2021 at 09:21:54AM +0300, Anton Kuchin wrote:
> 
> On 09/04/2021 18:56, Vivek Goyal wrote:
> > On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
> > > Make virtio-fs take into account server capabilities.
> > > 
> > > Just returning requested features assumes they all of then are implemented
> > > by server and results in setting unsupported configuration if some of them
> > > are absent.
> > > 
> > > Signed-off-by: Anton Kuchin<antonkuchin@yandex-team.ru>
> > [CC stefan and qemu-devel.]
> > 
> > Can you give more details of what problem exactly you are facing. Or
> > this fix is about avoiding a future problem where device can refuse
> > to support a feature qemu is requesting for.
> 
> This fixes existing problem that qemu ignores features (un)supported by
> backend and this works fine only if backend features match features of qemu.
> Otherwise qemu incorrectly assumes that backend suports all of them and
> calls vhost_set_features() not taking into account result of previous
> vhost_get_features() call. This breaks protocol and can crash server or
> cause incorrect behavior.
> 
> This is why I hope it to be accepted in time for 6.0 release.
> 
> > IIUC, this patch is preparing a list of features vhost-user-fs device
> > can support. Then it calls vhost_get_features() which makes sure that
> > all these features are support by real vhost-user device (hdev->features).
> > If not, then corresponding feature is reset and remaining features
> > are returned to caller.
> When this callback is executed in virtio_bus_device_plugged() list of
> features that vhost-backend supports has been already obtained earlier by
> vhost_user_get_features() in vuf_device_realize() and stored in
> hdev->features.

> vuf_get_features() should return bitmask of features that
> are common for vhost backend (hdev->features) and frontend
> (vdev->host_features).

But that's not what exactly this patch seems to be doing.
IIUC, It only resets some of the features from list passed from
the caller. So whatever has been defined in user_feature_bits[],
and if these features are not supported by vhost-user backend, then
that feature will be reset before returning to caller.

So the question is what are those features which should be in
user_feature_bits[]? For example, by default libvhost-user
also supports.

        /* vhost-user feature bits */
        1ULL << VHOST_F_LOG_ALL |
        1ULL << VHOST_USER_F_PROTOCOL_FEATURES;

Should that be in user_feature_bits[] too. So that if a customer
vhost-user-fs backend does not support VHOST_F_LOG_ALL or
VHOST_USER_F_PROTOCOL_FEATURES, it is reset.

IIUC, your current patch is not going to reset these features if
caller passed you those in vuf_get_features(,requested_features).

So to me this becomes more of a question that what are those common
features which both the ends of vhost-user device should support for
it to work and should be checked in vuf_get_features(). 

Thanks
Vivek

> But instead it just returns host features.
> > This feature negotion bit is called in so many places that I am kind of
> > lost that who should be doing what. Will leave it to Stefan who
> > understands it much better.
> > 
> > 
> > > ---
> > >   hw/virtio/vhost-user-fs.c | 17 +++++++++++++----
> > >   1 file changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > > index ac4fc34b36..6cf983ba0e 100644
> > > --- a/hw/virtio/vhost-user-fs.c
> > > +++ b/hw/virtio/vhost-user-fs.c
> > > @@ -24,6 +24,14 @@
> > >   #include "monitor/monitor.h"
> > >   #include "sysemu/sysemu.h"
> > > +static const int user_feature_bits[] = {
> > > +    VIRTIO_F_VERSION_1,
> > > +    VIRTIO_RING_F_INDIRECT_DESC,
> > > +    VIRTIO_RING_F_EVENT_IDX,
> > > +    VIRTIO_F_NOTIFY_ON_EMPTY,
> > > +    VHOST_INVALID_FEATURE_BIT
> > > +};
> > > +
> > >   static void vuf_get_config(VirtIODevice *vdev, uint8_t *config)
> > >   {
> > >       VHostUserFS *fs = VHOST_USER_FS(vdev);
> > > @@ -129,11 +137,12 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t status)
> > >   }
> > >   static uint64_t vuf_get_features(VirtIODevice *vdev,
> > > -                                      uint64_t requested_features,
> > > -                                      Error **errp)
> > > +                                 uint64_t features,
> > Will it make sense to keep the name requested_features. This kind of
> > makes it clear that caller is requesting these features.
> > 
> > I feel there should be few lines of comments also to make it clear
> > what this function is actually doing.
> 
> This fix was inspired by similar functions in hw/scsi/vhost-scsi-common.c,
> hw/virtio/vhost-user-vsock.c, hw/block/vhost-user-blk.c and
> hw/net/vhost_net.c and I borrowed naming from them just to be consistent.
> 
> IMO this looks like a good place for refactoring because we have more and
> more vhost-user devices that duplicate that code, but it can be done after
> the bug is fixed.
> 
> > Vivek
> > 
> > > +                                 Error **errp)
> > >   {
> > > -    /* No feature bits used yet */
> > > -    return requested_features;
> > > +    VHostUserFS *fs = VHOST_USER_FS(vdev);
> > > +
> > > +    return vhost_get_features(&fs->vhost_dev, user_feature_bits, features);
> > >   }
> > >   static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > > -- 
> > > 2.25.1
> > > 
> > > 
> > > _______________________________________________
> > > Virtio-fs mailing list
> > > Virtio-fs@redhat.com
> > > https://listman.redhat.com/mailman/listinfo/virtio-fs
> 

Re: [PATCH] vhost-user-fs: fix features handling

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
> Make virtio-fs take into account server capabilities.
> 
> Just returning requested features assumes they all of then are implemented
> by server and results in setting unsupported configuration if some of them
> are absent.
> 
> Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
> ---
>  hw/virtio/vhost-user-fs.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index ac4fc34b36..6cf983ba0e 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -24,6 +24,14 @@
>  #include "monitor/monitor.h"
>  #include "sysemu/sysemu.h"
>  
> +static const int user_feature_bits[] = {
> +    VIRTIO_F_VERSION_1,
> +    VIRTIO_RING_F_INDIRECT_DESC,
> +    VIRTIO_RING_F_EVENT_IDX,
> +    VIRTIO_F_NOTIFY_ON_EMPTY,
> +    VHOST_INVALID_FEATURE_BIT
> +};

Please add:

VIRTIO_F_RING_PACKED
VIRTIO_F_IOMMU_PLATFORM

QEMU's virtiofsd does not enable either of these for now, but it's worth
allowing the vhost-user device backend to participate in negotiation so
that this can change in the future (or alternative virtiofsd
implementations can support these features).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3603 bytes --]

On Mon, Apr 12, 2021 at 02:43:16PM -0400, Vivek Goyal wrote:
> On Sun, Apr 11, 2021 at 09:21:54AM +0300, Anton Kuchin wrote:
> > 
> > On 09/04/2021 18:56, Vivek Goyal wrote:
> > > On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
> > > > Make virtio-fs take into account server capabilities.
> > > > 
> > > > Just returning requested features assumes they all of then are implemented
> > > > by server and results in setting unsupported configuration if some of them
> > > > are absent.
> > > > 
> > > > Signed-off-by: Anton Kuchin<antonkuchin@yandex-team.ru>
> > > [CC stefan and qemu-devel.]
> > > 
> > > Can you give more details of what problem exactly you are facing. Or
> > > this fix is about avoiding a future problem where device can refuse
> > > to support a feature qemu is requesting for.
> > 
> > This fixes existing problem that qemu ignores features (un)supported by
> > backend and this works fine only if backend features match features of qemu.
> > Otherwise qemu incorrectly assumes that backend suports all of them and
> > calls vhost_set_features() not taking into account result of previous
> > vhost_get_features() call. This breaks protocol and can crash server or
> > cause incorrect behavior.
> > 
> > This is why I hope it to be accepted in time for 6.0 release.
> > 
> > > IIUC, this patch is preparing a list of features vhost-user-fs device
> > > can support. Then it calls vhost_get_features() which makes sure that
> > > all these features are support by real vhost-user device (hdev->features).
> > > If not, then corresponding feature is reset and remaining features
> > > are returned to caller.
> > When this callback is executed in virtio_bus_device_plugged() list of
> > features that vhost-backend supports has been already obtained earlier by
> > vhost_user_get_features() in vuf_device_realize() and stored in
> > hdev->features.
> 
> > vuf_get_features() should return bitmask of features that
> > are common for vhost backend (hdev->features) and frontend
> > (vdev->host_features).
> 
> But that's not what exactly this patch seems to be doing.
> IIUC, It only resets some of the features from list passed from
> the caller. So whatever has been defined in user_feature_bits[],
> and if these features are not supported by vhost-user backend, then
> that feature will be reset before returning to caller.
> 
> So the question is what are those features which should be in
> user_feature_bits[]? For example, by default libvhost-user
> also supports.
> 
>         /* vhost-user feature bits */
>         1ULL << VHOST_F_LOG_ALL |
>         1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> 
> Should that be in user_feature_bits[] too. So that if a customer
> vhost-user-fs backend does not support VHOST_F_LOG_ALL or
> VHOST_USER_F_PROTOCOL_FEATURES, it is reset.
> 
> IIUC, your current patch is not going to reset these features if
> caller passed you those in vuf_get_features(,requested_features).
> 
> So to me this becomes more of a question that what are those common
> features which both the ends of vhost-user device should support for
> it to work and should be checked in vuf_get_features(). 

VHOST_F_LOG_ALL and VHOST_USER_F_PROTOCOL_FEATURES are controlled by
hw/virtio/vhost.c and hw/virtio/vhost-user.c. These feature bits are
part of the vhost-user protocol and are not involved in guest-visible
VIRTIO feature negotiation. It's confusing because these bits use the
same namespace as VIRTIO features but it is correct to omit it from
user_feature_bits[].

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] vhost-user-fs: fix features handling

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
> > Make virtio-fs take into account server capabilities.
> > 
> > Just returning requested features assumes they all of then are implemented
> > by server and results in setting unsupported configuration if some of them
> > are absent.
> > 
> > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
> > ---
> >  hw/virtio/vhost-user-fs.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > index ac4fc34b36..6cf983ba0e 100644
> > --- a/hw/virtio/vhost-user-fs.c
> > +++ b/hw/virtio/vhost-user-fs.c
> > @@ -24,6 +24,14 @@
> >  #include "monitor/monitor.h"
> >  #include "sysemu/sysemu.h"
> >  
> > +static const int user_feature_bits[] = {
> > +    VIRTIO_F_VERSION_1,
> > +    VIRTIO_RING_F_INDIRECT_DESC,
> > +    VIRTIO_RING_F_EVENT_IDX,
> > +    VIRTIO_F_NOTIFY_ON_EMPTY,
> > +    VHOST_INVALID_FEATURE_BIT
> > +};
> 
> Please add:
> 
> VIRTIO_F_RING_PACKED
> VIRTIO_F_IOMMU_PLATFORM
> 
> QEMU's virtiofsd does not enable either of these for now, but it's worth
> allowing the vhost-user device backend to participate in negotiation so
> that this can change in the future (or alternative virtiofsd
> implementations can support these features).

OK, so:


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


and queued, I'll add those extra 2 lines.  We seem pretty inconsistent
about all the different vhost-user devices.

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling

[Vivek Goyal]

On Tue, Apr 13, 2021 at 09:47:14AM +0100, Stefan Hajnoczi wrote:
> On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
> > Make virtio-fs take into account server capabilities.
> > 
> > Just returning requested features assumes they all of then are implemented
> > by server and results in setting unsupported configuration if some of them
> > are absent.
> > 
> > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
> > ---
> >  hw/virtio/vhost-user-fs.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > index ac4fc34b36..6cf983ba0e 100644
> > --- a/hw/virtio/vhost-user-fs.c
> > +++ b/hw/virtio/vhost-user-fs.c
> > @@ -24,6 +24,14 @@
> >  #include "monitor/monitor.h"
> >  #include "sysemu/sysemu.h"
> >  
> > +static const int user_feature_bits[] = {
> > +    VIRTIO_F_VERSION_1,
> > +    VIRTIO_RING_F_INDIRECT_DESC,
> > +    VIRTIO_RING_F_EVENT_IDX,
> > +    VIRTIO_F_NOTIFY_ON_EMPTY,
> > +    VHOST_INVALID_FEATURE_BIT
> > +};
> 
> Please add:
> 
> VIRTIO_F_RING_PACKED
> VIRTIO_F_IOMMU_PLATFORM

Hi Stefan,

What about

VIRTIO_F_ANY_LAYOUT

I see this one is currently set in requested_features. IIUC, qemu will
assume that device supports VIRTIO_F_ANY_LAYOUT if we don't reset it.

And I see two more flags.

VIRTIO_F_ORDER_PLATFORM
VIRTIO_F_SR_IOV

Should this be part of user_feature_bits[] too?

Thanks
Vivek

> 
> QEMU's virtiofsd does not enable either of these for now, but it's worth
> allowing the vhost-user device backend to participate in negotiation so
> that this can change in the future (or alternative virtiofsd
> implementations can support these features).



> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2569 bytes --]

On Tue, Apr 13, 2021 at 09:35:34AM -0400, Vivek Goyal wrote:
> On Tue, Apr 13, 2021 at 09:47:14AM +0100, Stefan Hajnoczi wrote:
> > On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
> > > Make virtio-fs take into account server capabilities.
> > > 
> > > Just returning requested features assumes they all of then are implemented
> > > by server and results in setting unsupported configuration if some of them
> > > are absent.
> > > 
> > > Signed-off-by: Anton Kuchin <antonkuchin@yandex-team.ru>
> > > ---
> > >  hw/virtio/vhost-user-fs.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > > index ac4fc34b36..6cf983ba0e 100644
> > > --- a/hw/virtio/vhost-user-fs.c
> > > +++ b/hw/virtio/vhost-user-fs.c
> > > @@ -24,6 +24,14 @@
> > >  #include "monitor/monitor.h"
> > >  #include "sysemu/sysemu.h"
> > >  
> > > +static const int user_feature_bits[] = {
> > > +    VIRTIO_F_VERSION_1,
> > > +    VIRTIO_RING_F_INDIRECT_DESC,
> > > +    VIRTIO_RING_F_EVENT_IDX,
> > > +    VIRTIO_F_NOTIFY_ON_EMPTY,
> > > +    VHOST_INVALID_FEATURE_BIT
> > > +};
> > 
> > Please add:
> > 
> > VIRTIO_F_RING_PACKED
> > VIRTIO_F_IOMMU_PLATFORM
> 
> Hi Stefan,
> 
> What about
> 
> VIRTIO_F_ANY_LAYOUT
> 
> I see this one is currently set in requested_features. IIUC, qemu will
> assume that device supports VIRTIO_F_ANY_LAYOUT if we don't reset it.

virtio-fs requires VIRTIO 1.1+ where the "any layout" semantics are
mandatory. The Legacy device interface is not supported by virtio-fs so
this feature bit isn't used.

Here is the VIRTIO_F_ANY_LAYOUT section in the spec if you want to read
more about it:
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4130003

> And I see two more flags.
> 
> VIRTIO_F_ORDER_PLATFORM
> VIRTIO_F_SR_IOV
> 
> Should this be part of user_feature_bits[] too?

VIRTIO_F_ORDER_PLATFORM is unclear. It could be used in some way if the
vhost-user device backend passes the virtqueue memory to a physical PCI
device, but I think vhost-user doesn't support that (instead vDPA would
be used).

VIRTIO_F_SR_IOV is not relevant to vhost-user device backends. It's
unlikely to be implemented but if so, then the hypervisor would handle
it as part of virtio-pci device emulation and the vhost-user device
backend would be unaware.

So I think these 3 feature bits do not need to be negotiated with the
vhost-user device backend.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]