Re: [Xen-devel] [PATCH 2/2] xen/arm: domain_build: Don't expose IOMMU specific properties to the guest

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi,

On 03/10/2019 13:18, Oleksandr wrote:
> 
> On 01.10.19 22:07, Julien Grall wrote:
>> On 10/1/19 5:07 PM, Oleksandr wrote:
>>>
>>> On 01.10.19 18:36, Julien Grall wrote:
>>>> On 01/10/2019 16:25, Oleksandr wrote:
>>>>>
>>>>> On 01.10.19 18:04, Julien Grall wrote:
>>> > 1. Giving the IOMMU to Dom0 is a bad idea.
>>
>> Please to try expand your thoughts in the same e-mail when you say 
>> "this is a bad idea".
> 
> Well, this was a conclusion I had got from the discussion [1]. Sorry for 
> not being clear here.
> 
> 
>>
>> But, this is clearly what happen in current Xen setup if the driver is 
>> not enabled. What I want to avoid is exposing an half complete 
>> bindings to the guest (you don't know how it will behave).
>>
>> So we either remove all the properties and node related to the IOMMUs 
>> or nothing.
> I think, I got it. Our target is *not* to add a way for Dom0 to use 
> IOMMU, but to be consistent in removing IOMMU node/master device 
> properties. Now, we remove the IOMMU node if Xen identifies it (the 
> IOMMU driver is present in Xen),
> so looks like we have to remove master device properties only if this 
> master device is behind the IOMMU which node is removed. This, I hope, 
> will avoid exposing an half complete bindings to guest. Am I right?
> 
> 
> [1] 
> https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00858.html
> 
> 
> ----------
> 
> If you happy with that logic I will craft a proper patch.

The logic looks alright to me. One comment below, I will look at the 
rest once this is formally sent.

> 
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 67021d9..6d45e55 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 Dom0 (even if

I would prefer if we use "hwdom" over "Dom0". They are both the same on 
Arm, but this may change in the future (we may actually drop Dom0 ;)).

> +     * 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 Dom0.
> +     * Use "iommu_node" as an indicator of the master device which 
> properties
> +     * should be skipped.
> +     */
> +    iommu_node = dt_parse_phandle(node, "iommus", 0);
> +    if ( iommu_node )
> +    {
> +        if ( device_get_class(iommu_node) != DEVICE_IOMMU )
> +            iommu_node = NULL;
> +    }
> +
>       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 Dom0 */
> +            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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel