On 08.05.23 23:10, Stefan Hajnoczi wrote:
On Fri, May 05, 2023 at 02:51:55PM +0200, Hanna Czenczek wrote:
On 05.05.23 11:53, Eugenio Perez Martin wrote:
On Fri, May 5, 2023 at 11:03 AM Hanna Czenczek <hre...@redhat.com> wrote:
On 04.05.23 23:14, Stefan Hajnoczi wrote:
On Thu, 4 May 2023 at 13:39, Hanna Czenczek <hre...@redhat.com> wrote:
[...]
All state is lost and the Device Initialization process
must be followed to make the device operational again.
Existing vhost-user backends don't implement SET_STATUS 0 (it's new).
It's messy and not your fault. I think QEMU should solve this by
treating stateful devices differently from non-stateful devices. That
way existing vhost-user backends continue to work and new stateful
devices can also be supported.
It’s my understanding that SET_STATUS 0/RESET_DEVICE is problematic for
stateful devices. In a previous email, you wrote that these should
implement SUSPEND+RESUME so qemu can use those instead. But those are
separate things, so I assume we just use SET_STATUS 0 when stopping the
VM because this happens to also stop processing vrings as a side effect?
I.e. I understand “treating stateful devices differently” to mean that
qemu should use SUSPEND+RESUME instead of SET_STATUS 0 when the back-end
supports it, and stateful back-ends should support it.
Honestly I cannot think of any use case where the vhost-user backend
did not ignore set_status(0) and had to retrieve vq states. So maybe
we can totally remove that call from qemu?
I don’t know so I can’t really say; but I don’t quite understand why qemu
would reset a device at any point but perhaps VM reset (and even then I’d
expect the post-reset guest to just reset the device on boot by itself,
too).
DPDK stores the Device Status field value and uses it later:
https://github.com/DPDK/dpdk/blob/main/lib/vhost/vhost_user.c#L2791
While DPDK performs no immediate action upon SET_STATUS 0, omitting the
message will change the behavior of other DPDK code like
virtio_is_ready().
Changing the semantics of the vhost-user protocol in a way that's not
backwards compatible is something we should avoid unless there is no
other way.
Well, I have two opinions on this:
First, that in DPDK sounds wrong. vhost_dev_stop() is called mostly by
devices that call it when set_status is called on them. But they don’t
call it if status == 0, they call it if virtio_device_should_start()
returns false, which is the case when the VM is stopped. So basically
we set a status value on the back-end device that is not the status
value that is set in qemu. If DPDK makes actual use of this status value
that differs from that of the front-end in qemu, that sounds like it
probably actually wrong.
Second, it’s entirely possible and probably probable that DPDK doesn’t
make “actual use of this status value”; the only use it probably has is
to determine whether the device is supposed to be stopped, which is
exactly what qemu has tried to confer by setting it to 0. So it’s
basically two implementations that have agreed on abusing a value to
emulate behavior that isn’t otherwise implement (SUSPEND), and that
works because all devices are stateless. Then, I agree, we can’t change
this until it gets SUSPEND support.
The fundamental problem is that QEMU's vhost code is designed to reset
vhost devices because it assumes they are stateless. If an F_SUSPEND
protocol feature bit is added, then it becomes possible to detect new
backends and suspend/resume them rather than reset them.
That's the solution that I favor because it's backwards compatible and
the same model can be applied to stateful vDPA devices in the future.
So basically the idea is the following: vhost_dev_stop() should just
suspend the device, not reset it. For devices that don’t support
SUSPEND, we still need to do something, and just calling GET_VRING_BASE
on all vrings is deemed inadequate, so they are reset (SET_STATUS 0) as
a work-around, assuming that stateful devices that care (i.e. implement
SET_STATUS) will also implement SUSPEND to not have this “legacy reset”
happen to them.
Sounds good to me. (If I understood that right. :))
Hanna
_______________________________________________
Virtio-fs mailing list
Virtio-fs@redhat.com
https://listman.redhat.com/mailman/listinfo/virtio-fs
