Re: [PATCH V7 1/2] libxl: Add support for Virtio disk configuration

[Juergen Gross <jgross@xxxxxxxx>]

On 08.04.22 20:21, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> This patch adds basic support for configuring and assisting virtio-mmio
> based virtio-disk backend (emualator) which is intended to run out of
> Qemu and could be run in any domain.
> Although the Virtio block device is quite different from traditional
> Xen PV block device (vbd) from the toolstack's point of view:
>   - as the frontend is virtio-blk which is not a Xenbus driver, nothing
>     written to Xenstore are fetched by the frontend (the vdev is not
>     passed to the frontend)
I thought about the future support on x86.

There we don't have a device tree (and I don't want to introduce it),
so the only ways to specify the backend domain id would be to:

- add some information to ACPI tables
- use boot parameters
- use Xenstore

Thinking further of hotplugging virtio devices, Xenstore seems to be the
only real suitable alternative. Using virtio mechanisms doesn't seem
appropriate, as such information should be retrieved in "platform
specific" ways (see e.g. specifying an "endpoint" in the virtio IOMMU
device [1], [2]). I think the Xenstore information for that purpose
could be rather minimal and it should be device-type agnostic. Having
just a directory with endpoints and associated backend domids would
probably be enough (not needed in this series, of course).

This doesn't preclude the device tree variant you are using, as this
would be required for dom0less systems anyway.

OTOH I'd like you to modify the commit message to make it more clear
that in future frontend data might be written to Xenstore in order to
support other use cases.

>   - the ring-ref/event-channel are not used for the backend<->frontend
>     communication, the proposed IPC for Virtio is IOREQ/DM
> it is still a "block device" and ought to be integrated in existing
> "disk" handling. So, re-use (and adapt) "disk" parsing/configuration
> logic to deal with Virtio devices as well.
>
> For the immediate purpose and an ability to extend that support for
> other use-cases in future (Qemu, virtio-pci, etc) perform the following
> actions:
> - Add new disk backend type (LIBXL_DISK_BACKEND_OTHER) and reflect
>    that in the configuration
> - Introduce new disk protocol field to libxl_device_disk struct
>    (with LIBXL_DISK_PROTOCOL_XEN and LIBXL_DISK_PROTOCOL_VIRTIO_MMIO
>    types) and reflect that in the configuration (new "protocol" option
>    with "xen" protocol being default value)
And with the hotplug option in mind I start to feel unueasy with naming
the new Xenstore node "protocol", as the frontend disk nodes for "normal"
disks already have a "protocol" entry specifying 64- or 32-bit protocol.

Maybe we should really name it "transport" instead?

> - Add new device kind (LIBXL__DEVICE_KIND_VIRTIO_DISK) as current
>    one (LIBXL__DEVICE_KIND_VBD) doesn't fit into Virtio disk model
>
> An example of domain configuration for Virtio disk:
> disk = [ 'phy:/dev/mmcblk0p3, xvda1, backendtype=other, protocol=virtio-mmio']
With Roger's feedback this would then be "transport=virtio", the "mmio"
part should then be something like "adapter=mmio" (in contrast to
"adapter=pci"), and "adapter" only needed in case of a device tree and
PCI being available.

> Nothing has changed for default Xen disk configuration.
>
> Please note, this patch is not enough for virtio-disk to work
> on Xen (Arm), as for every Virtio device (including disk) we need
> to allocate Virtio MMIO params (IRQ and memory region) and pass
> them to the backend, also update Guest device-tree. The subsequent
> patch will add these missing bits. For the current patch,
> the default "irq" and "base" are just written to the Xenstore.
> This is not an ideal splitting, but this way we avoid breaking
> the bisectability.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
I'm fine with the overall approach and couldn't spot any real issues
in the code.


Juergen

[1]: https://github.com/oasis-tcs/virtio-spec/blob/master/virtio-iommu.tex
[2]: https://medium.com/@michael2012zhao_67085/virtio-iommu-789369049443

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature