Re: [PATCH] arm64: KVM: Skip PSTATE.PAN reest at EL2 in non-VHE

Re: [PATCH] arm64: KVM: Skip PSTATE.PAN reest at EL2 in non-VHE

[gengdongjiu]

> [...]
> 
> >> Nit:
> >> In general it is not polite to keep posting patches in a middle of
> >> the merge window - people are busy with more important stuff...
> > I do not know when you are busy and in merge window
> 
> But maybe it is about time you find out how we work if you intend to be a part of this community. A number of your colleagues (such as
> Hanjun Guo, which I've now cc'd) are perfectly aware of the kernel merge process and you could probably learn from talking to him.

Thanks very much Marc's reminder and CC, Hanjun Guo is my colleague. I will find free time to consult with him

> 
> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny.

Re: [PATCH] arm64: KVM: Skip PSTATE.PAN reest at EL2 in non-VHE

[gengdongjiu]

 
> On Mon, Sep 11 2017 at  7:16:52 pm BST, Dongjiu Geng <gengdongjiu@huawei.com> wrote:
> > PSTATE.PAN disables reading and/or writing to a userspace virtual
> > address from EL1 in non-VHE or from EL2 in VHE. In non-VHE, there is
> > no any userspace mapping at EL2, so no need to reest the PSTATE.PAN.
> >
> > Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> > Signed-off-by: Haibin Zhang <zhanghaibin7@huawei.com>
> > ---
> >  arch/arm64/kvm/hyp/entry.S | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> > index 12ee62d6d410..86a7549b1b4c 100644
> > --- a/arch/arm64/kvm/hyp/entry.S
> > +++ b/arch/arm64/kvm/hyp/entry.S
> > @@ -96,8 +96,12 @@ ENTRY(__guest_exit)
> >
> >  	add	x1, x1, #VCPU_CONTEXT
> >
> > -	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
> > +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> > +	b	2f			// skip PAN at EL2 in non-VHE
> > +alternative_else_nop_endif
> >
> > +	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
> > +2:
> >  	// Store the guest regs x2 and x3
> >  	stp	x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
> 

Hi Marc,
  I need to say why I found this issue. In my test, I found the PAN feature did not work in my platform for VHE, but it works for the no-VHE.
1. After checking the code, I found I missed this patch " arm64: KVM: VHE: reset PSTATE.PAN on entry to EL2", and then thought of UAO
feature. because UAO feature is similar with the PAN which all control the access address permit. After discussed with you, you think usually
we do not always call set_fs(KERNEL_DS), so leave it alone.
2. After checking the PAN and discussing with you, we think PAN does not needed at EL2 for the non-VHE, so make this change. 

> Aside from Vladimir's comment about why this may not be an important change in practice (both features are v8.1, and expected to be
> implemented at the same time as VHE), I'm not sure this brings us much.

I agree that using VHE is the usual case if CPU supports VHE. but we cannot sure other people must not use non-VHE, since the pstate.PAN is not needed, why we still enabled it

> 
> We're just trading a write to PSTATE (which will have no effect other than storing a bit in PSTATE) for a branch, and I'm not sure what is the
> worse. Your patch definitely makes the code less readable, and I value ease of maintenance very much.

This place will check two CAP feature ARM64_HAS_PAN and ARM64_HAS_VIRT_HOST_EXTN, the best way is using "ALTERNATIVE" instruction, as shown below.
But I found it will report error when build, to avoid changing " ALTERNATIVE " macro. so I use the 'branch' instruction.

alternative_if ARM64_HAS_VIRT_HOST_EXTN
	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
alternative_else_nop_endif

> 
> Do you have any data coming from a non-VHE, PAN-enabled system that shows a measurable, significant performance improvement with
> this patch?
> Because that would be the only reason why I'd consider such a change.

Frankly speaking, I do not test the performance.

> 
> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny.

Re: [PATCH] arm64: KVM: Skip PSTATE.PAN reest at EL2 in non-VHE

[Marc Zyngier]

On Mon, Sep 11 2017 at  7:16:52 pm BST, Dongjiu Geng <gengdongjiu@huawei.com> wrote:
> PSTATE.PAN disables reading and/or writing to a userspace virtual
> address from EL1 in non-VHE or from EL2 in VHE. In non-VHE, there is
> no any userspace mapping at EL2, so no need to reest the PSTATE.PAN.
>
> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> Signed-off-by: Haibin Zhang <zhanghaibin7@huawei.com>
> ---
>  arch/arm64/kvm/hyp/entry.S | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 12ee62d6d410..86a7549b1b4c 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -96,8 +96,12 @@ ENTRY(__guest_exit)
>  
>  	add	x1, x1, #VCPU_CONTEXT
>  
> -	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> +	b	2f			// skip PAN at EL2 in non-VHE
> +alternative_else_nop_endif
>  
> +	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
> +2:
>  	// Store the guest regs x2 and x3
>  	stp	x2, x3,   [x1, #CPU_XREG_OFFSET(2)]

Aside from Vladimir's comment about why this may not be an important
change in practice (both features are v8.1, and expected to be
implemented at the same time as VHE), I'm not sure this brings us
much.

We're just trading a write to PSTATE (which will have no effect other
than storing a bit in PSTATE) for a branch, and I'm not sure what is the
worse. Your patch definitely makes the code less readable, and I value
ease of maintenance very much.

