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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 14.12.21 19:44, Oleksandr wrote:

Hi Anthony

> 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.

I have just rechecked/experimented, we can use "backendtype=virtio" 
instead of an extra "virtio" flags.
disk = [ 'backend=DomD, phy:/dev/mmcblk0p3, xvda1, backendtype=virtio' ]

Also backendtype section in xl-disk-configuration.5.pod.in really wants 
updating as
at least the first sentence is not true for virtio backend:

"This does not affect the guest's view of the device.  It controls
which software implementation of the Xen backend driver is used."

So shall I go this direction?



> > > 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