|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 5/8] xen/arm: assign devices to boot domains
On Wed, 11 Sep 2019, Julien Grall wrote:
> Hi,
>
> On 8/21/19 4:53 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.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> > 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 | 66 ++++++++++++++++++++++++++++++++++---
> > 1 file changed, 62 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index c71b9f2889..256c83462a 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1721,6 +1721,9 @@ static int __init handle_prop_pfdt(struct kernel_info
> > *kinfo,
> > void *fdt = kinfo->fdt;
> > int propoff, nameoff, res;
> > const struct fdt_property *prop;
> > + struct dt_device_node *node;
> > + const __be32 *cell;
> > + unsigned int i, len;
> > for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
> > propoff >= 0;
> > @@ -1730,10 +1733,65 @@ static int __init handle_prop_pfdt(struct
> > kernel_info *kinfo,
> > return -FDT_ERR_INTERNAL;
> > nameoff = fdt32_to_cpu(prop->nameoff);
> > - res = fdt_property(fdt, fdt_string(pfdt, nameoff),
> > - prop->data, fdt32_to_cpu(prop->len));
> > - if ( res )
> > - return res;
> > +
> > + /* xen,reg specifies where to map the MMIO region */
> > + if ( dt_prop_cmp("xen,reg", fdt_string(pfdt, nameoff)) == 0 )
>
> The construct fdt_string(pfdt, nameoff) is used quite often within this
> function. I think it is warrant for a local variable.
OK
> > + {
> > + 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);
> > +
> > + res = guest_physmap_add_entry(kinfo->d,
> > gaddr_to_gfn(gstart),
> > + maddr_to_mfn(mstart),
> > + get_order_from_bytes(size),
>
> guest_physmap_add_entry is definitely not the correct function to call. It
> takes an order and means that if the user ask to map 12KB, it will effectively
> map 16KB. You want to use map_regions_p2mt as this is done everywhere else for
> mapping MMIO regions.
OK
> It also raises the question what should we do if the size passed in not
> page-aligned? Shall we just blindly round up/down? Should we warn?
>
> This was not important for dom0, but is potentially critical for domU as you
> may happen to inadvertently to export more than you hope to a guest.
A warning or even a panic would be OK because it is a static misconfiguration.
> > + p2m_mmio_direct_dev);
> > + if ( res < 0 )
> > + {
> > + dprintk(XENLOG_ERR,
> > + "Failed to map %"PRIpaddr" to the guest
> > at%"PRIpaddr"\n",
> > + mstart, gstart);
> > + return -EFAULT;
> > + }
> > + }
> > + }
> > + /*
> > + * 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", fdt_string(pfdt, nameoff)) == 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);
> > + 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;
> > + }
> > + /* copy all other properties */
> > + else
> > + {
> > + res = fdt_property(fdt, fdt_string(pfdt, nameoff), prop->data,
> > + fdt32_to_cpu(prop->len));
> > + if ( res )
> > + return res;
> > + }
> > }
> > /* FDT_ERR_NOTFOUND => There is no more properties for this 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 |