[PATCH 0/2] Dangling fixes for ARM iommu

[PATCH 0/2] Dangling fixes for ARM iommu

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

Dear All,

With moving our working setup to 4.12-rc2 (IPMMU series especially),I've
realized that we have a couple of patches from IPMMU series [1] which are
fixes, got RB/AB, but not reached mainline yet. I really hope those RB/AB
would not be treated as stale, and patches are OK to go into 4.12 release.

Oleksandr Tyshchenko (2):
  iommu/arm: Misc fixes for arch specific part
        RB from Julien is here [2]
  xen/arm: domain_build: Don't expose IOMMU specific properties to the
    guest
        AB from Julien is here [3]

[1] https://lists.xen.org/archives/html/xen-devel/2017-07/msg02545.html
[2] https://patchwork.kernel.org/patch/9862567/
[3] https://patchwork.kernel.org/patch/9862565/


 xen/arch/arm/domain_build.c         | 10 ++++++++++
 xen/drivers/passthrough/arm/iommu.c |  7 +++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.7.4


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

[PATCH 1/2] iommu/arm: Misc fixes for arch specific part

[Andrii Anisov]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

1. Add missing return in case if IOMMU ops have been already set.
2. Add check for shared IOMMU before returning an error.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---
 xen/drivers/passthrough/arm/iommu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 325997b..cbf9b82 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -32,7 +32,10 @@ void __init iommu_set_ops(const struct iommu_ops *ops)
     BUG_ON(ops == NULL);
 
     if ( iommu_ops && iommu_ops != ops )
+    {
         printk("WARNING: Cannot set IOMMU ops, already set to a different value\n");
+        return;
+    }
 
     iommu_ops = ops;
 }
@@ -70,8 +73,8 @@ void arch_iommu_domain_destroy(struct domain *d)
 
 int arch_iommu_populate_page_table(struct domain *d)
 {
-    /* The IOMMU shares the p2m with the CPU */
-    return -ENOSYS;
+    /* Return an error if the IOMMU shares the p2m with the CPU */
+    return iommu_use_hap_pt(d) ? -ENOSYS : 0;
 }
 
 void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
-- 
2.7.4


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

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

[Andrii Anisov]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

We don't passthrough IOMMU device to DOM0 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>
Acked-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/domain_build.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d2c63a8..15a08d6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -540,6 +540,16 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
             continue;
         }
 
+        /* Don't expose IOMMU specific properties to the guest */
+        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

Re: [PATCH 0/2] Dangling fixes for ARM iommu

[Julien Grall]

(+ Juergen)

On 21/01/2019 17:04, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> Dear All,

Hello,

All patches candidate for Xen 4.12 should have the release manager CCed and 
explain the pros/cons to have those patches for this release. It is also useful 
if you add for-4.12 (or similar) in the [...] so we can prioritize it.

> 
> With moving our working setup to 4.12-rc2 (IPMMU series especially),I've
> realized that we have a couple of patches from IPMMU series [1] which are
> fixes, got RB/AB, but not reached mainline yet. I really hope those RB/AB
> would not be treated as stale, and patches are OK to go into 4.12 release.

Firstly, the RB tag means the reviewer reviewed the patch in depth with the code 
base at that time. So I don't think the RBs should have been carried from a 
patch series sent 2 years ago. Indeed code base evolved, for instance 
iommu_use_hap_pt has a slight different behavior now.

So please stripped my RB. The AB is arguable as it is quite old.

Secondly, it often happens that some patches have the required acked-by and 
reviewed-by but are not merged because they makes little sense without the rest 
of the series. In that case, the current code base does not support the case 
where the P2M is not shared with the IOMMU and does not support new IOMMU 
bindings in the current setup.

So I am not convinced they should be included in Xen 4.12 or even without the 
rest of the series.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 0/2] Dangling fixes for ARM iommu

[Andrii Anisov]

Hello Julien,

On 21.01.19 19:29, Julien Grall wrote:
> (+ Juergen)
> All patches candidate for Xen 4.12 should have the release manager CCed and explain the pros/cons to have those patches for this release. It is also useful if you add for-4.12 (or similar) in the [...] so we can prioritize it.

I've got the point. Thank you for adding Juergen.

> Indeed code base evolved, for instance iommu_use_hap_pt has a slight different behavior now.

Actually you properly pointed the problem. I'm not sure how it happened, because I did test build and run for those patches yesterday. But now, progressing with our stuff porting, I realized that the patch 1 is missing dependency on `xen/sched.h` required by `iommu_use_hap_pt`.

> So please stripped my RB
OK.

> The AB is arguable as it is quite old.
I could drop it for v2.