Do you have any data coming from a non-VHE, PAN-enabled system that
shows a measurable, significant performance improvement with this patch?
Because that would be the only reason why I'd consider such a change.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH] arm64: KVM: Skip PSTATE.PAN reest at EL2 in non-VHE

[Marc Zyngier]

On Mon, Sep 11 2017 at  7:48:45 pm BST, gengdongjiu <gengdongjiu@huawei.com> wrote:
> Hi Vladimir,
>
>

[...]

>> Nit:
>> In general it is not polite to keep posting patches in a middle of the merge
>> window - people are busy with more important stuff...
> I do not know when you are busy and in merge window

But maybe it is about time you find out how we work if you intend to be
a part of this community. A number of your colleagues (such as Hanjun
Guo, which I've now cc'd) are perfectly aware of the kernel merge
process and you could probably learn from talking to him.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH] arm64: KVM: Skip PSTATE.PAN reest at EL2 in non-VHE

[gengdongjiu]

Hi Vladimir,

On 2017/9/11 19:20, Vladimir Murzin wrote:
> On 11/09/17 12:16, Dongjiu Geng wrote:
>> PSTATE.PAN disables reading and/or writing to a userspace virtual
>> address from EL1 in non-VHE or from EL2 in VHE. In non-VHE, there is
>> no any userspace mapping at EL2, so no need to reest the PSTATE.PAN.
>                                                  ^^^^^
>                                                  reset
>>
>> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
>> Signed-off-by: Haibin Zhang <zhanghaibin7@huawei.com>
>> ---
>>  arch/arm64/kvm/hyp/entry.S | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
>> index 12ee62d6d410..86a7549b1b4c 100644
>> --- a/arch/arm64/kvm/hyp/entry.S
>> +++ b/arch/arm64/kvm/hyp/entry.S
>> @@ -96,8 +96,12 @@ ENTRY(__guest_exit)
>>  
>>  	add	x1, x1, #VCPU_CONTEXT
>>  
>> -	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
>> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
>> +	b	2f			// skip PAN at EL2 in non-VHE
>> +alternative_else_nop_endif
>>  
>> +	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
>> +2:
>>  	// Store the guest regs x2 and x3
>>  	stp	x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
>>  
>>
> 
> Ok. Probably I need to say why original patch did not consider non-VHE case:
> - VHE and PAN features come within the same v8.1 extension bundle, so it is
>   unlucky to see IRL implementation with PAN but no VHE.
> - Given above the only case where extra PAN instruction could count is
>   VHE-enabled system with CONFIG_ARM64_VHE is not set; However, IMO, usecase for
>   such setup is kind of debugging; it is quite obvious that those who care of
>   performance should not disable VHE in the first place...
thanks for the explanation.


> 
> Nit:
> In general it is not polite to keep posting patches in a middle of the merge
> window - people are busy with more important stuff...
I do not know when you are busy and in merge window


> 
> Cheers
> Vladimir
> 
> .
> 

Re: [PATCH] arm64: KVM: Skip PSTATE.PAN reest at EL2 in non-VHE

[Vladimir Murzin]

On 11/09/17 12:16, Dongjiu Geng wrote:
> PSTATE.PAN disables reading and/or writing to a userspace virtual
> address from EL1 in non-VHE or from EL2 in VHE. In non-VHE, there is
> no any userspace mapping at EL2, so no need to reest the PSTATE.PAN.
                                                 ^^^^^
                                                 reset
> 
> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> Signed-off-by: Haibin Zhang <zhanghaibin7@huawei.com>
> ---
>  arch/arm64/kvm/hyp/entry.S | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 12ee62d6d410..86a7549b1b4c 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -96,8 +96,12 @@ ENTRY(__guest_exit)
>  
>  	add	x1, x1, #VCPU_CONTEXT
>  
> -	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> +	b	2f			// skip PAN at EL2 in non-VHE
> +alternative_else_nop_endif
>  
> +	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
> +2:
>  	// Store the guest regs x2 and x3
>  	stp	x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
>  
> 

Ok. Probably I need to say why original patch did not consider non-VHE case:
- VHE and PAN features come within the same v8.1 extension bundle, so it is
  unlucky to see IRL implementation with PAN but no VHE.
- Given above the only case where extra PAN instruction could count is
  VHE-enabled system with CONFIG_ARM64_VHE is not set; However, IMO, usecase for
  such setup is kind of debugging; it is quite obvious that those who care of
  performance should not disable VHE in the first place...

Nit:
In general it is not polite to keep posting patches in a middle of the merge
window - people are busy with more important stuff...

Cheers
Vladimir

[PATCH] arm64: KVM: Skip PSTATE.PAN reest at EL2 in non-VHE

[Dongjiu Geng]

PSTATE.PAN disables reading and/or writing to a userspace virtual
address from EL1 in non-VHE or from EL2 in VHE. In non-VHE, there is
no any userspace mapping at EL2, so no need to reest the PSTATE.PAN.

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
Signed-off-by: Haibin Zhang <zhanghaibin7@huawei.com>
---
 arch/arm64/kvm/hyp/entry.S | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index 12ee62d6d410..86a7549b1b4c 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -96,8 +96,12 @@ ENTRY(__guest_exit)
 
 	add	x1, x1, #VCPU_CONTEXT
 
-	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
+alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
+	b	2f			// skip PAN at EL2 in non-VHE
+alternative_else_nop_endif
 
+	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
+2:
 	// Store the guest regs x2 and x3
 	stp	x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
 
-- 
2.14.1