| Message ID | 20190821035315.12812-5-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/8] xen/arm: introduce handle_device_interrupts | expand |
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@xilinx.com> > --- > 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. > + { > + 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. 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. > + 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,
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@xilinx.com> > > --- > > 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 >
Hi Stefano, On 9/24/19 5:52 PM, Stefano Stabellini wrote: > On Wed, 11 Sep 2019, Julien Grall wrote: >> 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. Yes and no, there are platforms where devices are sharing the same pages (see the UART on Sunxi SoC for instance). So this is a valid configuration, but we don't support it. The problem does not arise for domU created by the toolstack because we request a frame. Note that I would not want to use a frame for Dom0less assignment as I think this is buggy. Anyway, I would rather not add a panic in this code and let the upper layer deciding what to do. Cheers,
On Tue, 24 Sep 2019, Julien Grall wrote: > Hi Stefano, > > On 9/24/19 5:52 PM, Stefano Stabellini wrote: > > On Wed, 11 Sep 2019, Julien Grall wrote: > > > 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. > > Yes and no, there are platforms where devices are sharing the same pages (see > the UART on Sunxi SoC for instance). So this is a valid configuration, but we > don't support it. Hopefully the user should know that the minimum granularity is 4K. > The problem does not arise for domU created by the toolstack because we > request a frame. Note that I would not want to use a frame for Dom0less > assignment as I think this is buggy. > > Anyway, I would rather not add a panic in this code and let the upper layer > deciding what to do. OK. I added a warning.
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 ) + { + 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), + 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 */
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@xilinx.com> --- 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(-)