Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain

[Julien Grall <julien@xen.org>]

Hi Penny,

On 13/10/2021 08:51, Penny Zheng wrote:
> 
>> -----Original Message-----
>> From: Penny Zheng
>> Sent: Wednesday, October 13, 2021 3:44 PM
>> To: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>
>> Subject: RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
>> domain
>>
>> Hi Julien
>>
>>> -----Original Message-----
>>> From: Julien Grall <julien@xen.org>
>>> Sent: Monday, October 11, 2021 7:14 PM
>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>>> sstabellini@kernel.org
>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>>> <Wei.Chen@arm.com>
>>> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
>>> direct-map domain
>>>
>>>
>>>
>>> On 09/10/2021 10:40, Penny Zheng wrote:
>>>> Hi Julien
>>>
>>> Hi Penny,
>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Julien Grall <julien@xen.org>
>>>>> Sent: Thursday, September 23, 2021 7:27 PM
>>>>> To: Penny Zheng <Penny.Zheng@arm.com>;
>>>>> xen-devel@lists.xenproject.org; sstabellini@kernel.org
>>>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>>>>> <Wei.Chen@arm.com>
>>>>> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
>>>>> direct-map domain
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 23/09/2021 08:11, Penny Zheng wrote:
>>>>>> User could do device passthrough, with
>>>>>> "xen,force-assign-without-iommu" in the device tree snippet, on
>>>>>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled
>>>>>> on
>>>>> hardware.
>>>>>
>>>>> At the moment, it would be possible to passthrough a non-DMA
>>>>> capable device with direct-mapping. After this patch, this is going to be
>> forbidden.
>>>>>
>>>>>>
>>>>>> In order to achieve that, this patch adds 1:1 direct-map check and
>>>>>> disables iommu-related action.
>>>>>>
>>>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>>>>> ---
>>>>>>     xen/arch/arm/domain_build.c | 12 ++++++++----
>>>>>>     1 file changed, 8 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/domain_build.c
>>>>>> b/xen/arch/arm/domain_build.c index c92e510ae7..9a9d2522b7 100644
>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>> @@ -2070,14 +2070,18 @@ static int __init
>>>>> handle_passthrough_prop(struct kernel_info *kinfo,
>>>>>>         if ( res < 0 )
>>>>>>             return res;
>>>>>>
>>>>>> +    /*
>>>>>> +     * If xen_force, we allow assignment of devices without IOMMU
>>>>> protection.
>>>>>> +     * And if IOMMU is disabled or absent, 1:1 direct-map is
>>>>>> + necessary > +
>>>>> */
>>>>>> +    if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
>>>>>> +         !dt_device_is_protected(node) )
>>>>>
>>>>> dt_device_is_protected() will be always false unless the device is
>>>>> protected behing an SMMU using the legacy binding. So I don't think
>>>>> this is correct to move this check ahead. In fact..
>>>>>
>>>>>> +        return 0;
>>>>>> +
>>>>>>         res = iommu_add_dt_device(node);
>>>>>
>>>>> ... the call should already be a NOP when the IOMMU is disabled or
>>>>> the device is not behind an IOMMU. So can you explain what you are
>>>>> trying to prevent here?
>>>>>
>>>>
>>>> If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
>>>> So we could not make it to the xen_force check...
>>>
>>> I disagree. The check is:
>>>
>>> if ( res < 0 )
>>>     return res;
>>>
>>> Given that res is 1, we wouldn't return and move to check whether the
>>> assignment can be done.
>>>
>>>>
>>>> So I tried to move all IOMMU action behind xen_force check.
>>>>
>>>> Now, device assignment without IOMMU protection is only applicable
>>>> on direct-map domains,
>>>
>>> It is fine to assign a non-DMA capable device without direct-mapping.
>>> So why do you want to add this restriction?
>>>
>>
>> When constructing direct-map-v2, found that, in
>> xen/arch/arm/domain_build.c
>>
>> if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
>>      d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>
>> And this flag XEN_DOMCTL_CDF_iommu determines whether iommu is
>> enabled.
>>
>> In ./xen/include/xen/sched.h
>>
>> static always_inline bool is_iommu_enabled(const struct domain *d) {
>>      return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu); }
>>
>> That is, even if we assign a non-DMA capable device, we request the platform
>> to be iommu enabled.
>>
> 
> I intend to change it to
> 
>          if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
>          {
>              if ( iommu_enabled )
>                  d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>              else if ( d_cfg.flags & XEN_DOMCTL_CDF_directmap )
>                  warning_add("Please be sure of having trusted guests, when doing device assignment without IOMMU protection\n");
>          }

I think the warning is misleading. You don't need to trust a guest in 
order to assign non-DMA capable device.

But, I think the message is not necessary because from my understanding, 
an admin would need to add the property xen,force-assign-without-iommu 
in order to passthrough. So they should be fully aware of the 
consequence to do the passthrough.

Cheers,

-- 
Julien Grall