|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 5/8] xen/arm: assign devices to boot domains
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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |