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

[Oleksandr <olekstysh@gmail.com>]

On 01.06.22 23:39, Stefano Stabellini wrote:

Hello Stefano

> 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@epam.com>
>>>>
>>>> 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@epam.com>
>>>> ---
>>>> !!! 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@gmail.com/
>>>>
>>>> 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@gmail.com/
>>>>
>>>> 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.

ok


>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks!


-- 
Regards,

Oleksandr Tyshchenko