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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 14.12.21 18:03, Anthony PERARD wrote:

Hi Anthony


> On Wed, Dec 08, 2021 at 06:59:43PM +0200, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > This patch adds basic support for configuring and assisting 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 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.
> How backend are intended to be created? Is there something listening on
> xenstore?
>
> You mention QEMU as been the backend, do you intend to have QEMU
> listening on xenstore to create a virtio backend? Or maybe it is on the
> command line? There is QMP as well, but it's probably a lot more
> complicated as I think libxl needs refactoring for that.

No, QEMU is not involved there. The backend is a standalone application,
it is launched from the command line. The backend reads the Xenstore to get
the configuration and to detect when guest with the frontend is 
created/destroyed.

> > Besides introducing new disk backend type (LIBXL_DISK_BACKEND_VIRTIO)
> > introduce new device kind (LIBXL__DEVICE_KIND_VIRTIO_DISK) as current
> > one (LIBXL__DEVICE_KIND_VBD) doesn't fit into Virtio disk model.
> >
> > In order to inform the toolstack that Virtio disk needs to be used
> > extend "disk" configuration by introducing new "virtio" flag.
> > An example of domain configuration:
> > disk = [ 'backend=DomD, phy:/dev/mmcblk1p3, xvda1, rw, virtio' ]
> This new "virtio" flags feels strange. Would having something like
> "backendtype=virtio" works?

IIRC I considered "backendtype=virtio" option, but decided to go "an 
extra virtio flag" route, however I don't remember what exactly affected 
my decision.
But, I see your point and agree, I will analyze whether we can use 
"backendtype=virtio", I hope that we can, but need to make sure.

> > 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 feels like the patches are in the wrong order. I don't think it is
> a good idea to allow to create broken configuration until a follow-up
> patch fixes things.
Yes, I also think this is not an ideal splitting, so I decided to write 
a few sentences to draw reviewer's attention to this.
The problem is that second patch adds Arm bits which are local to 
libs/light/libxl_arm.c and if I put it before the current one I will 
break the bisectability
as there will be no callers yet.


> > diff --git a/tools/xl/xl_block.c b/tools/xl/xl_block.c
> > index 70eed43..50a4d45 100644
> > --- a/tools/xl/xl_block.c
> > +++ b/tools/xl/xl_block.c
> > @@ -50,6 +50,11 @@ int main_blockattach(int argc, char **argv)
> >           return 0;
> >       }
> >   
> > +    if (disk.virtio) {
> > +        fprintf(stderr, "block-attach is not supported for Virtio device\n");
> > +        return 1;
> > +    }
> This might not be the right place. libxl might want to prevent hotplug
> instead.
Could you please point me the right place where to prevent hotplug?


Thank you.


> Thanks,
--
Regards,

Oleksandr Tyshchenko