On Tue, May 09, 2023 at 10:53:35AM +0200, Hanna Czenczek wrote: > 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 <[email protected]> > > > > wrote: > > > > > On 04.05.23 23:14, Stefan Hajnoczi wrote: > > > > > > On Thu, 4 May 2023 at 13:39, Hanna Czenczek <[email protected]> > > > > > > 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. :))
Yes. Stefan
signature.asc
Description: PGP signature
_______________________________________________ Virtio-fs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/virtio-fs
