On Wed, Dec 08, 2021 at 06:59:43PM +0200, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
>
> 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.
> 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?
> 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.
> 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.
Thanks,
--
Anthony PERARD
