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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 22.04.22 12:41, Roger Pau Monné wrote:


Hello Roger

> On Fri, Apr 08, 2022 at 09:21:04PM +0300, 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)
> >   - 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)
> > - 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']
> >
> > 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>
> > ---
> > Changes RFC -> V1:
> >     - no changes
> >
> > Changes V1 -> V2:
> >     - rebase according to the new location of libxl_virtio_disk.c
> >
> > Changes V2 -> V3:
> >     - no changes
> >
> > Changes V3 -> V4:
> >     - rebase according to the new argument for DEFINE_DEVICE_TYPE_STRUCT
> >
> > Changes V4 -> V5:
> >     - split the changes, change the order of the patches
> >     - update patch description
> >     - don't introduce new "vdisk" configuration option with own parsing logic,
> >       re-use Xen PV block "disk" parsing/configuration logic for the virtio-disk
> >     - introduce "virtio" flag and document it's usage
> >     - add LIBXL_HAVE_DEVICE_DISK_VIRTIO
> >     - update libxlu_disk_l.[ch]
> >     - drop num_disks variable/MAX_VIRTIO_DISKS
> >     - drop Wei's T-b
> >
> > Changes V5 -> V6:
> >     - rebase on current staging
> >     - use "%"PRIu64 instead of %lu for disk->base in device_disk_add()
> >     - update *.gen.go files
> >
> > Changes V6 -> V7:
> >     - rebase on current staging
> >     - update *.gen.go files and libxlu_disk_l.[ch] files
> >     - update patch description
> >     - rework significantly to support more flexible configuration
> >       and have more generic basic implementation for being able to extend
> >       that for other use-cases (virtio-pci, qemu, etc).
> > ---
> >   docs/man/xl-disk-configuration.5.pod.in   |  37 +-
> >   tools/golang/xenlight/helpers.gen.go      |   6 +
> >   tools/golang/xenlight/types.gen.go        |  11 +
> >   tools/include/libxl.h                     |   6 +
> >   tools/libs/light/libxl_device.c           |  57 +-
> >   tools/libs/light/libxl_disk.c             | 111 +++-
> >   tools/libs/light/libxl_internal.h         |   1 +
> >   tools/libs/light/libxl_types.idl          |  10 +
> >   tools/libs/light/libxl_types_internal.idl |   1 +
> >   tools/libs/light/libxl_utils.c            |   2 +
> >   tools/libs/util/libxlu_disk_l.c           | 952 +++++++++++++++---------------
> >   tools/libs/util/libxlu_disk_l.h           |   2 +-
> >   tools/libs/util/libxlu_disk_l.l           |   9 +
> >   tools/xl/xl_block.c                       |  11 +
> >   14 files changed, 736 insertions(+), 480 deletions(-)
> >
> > diff --git a/docs/man/xl-disk-configuration.5.pod.in 
> > b/docs/man/xl-disk-configuration.5.pod.in
> > index 71d0e86..36c851f 100644
> > --- a/docs/man/xl-disk-configuration.5.pod.in
> > +++ b/docs/man/xl-disk-configuration.5.pod.in
> > @@ -232,7 +232,7 @@ Specifies the backend implementation to use
> >   
> >   =item Supported values
> >   
> > -phy, qdisk
> > +phy, qdisk, other
> >   
> >   =item Mandatory
> >   
> > @@ -244,11 +244,13 @@ Automatically determine which backend to use.
> >   
> >   =back
> >   
> > -This does not affect the guest's view of the device.  It controls
> > -which software implementation of the Xen backend driver is used.
> > +It controls which software implementation of the backend driver is used.
> > +Depending on the "protocol" option this may affect the guest's view
> > +of the device.
> >   
> >   Not all backend drivers support all combinations of other options.
> > -For example, "phy" does not support formats other than "raw".
> > +For example, "phy" and "other" do not support formats other than "raw" and
> > +"other" does not support protocols other than "virtio-mmio".
> >   Normally this option should not be specified, in which case libxl will
> >   automatically determine the most suitable backend.
> >   
> > @@ -344,8 +346,35 @@ can be used to disable "hole punching" for file based backends which
> >   were intentionally created non-sparse to avoid fragmentation of the
> >   file.
> >   
> > +=item B<protocol>=I<PROTOCOL>
> > +
> > +=over 4
> > +
> > +=item Description
> > +
> > +Specifies the communication protocol to use for the chosen "backendtype" option
> > +
> > +=item Supported values
> > +
> > +xen, virtio-mmio
>  From a user PoV, I think it would be better to just select xen or
> virtio here, but not the underlying configuration mechanism used to
> expose the devices to the guest.
I got your point.



> We would likely need to add a different option to select mmio or pci
> then, but that should be set by default based on architecture/guest
> type.  For example on x86 it should default to pci, while on Arm I
> guess it will depend on whether the guest has PCI or not?
>
> In any case, I think we should offer an option that's selecting
> between xen or virtio protocol, and the way to expose the
> configuration of the device shouldn't need to be explicitly selected
> by the user.

ok, for now I will use "xen and virtio" values for the "protocol" 
option, then internally toolstack will assume that "virtio" really means 
"virtio-mmio".
When there is a need to expand that support to "virtio-pci", we will see 
how to deal with it from the configuration PoV, probably like you 
suggested above by adding another option (e.g. "transport") with default 
values based on the architecture/guest type.

> Thanks, Roger.
--
Regards,

Oleksandr Tyshchenko