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

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi Stefano,

On 25/09/2019 19:49, 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.
> 
> Also add the new flag XEN_DOMCTL_CDF_iommu to that dom0less domU can use
> the IOMMU.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> ---
> Changes in v5:
> - use local variable for name
> - use map_regions_p2mt
> - add warning for not page aligned addresses/sizes
> - introduce handle_passthrough_prop
> 
> Changes in v4:
> - use unsigned
> - improve commit message
> - code style
> - use dt_prop_cmp
> - use device_tree_get_reg
> - don't copy over xen,reg and xen,path
> - don't create special interrupt properties for domU: copy them from the
>    fragment
> - in-code comment
> 
> Changes in v3:
> - improve commit message
> - remove superfluous cast
> - merge code with the copy code
> - add interrup-parent
> - demove depth > 2 check
> - reuse code from handle_device_interrupts
> - copy interrupts from host dt
> 
> Changes in v2:
> - rename "path" to "xen,path"
> - grammar fix
> - use gaddr_to_gfn and maddr_to_mfn
> - remove depth <= 2 limitation in scanning the dtb fragment
> - introduce and parse xen,reg
> - code style
> - support more than one interrupt per device
> - specify only the GIC is supported
> ---
>   xen/arch/arm/domain_build.c | 101 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 97 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9d985d3bbe..414893bc24 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1701,6 +1701,85 @@ static int __init make_vpl011_uart_node(struct 
> kernel_info *kinfo)
>   }
>   #endif
>   
> +/*
> + * Scan device tree properties for passthrough specific information.
> + * Returns -ENOENT when no passthrough properties are found
> + *         < 0 on error
> + *         0 on success
> + */
> +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
> +                                          const struct fdt_property *prop,
> +                                          const char *name,
> +                                          uint32_t address_cells, uint32_t 
> size_cells)
> +{
> +    const __be32 *cell;
> +    unsigned int i, len;
> +    struct dt_device_node *node;
> +    int res;
> +
> +    /* xen,reg specifies where to map the MMIO region */
> +    if ( dt_prop_cmp("xen,reg", name) == 0 )
> +    {
> +        paddr_t mstart, size, gstart;
> +        cell = (const __be32 *)prop->data;
> +        len = fdt32_to_cpu(prop->len) /
> +            ((address_cells * 2 + size_cells) * sizeof(uint32_t));
> +
> +        for ( i = 0; i < len; i++ )
> +        {
> +            device_tree_get_reg(&cell, address_cells, size_cells,
> +                    &mstart, &size);
> +            gstart = dt_next_cell(address_cells, &cell);
> +
> +            if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size & 
> ~PAGE_MASK )
> +                dprintk(XENLOG_WARNING,
> +                        "DomU passthrough config has not page aligned 
> addresses/sizes\n");

I don't think this is wise to continue, the more this is a printk that 
can only happen in debug build. So someone using a release build may not 
notice the error.

So I think this wants to be a printk(XENLOG_ERR,...) and also return an 
error.

> +
> +            res = map_regions_p2mt(kinfo->d,
> +                    gaddr_to_gfn(gstart),
> +                    PFN_DOWN(size),
> +                    maddr_to_mfn(mstart),
> +                    p2m_mmio_direct_dev);

Coding style.

> +            if ( res < 0 )
> +            {
> +                dprintk(XENLOG_ERR,
> +                        "Failed to map %"PRIpaddr" to the guest 
> at%"PRIpaddr"\n",
> +                        mstart, gstart);

Similarly, this wants to be a printk.

> +                return -EFAULT;
> +            }
> +        }
> +
> +        return 0;
> +    }
> +    /*
> +     * xen,path specifies the corresponding node in the host DT.
> +     * Both interrupt mappings and IOMMU settings are based on it,
> +     * as they are done based on the corresponding host DT node.
> +     */
> +    else if ( dt_prop_cmp("xen,path", name) == 0 )
> +    {
> +        node = dt_find_node_by_path(prop->data);
> +        if ( node == NULL )
> +        {
> +            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
> +                    (char *)prop->data);

Same here.

> +            return -EINVAL;
> +        }
> +
> +        res = iommu_assign_dt_device(kinfo->d, node);
> +        if ( res < 0 )
> +            return res;
> +
> +        res = handle_device_interrupts(kinfo->d, node, true);
> +        if ( res < 0 )
> +            return res;
> +
> +        return 0;
> +    }
> +
> +    return -ENOENT;
> +}
> +
>   static int __init handle_prop_pfdt(struct kernel_info *kinfo,
>                                      const void *pfdt, int nodeoff,
>                                      uint32_t address_cells, uint32_t 
> size_cells,
> @@ -1709,6 +1788,7 @@ static int __init handle_prop_pfdt(struct kernel_info 
> *kinfo,
>       void *fdt = kinfo->fdt;
>       int propoff, nameoff, res;
>       const struct fdt_property *prop;
> +    const char *name;
>   
>       for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
>             propoff >= 0;
> @@ -1717,11 +1797,23 @@ static int __init handle_prop_pfdt(struct kernel_info 
> *kinfo,
>           if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
>               return -FDT_ERR_INTERNAL;
>   
> +        res = 0;
>           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 )
> +            res = handle_passthrough_prop(kinfo, prop, name,
> +                                          address_cells, size_cells);
> +        if ( res < 0 && res != -ENOENT )
>               return res;
> +
> +        /* copy all other properties */
> +        if ( !scan_passthrough_prop || res == -ENOENT )
> +        {
> +            res = fdt_property(fdt, name, prop->data, 
> fdt32_to_cpu(prop->len));
> +            if ( res )
> +                return res;
> +        }
>       }
>   
>       /* FDT_ERR_NOTFOUND => There is no more properties for this node */
> @@ -2254,7 +2346,8 @@ void __init create_domUs(void)
>           struct xen_domctl_createdomain d_cfg = {
>               .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
>               .arch.nr_spis = 0,
> -            .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> +            .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> +                     XEN_DOMCTL_CDF_iommu,

This will break dom0less on platform without an IOMMU because setting 
CDF_iommu for them will be invalid.

I also don't think this is wise to always enable the IOMMU. This should 
only be done if there is a partial device-tree present (most likely it 
means passthrough will be used).

>               .max_evtchn_port = -1,
>               .max_grant_frames = 64,
>               .max_maptrack_frames = 1024,
> 

Cheers,

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