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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Sat, Apr 23, 2022 at 10:39:14AM +0300, Oleksandr wrote:
> 
> 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.

I think this likely also wants to be a separate field in libxl_device_disk,
which could be left empty and libxl will attempt to set a default.
For example have the following in libxl_types.idl:

libxl_device_protocol = Enumeration("device_protocol", [
    (0, "UNKNOWN"),
    (1, "XEN"),
    (2, "VIRTIO"),
    ])

libxl_device_configuration = Enumeration("device_configuration", [
    (0, "UNKNOWN"),
    (1, "XENBUS"),
    (2, "MMIO"),
    ])

libxl_device_disk = Struct("device_disk", [
    ("protocol", libxl_device_protocol),
    ("configuration", libxl_device_configuration),
    ])

I don't like libxl_device_configuration much, I think is too generic,
but I can't think of anything better.  Maybe others can provide better
names.

Thanks, Roger.