Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [PATCH for-4.13 v2] xen/arm: domain_build: Don't expose IOMMU specific properties to hwdom
@ 2019-10-08 15:25 Oleksandr Tyshchenko
  2019-10-10 15:18 ` Julien Grall
  0 siblings, 1 reply; 5+ messages in thread
From: Oleksandr Tyshchenko @ 2019-10-08 15:25 UTC (permalink / raw)
  To: xen-devel
  Cc: Oleksandr Tyshchenko, julien.grall, sstabellini, volodymyr_babchuk

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

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);
+    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 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 )
-- 
2.7.4


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH for-4.13 v2] xen/arm: domain_build: Don't expose IOMMU specific properties to hwdom
  2019-10-08 15:25 [Xen-devel] [PATCH for-4.13 v2] xen/arm: domain_build: Don't expose IOMMU specific properties to hwdom Oleksandr Tyshchenko
@ 2019-10-10 15:18 ` Julien Grall
  2019-10-10 15:27   ` Oleksandr
  0 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2019-10-10 15:18 UTC (permalink / raw)
  To: Oleksandr Tyshchenko, xen-devel
  Cc: Juergen Gross, Oleksandr Tyshchenko, sstabellini, volodymyr_babchuk

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH for-4.13 v2] xen/arm: domain_build: Don't expose IOMMU specific properties to hwdom
  2019-10-10 15:18 ` Julien Grall
@ 2019-10-10 15:27   ` Oleksandr
  2019-10-10 15:32     ` Julien Grall
  0 siblings, 1 reply; 5+ messages in thread
From: Oleksandr @ 2019-10-10 15:27 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: Juergen Gross, Oleksandr Tyshchenko, sstabellini, volodymyr_babchuk


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?


>
> 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.

Will do.


>
>> +    if ( iommu_node )
>> +    {
>> +        if ( device_get_class(iommu_node) != DEVICE_IOMMU )
>> +            iommu_node = NULL;
>> +    }
>
> Could we gather the two conditions in one if?

Yes.


-- 
Regards,

Oleksandr Tyshchenko


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH for-4.13 v2] xen/arm: domain_build: Don't expose IOMMU specific properties to hwdom
  2019-10-10 15:27   ` Oleksandr
@ 2019-10-10 15:32     ` Julien Grall
  2019-10-10 15:42       ` Oleksandr
  0 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2019-10-10 15:32 UTC (permalink / raw)
  To: Oleksandr, xen-devel
  Cc: Juergen Gross, Oleksandr Tyshchenko, sstabellini, volodymyr_babchuk

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Xen-devel] [PATCH for-4.13 v2] xen/arm: domain_build: Don't expose IOMMU specific properties to hwdom
  2019-10-10 15:32     ` Julien Grall
@ 2019-10-10 15:42       ` Oleksandr
  0 siblings, 0 replies; 5+ messages in thread
From: Oleksandr @ 2019-10-10 15:42 UTC (permalink / raw)
  To: Julien Grall, xen-devel
  Cc: Juergen Gross, Oleksandr Tyshchenko, sstabellini, volodymyr_babchuk


On 10.10.19 18:32, Julien Grall wrote:
> Hi,

Hi


>
>>>> [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).

Got it. Will clarify a reason.


-- 
Regards,

Oleksandr Tyshchenko


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-08 15:25 [Xen-devel] [PATCH for-4.13 v2] xen/arm: domain_build: Don't expose IOMMU specific properties to hwdom Oleksandr Tyshchenko
2019-10-10 15:18 ` Julien Grall
2019-10-10 15:27   ` Oleksandr
2019-10-10 15:32     ` Julien Grall
2019-10-10 15:42       ` Oleksandr

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org xen-devel@archiver.kernel.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox