Re: [PATCH V2] libxl/arm: Create specific IOMMU node to be referred by virtio-mmio device

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 1 Jun 2022, Oleksandr wrote:
> On 01.06.22 04:04, Stefano Stabellini wrote:
> > On Tue, 31 May 2022, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > 
> > > Reuse generic IOMMU device tree bindings to communicate Xen specific
> > > information for the virtio devices for which the restricted memory
> > > access using Xen grant mappings need to be enabled.
> > > 
> > > Insert "iommus" property pointed to the IOMMU node with "xen,grant-dma"
> > > compatible to all virtio devices which backends are going to run in
> > > non-hardware domains (which are non-trusted by default).
> > > 
> > > Based on device-tree binding from Linux:
> > > Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml
> > > 
> > > The example of generated nodes:
> > > 
> > > xen_iommu {
> > >      compatible = "xen,grant-dma";
> > >      #iommu-cells = <0x01>;
> > >      phandle = <0xfde9>;
> > > };
> > > 
> > > virtio@2000000 {
> > >      compatible = "virtio,mmio";
> > >      reg = <0x00 0x2000000 0x00 0x200>;
> > >      interrupts = <0x00 0x01 0xf01>;
> > >      interrupt-parent = <0xfde8>;
> > >      dma-coherent;
> > >      iommus = <0xfde9 0x01>;
> > > };
> > > 
> > > virtio@2000200 {
> > >      compatible = "virtio,mmio";
> > >      reg = <0x00 0x2000200 0x00 0x200>;
> > >      interrupts = <0x00 0x02 0xf01>;
> > >      interrupt-parent = <0xfde8>;
> > >      dma-coherent;
> > >      iommus = <0xfde9 0x01>;
> > > };
> > > 
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > ---
> > > !!! This patch is based on non upstreamed yet “Virtio support for
> > > toolstack
> > > on Arm” V8 series which is on review now:
> > > https://lore.kernel.org/xen-devel/1651598763-12162-1-git-send-email-olekstysh@xxxxxxxxx/
> > > 
> > > New device-tree binding (commit #5) is a part of solution to restrict
> > > memory
> > > access under Xen using xen-grant DMA-mapping layer (which is also on
> > > review):
> > > https://lore.kernel.org/xen-devel/1653944417-17168-1-git-send-email-olekstysh@xxxxxxxxx/
> > > 
> > > Changes RFC -> V1:
> > >     - update commit description
> > >     - rebase according to the recent changes to
> > >       "libxl: Introduce basic virtio-mmio support on Arm"
> > > 
> > > Changes V1 -> V2:
> > >     - Henry already gave his Reviewed-by, I dropped it due to the changes
> > >     - use generic IOMMU device tree bindings instead of custom property
> > >       "xen,dev-domid"
> > >     - change commit subject and description, was
> > >       "libxl/arm: Insert "xen,dev-domid" property to virtio-mmio device
> > > node"
> > > ---
> > >   tools/libs/light/libxl_arm.c          | 49
> > > ++++++++++++++++++++++++++++++++---
> > >   xen/include/public/device_tree_defs.h |  1 +
> > >   2 files changed, 47 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > > index 9be9b2a..72da3b1 100644
> > > --- a/tools/libs/light/libxl_arm.c
> > > +++ b/tools/libs/light/libxl_arm.c
> > > @@ -865,9 +865,32 @@ static int make_vpci_node(libxl__gc *gc, void *fdt,
> > >       return 0;
> > >   }
> > >   +static int make_xen_iommu_node(libxl__gc *gc, void *fdt)
> > > +{
> > > +    int res;
> > > +
> > > +    /* See Linux
> > > Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml */
> > > +    res = fdt_begin_node(fdt, "xen_iommu");
> > > +    if (res) return res;
> > > +
> > > +    res = fdt_property_compat(gc, fdt, 1, "xen,grant-dma");
> > > +    if (res) return res;
> > > +
> > > +    res = fdt_property_cell(fdt, "#iommu-cells", 1);
> > > +    if (res) return res;
> > > +
> > > +    res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_IOMMU);
> > > +    if (res) return res;
> > > +
> > > +    res = fdt_end_node(fdt);
> > > +    if (res) return res;
> > > +
> > > +    return 0;
> > > +}
> > >     static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
> > > -                                 uint64_t base, uint32_t irq)
> > > +                                 uint64_t base, uint32_t irq,
> > > +                                 uint32_t backend_domid)
> > >   {
> > >       int res;
> > >       gic_interrupt intr;
> > > @@ -890,6 +913,16 @@ static int make_virtio_mmio_node(libxl__gc *gc, void
> > > *fdt,
> > >       res = fdt_property(fdt, "dma-coherent", NULL, 0);
> > >       if (res) return res;
> > >   +    if (backend_domid != LIBXL_TOOLSTACK_DOMID) {
> > > +        uint32_t iommus_prop[2];
> > > +
> > > +        iommus_prop[0] = cpu_to_fdt32(GUEST_PHANDLE_IOMMU);
> > > +        iommus_prop[1] = cpu_to_fdt32(backend_domid);
> > > +
> > > +        res = fdt_property(fdt, "iommus", iommus_prop,
> > > sizeof(iommus_prop));
> > > +        if (res) return res;
> > > +    }
> > > +
> > >       res = fdt_end_node(fdt);
> > >       if (res) return res;
> > >   @@ -1097,6 +1130,7 @@ static int libxl__prepare_dtb(libxl__gc *gc,
> > > libxl_domain_config *d_config,
> > >       size_t fdt_size = 0;
> > >       int pfdt_size = 0;
> > >       libxl_domain_build_info *const info = &d_config->b_info;
> > > +    bool iommu_created;
> > >       unsigned int i;
> > >         const libxl_version_info *vers;
> > > @@ -1204,11 +1238,20 @@ next_resize:
> > >           if (d_config->num_pcidevs)
> > >               FDT( make_vpci_node(gc, fdt, ainfo, dom) );
> > >   +        iommu_created = false;
> > >           for (i = 0; i < d_config->num_disks; i++) {
> > >               libxl_device_disk *disk = &d_config->disks[i];
> > >   -            if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO)
> > > -                FDT( make_virtio_mmio_node(gc, fdt, disk->base,
> > > disk->irq) );
> > > +            if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
> > > +                if (disk->backend_domid != LIBXL_TOOLSTACK_DOMID &&
> > > +                    !iommu_created) {
> > > +                    FDT( make_xen_iommu_node(gc, fdt) );
> > > +                    iommu_created = true;
> > > +                }
> > > +
> > > +                FDT( make_virtio_mmio_node(gc, fdt, disk->base,
> > > disk->irq,
> > > +                                           disk->backend_domid) );
> > > +            }
> > This is a matter of taste as the code would also work as is, but I would
> > do the following instead:
> > 
> > 
> > if ( d_config->num_disks > 0 &&
> >       disk->backend_domid != LIBXL_TOOLSTACK_DOMID) {
> >       FDT( make_xen_iommu_node(gc, fdt) );
> > }
> > 
> > for (i = 0; i < d_config->num_disks; i++) {
> >      libxl_device_disk *disk = &d_config->disks[i];
> > 
> >      if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO)
> >          FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq) );
> > }
> 
> I got your idea to avoid using local "iommu_created". For that, I think, we
> need to modify the first check to make sure that we have at least one virtio
> device, otherwise we might end up inserting unused IOMMU node. But, it will
> turn into an extra loop to go through num_disks and look for
> LIBXL_DISK_SPECIFICATION_VIRTIO.

I see, then just keep it as is.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>