> Secondly, it often happens that some patches have the required acked-by and reviewed-by but are not merged because they makes little sense without the rest of the series. 

Yes, sure, but fixes are still relevant and are quite autonomous. The only relation to the series is that they were discovered during its implementation.

> In that case, the current code base does not support the case where the P2M is not shared with the IOMMU and does not support new IOMMU bindings in the current setup.

It is the intention of the rest of the series.

> So I am not convinced they should be included in Xen 4.12 or even without the rest of the series.
IMO, the pure fixes, like patch 2, and the first hunk of patch 1 should be OK for 4.12.

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH 0/2] Dangling fixes for ARM iommu

[Julien Grall]

On 1/22/19 1:48 PM, Andrii Anisov wrote:
> Hello Julien,

Hi,

> On 21.01.19 19:29, Julien Grall wrote:
>> (+ Juergen)
>> All patches candidate for Xen 4.12 should have the release manager 
>> CCed and explain the pros/cons to have those patches for this release. 
>> It is also useful if you add for-4.12 (or similar) in the [...] so we 
>> can prioritize it.
> 
> I've got the point. Thank you for adding Juergen.
> 
>> Indeed code base evolved, for instance iommu_use_hap_pt has a slight 
>> different behavior now.
> 
> Actually you properly pointed the problem. I'm not sure how it happened, 
> because I did test build and run for those patches yesterday. But now, 
> progressing with our stuff porting, I realized that the patch 1 is 
> missing dependency on `xen/sched.h` required by `iommu_use_hap_pt`.
> 
>> So please stripped my RB
> OK.
> 
>> The AB is arguable as it is quite old.
> I could drop it for v2.

Yes please. But see below.

> 
>> Secondly, it often happens that some patches have the required 
>> acked-by and reviewed-by but are not merged because they makes little 
>> sense without the rest of the series. 
> 
> Yes, sure, but fixes are still relevant and are quite autonomous. The 
> only relation to the series is that they were discovered during its 
> implementation.

A fix would be if you found a bug in a configuration that we are meant 
to support.

> 
>> In that case, the current code base does not support the case where 
>> the P2M is not shared with the IOMMU and does not support new IOMMU 
>> bindings in the current setup.
> 
> It is the intention of the rest of the series.

The rest of series is not ready and not targeting 4.12. So why should we 
get them merged?

> 
>> So I am not convinced they should be included in Xen 4.12 or even 
>> without the rest of the series.
> IMO, the pure fixes, like patch 2, and the first hunk of patch 1 should 
> be OK for 4.12.

The first hunk of patch 1 aside, we never supported the new IOMMU 
bindings nor unsharing the P2M. So what do you actually fix?

Supporting unshared P2M will require more work given because the code 
relies on mfn_to_gmfn that is not implemented on Arm. So accepting this 
patch standalone would be misleading as the rest of the series is not 
going to be merged in Xen 4.12.

Similarly, we don't support new IOMMU bindings. The patch #2 alone is 
going to add more trouble as now Dom0 would not be able to use the IOMMU 
if it were not hidden.

So I still question the usefulness of those 2 patches outside of the series.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 0/2] Dangling fixes for ARM iommu

[Andrii Anisov]

Hello Julien,

On 22.01.19 16:31, Julien Grall wrote:
>> IMO, the pure fixes, like patch 2, and the first hunk of patch 1 should be OK for 4.12.
> 
> The first hunk of patch 1 aside, we never supported the new IOMMU bindings nor unsharing the P2M. So what do you actually fix?
> 
> Supporting unshared P2M will require more work given because the code relies on mfn_to_gmfn that is not implemented on Arm. So accepting this patch standalone would be misleading as the rest of the series is not going to be merged in Xen 4.12.
I'm not saying I want something from non-shared p2m push into 4.12.
I would strip the first patch to its first chunk.

> Similarly, we don't support new IOMMU bindings. The patch #2 alone is going to add more trouble as now Dom0 would not be able to use the IOMMU if it were not hidden.
Are you sure we should pass IOMMU devices or related info to Dom0 if they are not supported by hypervisor?
The commit message states "We don't passthrough IOMMU device to DOM0 even if it is not used by Xen.", and you agreed that some time ago. Could you please clarify why you have changed your mind?

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH 0/2] Dangling fixes for ARM iommu

[Julien Grall]

On 23/01/2019 17:53, Andrii Anisov wrote:
> Hello Julien,

Hi,

> On 22.01.19 16:31, Julien Grall wrote:
>>> IMO, the pure fixes, like patch 2, and the first hunk of patch 1 should be OK 
>>> for 4.12.
>>
>> The first hunk of patch 1 aside, we never supported the new IOMMU bindings nor 
>> unsharing the P2M. So what do you actually fix?
>>
>> Supporting unshared P2M will require more work given because the code relies 
>> on mfn_to_gmfn that is not implemented on Arm. So accepting this patch 
>> standalone would be misleading as the rest of the series is not going to be 
>> merged in Xen 4.12.
> I'm not saying I want something from non-shared p2m push into 4.12.

I know and this is not what I implied in my previous e-mail. I pointed out that 
the rest of the series is not planned for Xen 4.12, hence it does not make sense 
to get that patch merged.

> I would strip the first patch to its first chunk.

I can queue the first hunk for Xen 4.13.

> 
>> Similarly, we don't support new IOMMU bindings. The patch #2 alone is going to 
>> add more trouble as now Dom0 would not be able to use the IOMMU if it were not 
>> hidden.
> Are you sure we should pass IOMMU devices or related info to Dom0 if they are 
> not supported by hypervisor?
> The commit message states "We don't passthrough IOMMU device to DOM0 even if it 
> is not used by Xen.", and you agreed that some time ago. Could you please 
> clarify why you have changed your mind?

This statement is only correct if the IOMMU has been actively blacklisted by 
Xen. In the case of the SMMU driver, this is done by arm_smmu_dt_init(). The 
function is only called when the IOMMU has been enabled.

So if you pass "iommu=disabled" to Xen commandline, the SMMU will be accessible 
by Dom0.

When there are no drivers for the IOMMU used, then you end up to assign the 
IOMMU to Dom0. This means that Dom0 can use them.

It does not make sense to remove the properties "iommus", "iommu-map", 
"iommu-map-mask" if Dom0 is already touching the IOMMU. It is either everything 
or nothing.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 0/2] Dangling fixes for ARM iommu

[Andrii Anisov]

On 23.01.19 20:14, Julien Grall wrote:
> I can queue the first hunk for Xen 4.13.
For 4.13? Well I hope we will manage to upstream the whole series until then.

> This statement is only correct if the IOMMU has been actively blacklisted by Xen. In the case of the SMMU driver, this is done by arm_smmu_dt_init(). The function is only called when the IOMMU has been enabled.
> 
> So if you pass "iommu=disabled" to Xen commandline, the SMMU will be accessible by Dom0.
> 
> When there are no drivers for the IOMMU used, then you end up to assign the IOMMU to Dom0. This means that Dom0 can use them.
> 
> It does not make sense to remove the properties "iommus", "iommu-map", "iommu-map-mask" if Dom0 is already touching the IOMMU. It is either everything or nothing.
So that patch should be totally rewritten. At least to support iommu=disabled.

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH 0/2] Dangling fixes for ARM iommu

[Julien Grall]

On 23/01/2019 18:29, Andrii Anisov wrote:
> 
> On 23.01.19 20:14, Julien Grall wrote:
>> I can queue the first hunk for Xen 4.13.
> For 4.13? Well I hope we will manage to upstream the whole series until then.

If you plan to resend it separately, I can apply to the next branch regardless 
the rest of the series.

>> It does not make sense to remove the properties "iommus", "iommu-map", 
>> "iommu-map-mask" if Dom0 is already touching the IOMMU. It is either 
>> everything or nothing.
> So that patch should be totally rewritten. At least to support iommu=disabled.

And the case where IOMMU is enabled but no IOMMU driver is present in Xen.

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi,

I am reviving the thread. I think we need a patch similar to this one for Xen 
4.13. This is because generic are now used by Xen so they should be hidden from 
the hardware domain.

Andrii, Oleksandr, can one of you look at it?

Cheers,

On 21/01/2019 17:04, Andrii Anisov wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> We don't passthrough IOMMU device to DOM0 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>
> Acked-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/domain_build.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d2c63a8..15a08d6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -540,6 +540,16 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>               continue;
>           }
>   
> +        /* Don't expose IOMMU specific properties to the guest */
> +        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 )
> 

-- 
Julien Grall

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

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

[Oleksandr]

On 01.10.19 18:04, Julien Grall wrote:
> Hi,

Hi Julien


>
> I am reviving the thread. I think we need a patch similar to this one 
> for Xen 4.13. This is because generic are now used by Xen so they 
> should be hidden from the hardware domain.
>
> Andrii, Oleksandr, can one of you look at it?

I will be able to look at it probably at the end of the week if there is 
no urgency.


>
> Cheers,
>
> On 21/01/2019 17:04, Andrii Anisov wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> We don't passthrough IOMMU device to DOM0 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>
>> Acked-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/domain_build.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index d2c63a8..15a08d6 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -540,6 +540,16 @@ static int __init write_properties(struct domain 
>> *d, struct kernel_info *kinfo,
>>               continue;
>>           }
>>   +        /* Don't expose IOMMU specific properties to the guest */
>> +        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 )
>>
>
Julien, are you happy to see this patch as is, or do you have some 
comments regarding it?

-- 
Regards,

Oleksandr Tyshchenko


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

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

[Julien Grall]

Hi Oleksandr,

On 01/10/2019 16:25, Oleksandr wrote:
> 
> On 01.10.19 18:04, Julien Grall wrote:
>> Hi,
> 
> Hi Julien
> 
> 
>>
>> I am reviving the thread. I think we need a patch similar to this one for Xen 
>> 4.13. This is because generic are now used by Xen so they should be hidden 
>> from the hardware domain.
>>
>> Andrii, Oleksandr, can one of you look at it?
> 
> I will be able to look at it probably at the end of the week if there is no 
> urgency.

That's fine, I think we can make a case to add it in Xen 4.13.

> 
> 
>>
>> Cheers,
>>
>> On 21/01/2019 17:04, Andrii Anisov wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> We don't passthrough IOMMU device to DOM0 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>
>>> Acked-by: Julien Grall <julien.grall@arm.com>
>>> ---
>>>   xen/arch/arm/domain_build.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index d2c63a8..15a08d6 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -540,6 +540,16 @@ static int __init write_properties(struct domain *d, 
>>> struct kernel_info *kinfo,
>>>               continue;
>>>           }
>>>   +        /* Don't expose IOMMU specific properties to the guest */
>>> +        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 )
>>>
>>
> Julien, are you happy to see this patch as is, or do you have some comments 
> regarding it?

I have some comments on the cover letter for this patch. Please see [1].

Thank you for having a look at the patch.

Cheers,

[1] <ed087980-a2b9-2fd4-7e84-446142e8176b@arm.com>

-- 
Julien Grall

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

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

[Oleksandr]

On 01.10.19 18:36, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien



>
> On 01/10/2019 16:25, Oleksandr wrote:
>>
>> On 01.10.19 18:04, Julien Grall wrote:
>>> Hi,
>>
>> Hi Julien
>>
>>
>>>
>>> I am reviving the thread. I think we need a patch similar to this 
>>> one for Xen 4.13. This is because generic are now used by Xen so 
>>> they should be hidden from the hardware domain.
>>>
>>> Andrii, Oleksandr, can one of you look at it?
>>
>> I will be able to look at it probably at the end of the week if there 
>> is no urgency.
>
> That's fine, I think we can make a case to add it in Xen 4.13.
>
>>
>>
>>>
>>> Cheers,
>>>
>>> On 21/01/2019 17:04, Andrii Anisov wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> We don't passthrough IOMMU device to DOM0 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>
>>>> Acked-by: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>   xen/arch/arm/domain_build.c | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index d2c63a8..15a08d6 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -540,6 +540,16 @@ static int __init write_properties(struct 
>>>> domain *d, struct kernel_info *kinfo,
>>>>               continue;
>>>>           }
>>>>   +        /* Don't expose IOMMU specific properties to the guest */
>>>> +        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 )
>>>>
>>>
>> Julien, are you happy to see this patch as is, or do you have some 
>> comments regarding it?
>
> I have some comments on the cover letter for this patch. Please see [1].
>
> Thank you for having a look at the patch.
>
> Cheers,
>
> [1] <ed087980-a2b9-2fd4-7e84-446142e8176b@arm.com>

Looking briefly, I found two main points regarding that patch. This is 
how I understand them (please, correct me if I am wrong):

1. The IOMMU can be accessible by Dom0 (for example, if we pass 
"iommu=disabled" to Xen command line or it is enabled, but there is not 
suitable driver in Xen found).
There is no need to remove properties if Dom0 is already touching the IOMMU.
2. Generic IOMMU DT bindings is not used in Xen so far. There is no need 
to remove properties.

As I understand, both points are not actual anymore and nothing to 
modify in that patch, correct?

Because:

1. Giving the IOMMU to Dom0 is a bad idea.
2. Already supported.


-- 
Regards,

Oleksandr Tyshchenko


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

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

[Julien Grall]

Hi Oleksandr,

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

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.

Cheers,

-- 
Julien Grall

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

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

[Oleksandr]

On 01.10.19 22:07, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


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


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
+     * 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 )


-- 
Regards,

Oleksandr Tyshchenko


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

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

[Julien Grall]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Oleksandr]

On 08.10.19 00:02, Julien Grall wrote:
> Hi,

Hi Julien


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

ok


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

Already sent v2, please see:

https://lists.xenproject.org/archives/html/xen-devel/2019-10/msg00673.html

-- 
Regards,

Oleksandr Tyshchenko


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