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

[Stefano Stabellini <sstabellini@kernel.org>]

On Sun, 29 Sep 2019, Julien Grall wrote:
> Hi,
> 
> On 9/27/19 12:11 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.
> > 
> > Also set add the new flag XEN_DOMCTL_CDF_iommu so that dom0less domU
> > can use the IOMMU if a partial dtb is specified.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > Changes in v6:
> > - turn dprintks into printks
> > - return error on page alignment check failure
> > - set XEN_DOMCTL_CDF_iommu if partial dtb is specified
> > 
> > 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 | 104 ++++++++++++++++++++++++++++++++++--
> >   1 file changed, 101 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 08d6d238e3..a461816345 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1699,6 +1699,88 @@ 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
> 
> Such things only work if you control the return value. Looking at the code,
> you propagate the value from iommu_assign_dt_device(). While today we return
> -EINVAL when the device is not protected, it may make sense to return -ENOENT
> (after all the device is not behind an IOMMU and therefore technically not
> found).
> 
> You would end up to present the property when it should not. So it would be
> better to consider returning a positive value when the property needs to be
> copied.

Yes, I understand your point and it is valid. I'll make the change. I'll
return "1" and also change this comment on top of the function.


> > + *         < 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;
> 
> NIT: Newline between declaration and code.
> 
> > +        cell = (const __be32 *)prop->data;
> > +        len = fdt32_to_cpu(prop->len) /
> > +            ((address_cells * 2 + size_cells) * sizeof(uint32_t));
> 
> NIT: The indentation looks incorrect. I missed this one on the previous
> review.
> 
> > +
> > +        for ( i = 0; i < len; i++ )
> > +        {
> > +            device_tree_get_reg(&cell, address_cells, size_cells,
> > +                    &mstart, &size);
> 
> Same here. It might be worth checking your editor configuration to avoid such
> things happening in the future...

Yes, my editor doesn't do Xen alignment right. I have fixed these three
instances.


> > +            gstart = dt_next_cell(address_cells, &cell);
> > +
> > +            if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size &
> > ~PAGE_MASK )
> > +            {
> > +                printk(XENLOG_ERR
> > +                       "DomU passthrough config has not page aligned
> > addresses/sizes\n");
> > +                return -EINVAL;
> > +            }
> > +
> > +            res = map_regions_p2mt(kinfo->d,
> > +                                   gaddr_to_gfn(gstart),
> > +                                   PFN_DOWN(size),
> > +                                   maddr_to_mfn(mstart),
> > +                                   p2m_mmio_direct_dev);
> > +            if ( res < 0 )
> > +            {
> > +                printk(XENLOG_ERR
> > +                       "Failed to map %"PRIpaddr" to the guest
> > at%"PRIpaddr"\n",
> > +                       mstart, gstart);
> > +                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 )
> > +        {
> > +            printk(XENLOG_ERR "Couldn't find node %s in host_dt!\n",
> > +                   (char *)prop->data);
> > +            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;
> 
> Same here.

You are talking about return values, right? Not code style to be fixed
-- I cannot see anything wrong with the code style.


> > +
> > +        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,
> > @@ -1707,6 +1789,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;
> > @@ -1715,11 +1798,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 */
> 
> As you now moved the code to passthrough properties in a separate function, it
> now a bit unclear what "other" mean here.

Yes, indeed. I'll use:

+
+        /*
+         * Copy properties other than xen,reg and xen,path, which are
+         * handled by handle_passthrough_prop.
+         */


> > +        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 */
> > @@ -2277,6 +2372,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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel