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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 27 Sep 2019, Julien Grall wrote:
> Hi,
> 
> On 27/09/2019 15:40, Oleksandr wrote:
> > 
> > On 26.09.19 00:12, Julien Grall wrote:
> >> 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;
> >>> +        }
> > 
> > I have to admit that I don't know about dom0less feature enough ...
> > 
> > 
> > But, shouldn't we check if the device is behind the IOMMU and try to add 
> > it (iommu_add_dt_device) before assigning it (this is needed for drivers 
> > which support generic IOMMU DT bindings in the first place).
> > 
> > [please take a look at 
> > https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html 
> > if so]
> > 
> > Julien, what do you think?
> 
> Yes you are right.
> 
> @Stefano, this is a recently merged feature. Without it, you will not be 
> able to use passthrough with dom0less guest when the IOMMU (such as 
> IPMMU) is using the generic DT bindings.

Just double-checking but it should be only a matter of the following,
right?

+        res = iommu_add_dt_device(node);
+        if ( res < 0 )
+            return res;
+
+        if ( dt_device_is_protected(node) )
+        {
+            res = iommu_assign_dt_device(kinfo->d, node);
+            if ( res < 0 )
+                return res;
+        }
+

(I am asking because I couldn't quite test it due to the error with
mmu-masters I mentioned in the other email.)

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