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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 25 Sep 2019, Julien Grall wrote:
> 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.

I'll fix the code style issues, use printk instead of dprintk and return
error here.


> > +
> > +            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).

That's interesting, good idea, I'll do that

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