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/10/19 4:27 PM, Oleksandr wrote:
> 
> On 10.10.19 18:18, Julien Grall wrote:
>> Hi,
> 
> Hi Julien
> 
> 
>>
>> 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.
> 
> Will clarify.
> 
> 
>>
>>>
>>> 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.
> 
> Do you mean "the comment" is confusing to read?

The code is confusing because "iommus" should always point to a IOMMU 
node, but then you check whether this is an IOMMU. So it is not clear if 
this is done for sanity check (or for a different reason).

Cheers,

-- 
Julien Grall

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