Re: [Xen-devel] [PATCH for-4.13 v2] xen/arm: domain_build: Don't expose IOMMU specific properties to hwdom

[Julien Grall <julien.grall@arm.com>]

Hi,

On 10/8/19 4:25 PM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> We don't passthrough IOMMU device to hwdom even if it is not used by Xen.
> Therefore exposing the properties that describe relationship between
> master devices and IOMMUs does not make any sense.
> 
> According to the:
> 1. Documentation/devicetree/bindings/iommu/iommu.txt
> 2. Documentation/devicetree/bindings/pci/pci-iommu.txt

It is not entirely clear that documentation is from Linux.

> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> ---
> Changes V1 [1] -> V2:
>     - Only skip IOMMU specific properties of the master device if we
>       skip the corresponding IOMMU device
>     - Use "hwdom" over "Dom0"
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-10/msg00104.html
> ---
>   xen/arch/arm/domain_build.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 6d6dd52..a7321b8 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -480,10 +480,26 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>       const struct dt_property *prop, *status = NULL;
>       int res = 0;
>       int had_dom0_bootargs = 0;
> +    struct dt_device_node *iommu_node;
>   
>       if ( kinfo->cmdline && kinfo->cmdline[0] )
>           bootargs = &kinfo->cmdline[0];
>   
> +    /*
> +     * If we skip the IOMMU device when creating DT for hwdom (even if
> +     * the IOMMU device is not used by Xen), we should also skip the IOMMU
> +     * specific properties of the master device behind it in order to avoid
> +     * exposing an half complete IOMMU bindings to hwdom.
> +     * Use "iommu_node" as an indicator of the master device which properties
> +     * should be skipped.
> +     */
> +    iommu_node = dt_parse_phandle(node, "iommus", 0);

The code is slightly confusing to read. I don't think we should care of 
invalid DT here, so let's only consider valid one.

For valid DT, any non-NULL return should point to an IOMMU. The comment 
above suggests that all IOMMU will be skipped. However, the check below 
(device_get_class(iommu_node)) will not return DEVICE_IOMMU when there 
are not have a driver for the IOMMU.

So this needs to be clarified in the commit message.

> +    if ( iommu_node )
> +    {
> +        if ( device_get_class(iommu_node) != DEVICE_IOMMU )
> +            iommu_node = NULL;
> +    }

Could we gather the two conditions in one if?

> +
>       dt_for_each_property_node (node, prop)
>       {
>           const void *prop_data = prop->value;
> @@ -540,6 +556,19 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>               continue;
>           }
>   
> +        if ( iommu_node )
> +        {
> +            /* Don't expose IOMMU specific properties to hwdom */
> +            if ( dt_property_name_is_equal(prop, "iommus") )
> +                continue;
> +
> +            if ( dt_property_name_is_equal(prop, "iommu-map") )
> +                continue;
> +
> +            if ( dt_property_name_is_equal(prop, "iommu-map-mask") )
> +                continue;
> +        }
> +
>           res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len);
>   
>           if ( res )
> 

Cheers,

-- 
Julien Grall

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