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

[Juergen Gross <jgross@xxxxxxxx>]

On 27.04.22 10:27, Roger Pau Monné wrote:
> On Wed, Apr 27, 2022 at 11:20:04AM +0300, Oleksandr wrote:
> > On 25.04.22 16:12, Roger Pau Monné wrote:
> >
> >
> > Hello Roger
> >
> >
> > > 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>
> > > > > > --- 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.
> >
> > Here [1] we had a discussion regarding user configuration options.
> > "protocol" cannot be used as it will lead to the confusion (at least with
> > Xen PV block device which already has "protocol" frontend's entry in
> > Xenstore).
> >
> > Preliminary, we had agreed on the following names:
> > 1. specification: xen or virtio
> > 2. transport: mmio or pci
> >
> > Please tell me, are you OK with them?
> Yes, that's fine. My main point is that libxl_device_disk should
> contain both fields, so a 3rd party toolstack can set 'specification =
> virtio' and let Xen decide the transport to use.
>
> I'm dubious whether we want to have a xenbus transport for xen
> specification to use, but I guess it's fine to say that for
> specification == xen transport is ignored.
At least for now, I guess. It might be a thing in case someone decides
to add argo support.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature