[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH V2] libxl/arm: Create specific IOMMU node to be referred by virtio-mmio device
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>
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |