| Message ID | 20230414160433.2096866-1-alex.bennee@linaro.org |
|---|---|
| Headers | show |
| Series | virtio: add vhost-user-generic and reduce copy and paste | expand |
On 14-04-23, 17:04, Alex Bennée wrote: > hw/virtio/vhost-user-device-pci.c | 71 +++++ > hw/virtio/vhost-user-device.c | 359 ++++++++++++++++++++++ > hw/virtio/vhost-user-fs.c | 4 +- > hw/virtio/vhost-user-gpio.c | 405 +------------------------ > hw/virtio/vhost-user-rng.c | 264 +--------------- I wonder why isn't i2c removed as well ?
Viresh Kumar <viresh.kumar@linaro.org> writes: > On 14-04-23, 17:04, Alex Bennée wrote: >> hw/virtio/vhost-user-device-pci.c | 71 +++++ >> hw/virtio/vhost-user-device.c | 359 ++++++++++++++++++++++ >> hw/virtio/vhost-user-fs.c | 4 +- >> hw/virtio/vhost-user-gpio.c | 405 +------------------------ >> hw/virtio/vhost-user-rng.c | 264 +--------------- > > I wonder why isn't i2c removed as well ? I will do - mainly I want to do a similar dummy device addition to the i2c daemon for testing.
On Fri, 14 Apr 2023 at 12:06, Alex Bennée <alex.bennee@linaro.org> wrote: > > A lot of our vhost-user stubs are large chunks of boilerplate that do > (mostly) the same thing. This series attempts to fix that by defining > a new base class for vhost-user devices and then converting the rng > and gpio devices to be based off them. You can even use > vhost-user-device directly if you supply it with the right magic > numbers (which is helpful for development). > > However the final patch runs into the weeds because I don't yet have a > clean way to represent in QOM the fixing of certain properties for the > specialised classes. > > The series is a net reduction in code and an increase in > documentation but obviously needs to iron out a few more warts. I'm > open to suggestions on the best way to tweak the QOM stuff. --device vhost-user-device is not really possible because vhost-user devices are not full VIRTIO devices. vhost-user devices depend on device-specific code in the VMM by design. The "subset of a VIRTIO device" design made sense for vhost_net. Nowadays there are other device types that are close to full VIRTIO devices, although the vhost-user protocol doesn't support the full VIRTIO device lifecycle. I think a user-creatable --device vhost-user-device is not a good idea today. It creates confusion. Many people aren't aware of the architectural difference between vhost-user and VIRTIO devices. The result is that VMMs and vhost-user backends implement increasingly brittle VIRTIO configuration space and feature bit logic as they knowingly or unknowingly try to paper over the fact that a traditional vhost-user device isn't a full VIRTIO device. It is possible to resolve this difference and make --device vhost-user-device work properly for devices that want to be full VIRTIO devices. See "Making VMM device shims optional" here: https://blog.vmsplice.net/2020/09/on-unifying-vhost-user-and-virtio.html Even after extending the vhost-user protocol to solve the current limitations, existing backends would still only be partial VIRTIO devices that wouldn't work with --device vhost-user-device. Reducing boilerplate is helpful, but I think --device vhost-user-device should not be user-creatable. Stefan > > Alex. > > Alex Bennée (12): > hw/virtio: fix typo in VIRTIO_CONFIG_IRQ_IDX comments > include/hw/virtio: document virtio_notify_config > include/hw/virtio: add kerneldoc for virtio_init > include/hw/virtio: document some more usage of notifiers > virtio: add generic vhost-user-device > virtio: add PCI stub for vhost-user-device > include: attempt to document device_class_set_props > qom: allow for properties to become "fixed" > hw/virtio: derive vhost-user-rng from vhost-user-device > hw/virtio: add config support to vhost-user-device > hw/virtio: derive vhost-user-gpio from vhost-user-device (!BROKEN) > docs/system: add a basic enumeration of vhost-user devices > > docs/system/devices/vhost-user-rng.rst | 2 + > docs/system/devices/vhost-user.rst | 41 +++ > qapi/qom.json | 2 + > include/hw/qdev-core.h | 9 + > include/hw/virtio/vhost-user-device.h | 33 ++ > include/hw/virtio/vhost-user-gpio.h | 23 +- > include/hw/virtio/vhost-user-rng.h | 11 +- > include/hw/virtio/virtio.h | 21 ++ > include/qom/object.h | 16 +- > hw/display/vhost-user-gpu.c | 4 +- > hw/net/virtio-net.c | 4 +- > hw/virtio/vhost-user-device-pci.c | 71 +++++ > hw/virtio/vhost-user-device.c | 359 ++++++++++++++++++++++ > hw/virtio/vhost-user-fs.c | 4 +- > hw/virtio/vhost-user-gpio.c | 405 +------------------------ > hw/virtio/vhost-user-rng.c | 264 +--------------- > hw/virtio/vhost-vsock-common.c | 4 +- > hw/virtio/virtio-crypto.c | 4 +- > qom/object.c | 14 + > qom/object_interfaces.c | 9 +- > qom/qom-qmp-cmds.c | 1 + > softmmu/qdev-monitor.c | 1 + > hw/virtio/meson.build | 3 + > 23 files changed, 613 insertions(+), 692 deletions(-) > create mode 100644 include/hw/virtio/vhost-user-device.h > create mode 100644 hw/virtio/vhost-user-device-pci.c > create mode 100644 hw/virtio/vhost-user-device.c > > -- > 2.39.2 > >
Stefan Hajnoczi <stefanha@gmail.com> writes: > On Fri, 14 Apr 2023 at 12:06, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> A lot of our vhost-user stubs are large chunks of boilerplate that do >> (mostly) the same thing. This series attempts to fix that by defining >> a new base class for vhost-user devices and then converting the rng >> and gpio devices to be based off them. You can even use >> vhost-user-device directly if you supply it with the right magic >> numbers (which is helpful for development). >> >> However the final patch runs into the weeds because I don't yet have a >> clean way to represent in QOM the fixing of certain properties for the >> specialised classes. >> >> The series is a net reduction in code and an increase in >> documentation but obviously needs to iron out a few more warts. I'm >> open to suggestions on the best way to tweak the QOM stuff. > > --device vhost-user-device is not really possible because vhost-user > devices are not full VIRTIO devices. vhost-user devices depend on > device-specific code in the VMM by design. What device specific code? You certainly need to instantiate stuff in the DTB/ACPI tables for -M virt but everything else can be handed off to the vhost-user daemon. Indeed the split brain is a bit silly in some places. For example is QEMU really the best arbiter of a block device config when the actual backend is a separate process. We have config passing in the vhost-user spec. > The "subset of a VIRTIO device" design made sense for vhost_net. > Nowadays there are other device types that are close to full VIRTIO > devices, although the vhost-user protocol doesn't support the full > VIRTIO device lifecycle. What are we missing? > I think a user-creatable --device vhost-user-device is not a good idea > today. It creates confusion. Many people aren't aware of the > architectural difference between vhost-user and VIRTIO devices. The > result is that VMMs and vhost-user backends implement increasingly > brittle VIRTIO configuration space and feature bit logic as they > knowingly or unknowingly try to paper over the fact that a traditional > vhost-user device isn't a full VIRTIO device. I've always found the device feature gating in QEMU confusing. Surely we can rely on the daemon to properly enumerate the features it supports? > It is possible to resolve this difference and make --device > vhost-user-device work properly for devices that want to be full > VIRTIO devices. See "Making VMM device shims optional" here: > https://blog.vmsplice.net/2020/09/on-unifying-vhost-user-and-virtio.html > > Even after extending the vhost-user protocol to solve the current > limitations, existing backends would still only be partial VIRTIO > devices that wouldn't work with --device vhost-user-device. It works for RNG, GPIO (and soon I'll be testing i2c and SCSI). All we need is the virtioid and the number of virtio queues. > Reducing boilerplate is helpful, but I think --device > vhost-user-device should not be user-creatable. After this series lands it will certainly make adding new shims easier but having a vhost-user-device will make testing of new backends easier. Can we not simply document it as an advanced feature for those who know what they are doing? I'm not intending to deprecate the existing shims. > > Stefan > >> >> Alex. >> >> Alex Bennée (12): >> hw/virtio: fix typo in VIRTIO_CONFIG_IRQ_IDX comments >> include/hw/virtio: document virtio_notify_config >> include/hw/virtio: add kerneldoc for virtio_init >> include/hw/virtio: document some more usage of notifiers >> virtio: add generic vhost-user-device >> virtio: add PCI stub for vhost-user-device >> include: attempt to document device_class_set_props >> qom: allow for properties to become "fixed" >> hw/virtio: derive vhost-user-rng from vhost-user-device >> hw/virtio: add config support to vhost-user-device >> hw/virtio: derive vhost-user-gpio from vhost-user-device (!BROKEN) >> docs/system: add a basic enumeration of vhost-user devices >> >> docs/system/devices/vhost-user-rng.rst | 2 + >> docs/system/devices/vhost-user.rst | 41 +++ >> qapi/qom.json | 2 + >> include/hw/qdev-core.h | 9 + >> include/hw/virtio/vhost-user-device.h | 33 ++ >> include/hw/virtio/vhost-user-gpio.h | 23 +- >> include/hw/virtio/vhost-user-rng.h | 11 +- >> include/hw/virtio/virtio.h | 21 ++ >> include/qom/object.h | 16 +- >> hw/display/vhost-user-gpu.c | 4 +- >> hw/net/virtio-net.c | 4 +- >> hw/virtio/vhost-user-device-pci.c | 71 +++++ >> hw/virtio/vhost-user-device.c | 359 ++++++++++++++++++++++ >> hw/virtio/vhost-user-fs.c | 4 +- >> hw/virtio/vhost-user-gpio.c | 405 +------------------------ >> hw/virtio/vhost-user-rng.c | 264 +--------------- >> hw/virtio/vhost-vsock-common.c | 4 +- >> hw/virtio/virtio-crypto.c | 4 +- >> qom/object.c | 14 + >> qom/object_interfaces.c | 9 +- >> qom/qom-qmp-cmds.c | 1 + >> softmmu/qdev-monitor.c | 1 + >> hw/virtio/meson.build | 3 + >> 23 files changed, 613 insertions(+), 692 deletions(-) >> create mode 100644 include/hw/virtio/vhost-user-device.h >> create mode 100644 hw/virtio/vhost-user-device-pci.c >> create mode 100644 hw/virtio/vhost-user-device.c >> >> -- >> 2.39.2 >> >>
On Mon, Apr 17, 2023 at 05:14:59PM +0100, Alex Bennée wrote: > > Stefan Hajnoczi <stefanha@gmail.com> writes: > > > On Fri, 14 Apr 2023 at 12:06, Alex Bennée <alex.bennee@linaro.org> wrote: > >> > >> A lot of our vhost-user stubs are large chunks of boilerplate that do > >> (mostly) the same thing. This series attempts to fix that by defining > >> a new base class for vhost-user devices and then converting the rng > >> and gpio devices to be based off them. You can even use > >> vhost-user-device directly if you supply it with the right magic > >> numbers (which is helpful for development). > >> > >> However the final patch runs into the weeds because I don't yet have a > >> clean way to represent in QOM the fixing of certain properties for the > >> specialised classes. > >> > >> The series is a net reduction in code and an increase in > >> documentation but obviously needs to iron out a few more warts. I'm > >> open to suggestions on the best way to tweak the QOM stuff. > > > > --device vhost-user-device is not really possible because vhost-user > > devices are not full VIRTIO devices. vhost-user devices depend on > > device-specific code in the VMM by design. > > What device specific code? You certainly need to instantiate stuff in > the DTB/ACPI tables for -M virt but everything else can be handed off to > the vhost-user daemon. There are vhost-user device types that lack functionality entirely, like vhost-user-net's lack of the controlq virtqueue and configuration space. It is not even possible to query the MAC address from a vhost-user-net device. It's not a full virtio-net device and the VMM has to fill in the gaps to emulate the missing virtqueues and configuration space. There are device type-specific vhost-user messages like VHOST_USER_SEND_RARP and VHOST_USER_CREATE_CRYPTO_SESSION that can't really be supported by a generic --device vhost-user-device. Live migration is another device-specific aspect that is handled by the vhost-user frontend. > Indeed the split brain is a bit silly in some places. For example is > QEMU really the best arbiter of a block device config when the actual > backend is a separate process. We have config passing in the vhost-user > spec. Optional vhost-user messages like configuration space access may be in the spec, but existing device types cannot rely on them for backwards compatibility reasons. Therefore most existing device types don't use the configuration space. Those that do may only access parts of the configuration space from the backend and emulate the rest. The blog post I linked mentions a new protocol feature bit (VHOST_USER_PROTOCOL_F_VDPA) to distinguish new vhost-user backends that are full VIRTIO devices. This doesn't exist today, but I think something like that is necessary to detect devices that will work with --device vhost-user-device. > > The "subset of a VIRTIO device" design made sense for vhost_net. > > Nowadays there are other device types that are close to full VIRTIO > > devices, although the vhost-user protocol doesn't support the full > > VIRTIO device lifecycle. > > What are we missing? vhost-user needs: - A GET_DEVICE_ID message. - A GET_CONFIG_SIZE message. Today it is assumed that the vhost-user frontend already knows the configuration space size. - A protocol feature bit indicating that the device is a full VIRTIO device. These devices also need to implement the SET_STATUS message, which is rarely implemented today. > > I think a user-creatable --device vhost-user-device is not a good idea > > today. It creates confusion. Many people aren't aware of the > > architectural difference between vhost-user and VIRTIO devices. The > > result is that VMMs and vhost-user backends implement increasingly > > brittle VIRTIO configuration space and feature bit logic as they > > knowingly or unknowingly try to paper over the fact that a traditional > > vhost-user device isn't a full VIRTIO device. > > I've always found the device feature gating in QEMU confusing. Surely we > can rely on the daemon to properly enumerate the features it supports? The backend's features cannot be passed through to the guest because there is also an emulated VIRTIO transport (virtio-pci, virtio-mmio, virtio-ccw) involved. The transport may support a different feature set from the vhost-user backend. For example, the transport may support Packed Virtqueues but the backend may not. So some filtering is necessary in the VMM. Since some of the device-specific functionality may be handled by the VMM, then this extends beyond just the transport feature bits. But I agree with you that it's ugly and complex. I have to re-read the code every time because it works in a strange way. > > It is possible to resolve this difference and make --device > > vhost-user-device work properly for devices that want to be full > > VIRTIO devices. See "Making VMM device shims optional" here: > > https://blog.vmsplice.net/2020/09/on-unifying-vhost-user-and-virtio.html > > > > Even after extending the vhost-user protocol to solve the current > > limitations, existing backends would still only be partial VIRTIO > > devices that wouldn't work with --device vhost-user-device. > > It works for RNG, GPIO (and soon I'll be testing i2c and SCSI). All we > need is the virtioid and the number of virtio queues. There is already a vhost-user message for querying the number of virtqueues: VHOST_USER_GET_QUEUE_NUM. See above for other missing stuff. > > Reducing boilerplate is helpful, but I think --device > > vhost-user-device should not be user-creatable. > > After this series lands it will certainly make adding new shims easier > but having a vhost-user-device will make testing of new backends easier. > Can we not simply document it as an advanced feature for those who know > what they are doing? I'm not intending to deprecate the existing shims. If you add the missing pieces to vhost-user then --device vhost-user-device can be done properly. Two messages and one protocol feature bit are required, it's not that bad. Stefan