qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Hanna Czenczek <hreitz@redhat.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Alex Bennée" <alex.bennee@linaro.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"virtio-fs@redhat.com" <virtio-fs@redhat.com>,
	"Eugenio Pérez" <eperezma@redhat.com>,
	"maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
	"Parav Pandit" <parav@nvidia.com>,
	"Anton Kuchin" <antonkuchin@yandex-team.ru>,
	"Yajun Wu" <yajunw@nvidia.com>
Subject: Re: [Virtio-fs] (no subject)
Date: Tue, 17 Oct 2023 10:13:34 +0200	[thread overview]
Message-ID: <3e26e29a-b861-deb2-b67a-5e505cf58429@redhat.com> (raw)
In-Reply-To: <20231017074943.gft6f672o6xzeepa@vireshk-i7>

On 17.10.23 09:49, Viresh Kumar wrote:
> On 13-10-23, 20:02, Hanna Czenczek wrote:
>> On 10.10.23 16:35, Alex Bennée wrote:
>>> I was going to say there is also the rust-vmm vhost-user-master crates
>>> which we've imported:
>>>
>>>     https://github.com/vireshk/vhost
>>>
>>> for the Xen Vhost Frontend:
>>>
>>>     https://github.com/vireshk/xen-vhost-frontend
>>>
>>> but I can't actually see any handling for GET/SET_STATUS at all which
>>> makes me wonder how we actually work. Viresh?
>> As far as I know the only back-end implementation of F_STATUS is in DPDK.
>> As I said, if anyone else implemented it right now, that would be dangerous,
>> because qemu doesn’t adhere to the virtio protocol when it comes to the
>> status byte.
> Yeah, none of the Rust based Virtio backends enable `STATUS` in
> `VhostUserProtocolFeatures` and so these messages are never exchanged.
>
> The generic Rust code for the backends, doesn't even implement them.
> Not sure if they should or not.

It absolutely should not, for evidence see this whole thread.  qemu 
sends a SET_STATUS 0, which amounts to a reset, when the VM is merely 
paused[1], and when it sets status bytes, it does not set them according 
to virtio specification.  Implementing it right now means relying on and 
working around qemu’s implementation-defined spec-breaking behavior.  
Also, note that qemu ignores feature negotiation response through 
FEATURES_OK, and DEVICE_NEEDS_RESET, so unless it’s worth working around 
the problems just to get some form of DRIVER_OK information (note this 
information does not come from the driver, but qemu makes it up), I 
absolutely would not implement it.

[1] Notably, it does restore the virtio state to the best of its 
abilities when the VM is resumed, but this is all still wrong (there is 
no point in doing so much on a pause/resume, it needlessly costs time) 
and any implementation that does a reset then will rely on the 
implementation-defined behavior that qemu is actually able to restore 
all the state that the back-end would lose during a reset. Notably, 
reset is not even well-defined in the vhost-user specification.  It was 
argued, in this thread, that DPDK works just fine with this, precisely 
because it ignores SET_STATUS 0.  Finally, if virtiofsd in particular, 
as a user of the Rust crates, is reset, it would lose its internal 
state, which qemu cannot restore short of using the upcoming migration 
facilities.



  reply	other threads:[~2023-10-17  8:14 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-04 12:58 [PATCH v4 0/8] vhost-user: Back-end state migration Hanna Czenczek
2023-10-04 12:58 ` [PATCH v4 1/8] vhost-user.rst: Deprecate [GS]ET_STATUS Hanna Czenczek
2023-10-05 17:08   ` Stefan Hajnoczi
2023-10-05 17:15     ` Michael S. Tsirkin
2023-10-06  7:48       ` [Virtio-fs] (no subject) Hanna Czenczek
2023-10-06  8:45         ` Michael S. Tsirkin
2023-10-06  9:15           ` Hanna Czenczek
2023-10-06  9:26             ` Michael S. Tsirkin
2023-10-06  9:47               ` Hanna Czenczek
2023-10-06 10:34                 ` Michael S. Tsirkin
2023-10-06 11:42                   ` Hanna Czenczek
2023-10-06 15:17                     ` Alex Bennée
2023-10-06 15:47                       ` Hanna Czenczek
2023-10-06 20:49                         ` Alex Bennée
2023-10-09  8:07                           ` Hanna Czenczek
2023-10-07  2:22                   ` Yajun Wu
2023-10-09  8:21                     ` Hanna Czenczek
2023-10-09  9:07                       ` Hanna Czenczek
2023-10-09  9:13                         ` Hanna Czenczek
2023-10-10  4:00                           ` Yajun Wu
2023-10-10  8:18                             ` Hanna Czenczek
2023-10-10 10:36                               ` Alex Bennée
2023-10-10 13:18                                 ` Hanna Czenczek
2023-10-10 14:35                                   ` Alex Bennée
2023-10-13 18:02                                     ` Hanna Czenczek
2023-10-17  7:49                                       ` Viresh Kumar
2023-10-17  8:13                                         ` Hanna Czenczek [this message]
2023-10-09 10:28                     ` German Maglione
2023-10-10  2:56                       ` Yajun Wu
2023-10-10 10:04                         ` German Maglione
2023-10-04 12:58 ` [PATCH v4 2/8] vhost-user.rst: Improve [GS]ET_VRING_BASE doc Hanna Czenczek
2023-10-05 17:38   ` Stefan Hajnoczi
2023-10-06  7:53     ` [Virtio-fs] " Hanna Czenczek
2023-10-06  8:49       ` Michael S. Tsirkin
2023-10-06 13:55         ` Hanna Czenczek
2023-10-06 13:58           ` Hanna Czenczek
2023-10-07 21:29             ` Michael S. Tsirkin
2023-10-07 21:27           ` Michael S. Tsirkin
2023-10-04 12:58 ` [PATCH v4 3/8] vhost-user.rst: Clarify enabling/disabling vrings Hanna Czenczek
2023-10-05 17:43   ` Stefan Hajnoczi
2023-10-18 12:14   ` Michael S. Tsirkin
2023-10-18 16:17     ` Hanna Czenczek
2023-10-04 12:59 ` [PATCH v4 4/8] vhost-user.rst: Introduce suspended state Hanna Czenczek
2023-10-05 17:44   ` Stefan Hajnoczi
2023-10-04 12:59 ` [PATCH v4 5/8] vhost-user.rst: Migrating back-end-internal state Hanna Czenczek
2023-10-05 17:46   ` Stefan Hajnoczi
2023-10-04 12:59 ` [PATCH v4 6/8] vhost-user: Interface for migration state transfer Hanna Czenczek
2023-10-05 17:46   ` Stefan Hajnoczi
2023-10-04 12:59 ` [PATCH v4 7/8] vhost: Add high-level state save/load functions Hanna Czenczek
2023-10-05 17:46   ` Stefan Hajnoczi
2023-10-04 12:59 ` [PATCH v4 8/8] vhost-user-fs: Implement internal migration Hanna Czenczek
2023-10-05 17:46   ` Stefan Hajnoczi
2023-10-05 17:48 ` [PATCH v4 0/8] vhost-user: Back-end state migration 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=3e26e29a-b861-deb2-b67a-5e505cf58429@redhat.com \
    --to=hreitz@redhat.com \
    --cc=alex.bennee@linaro.org \
    --cc=antonkuchin@yandex-team.ru \
    --cc=eperezma@redhat.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=mst@redhat.com \
    --cc=parav@nvidia.com \
    --cc=qemu-devel@nongnu.org \
    --cc=viresh.kumar@linaro.org \
    --cc=virtio-fs@redhat.com \
    --cc=yajunw@nvidia.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).