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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature