[PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

In case if the p2m table is shared to IOMMU, invalidating it turns IOMMU to
translation faults that could be not repaired.

Fixed patch check for the corresponded condition and has a comment for one
introduced p2m_invalidate_root() call, but miss them for another. So put the
`if` and the comment in place.

Fixes: 2148a12 ("xen/arm: Track page accessed between batch of Set/Way operations")
Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
---
 xen/arch/arm/p2m.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 059a391..2367e09 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1708,8 +1708,13 @@ void p2m_flush_vm(struct vcpu *v)
     /*
      * Invalidate the p2m to track which page was modified by the guest
      * between call of p2m_flush_vm().
+     *
+     * This is only turned when IOMMU is not used or the page-table are
+     * not shared because bit[0] (e.g valid bit) unset will result
+     * IOMMU fault that could be not fixed-up.
      */
-    p2m_invalidate_root(p2m);
+    if ( !iommu_use_hap_pt(v->domain) )
+        p2m_invalidate_root(p2m);
 
     v->arch.need_flush_to_ram = false;
 }
-- 
2.7.4


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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Julien Grall]

Hi,

On 1/28/19 3:34 PM, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> In case if the p2m table is shared to IOMMU, invalidating it turns IOMMU to
> translation faults that could be not repaired.
> 
> Fixed patch check for the corresponded condition and has a comment for one
> introduced p2m_invalidate_root() call, but miss them for another. So put the
> `if` and the comment in place.

This was missed on purpose. Let me explain why. The call to 
p2m_invalidate_root() arch_domain_creation_finished is called by *all* 
the domain at boot to try to optimize the set/way case.

The check iommu_use_hap_pt in that context is to prevent guest not using 
Set/Way to become unusable under the IOMMU use-case.

In your case, you seem to have a guest OS using set/way and yet sharing 
the P2M with the IOMMU. You have the choice between:
	1) Crashing on IOMMU fault
	2) Become very slow and potentially unusable because you now have to go 
through the full P2M every time you do a Set/Way.

1) was my favored option because Set/Way should really not be used by 
the guest. It was implemented by courtesy to the guest OS and I would 
not rely on everything working.

Can you explain what is your use-case (OS used, IOMMU, platform...)?

Cheers,

> 
> Fixes: 2148a12 ("xen/arm: Track page accessed between batch of Set/Way operations")
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
>   xen/arch/arm/p2m.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 059a391..2367e09 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1708,8 +1708,13 @@ void p2m_flush_vm(struct vcpu *v)
>       /*
>        * Invalidate the p2m to track which page was modified by the guest
>        * between call of p2m_flush_vm().
> +     *
> +     * This is only turned when IOMMU is not used or the page-table are
> +     * not shared because bit[0] (e.g valid bit) unset will result
> +     * IOMMU fault that could be not fixed-up.
>        */
> -    p2m_invalidate_root(p2m);
> +    if ( !iommu_use_hap_pt(v->domain) )
> +        p2m_invalidate_root(p2m);
>   
>       v->arch.need_flush_to_ram = false;
>   }
> 

-- 
Julien Grall

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Andrii Anisov]

Hello Julien,

Actually I was going to send this patch as RFC, but dropped it at the last moment.

On 28.01.19 17:55, Julien Grall wrote:
> This was missed on purpose. Let me explain why. The call to p2m_invalidate_root() arch_domain_creation_finished is called by *all* the domain at boot to try to optimize the set/way case.
> 
> The check iommu_use_hap_pt in that context is to prevent guest not using Set/Way to become unusable under the IOMMU use-case.
> 
> In your case, you seem to have a guest OS using set/way and yet sharing the P2M with the IOMMU. You have the choice between:
>      1) Crashing on IOMMU fault
>      2) Become very slow and potentially unusable because you now have to go through the full P2M every time you do a Set/Way.
> 
> 1) was my favored option because Set/Way should really not be used by the guest. It was implemented by courtesy to the guest OS and I would not rely on everything working.
> 
> Can you explain what is your use-case (OS used, IOMMU, platform...)
Yep, I'm nearly finished moving our development setup to XEN 4.12-rc.
The platform is R-Car Gen3 (CA57+CA53), Renesas'es IOMMU named IPMMU is utilized for all peripherals assigned to DomD and GPU shared between DomD and DomA.
Three OS'es are running on the HW: HW-less Dom0, driver domain DomD with GPU sharing, Android running on PV drivers and GPU sharing.
So on system start (DomD booting) I see few translation faults from DomD only assigned peripherals. But then Android starts and there are lot of translation faults from GPU trying access DomA memory. Then GPU FW dies.

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Julien Grall]

On 1/28/19 4:32 PM, Andrii Anisov wrote:
> Hello Julien,
> 
> Actually I was going to send this patch as RFC, but dropped it at the 
> last moment.
> 
> On 28.01.19 17:55, Julien Grall wrote:
>> This was missed on purpose. Let me explain why. The call to 
>> p2m_invalidate_root() arch_domain_creation_finished is called by *all* 
>> the domain at boot to try to optimize the set/way case.
>>
>> The check iommu_use_hap_pt in that context is to prevent guest not 
>> using Set/Way to become unusable under the IOMMU use-case.
>>
>> In your case, you seem to have a guest OS using set/way and yet 
>> sharing the P2M with the IOMMU. You have the choice between:
>>      1) Crashing on IOMMU fault
>>      2) Become very slow and potentially unusable because you now have 
>> to go through the full P2M every time you do a Set/Way.
>>
>> 1) was my favored option because Set/Way should really not be used by 
>> the guest. It was implemented by courtesy to the guest OS and I would 
>> not rely on everything working.
>>
>> Can you explain what is your use-case (OS used, IOMMU, platform...)
> Yep, I'm nearly finished moving our development setup to XEN 4.12-rc.
> The platform is R-Car Gen3 (CA57+CA53), Renesas'es IOMMU named IPMMU is 
> utilized for all peripherals assigned to DomD and GPU shared between 
> DomD and DomA.
> Three OS'es are running on the HW: HW-less Dom0, driver domain DomD with 
> GPU sharing, Android running on PV drivers and GPU sharing.
> So on system start (DomD booting) I see few translation faults from DomD 
> only assigned peripherals. But then Android starts and there are lot of 
> translation faults from GPU trying access DomA memory. Then GPU FW dies.

Hold on, CA57 and CA53 are ARMv8 cores. So are you using 32-bit or 
64-bit guests?

64-bit guest should not have any Set/Way operations unless you are using 
a very very old Linux. So what is the version of each guest?

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Andrii Anisov]

On 28.01.19 18:36, Julien Grall wrote:
> Hold on, CA57 and CA53 are ARMv8 cores. So are you using 32-bit or 64-bit guests?
64-bit guests.

> 64-bit guest should not have any Set/Way operations unless you are using a very very old Linux. So what is the version of each guest?
All of them derived from 4.14.35.

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Julien Grall]

On 1/28/19 4:40 PM, Andrii Anisov wrote:
> 
> 
> On 28.01.19 18:36, Julien Grall wrote:
>> Hold on, CA57 and CA53 are ARMv8 cores. So are you using 32-bit or 
>> 64-bit guests?
> 64-bit guests.
> 
>> 64-bit guest should not have any Set/Way operations unless you are 
>> using a very very old Linux. So what is the version of each guest?
> All of them derived from 4.14.35.

There should be no Set/Way in Linux 4.14. So I am a bit confused how can 
even reach this code.

Can you look if there are a Set/Way executed in Linux? And where? You 
can add some code in arm64/vsysreg.c to track that down.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Andrii Anisov]

On 28.01.19 18:54, Julien Grall wrote:
> 
> 
> On 1/28/19 4:40 PM, Andrii Anisov wrote:
>>
>>
>> On 28.01.19 18:36, Julien Grall wrote:
>>> Hold on, CA57 and CA53 are ARMv8 cores. So are you using 32-bit or 64-bit guests?
>> 64-bit guests.
>>
>>> 64-bit guest should not have any Set/Way operations unless you are using a very very old Linux. So what is the version of each guest?
>> All of them derived from 4.14.35.
> 
> There should be no Set/Way in Linux 4.14. So I am a bit confused how can even reach this code.
> 
> Can you look if there are a Set/Way executed in Linux? And where? You can add some code in arm64/vsysreg.c to track that down.
Yep, I'm still looking into details of that stuff.
Yet this patch makes the system functional.

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Andrii Anisov]

Hello Julien,

On 28.01.19 18:54, Julien Grall wrote:
> There should be no Set/Way in Linux 4.14. So I am a bit confused how can even reach this code.

Well, that is a proprietary kernel driver doing cache maintenance using Set/Way operations. And I'm not sure if we can avoid that :(

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Julien Grall]

On 29/01/2019 13:36, Andrii Anisov wrote:
> Hello Julien,

Hi,

> On 28.01.19 18:54, Julien Grall wrote:
>> There should be no Set/Way in Linux 4.14. So I am a bit confused how can 
>> even reach this code.
> 
> Well, that is a proprietary kernel driver doing cache maintenance using Set/Way 
> operations. And I'm not sure if we can avoid that :(

Which proprietary kernel driver? And in which context are Set/Way operations used?

Using Set/Way is fundamentally broken partly because it does not deal with 
system caches. It also only applies to a given cache and the behavior will not 
be replicated to other CPUs. For more details, have a look at [1].

If you wonder, this worked by pure chance in older release. Running such 
software is a call for nasty behavior in your guest.

Cheers,

[1] https://www.youtube.com/watch?v=s07f82iiI-E

-- 
Julien Grall

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Andrii Anisov]

On 29.01.19 15:53, Julien Grall wrote:
> Which proprietary kernel driver? And in which context are Set/Way operations used?
I would not name it, it is proprietary ;) But we can't live without it.

> Using Set/Way is fundamentally broken partly because it does not deal with system caches.
I already know it. I've looked at KVM presentation from 2015, your last year video from China, reading through ARM ARM.


> It also only applies to a given cache and the behavior will not be replicated to other CPUs.
In our case cpu cache flushing by Set/Way it is propagated to all cpus with `on_each_cpu()`. (Here should be a facepalm emoji)

> Running such software is a call for nasty behavior in your guest.
I do understand that.
But we have to handle that. And it is a question to us how to do that.

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Julien Grall]

Hi Andrii,

On 29/01/2019 14:33, Andrii Anisov wrote:
> On 29.01.19 15:53, Julien Grall wrote:
>> Using Set/Way is fundamentally broken partly because it does not deal with 
>> system caches.
> I already know it. I've looked at KVM presentation from 2015, your last year 
> video from China, reading through ARM ARM.
> 
> 
>> It also only applies to a given cache and the behavior will not be replicated 
>> to other CPUs.
> In our case cpu cache flushing by Set/Way it is propagated to all cpus with 
> `on_each_cpu()`. (Here should be a facepalm emoji)

:/

> 
>> Running such software is a call for nasty behavior in your guest.
> I do understand that.
> But we have to handle that. And it is a question to us how to do that.

No we don't have to. They have been lucky to see this working even on baremetal.
Set/Way operations have been dropped from Linux for a long time, so I really 
can't see why a proprietary driver is still using them.

I think the policy for Set/Way operations is correct in Xen. This works to avoid 
breaking basic case but the most complex one are going to break.

You solution is only delaying the real fix (i.e removing Set/Way operation from 
the software). So here the best solution is to go to the vendor and ask them to 
fix their software.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Andrii Anisov]

On 29.01.19 17:17, Julien Grall wrote:
> No we don't have to.
I mean, EPAM systems as a XEN based virtualization solution provider have to handle that issue.

> They have been lucky to see this working even on baremetal.
> Set/Way operations have been dropped from Linux for a long time, so I really can't see why a proprietary driver is still using them.
I guess there were some performance concerns. And they are really lucky, so they did not face related issues on baremetal.

> I think the policy for Set/Way operations is correct in Xen. This works to avoid breaking basic case but the most complex one are going to break.
Maybe it worth to get a notification from XEN, i.e. WARN_ONCE, saying that Set/Way cache operations are undesirable in VM and will lead to the system performance drop, or even instability in case of IOMMU sharing p2m with CPU.
This might save days of debugging to XEN on ARM users.

> You solution is only delaying the real fix
I would not say it is delaying the fix, but allows us to have a functional system until we get that fix.

> (i.e removing Set/Way operation from the software). So here the best solution is to go to the vendor and ask them to fix their software.
Actually escalation the issue to the vendor is one of our next steps here.

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Julien Grall]

Hi Andrii,

On 29/01/2019 16:00, Andrii Anisov wrote:
> 
> On 29.01.19 17:17, Julien Grall wrote:
>> They have been lucky to see this working even on baremetal.
>> Set/Way operations have been dropped from Linux for a long time, so I really 
>> can't see why a proprietary driver is still using them.
> I guess there were some performance concerns. And they are really lucky, so they 
> did not face related issues on baremetal.

