[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.