Re: [PATCH V7 2/2] libxl: Introduce basic virtio-mmio support on Arm

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 19 Apr 2022, Oleksandr wrote:
> On 19.04.22 00:41, Stefano Stabellini wrote:
> Hello Stefano
> 
> > On Fri, 8 Apr 2022, Oleksandr Tyshchenko wrote:
> > > From: Julien Grall <julien.grall@xxxxxxx>
> > > 
> > > This patch introduces helpers to allocate Virtio MMIO params
> > > (IRQ and memory region) and create specific device node in
> > > the Guest device-tree with allocated params. In order to deal
> > > with multiple Virtio devices, reserve corresponding ranges.
> > > For now, we reserve 1MB for memory regions and 10 SPIs.
> > > 
> > > As these helpers should be used for every Virtio device attached
> > > to the Guest, call them for Virtio disk(s).
> > > 
> > > Please note, with statically allocated Virtio IRQs there is
> > > a risk of a clash with a physical IRQs of passthrough devices.
> > > For the first version, it's fine, but we should consider allocating
> > > the Virtio IRQs automatically. Thankfully, we know in advance which
> > > IRQs will be used for passthrough to be able to choose non-clashed
> > > ones.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > Tested-by: Jiamei Xie <Jiamei.xie@xxxxxxx>
> > > Reviewed-by: Henry Wang <Henry.Wang@xxxxxxx>
> > I realize that we are at v7 and I haven't reviewed this before, so I'll
> > limit my comments. I'll clarify the ones that I think are more important
> > from the one that are less important.
> 
> thank you
> 
> 
> > 
> > 
> > > ---
> > > @Jiamei, @Henry I decided to leave your T-b and R-b tags with the minor
> > > change I made, are you still happy with that?
> > > 
> > > s/if (disk->virtio)/if (disk->protocol == LIBXL_DISK_PROTOCOL_VIRTIO_MMIO)
> > > 
> > > Please note, this is a split/cleanup/hardening of Julien's PoC:
> > > "Add support for Guest IO forwarding to a device emulator"
> > > 
> > > Changes RFC -> V1:
> > >     - was squashed with:
> > >       "[RFC PATCH V1 09/12] libxl: Handle virtio-mmio irq in more correct
> > > way"
> > >       "[RFC PATCH V1 11/12] libxl: Insert "dma-coherent" property into
> > > virtio-mmio device node"
> > >       "[RFC PATCH V1 12/12] libxl: Fix duplicate memory node in DT"
> > >     - move VirtIO MMIO #define-s to xen/include/public/arch-arm.h
> > > 
> > > Changes V1 -> V2:
> > >     - update the author of a patch
> > > 
> > > Changes V2 -> V3:
> > >     - no changes
> > > 
> > > Changes V3 -> V4:
> > >     - no changes
> > > 
> > > Changes V4 -> V5:
> > >     - split the changes, change the order of the patches
> > >     - drop an extra "virtio" configuration option
> > >     - update patch description
> > >     - use CONTAINER_OF instead of own implementation
> > >     - reserve ranges for Virtio MMIO params and put them
> > >       in correct location
> > >     - create helpers to allocate Virtio MMIO params, add
> > >       corresponding sanity-сhecks
> > >     - add comment why MMIO size 0x200 is chosen
> > >     - update debug print
> > >     - drop Wei's T-b
> > > 
> > > Changes V5 -> V6:
> > >     - rebase on current staging
> > > 
> > > Changes V6 -> V7:
> > >     - rebase on current staging
> > >     - add T-b and R-b tags
> > >     - update according to the recent changes to
> > >       "libxl: Add support for Virtio disk configuration"
> > > ---
> > >   tools/libs/light/libxl_arm.c  | 131
> > > +++++++++++++++++++++++++++++++++++++++++-
> > >   xen/include/public/arch-arm.h |   7 +++
> > >   2 files changed, 136 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > > index eef1de0..8132a47 100644
> > > --- a/tools/libs/light/libxl_arm.c
> > > +++ b/tools/libs/light/libxl_arm.c
> > > @@ -8,6 +8,56 @@
> > >   #include <assert.h>
> > >   #include <xen/device_tree_defs.h>
> > >   +/*
> > > + * There is no clear requirements for the total size of Virtio MMIO
> > > region.
> > > + * The size of control registers is 0x100 and device-specific
> > > configuration
> > > + * registers starts at the offset 0x100, however it's size depends on the
> > > device
> > > + * and the driver. Pick the biggest known size at the moment to cover
> > > most
> > > + * of the devices (also consider allowing the user to configure the size
> > > via
> > > + * config file for the one not conforming with the proposed value).
> > > + */
> > > +#define VIRTIO_MMIO_DEV_SIZE   xen_mk_ullong(0x200)
> > Actually, I don't think we need to make this generic. We only support
> > virtio-disk now and I think it is fine if this was called
> > VIRTIO_DISK_MMIO_DEV_SIZE and the size was exactly the size needed by
> > virtio-disk. When/if we support another virtio protocol we can add set
> > the appropriate size of that one as well.
> 
> I would prefer if we keep this generic, although we are going to support
> virtio-blk as the first virtio-mmio device, there is nothing disk specific in
> that value. The same value (0x200) is used
> by SW which also generates virtio-mmio device tree nodes like we do in Xen
> libxl, I am speaking about kvmtool, Qemu here.

OK