I am not entirely convinced this is related to performance. You have to flush 
all the cache level one by one and may miss some caches (i.e system caches).

I suspect this is a relic from early version of Linux and no-one have seen the 
issue and/or "difficult" to fix. Set/Way is quite convenient to use over going 
over virtual address.

> 
>> I think the policy for Set/Way operations is correct in Xen. This works to 
>> avoid breaking basic case but the most complex one are going to break.
> Maybe it worth to get a notification from XEN, i.e. WARN_ONCE, saying that 
> Set/Way cache operations are undesirable in VM and will lead to the system 
> performance drop, or even instability in case of IOMMU sharing p2m with CPU.
> This might save days of debugging to XEN on ARM users.

I would be happy with a printk warning the user.

> 
>> You solution is only delaying the real fix
> I would not say it is delaying the fix, but allows us to have a functional 
> system until we get that fix.

By experience, if you workaround something in the code then you have less 
incentive to fix the real issue. So I am not willing to merge any patch that 
could delay a vendor to fix their software.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Stefano Stabellini]

On Tue, 29 Jan 2019, Andrii Anisov wrote:
> On 29.01.19 17:17, Julien Grall wrote:
> > No we don't have to.
> I mean, EPAM systems as a XEN based virtualization solution provider have to
> handle that issue.
> 
> > They have been lucky to see this working even on baremetal.
> > Set/Way operations have been dropped from Linux for a long time, so I really
> > can't see why a proprietary driver is still using them.
> I guess there were some performance concerns. And they are really lucky, so
> they did not face related issues on baremetal.
> 
> > I think the policy for Set/Way operations is correct in Xen. This works to
> > avoid breaking basic case but the most complex one are going to break.
> Maybe it worth to get a notification from XEN, i.e. WARN_ONCE, saying that
> Set/Way cache operations are undesirable in VM and will lead to the system
> performance drop, or even instability in case of IOMMU sharing p2m with CPU.
> This might save days of debugging to XEN on ARM users.

A WARN_ONCE would be nice. If you send a patch I would be happy to take
in 4.12 (if the release manager agrees).


> > You solution is only delaying the real fix
> I would not say it is delaying the fix, but allows us to have a functional
> system until we get that fix.
> 
> > (i.e removing Set/Way operation from the software). So here the best
> > solution is to go to the vendor and ask them to fix their software.
> Actually escalation the issue to the vendor is one of our next steps here.

You really want to fix this in the Linux driver if you can, because
you'll get better performance and a more stable system. In the short
term, I would suggest you keep the work-around in your private tree.
However, do let us know how the escalation with the vendor proceeds and
if you have any troubles with it. Julien and I would be happy to provide
help in terms of information and docs on the reasons why this should be
fixed in the driver. Julien might even be able to help further via his
employer.

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Andrii Anisov]

On 05.02.19 03:10, Stefano Stabellini wrote:
> You really want to fix this in the Linux driver if you can, because
> you'll get better performance and a more stable system.

I also have an idea that it might improve performance. And it is what we need.

> In the short
> term, I would suggest you keep the work-around in your private tree.

As I do now.

> However, do let us know how the escalation with the vendor proceeds and
> if you have any troubles with it. Julien and I would be happy to provide
> help in terms of information and docs on the reasons why this should be
> fixed in the driver. Julien might even be able to help further via his
> employer.

Thank you.
I already referred this thread in the email to the vendor as the first step.
But have no answer yet :(

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Andrii Anisov]

Hello Stefano,

On 05.02.19 11:17, Andrii Anisov wrote:
> Thank you.
> I already referred this thread in the email to the vendor as the first step.
> But have no answer yet :(

We've got an answer from the vendor. They agree with arguments and have provided a patch to eliminate Set/Way usage.
We've checked and it does what is needed. So we do not need this WA any more.

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Stefano Stabellini]

On Wed, 6 Feb 2019, Andrii Anisov wrote:
> Hello Stefano,
> 
> On 05.02.19 11:17, Andrii Anisov wrote:
> > Thank you.
> > I already referred this thread in the email to the vendor as the first step.
> > But have no answer yet :(
> 
> We've got an answer from the vendor. They agree with arguments and have
> provided a patch to eliminate Set/Way usage.
> We've checked and it does what is needed. So we do not need this WA any more.

Great, best for everybody!

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