Re: [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 16/05/2024 11:03, Henry Wang wrote:
> There are some use cases in which the dom0less domUs need to have
> the XEN_DOMCTL_CDF_iommu set at the domain construction time. For
> example, the dynamic dtbo feature allows the domain to be assigned
> a device that is behind the IOMMU at runtime. For these use cases,
> we need to have a way to specify the domain will need the IOMMU
> mapping at domain construction time.
>
> Introduce a "passthrough" DT property for Dom0less DomUs following
> the same entry as the xl.cfg. Currently only provide two options,
> i.e. "enable" and "disable". Set the XEN_DOMCTL_CDF_iommu at domain
> construction time based on the property.
>
> Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
> ---
> v2:
> - New patch to replace the original patch in v1:
>    "[PATCH 03/15] xen/arm: Always enable IOMMU"
> ---
>   docs/misc/arm/device-tree/booting.txt | 13 +++++++++++++
>   xen/arch/arm/dom0less-build.c         |  7 +++++--
>   2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index bbd955e9c2..61f9082553 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -260,6 +260,19 @@ with the following properties:
>       value specified by Xen command line parameter gnttab_max_maptrack_frames
>       (or its default value if unspecified, i.e. 1024) is used.
>   
> +- passthrough
> +
> +    A string property specifying whether IOMMU mappings are enabled for the
> +    domain and hence whether it will be enabled for passthrough hardware.
> +    Possible property values are:
> +
> +    - "enabled"
> +    IOMMU mappings are enabled for the domain.
> +
> +    - "disabled"
> +    IOMMU mappings are disabled for the domain and so hardware may not be
> +    passed through. This option is the default if this property is missing.
Looking at the code below, it seems like the default will depend on 
whether the partial device-tree is present. Did I misunderstand?
> +
>   Under the "xen,domain" compatible node, one or more sub-nodes are present
>   for the DomU kernel and ramdisk.
>   
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 74f053c242..1396a102e1 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -848,6 +848,7 @@ static int __init construct_domU(struct domain *d,
>   void __init create_domUs(void)
>   {
>       struct dt_device_node *node;
> +    const char *dom0less_iommu;
>       const struct dt_device_node *cpupool_node,
>                                   *chosen = dt_find_node_by_path("/chosen");
>   
> @@ -895,8 +896,10 @@ 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") &&
> -             iommu_enabled )
> +        if ( iommu_enabled &&
> +             ((!dt_property_read_string(node, "passthrough", &dom0less_iommu) 
> &&
> +               !strcmp(dom0less_iommu, "enabled")) ||
> +              dt_find_compatible_node(node, NULL, "multiboot,device-tree")) )
This condition is getting a little bit harder to read. Can we cache the 
"passthrough" flag separately?
Also, shouldn't we throw a panic if passthrough = "enabled" but the 
IOMMU is enabled?
>               d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>   
>           if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
Cheers,

--
Julien Grall