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

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

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.

Thanks, Roger.