Re: [PATCH 2/5] xen/arm: assign devices to boot domains

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> Scan the user provided dtb fragment at boot. For each device node, map
> memory to guests, and route interrupts and setup the iommu.
> 
> Device memory is only mapped 1:1. It is not possible to assign devices at
> locations that conflict with the DomU memory map.
I think 1:1 mapped is a pretty bad idea. You limit yourself a lot as the 
user does not control neither the HW nor the guest memory map.

So you need to provide a way for the user to specify the mapping.

> 
> The iommu is setup by passing the to the device to assign on the host

NIT: "the node to..."

> device tree. The path is specified in the device tree fragment as the
> "path" string property.

Path is too generic and may clash in the future with other bindings. 
Please add "xen," in front.

> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>   xen/arch/arm/bootfdt.c        |  4 +-
>   xen/arch/arm/domain_build.c   | 85 +++++++++++++++++++++++++++++++++++++++++++
>   xen/include/xen/device_tree.h |  2 +
>   3 files changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 891b4b6..72cb8d6 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -55,8 +55,8 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
>       return false;
>   }
>   
> -static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> -                                       u32 size_cells, u64 *start, u64 *size)
> +void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> +                                u32 size_cells, u64 *start, u64 *size)
>   {
>       *start = dt_next_cell(address_cells, cell);
>       *size = dt_next_cell(size_cells, cell);
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index cc6b464..d48f77e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2094,6 +2094,88 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
>       return 0;
>   }
>   
> +static int __init scan_pt_node(const void *pfdt,
> +                               int nodeoff, const char *name, int depth,
> +                               u32 address_cells, u32 size_cells,
> +                               void *data)

Is it really necessary to parse the device-tree twice?

> +{
> +    int rc;
> +    struct dt_device_node *node;
> +    int len, i;
> +    const struct fdt_property *prop;
> +    struct kernel_info *kinfo = data;
> +    struct domain *d = kinfo->d;
> +    const __be32 *cell;
> +
> +    if ( depth > 2 )
> +        return 0;

Why do you limit yourself to depth 2? It is possible to have nested node 
describing memory.

> +
> +    prop = fdt_get_property_namelen(pfdt, nodeoff, "reg", strlen("reg"), &len);
> +    if ( prop != NULL )
> +    {
> +        paddr_t start, size;
> +        cell = (const __be32 *)prop->data;
> +        len = fdt32_to_cpu(prop->len) /
> +              ((address_cells + size_cells) * sizeof (u32));
> +
> +        for ( i = 0; i < len; i++ )
> +        {
> +            device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);

Here you assume the value in regs correspond to host physical address. 
This may not be the case if a device is under a bus.

> +
> +            rc = guest_physmap_add_entry(d, _gfn(start >> PAGE_SHIFT),

Please use gaddr_to_gfn and ...

> +                                         _mfn(start >> PAGE_SHIFT),

... maddr_to_mfn

> +                                         get_order_from_bytes(size),
> +                                         p2m_mmio_direct_dev);

I think the restriction on the memory attributes should be documented in 
patch #5.

> +            if ( rc < 0 )
> +            {
> +                dprintk(XENLOG_ERR, "Failed to map %"PRIpaddr" to the guest\n", start);
> +                return -EFAULT;
> +            }
> +        }
> +    }
> +
> +    prop = fdt_get_property_namelen(pfdt, nodeoff, "path", strlen("path"), &len);
> +    if ( prop != NULL ) {
> +        node = dt_find_node_by_path((char *)&prop->data[0]);

What's wrong with giving directly prop->data?

> +        if ( node != NULL )
> +            rc = iommu_assign_dt_device(d, node);
> +        else
> +            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
> +                    (char *)&prop->data[0]);

Same here.

> +    }
> +
> +    prop = fdt_get_property_namelen(pfdt, nodeoff, "interrupts", strlen("interrupts"), &len);
> +    if ( prop != NULL )
> +    {
> +        int pt_irq;
> +        u32 *u = (u32*) &prop->data[0];

Same here for &prop->data[0]. But this stores a fdt_32 and not u32.

> +
> +        pt_irq = fdt32_to_cpu(*(u + 1)) + 32;
> +
> +        vgic_reserve_virq(d, pt_irq);
> +        rc = route_irq_to_guest(d, pt_irq, pt_irq, "routed IRQ");
> +        if ( rc < 0 )
> +            return rc;

You are assuming the device can only generate one interrupt. 
Furthermore, this is assuming all the nodes in the fragment will be 
under the GIC controller. You may want to passthrough a interrupt 
controller (i.e GPIO) to the guest and the related device.

> +    }

NIT: newline here.

> +    return 0;
> +}
> +
> +static int __init domain_addign_devices(struct domain *d,

s/addign/adding/

> +                                        struct kernel_info *kinfo)
> +{
> +    void *pfdt;
> +
> +    pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
> +            kinfo->dtb_bootmodule->size);
> +    if ( pfdt == NULL )
> +        return -EFAULT;
> +
> +    device_tree_for_each_node(pfdt, scan_pt_node, kinfo);
> +
> +    iounmap(pfdt);
> +    return 0;
> +}
> +
>   static int __init construct_domU(struct domain *d,
>                                    const struct dt_device_node *node)
>   {
> @@ -2140,6 +2222,9 @@ static int __init construct_domU(struct domain *d,
>       if ( kinfo.vpl011 )
>           rc = domain_vpl011_init(d, NULL);
>   
> +    if ( kinfo.dtb_bootmodule )
> +        rc = domain_addign_devices(d, &kinfo);
> +
>       return rc;
>   }
>   
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 7408a6c..356a422 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -161,6 +161,8 @@ extern const void *device_tree_flattened;
>   int device_tree_for_each_node(const void *fdt,
>                                        device_tree_node_func func,
>                                        void *data);
> +void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> +                         u32 size_cells, u64 *start, u64 *size);
>   
>   /**
>    * dt_unflatten_host_device_tree - Unflatten the host device tree
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel