On Fri, 27 Sep 2019, Julien Grall wrote:
> Hi,
>
> On 27/09/2019 15:40, Oleksandr wrote:
> >
> > On 26.09.19 00:12, Julien Grall wrote:
> >> On 25/09/2019 19:49, 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 add the new flag XEN_DOMCTL_CDF_iommu to that dom0less domU can use
> >>> the IOMMU.
> >>>
> >>> Signed-off-by: Stefano Stabellini <stefa...@xilinx.com>
> >>> ---
> >>> 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 | 101
> >>> ++++++++++++++++++++++++++++++++++--
> >>> 1 file changed, 97 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >>> index 9d985d3bbe..414893bc24 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -1701,6 +1701,85 @@ 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
> >>> + * < 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;
> >>> + 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);
> >>> +
> >>> + if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size
> >>> & ~PAGE_MASK )
> >>> + dprintk(XENLOG_WARNING,
> >>> + "DomU passthrough config has not page
> >>> aligned addresses/sizes\n");
> >> I don't think this is wise to continue, the more this is a printk that
> >> can only happen in debug build. So someone using a release build may not
> >> notice the error.
> >>
> >> So I think this wants to be a printk(XENLOG_ERR,...) and also return an
> >> error.
> >>
> >>> +
> >>> + res = map_regions_p2mt(kinfo->d,
> >>> + gaddr_to_gfn(gstart),
> >>> + PFN_DOWN(size),
> >>> + maddr_to_mfn(mstart),
> >>> + p2m_mmio_direct_dev);
> >> Coding style.
> >>
> >>> + if ( res < 0 )
> >>> + {
> >>> + dprintk(XENLOG_ERR,
> >>> + "Failed to map %"PRIpaddr" to the guest
> >>> at%"PRIpaddr"\n",
> >>> + mstart, gstart);
> >> Similarly, this wants to be a printk.
> >>
> >>> + 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 )
> >>> + {
> >>> + dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
> >>> + (char *)prop->data);
> >> Same here.
> >>
> >>> + return -EINVAL;
> >>> + }
> >
> > I have to admit that I don't know about dom0less feature enough ...
> >
> >
> > But, shouldn't we check if the device is behind the IOMMU and try to add
> > it (iommu_add_dt_device) before assigning it (this is needed for drivers
> > which support generic IOMMU DT bindings in the first place).
> >
> > [please take a look at
> > https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html
> > if so]
> >
> > Julien, what do you think?
>
> Yes you are right.
>
> @Stefano, this is a recently merged feature. Without it, you will not be
> able to use passthrough with dom0less guest when the IOMMU (such as
> IPMMU) is using the generic DT bindings.
Just double-checking but it should be only a matter of the following,
right?
+ res = iommu_add_dt_device(node);
+ if ( res < 0 )
+ return res;
+
+ if ( dt_device_is_protected(node) )
+ {
+ res = iommu_assign_dt_device(kinfo->d, node);
+ if ( res < 0 )
+ return res;
+ }
+
(I am asking because I couldn't quite test it due to the error with
mmu-masters I mentioned in the other email.)_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
