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

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

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

FWIW the Golang changes look fine.

 -George

Attachment: signature.asc
Description: Message signed with OpenPGP