Re: [PATCH V10 1/3] libxl: Add support for Virtio disk configuration

[Oleksandr <olekstysh@xxxxxxxxx>]

On 30.06.22 15:24, Juergen Gross wrote:


Hello Juergen

> On 30.06.22 14:18, Oleksandr wrote:
> > Dear all.
> >
> >
> > On 25.06.22 17:32, Oleksandr wrote:
> > > On 24.06.22 15:59, George Dunlap wrote:
> > >
> > > Hello George
> > >
> > > > > On 17 Jun 2022, at 17:14, Oleksandr Tyshchenko 
> > > > > <olekstysh@xxxxxxxxx> wrote:
> > > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > > >
> > > > > This patch adds basic support for configuring and assisting 
> > > > > virtio-mmio
> > > > > based virtio-disk backend (emulator) 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 currently ("vdev"
> > > > >    is not passed to the frontend). But this might need to be revised
> > > > >    in future, so frontend data might be written to Xenstore in 
> > > > > order to
> > > > >    support hotplugging virtio devices or passing the backend 
> > > > > domain id
> > > > >    on arch where the device-tree is not available.
> > > > > - 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 "specification" and "transport" fields to struct
> > > > >   libxl_device_disk. Both are written to the Xenstore. The transport
> > > > >   field is only used for the specification "virtio" and it assumes
> > > > >   only "mmio" value for now.
> > > > > - Introduce new "specification" option with "xen" communication
> > > > >   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, 
> > > > > specification=virtio']
> > > > > 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>
> > > > OK, I am *really* sorry for coming in here at the last minute and 
> > > > quibbling about things.
> > >
> > > no problem
> > >
> > >
> > > >   But this introduces a public interface which looks really wrong 
> > > > to me.  I’ve replied to the mail from December where Juergen 
> > > > proposed the “Other” protocol.
> > > > Hopefully this will be a simple matter of finding a better name 
> > > > than “other”.  (Or you guys convincing me that “other” is really 
> > > > the best name for this value; or even Anthony asserting his right 
> > > > as a maintainer to overrule my objection if he thinks I’m out of 
> > > > line.)
> > >
> > > I saw your reply to V6 and Juergen's answer. I share Juergen's 
> > > opinion as well as I understand your concern. I think, this is 
> > > exactly the situation when finding a perfect name (obvious, short, 
> > > etc) for the backendtype (in our particular case) is really difficult.
> > > Personally I tend to leave "other", because there is no better 
> > > alternative from my PoV. Also I might be completely wrong here, but 
> > > I don't think we will have to extend the "backendtype" for 
> > > supporting other possible virtio backend implementations in the 
> > > foreseeable future:
> > > - when Qemu gains the required support we will choose qdisk: 
> > > backendtype qdisk specification virtio
> > > - for the possible virtio alternative of the blkback we will choose 
> > > phy: backendtype phy specification virtio
> > > If there will be a need to keep various implementation, we will be 
> > > able to describe that by using "transport" (mmio, pci, xenbus, argo, 
> > > whatever).
> > > Actually this is why we also introduced "specification" and 
> > > "transport".
> > > IIRC, there were other (suggested?) names except "other" which are 
> > > "external" and "daemon". If you think that one of them is better 
> > > than "other", I will happy to use it.
> >
> > Could we please make a decision on this?
> >
> > If "other" is not unambiguous, then maybe we could choose "daemon" to 
> > describe arbitrary user-level backends, any thought?
> IMO this would exclude other cases, like special kernel drivers.

I got it.


> Maybe "standalone"? "only-relying-on-xenstore-data" would be a bit
> exaggerated, while conveying the idea quite nicely.

"standalone" sounds good to me, thank you. I will wait a little bit for 
other opinions before making changes.

> Juergen
--
Regards,

Oleksandr Tyshchenko