From: Stefan Hajnoczi <firstname.lastname@example.org> To: Vivek Goyal <email@example.com> Cc: firstname.lastname@example.org, email@example.com, Anton Kuchin <firstname.lastname@example.org>, "Michael S. Tsirkin" <email@example.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<firstname.lastname@example.org> > > > [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 --]
next prev parent 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 \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --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).