qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Stefan Hajnoczi <stefanha@redhat.com>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: virtio-fs@redhat.com, qemu-devel@nongnu.org,
	Anton Kuchin <antonkuchin@yandex-team.ru>,
	"Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling
Date: Tue, 13 Apr 2021 09:53:35 +0100	[thread overview]
Message-ID: <YHVcDwX7bCW6T1B2@stefanha-x1.localdomain> (raw)
In-Reply-To: <20210412184316.GF1184147@redhat.com>

[-- 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 --]

  reply	other threads:[~2021-04-13  8:55 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-08 19:55 Anton Kuchin
2021-04-09 15:56 ` [Virtio-fs] " Vivek Goyal
2021-04-11  6:21   ` Anton Kuchin
2021-04-12 18:43     ` Vivek Goyal
2021-04-13  8:53       ` Stefan Hajnoczi [this message]
2021-04-13  8:47 ` Stefan Hajnoczi
2021-04-13 11:36   ` Dr. David Alan Gilbert
2021-04-13 13:35   ` [Virtio-fs] " Vivek Goyal
2021-04-14  7:00     ` Stefan Hajnoczi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YHVcDwX7bCW6T1B2@stefanha-x1.localdomain \
    --to=stefanha@redhat.com \
    --cc=antonkuchin@yandex-team.ru \
    --cc=mst@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=vgoyal@redhat.com \
    --cc=virtio-fs@redhat.com \
    --subject='Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).