Re: [Xen-devel] [PATCH v8 5/8] xen/arm: assign devices to boot domains

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 3 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/10/2019 02:35, 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> > Changes in v8:
> > - better in-code comment
> > - code style
> > - add a prink in case of error
> > 
> > Changes in v7:
> > - improve in-code comment
> > - code style
> > - return 1 instead of ENOENT
> > - introduce "xen,force-assign-without-iommu"
> > - require both "xen,reg" and "xen,path" unless
> >    "xen,force-assign-without-iommu"
> > 
> > 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 | 139 ++++++++++++++++++++++++++++++++++--
> >   1 file changed, 135 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 84b65b8f25..b90902ad97 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));
> > +
> > +    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 )
> > +        {
> > +            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;
> > +        }
> > +    }
> > +
> > +    /*
> > +     * If xen_force, we let the user assign a MMIO region with no
> > +     * associated path.
> > +     */
> > +    if ( xen_path == NULL )
> > +        return xen_force ? 0 : -EINVAL;
> > +
> > +    /*
> > +     * 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.
> > +     */
> > +    node = dt_find_node_by_path(xen_path->data);
> > +    if ( node == NULL )
> > +    {
> > +        printk(XENLOG_ERR "Couldn't find node %s in host_dt!\n",
> > +               (char *)xen_path->data);
> > +        return -EINVAL;
> > +    }
> > +
> > +    res = handle_device_interrupts(kinfo->d, node, true);
> > +    if ( res < 0 )
> > +        return res;
> > +
> > +    /* If xen_force, we ignore IOMMU failures. */
> > +    res = iommu_add_dt_device(node);
> > +    if ( res < 0 )
> > +        return xen_force ? 0 : -EINVAL;
> 
> Actually, this code is wrong. iommu_add_dt_device(node) is returning 0 when
> the IOMMU is not in-use. Furthermore, this function may fail for other reasons
> than the Device is not behind an IOMMU. In those cases, you don't want to
> ignore the error as this is most likely going to introduce an unstable
> platform.
> 
> We we want is allowing a user to passthrough a device which is not behind an
> IOMMU.
> 
> Something like below would be better:
> 
> res = iommu_add_dt_device(node);
> if ( res < 0 )
>   return res;
> 
> /* We IOMMU possible
> if ( !dt_device_is_protect(dev) && xen_force )
>   return 0;
> 
> return iommu_assign_dt_device(kinfo->d, node);

Yes, I'll do that.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel