|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7b 5/8] xen/arm: assign devices to boot domains
On Wed, 2 Oct 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 10/1/19 12:30 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.
> >
> > Require both xen,reg and xen,path to be present, unless
> > xen,force-assign-without-iommu is also set. In that case, tolerate a
> > missing xen,path, also tolerate iommu setup failure for the passthrough
> > device.
> >
> > Also set add the new flag XEN_DOMCTL_CDF_iommu so that dom0less domU
> > can use the IOMMU if a partial dtb is specified.
>
> The patch looks good a few comments below.
Thanks
> [...]
>
> > xen/arch/arm/domain_build.c | 133 ++++++++++++++++++++++++++++++++++--
> > 1 file changed, 129 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 84b65b8f25..47f9bb31df 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1714,6 +1714,88 @@ static int __init make_vpl011_uart_node(struct
> > kernel_info *kinfo)
> > }
> > #endif
> > +/*
> > + * Scan device tree properties for passthrough specific information.
> > + * Returns < 0 on error
> > + * 0 on success
> > + */
> > +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
> > + const struct fdt_property
> > *xen_reg,
> > + const struct fdt_property
> > *xen_path,
> > + bool xen_force,
> > + uint32_t address_cells, uint32_t
> > size_cells)
> > +{
> > + const __be32 *cell;
> > + unsigned int i, len;
> > + struct dt_device_node *node;
> > + int res;
> > + paddr_t mstart, size, gstart;
> > +
> > + /* xen,reg specifies where to map the MMIO region */
> > + cell = (const __be32 *)xen_reg->data;
> > + len = fdt32_to_cpu(xen_reg->len) /
> > + ((address_cells * 2 + size_cells) * sizeof(uint32_t));
>
> Coding style again. I was kind of expecting you configured your editor
> properly after the last discussion...
Actually I fail to see the coding style issue on this one. Is it still
an alignment issue you are talking about?
>
> > static int __init handle_prop_pfdt(struct kernel_info *kinfo,
> > const void *pfdt, int nodeoff,
> > uint32_t address_cells, uint32_t
> > size_cells,
> > @@ -1721,7 +1803,9 @@ static int __init handle_prop_pfdt(struct kernel_info
> > *kinfo,
> > {
> > void *fdt = kinfo->fdt;
> > int propoff, nameoff, res;
> > - const struct fdt_property *prop;
> > + const struct fdt_property *prop, *xen_reg = NULL, *xen_path = NULL;
> > + const char *name;
> > + bool found, xen_force = false;
> > for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
> > propoff >= 0;
> > @@ -1730,10 +1814,48 @@ static int __init handle_prop_pfdt(struct
> > kernel_info *kinfo,
> > if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
> > return -FDT_ERR_INTERNAL;
> > + found = false;
> > 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 )
> > + {
> > + if ( dt_prop_cmp("xen,reg", name) == 0 )
> > + {
> > + xen_reg = prop;
> > + found = true;
> > + }
> > + else if ( dt_prop_cmp("xen,path", name) == 0 )
> > + {
> > + xen_path = prop;
> > + found = true;
> > + }
> > + else if ( dt_prop_cmp("xen,force-assign-without-iommu",
> > + name) == 0 )
>
> Coding style.
Ah, this one I can see, it should be:
else if ( dt_prop_cmp("xen,force-assign-without-iommu",
name) == 0 )
> > + {
> > + xen_force = true;
> > + found = true;
> > + }
> > + }
> > +
> > + /* Copy all other properties */
>
> It is not entirely clear what you mean by "other" here.
I can spell it out. It meant other than the one above: xen,reg xen,path
and xen,force-assign-without-iommu. I'll reword it to:
Copy properties other than the one above.
> > + if ( !found )
> > + {
> > + res = fdt_property(fdt, name, prop->data,
> > fdt32_to_cpu(prop->len));
> > + if ( res )
> > + return res;
> > + }
> > + }
> > +
> > + /*
> > + * Only handle passthrough properties if both xen,reg and xen,path
> > + * are present, or if xen,force-assign-without-iommu is specified.
> > + */
> > + if ( xen_reg != NULL && (xen_path != NULL || xen_force) )
> > + {
> > + res = handle_passthrough_prop(kinfo, xen_reg, xen_path, xen_force,
> > + address_cells, size_cells);
> > + if ( res < 0 )
> > return res;
> > }
>
> I would print an error so the user knows what happen here.
All right, I'll add:
printk(XENLOG_ERR "Failed to assign device to %pd\n", kinfo->d);
More specific information about the type of failure is already printed
by handle_passthrough_prop.
> > @@ -2291,6 +2413,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 |