Re: [Xen-devel] [PATCH v7b 5/8] xen/arm: assign devices to boot domains

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 10/1/19 12:30 AM, 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.
>
> The memory region to remap is specified by the "xen,reg" property.
>
> The iommu is setup by passing the node of the device to assign on the
> host device tree. The path is specified in the device tree fragment as
> the "xen,path" string property.
>
> The interrupts are remapped based on the information from the
> corresponding node on the host device tree. Call
> handle_device_interrupts to remap interrupts. Interrupts related device
> tree properties are copied from the device tree fragment, same as all
> the other properties.
>
> Require both xen,reg and xen,path to be present, unless
> xen,force-assign-without-iommu is also set. In that case, tolerate a
> missing xen,path, also tolerate iommu setup failure for the passthrough
> device.
>
> Also set add the new flag XEN_DOMCTL_CDF_iommu so that dom0less domU
> can use the IOMMU if a partial dtb is specified.
The patch looks good a few comments below.

[...]

>   xen/arch/arm/domain_build.c | 133 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 129 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 84b65b8f25..47f9bb31df 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1714,6 +1714,88 @@ static int __init make_vpl011_uart_node(struct 
> kernel_info *kinfo)
>   }
>   #endif
>   
> +/*
> + * Scan device tree properties for passthrough specific information.
> + * Returns < 0 on error
> + *         0 on success
> + */
> +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
> +                                          const struct fdt_property *xen_reg,
> +                                          const struct fdt_property *xen_path,
> +                                          bool xen_force,
> +                                          uint32_t address_cells, uint32_t 
> size_cells)
> +{
> +    const __be32 *cell;
> +    unsigned int i, len;
> +    struct dt_device_node *node;
> +    int res;
> +    paddr_t mstart, size, gstart;
> +
> +    /* xen,reg specifies where to map the MMIO region */
> +    cell = (const __be32 *)xen_reg->data;
> +    len = fdt32_to_cpu(xen_reg->len) /
> +          ((address_cells * 2 + size_cells) * sizeof(uint32_t));
Coding style again. I was kind of expecting you configured your editor 
properly after the last discussion...
[...]

>   static int __init handle_prop_pfdt(struct kernel_info *kinfo,
>                                      const void *pfdt, int nodeoff,
>                                      uint32_t address_cells, uint32_t 
> size_cells,
> @@ -1721,7 +1803,9 @@ static int __init handle_prop_pfdt(struct kernel_info 
> *kinfo,
>   {
>       void *fdt = kinfo->fdt;
>       int propoff, nameoff, res;
> -    const struct fdt_property *prop;
> +    const struct fdt_property *prop, *xen_reg = NULL, *xen_path = NULL;
> +    const char *name;
> +    bool found, xen_force = false;
>   
>       for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
>             propoff >= 0;
> @@ -1730,10 +1814,48 @@ static int __init handle_prop_pfdt(struct kernel_info 
> *kinfo,
>           if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
>               return -FDT_ERR_INTERNAL;
>   
> +        found = false;
>           nameoff = fdt32_to_cpu(prop->nameoff);
> -        res = fdt_property(fdt, fdt_string(pfdt, nameoff),
> -                           prop->data, fdt32_to_cpu(prop->len));
> -        if ( res )
> +        name = fdt_string(pfdt, nameoff);
> +
> +        if ( scan_passthrough_prop )
> +        {
> +            if ( dt_prop_cmp("xen,reg", name) == 0 )
> +            {
> +                xen_reg = prop;
> +                found = true;
> +            }
> +            else if ( dt_prop_cmp("xen,path", name) == 0 )
> +            {
> +                xen_path = prop;
> +                found = true;
> +            }
> +            else if ( dt_prop_cmp("xen,force-assign-without-iommu",
> +                      name) == 0 )
Coding style.

> +            {
> +                xen_force = true;
> +                found = true;
> +            }
> +        }
> +
> +        /* Copy all other properties */
It is not entirely clear what you mean by "other" here.

> +        if ( !found )
> +        {
> +            res = fdt_property(fdt, name, prop->data, fdt32_to_cpu(prop->len));
> +            if ( res )
> +                return res;
> +        }
> +    }
> +
> +    /*
> +     * Only handle passthrough properties if both xen,reg and xen,path
> +     * are present, or if xen,force-assign-without-iommu is specified.
> +     */
> +    if ( xen_reg != NULL && (xen_path != NULL || xen_force) )
> +    {
> +        res = handle_passthrough_prop(kinfo, xen_reg, xen_path, xen_force,
> +                                      address_cells, size_cells);
> +        if ( res < 0 )
>               return res;
>       }
I would print an error so the user knows what happen here.

>   
> @@ -2291,6 +2413,9 @@ void __init create_domUs(void)
>               panic("Missing property 'cpus' for domain %s\n",
>                     dt_node_name(node));
>   
> +        if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
> +            d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> +
>           d = domain_create(++max_init_domid, &d_cfg, false);
>           if ( IS_ERR(d) )
>               panic("Error creating domain %s\n", dt_node_name(node));
Cheers,

--
Julien Grall

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