Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[gengdongjiu]

> On 07/09/17 12:49, gengdongjiu wrote:
> >
> >
> > On 2017/9/7 18:13, Marc Zyngier wrote:
> >> On 07/09/17 11:05, gengdongjiu wrote:
> >>> Hi James,
> >>>
> >>> On 2017/9/7 17:20, James Morse wrote:
> >>>> Hi Dongjiu Geng,
> >>>>
> >>>> On 07/09/17 06:54, Dongjiu Geng wrote:
> >>>>> In VHE mode, host kernel runs in the EL2 and can enable 'User
> >>>>> Access Override' when fs==KERNEL_DS so that it can access kernel
> >>>>> memory. However, PSTATE.UAO is set to 0 on an exception taken from
> >>>>> EL1 to EL2. Thus when VHE is used and exception taken from a guest
> >>>>> UAO will be disabled and host will use the incorrect PSTATE.UAO.
> >>>>> So check and reset the PSTATE.UAO when switching to host.
> >>>>
> >>>> This would only be a problem if KVM were calling into world-switch
> >>>> with fs==KERNEL_DS. I can't see where this happens.
> >>>  Not only KVM, may also kernel sets the fs == KERNEL_DS before
> >>> calling into world-switch
> >>
> >> How? Please describe the exact sequence of event that lead to this
> >> situation with the current code base.
> >
> > Hi Marc,
> >
> >    Different tasks have different fs, such as USER_DS or KERNEL_DS. In
> > the context switch, it will restore the task's fs. Thus, that depends
> > on task itself, as shown below code. UAO is different with PAN, PAN will be always enabled if hardware CPU supports PAN feature, but
> UAO is dynamical change.
> 
> You haven't answered my question: There is exactly one point where we enter the world-switch. Show me that, at this point, PSTATE.UAO
> *before* the call is different from PSTATE.UAO after the call. Give me the exact sequence of event that leads to this situation. Show me a
> stack trace.

Hi Marc,

  If using current mainline KVM code + Qemu and modify nothing, may not exist broken issue,  because the Qemu progress FS should be USER_DS. But if I make a modification for user space( Qemu or KVM tools) to change its FS property to KERNEL_DS or use third party application with KERNEL_DS FS to run the guest, it will have problem(the KERNEL_DS is cleared). If you think my case is reasonable and should support, I can show you the calling stack trace. If you think, my case is not reasonable and KVM should not support the application with KERNEL_DS fs to run guest. You can ignore this patch, thanks.


> 
> Until you do this, I will ignore any further comment coming from you on this subject.
> 
> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[Marc Zyngier]

On 07/09/17 16:03, gengdongjiu wrote:
>> On 07/09/17 12:49, gengdongjiu wrote:
>>>
>>>
>>> On 2017/9/7 18:13, Marc Zyngier wrote:
>>>> On 07/09/17 11:05, gengdongjiu wrote:
>>>>> Hi James,
>>>>>
>>>>> On 2017/9/7 17:20, James Morse wrote:
>>>>>> Hi Dongjiu Geng,
>>>>>>
>>>>>> On 07/09/17 06:54, Dongjiu Geng wrote:
>>>>>>> In VHE mode, host kernel runs in the EL2 and can enable 'User
>>>>>>> Access Override' when fs==KERNEL_DS so that it can access kernel
>>>>>>> memory. However, PSTATE.UAO is set to 0 on an exception taken from
>>>>>>> EL1 to EL2. Thus when VHE is used and exception taken from a guest
>>>>>>> UAO will be disabled and host will use the incorrect PSTATE.UAO.
>>>>>>> So check and reset the PSTATE.UAO when switching to host.
>>>>>>
>>>>>> This would only be a problem if KVM were calling into world-switch
>>>>>> with fs==KERNEL_DS. I can't see where this happens.
>>>>>  Not only KVM, may also kernel sets the fs == KERNEL_DS before
>>>>> calling into world-switch
>>>>
>>>> How? Please describe the exact sequence of event that lead to this
>>>> situation with the current code base.
>>>
>>> Hi Marc,
>>>
>>>    Different tasks have different fs, such as USER_DS or KERNEL_DS. In
>>> the context switch, it will restore the task's fs. Thus, that depends
>>> on task itself, as shown below code. UAO is different with PAN, PAN will be always enabled if hardware CPU supports PAN feature, but
>> UAO is dynamical change.
>>
>> You haven't answered my question: There is exactly one point where we enter the world-switch. Show me that, at this point, PSTATE.UAO
>> *before* the call is different from PSTATE.UAO after the call. Give me the exact sequence of event that leads to this situation. Show me a
>> stack trace.
> 
> Hi Marc,
> 
> If using current mainline KVM code + Qemu and modify nothing, may not
> exist broken issue,  because the Qemu progress FS should be USER_DS.

Thanks. You've just been wasting everybody's time for two days. In case
you haven't noticed, mainline KVM code is the *ONLY* thing we care about.

> But if I make a modification for user space( Qemu or KVM tools) to
> change its FS property to KERNEL_DS or use third party application
> with KERNEL_DS FS to run the guest, it will have problem(the
> KERNEL_DS is cleared). If you think my case is reasonable and should
> support, I can show you the calling stack trace. If you think, my
> case is not reasonable and KVM should not support the application
> with KERNEL_DS fs to run guest. You can ignore this patch, thanks.

I really cannot think of a good reason why we'd want to do that. Playing
with set_fs() is almost universally wrong, and I'm certainly going to
oppose to any change in that area unless the code that calls set_fs()
has been made public and properly reviewed. Until then, UAO/PAN will
stay as they are unless you prove that our current code is wrong.

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

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[gengdongjiu]

On 2017/9/7 23:23, Marc Zyngier wrote:
> On 07/09/17 16:03, gengdongjiu wrote:
>>> On 07/09/17 12:49, gengdongjiu wrote:
>>>>
[...]

> 
> I really cannot think of a good reason why we'd want to do that. Playing
> with set_fs() is almost universally wrong, and I'm certainly going to
> oppose to any change in that area unless the code that calls set_fs()
> has been made public and properly reviewed. Until then, UAO/PAN will
> stay as they are unless you prove that our current code is wrong.

Marc,

sorry I have another question for the PAN.

In the non-VHE mode, The host kernel is running in the EL1. Before host kernel enter guest, host OS will call 'HVC' instruction to do the world-switch,
and the pstate.PAN will be saved into the SPSR_EL2. When world-switch back to host kernel from EL2, it will call 'eret' instruction to EL1 host,
this 'eret' instruction will restore the SPSR_EL2 to the PSTATE. so the PSTATE.PAN will be restored.

For the Non-VHE mode, in the EL2 where mainly have word-switch code, do you think it needs to reset the PSTATE.PAN? From the spec, it does not provide SCTLR_EL2.SPAN bit for non-VHE mode,
so reset the PSTATE.PAN does not sure whether it is needed or whether affects the performance. If you think it is needed for El2 in Non-VHE mode, moving the reset PSTATE.PAN to
the exception entry to EL2 may be better, such as "el1_sync", because host can also call 'hvc' instruction without guest running.

> 
> 	M.
> 

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[Marc Zyngier]

On Fri, 8 Sep 2017 15:19:21 +0800
gengdongjiu <gengdongjiu@huawei.com> wrote:

> On 2017/9/7 23:23, Marc Zyngier wrote:
> > On 07/09/17 16:03, gengdongjiu wrote:  
> >>> On 07/09/17 12:49, gengdongjiu wrote:  
> >>>>  
> [...]
> 
> > 
> > I really cannot think of a good reason why we'd want to do that. Playing
> > with set_fs() is almost universally wrong, and I'm certainly going to
> > oppose to any change in that area unless the code that calls set_fs()
> > has been made public and properly reviewed. Until then, UAO/PAN will
> > stay as they are unless you prove that our current code is wrong.  
> 
> Marc,
> 
> sorry I have another question for the PAN.
> 
> In the non-VHE mode, The host kernel is running in the EL1. Before
> host kernel enter guest, host OS will call 'HVC' instruction to do
> the world-switch, and the pstate.PAN will be saved into the SPSR_EL2.
> When world-switch back to host kernel from EL2, it will call 'eret'
> instruction to EL1 host, this 'eret' instruction will restore the
> SPSR_EL2 to the PSTATE. so the PSTATE.PAN will be restored.
> 
> For the Non-VHE mode, in the EL2 where mainly have word-switch code,
> do you think it needs to reset the PSTATE.PAN? From the spec, it does
> not provide SCTLR_EL2.SPAN bit for non-VHE mode, so reset the
> PSTATE.PAN does not sure whether it is needed or whether affects the
> performance. If you think it is needed for El2 in Non-VHE mode,
> moving the reset PSTATE.PAN to the exception entry to EL2 may be
> better, such as "el1_sync", because host can also call 'hvc'
> instruction without guest running.

So let's see if I correctly understand your question:

You're worried that we don't set/reset PSTATE.PAN at EL2 in non-VHE?
In non-VHE, there is no user-space mapping that is present at the
same time as the hypervisor mappings. Actually, we hardly have any
mapping other than the HYP text/data and the vcpu/vm structures.

So how is PAN relevant in this context? What does it even mean?
If you have a ARMv8.0 behaviour, PAN doesn't even seem to *exist* at
EL2.

Or am I completely missing the point here?

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[gengdongjiu]

Marc,
   Thanks for reply.

On 2017/9/8 16:21, Marc Zyngier wrote:
>> Marc,
>>
>> sorry I have another question for the PAN.
>>
>> In the non-VHE mode, The host kernel is running in the EL1. Before
>> host kernel enter guest, host OS will call 'HVC' instruction to do
>> the world-switch, and the pstate.PAN will be saved into the SPSR_EL2.
>> When world-switch back to host kernel from EL2, it will call 'eret'
>> instruction to EL1 host, this 'eret' instruction will restore the
>> SPSR_EL2 to the PSTATE. so the PSTATE.PAN will be restored.
>>
>> For the Non-VHE mode, in the EL2 where mainly have word-switch code,
>> do you think it needs to reset the PSTATE.PAN? From the spec, it does
>> not provide SCTLR_EL2.SPAN bit for non-VHE mode, so reset the
>> PSTATE.PAN does not sure whether it is needed or whether affects the
>> performance. If you think it is needed for El2 in Non-VHE mode,
>> moving the reset PSTATE.PAN to the exception entry to EL2 may be
>> better, such as "el1_sync", because host can also call 'hvc'
>> instruction without guest running.
> So let's see if I correctly understand your question:
> 
> You're worried that we don't set/reset PSTATE.PAN at EL2 in non-VHE?
> In non-VHE, there is no user-space mapping that is present at the
> same time as the hypervisor mappings. Actually, we hardly have any
> mapping other than the HYP text/data and the vcpu/vm structures.

Not that meaning.
there are two meanings:

In short, we should not set PAN for El2 in non-VHE; If you think we should, current code does not cover all scenarios.


1. In the current mainline code it sets the PSTATE.PAN at EL2 in non-VHE. As you said,
in non-VHE, there is no user-space mapping that is present at the same time as the
hypervisor mappings, so I think it may not need to set both for EL1 and El2 in non-VHE,
but current code sets it. As you see[1], the code does not check VHE.

2. Conversely, in non-VHE, if you think we should set PAN in the EL2,
current code only sets it in the guest_exit path, do not cover all scenarios.
For example, when there is no guest, only have host, host calling 'HVC' instruction enter to El2 to do somethings,
then it will not call the guest_exit, so the PAN will not be set.
In order to handle this case, we should move it to the 'el1_sync'


ENTRY(__guest_exit)
        // x0: return code
        // x1: vcpu
        // x2-x29,lr: vcpu regs
        // vcpu x0-x1 on the stack

        add     x1, x1, #VCPU_CONTEXT

        ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)  	[1]

        // Store the guest regs x2 and x3
        stp     x2, x3,   [x1, #CPU_XREG_OFFSET(2)]

        // Retrieve the guest regs x0-x1 from the stack
        ldp     x2, x3, [sp], #16       // x0, x1


> 
> So how is PAN relevant in this context? What does it even mean?
> If you have a ARMv8.0 behaviour, PAN doesn't even seem to *exist* at
> EL2.
> 
> Or am I completely missing the point here?

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[Marc Zyngier]

On 08/09/17 10:05, gengdongjiu wrote:
> Marc,
>    Thanks for reply.
> 
> On 2017/9/8 16:21, Marc Zyngier wrote:
>>> Marc,
>>>
>>> sorry I have another question for the PAN.
>>>
>>> In the non-VHE mode, The host kernel is running in the EL1. Before
>>> host kernel enter guest, host OS will call 'HVC' instruction to do
>>> the world-switch, and the pstate.PAN will be saved into the SPSR_EL2.
>>> When world-switch back to host kernel from EL2, it will call 'eret'
>>> instruction to EL1 host, this 'eret' instruction will restore the
>>> SPSR_EL2 to the PSTATE. so the PSTATE.PAN will be restored.
>>>
>>> For the Non-VHE mode, in the EL2 where mainly have word-switch code,
>>> do you think it needs to reset the PSTATE.PAN? From the spec, it does
>>> not provide SCTLR_EL2.SPAN bit for non-VHE mode, so reset the
>>> PSTATE.PAN does not sure whether it is needed or whether affects the
>>> performance. If you think it is needed for El2 in Non-VHE mode,
>>> moving the reset PSTATE.PAN to the exception entry to EL2 may be
>>> better, such as "el1_sync", because host can also call 'hvc'
>>> instruction without guest running.
>> So let's see if I correctly understand your question:
>>
>> You're worried that we don't set/reset PSTATE.PAN at EL2 in non-VHE?
>> In non-VHE, there is no user-space mapping that is present at the
>> same time as the hypervisor mappings. Actually, we hardly have any
>> mapping other than the HYP text/data and the vcpu/vm structures.
> 
> Not that meaning.
> there are two meanings:
> 
> In short, we should not set PAN for El2 in non-VHE; If you think we should, current code does not cover all scenarios.
> 
> 
> 1. In the current mainline code it sets the PSTATE.PAN at EL2 in non-VHE. As you said,
> in non-VHE, there is no user-space mapping that is present at the same time as the
> hypervisor mappings, so I think it may not need to set both for EL1 and El2 in non-VHE,
> but current code sets it. As you see[1], the code does not check VHE.
> 
> 2. Conversely, in non-VHE, if you think we should set PAN in the EL2,

It is not about what I think. It is about what the architecture gives you.

There cannot be any userspace mapping at EL2 when non-VHE, so there
cannot be any valid PAN setting. I repeat: there is not such thing as
PAN at EL2 when HCR_EL2.E2H==0. This bit *has no effect*. Just read the
documentation (ARM DDI 0487B.a, D4.4.2).

If you're going to change this kind of code, please start by
understanding the architecture.

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

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[gengdongjiu]

Hi Marc,

> 
> On 08/09/17 10:05, gengdongjiu wrote:
> > Marc,
> >    Thanks for reply.
> >
> > On 2017/9/8 16:21, Marc Zyngier wrote:
> >>> Marc,
> >>>
> >>> sorry I have another question for the PAN.
[...]
> There cannot be any userspace mapping at EL2 when non-VHE, so there cannot be any valid PAN setting. I repeat: there is not such thing as
> PAN at EL2 when HCR_EL2.E2H==0. This bit *has no effect*. Just read the documentation (ARM DDI 0487B.a, D4.4.2).

Thanks Marc's comments, I completely agree with you, I understand the architecture, in non-VHE, user space mapping only exists at EL1 and pointed by the ttbr0_el1.
In the EL1, it uses ttbr0_el1 to point userspace page table, uses ttbr1_el1 to point kernel mapping.
In the EL2, it uses ttbr0_el2 or vttbr_el2 for address translation, cannot use the ttbr0_el1. 

> 
> If you're going to change this kind of code, please start by understanding the architecture.

OK, so I plant to remove the PAN setting at EL2 in non-VHE.

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

[PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[Dongjiu Geng]

In VHE mode, host kernel runs in the EL2 and can enable
'User Access Override' when fs==KERNEL_DS so that it can
access kernel memory. However, PSTATE.UAO is set to 0 on
an exception taken from EL1 to EL2. Thus when VHE is used
and exception taken from a guest UAO will be disabled and
host will use the incorrect PSTATE.UAO. So check and reset
the PSTATE.UAO when switching to host.

Move the reset PSTATE.PAN on entry to EL2 together with
PSTATE.UAO reset.

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
Signed-off-by: Haibin Zhang <zhanghaibin7@huawei.com>
Tested-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
 arch/arm64/kvm/hyp/entry.S  |  2 --
 arch/arm64/kvm/hyp/switch.c | 12 ++++++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index 12ee62d..7662ef5 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -96,8 +96,6 @@ ENTRY(__guest_exit)
 
 	add	x1, x1, #VCPU_CONTEXT
 
-	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
-
 	// Store the guest regs x2 and x3
 	stp	x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
 
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index a733461..715b3941 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -22,6 +22,7 @@
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_hyp.h>
 #include <asm/fpsimd.h>
+#include <asm/exec.h>
 
 static bool __hyp_text __fpsimd_enabled_nvhe(void)
 {
@@ -399,6 +400,17 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 
 	__sysreg_restore_host_state(host_ctxt);
 
+	if (has_vhe()) {
+		/*
+		 * PSTATE was not saved over guest enter/exit, re-enable
+		 * any detecte features that might not have been set
+		 * correctly.
+		 */
+		uao_thread_switch(current);
+		asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1),
+			ARM64_HAS_PAN, CONFIG_ARM64_PAN));
+	}
+
 	if (fp_enabled) {
 		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
 		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
-- 
1.8.3.1

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[James Morse]

Hi Dongjiu Geng,

On 07/09/17 06:54, Dongjiu Geng wrote:
> In VHE mode, host kernel runs in the EL2 and can enable
> 'User Access Override' when fs==KERNEL_DS so that it can
> access kernel memory. However, PSTATE.UAO is set to 0 on
> an exception taken from EL1 to EL2. Thus when VHE is used
> and exception taken from a guest UAO will be disabled and
> host will use the incorrect PSTATE.UAO. So check and reset
> the PSTATE.UAO when switching to host.

This would only be a problem if KVM were calling into world-switch with
fs==KERNEL_DS. I can't see where this happens.

kvm_arch_vcpu_ioctl_run() is the only place KVM calls world-switch, there are no
set_fs() calls in it, or on the path to it. The addr_limit should be USER_DS,
PSTATE.UAO will be clear, as it is when we come back from a guest.

This isn't broken today. I agree it will break if KVM decides to
set_fs(KERNEL_DS) around world switch, but until then we don't need this patch.


> Move the reset PSTATE.PAN on entry to EL2 together with
> PSTATE.UAO reset.

Moving this breaks PAN-at-HYP for systems with PAN but without VHE.


> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 12ee62d..7662ef5 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -96,8 +96,6 @@ ENTRY(__guest_exit)
>  
>  	add	x1, x1, #VCPU_CONTEXT
>  
> -	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
> -
>  	// Store the guest regs x2 and x3
>  	stp	x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
>  
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index a733461..715b3941 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -22,6 +22,7 @@
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_hyp.h>
>  #include <asm/fpsimd.h>
> +#include <asm/exec.h>
>  
>  static bool __hyp_text __fpsimd_enabled_nvhe(void)
>  {
> @@ -399,6 +400,17 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  
>  	__sysreg_restore_host_state(host_ctxt);
>  
> +	if (has_vhe()) {
> +		/*
> +		 * PSTATE was not saved over guest enter/exit, re-enable
> +		 * any detecte features that might not have been set
> +		 * correctly.
> +		 */
> +		uao_thread_switch(current);

I don't see how addr_limit will ever be KERNEL_DS, so this is always clearing
PSTATE.UAO, which was already clear from the guest-exit exception.

(Also, the uao_thread_switch() code isn't accessible from EL2, neither is current)


> +		asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1),
> +			ARM64_HAS_PAN, CONFIG_ARM64_PAN));

... and this is setting PSTATE.PAN on VHE, which was already set, and breaking
PAN-at-HYP on non-VHE systems.

Vladimir's commit message for that patch that added this enabling explained it
is needed for !VHE as SCTLR_EL2 when HCR_EL2.E2H is clear doesn't have a SPAN bit.

When we have VHE clearing SCTLR_EL2.SPAN (clearing because it was RES1 on v8.0)
will cause the CPU to set PSTATE.PAN when we take an exception.


> +	}
> +
>  	if (fp_enabled) {
>  		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
>  		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> 


James

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[gengdongjiu]

Hi James,

On 2017/9/7 17:20, James Morse wrote:
> Hi Dongjiu Geng,
> 
> On 07/09/17 06:54, Dongjiu Geng wrote:
>> In VHE mode, host kernel runs in the EL2 and can enable
>> 'User Access Override' when fs==KERNEL_DS so that it can
>> access kernel memory. However, PSTATE.UAO is set to 0 on
>> an exception taken from EL1 to EL2. Thus when VHE is used
>> and exception taken from a guest UAO will be disabled and
>> host will use the incorrect PSTATE.UAO. So check and reset
>> the PSTATE.UAO when switching to host.
> 
> This would only be a problem if KVM were calling into world-switch with
> fs==KERNEL_DS. I can't see where this happens.
 Not only KVM, may also kernel sets the fs == KERNEL_DS before calling into world-switch

> 
> kvm_arch_vcpu_ioctl_run() is the only place KVM calls world-switch, there are no
> set_fs() calls in it, or on the path to it. The addr_limit should be USER_DS,
> PSTATE.UAO will be clear, as it is when we come back from a guest.
how about if kernel set the KERNEL_DS? but not the kvm_arch_vcpu_ioctl_run().

> 
> This isn't broken today. I agree it will break if KVM decides to
> set_fs(KERNEL_DS) around world switch, but until then we don't need this patch.
KVM and host kernel set_fs(KERNEL_DS) all can break this.
In the normal way, after world-switch, I think it should check whether it needs to restore to its previous state.
we should not always consider the set_fs(KERNEL_DS) is disabled for the host.

> 
> 
>> Move the reset PSTATE.PAN on entry to EL2 together with
>> PSTATE.UAO reset.
> 
> Moving this breaks PAN-at-HYP for systems with PAN but without VHE.
No, without VHE, the host kernel is running in the EL1.
Before host kernel enter guest, host OS will call 'HVC' instruction to do the world-switch, and the pstate.PAN will be saved into the SPSR_EL2.
When world-switch back to host kernel from EL2, it will call 'eret' instruction to EL1 host, this 'eret' instruction will restore the SPSR_EL2 to the PSTATE.
so the PSTATE.PAN will be restored.
So without VHE, we should not reset the PAN. I paste the spec statement

------------------------------
PSTATE.PAN is copied to SPSR_ELx.PAN on an exception taken from AArch64 to AArch64
SPSR_ELx.PAN is copied to PSTATE.PAN on an exception return to AArch64 from AArch64


> 
> 
>> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
>> index 12ee62d..7662ef5 100644
>> --- a/arch/arm64/kvm/hyp/entry.S
>> +++ b/arch/arm64/kvm/hyp/entry.S
>> @@ -96,8 +96,6 @@ ENTRY(__guest_exit)
>>  
>>  	add	x1, x1, #VCPU_CONTEXT
>>  
>> -	ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
>> -
>>  	// Store the guest regs x2 and x3
>>  	stp	x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
>>  
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index a733461..715b3941 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -22,6 +22,7 @@
>>  #include <asm/kvm_emulate.h>
>>  #include <asm/kvm_hyp.h>
>>  #include <asm/fpsimd.h>
>> +#include <asm/exec.h>
>>  
>>  static bool __hyp_text __fpsimd_enabled_nvhe(void)
>>  {
>> @@ -399,6 +400,17 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>>  
>>  	__sysreg_restore_host_state(host_ctxt);
>>  
>> +	if (has_vhe()) {
>> +		/*
>> +		 * PSTATE was not saved over guest enter/exit, re-enable
>> +		 * any detecte features that might not have been set
>> +		 * correctly.
>> +		 */
>> +		uao_thread_switch(current);
> 
> I don't see how addr_limit will ever be KERNEL_DS, so this is always clearing
> PSTATE.UAO, which was already clear from the guest-exit exception.
I think we should not always consider the host kernel does not set the KERNEL_DS before entering guest.

> 
> (Also, the uao_thread_switch() code isn't accessible from EL2, neither is current)
No,
for the VHE, both the uao_thread_switch() and current can be accessible from the EL2.
all the host kernel runs in the EL2.
The API can be accessible from EL2 to the VHE.
The current is Qemu or other kvm tools
I have tested this patch, it is workable.

> 
> 
>> +		asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1),
>> +			ARM64_HAS_PAN, CONFIG_ARM64_PAN));
> 
> ... and this is setting PSTATE.PAN on VHE, which was already set, and breaking
> PAN-at-HYP on non-VHE systems.
> 
> Vladimir's commit message for that patch that added this enabling explained it
> is needed for !VHE as SCTLR_EL2 when HCR_EL2.E2H is clear doesn't have a SPAN bit.
> 
> When we have VHE clearing SCTLR_EL2.SPAN (clearing because it was RES1 on v8.0)
> will cause the CPU to set PSTATE.PAN when we take an exception.
> 
> 
>> +	}
>> +
>>  	if (fp_enabled) {
>>  		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
>>  		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
>>
> 
> 
> James
> 
> 
> .
> 

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[Marc Zyngier]

On 07/09/17 11:05, gengdongjiu wrote:
> Hi James,
> 
> On 2017/9/7 17:20, James Morse wrote:
>> Hi Dongjiu Geng,
>>
>> On 07/09/17 06:54, Dongjiu Geng wrote:
>>> In VHE mode, host kernel runs in the EL2 and can enable
>>> 'User Access Override' when fs==KERNEL_DS so that it can
>>> access kernel memory. However, PSTATE.UAO is set to 0 on
>>> an exception taken from EL1 to EL2. Thus when VHE is used
>>> and exception taken from a guest UAO will be disabled and
>>> host will use the incorrect PSTATE.UAO. So check and reset
>>> the PSTATE.UAO when switching to host.
>>
>> This would only be a problem if KVM were calling into world-switch with
>> fs==KERNEL_DS. I can't see where this happens.
>  Not only KVM, may also kernel sets the fs == KERNEL_DS before calling into world-switch

How? Please describe the exact sequence of event that lead to this
situation with the current code base.

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

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[gengdongjiu]

On 2017/9/7 18:13, Marc Zyngier wrote:
> On 07/09/17 11:05, gengdongjiu wrote:
>> Hi James,
>>
>> On 2017/9/7 17:20, James Morse wrote:
>>> Hi Dongjiu Geng,
>>>
>>> On 07/09/17 06:54, Dongjiu Geng wrote:
>>>> In VHE mode, host kernel runs in the EL2 and can enable
>>>> 'User Access Override' when fs==KERNEL_DS so that it can
>>>> access kernel memory. However, PSTATE.UAO is set to 0 on
>>>> an exception taken from EL1 to EL2. Thus when VHE is used
>>>> and exception taken from a guest UAO will be disabled and
>>>> host will use the incorrect PSTATE.UAO. So check and reset
>>>> the PSTATE.UAO when switching to host.
>>>
>>> This would only be a problem if KVM were calling into world-switch with
>>> fs==KERNEL_DS. I can't see where this happens.
>>  Not only KVM, may also kernel sets the fs == KERNEL_DS before calling into world-switch
> 
> How? Please describe the exact sequence of event that lead to this
> situation with the current code base.

Hi Marc,

   Different tasks have different fs, such as USER_DS or KERNEL_DS. In the context switch, it will restore the
task's fs. Thus, that depends on task itself, as shown below code. UAO is different with PAN, PAN will be always enabled if
hardware CPU supports PAN feature, but UAO is dynamical change.

/*
 * Thread switching.
 */
__notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
				struct task_struct *next)
{
	struct task_struct *last;

	fpsimd_thread_switch(next);
	tls_thread_switch(next);
	hw_breakpoint_thread_switch(next);
	contextidr_thread_switch(next);
	entry_task_switch(next);
	uao_thread_switch(next);
 	..........
}

/* Restore the UAO state depending on next's addr_limit */
void uao_thread_switch(struct task_struct *next)
{
	if (IS_ENABLED(CONFIG_ARM64_UAO)) {
		if (task_thread_info(next)->addr_limit == KERNEL_DS)
			asm(ALTERNATIVE("nop", SET_PSTATE_UAO(1), ARM64_HAS_UAO));
		else
			asm(ALTERNATIVE("nop", SET_PSTATE_UAO(0), ARM64_HAS_UAO));
	}
}

> 
> 	M.
> 

Re: [PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

[Marc Zyngier]

On 07/09/17 12:49, gengdongjiu wrote:
> 
> 
> On 2017/9/7 18:13, Marc Zyngier wrote:
>> On 07/09/17 11:05, gengdongjiu wrote:
>>> Hi James,
>>>
>>> On 2017/9/7 17:20, James Morse wrote:
>>>> Hi Dongjiu Geng,
>>>>
>>>> On 07/09/17 06:54, Dongjiu Geng wrote:
>>>>> In VHE mode, host kernel runs in the EL2 and can enable
>>>>> 'User Access Override' when fs==KERNEL_DS so that it can
>>>>> access kernel memory. However, PSTATE.UAO is set to 0 on
>>>>> an exception taken from EL1 to EL2. Thus when VHE is used
>>>>> and exception taken from a guest UAO will be disabled and
>>>>> host will use the incorrect PSTATE.UAO. So check and reset
>>>>> the PSTATE.UAO when switching to host.
>>>>
>>>> This would only be a problem if KVM were calling into world-switch with
>>>> fs==KERNEL_DS. I can't see where this happens.
>>>  Not only KVM, may also kernel sets the fs == KERNEL_DS before calling into world-switch
>>
>> How? Please describe the exact sequence of event that lead to this
>> situation with the current code base.
> 
> Hi Marc,
> 
>    Different tasks have different fs, such as USER_DS or KERNEL_DS. In the context switch, it will restore the
> task's fs. Thus, that depends on task itself, as shown below code. UAO is different with PAN, PAN will be always enabled if
> hardware CPU supports PAN feature, but UAO is dynamical change.

You haven't answered my question: There is exactly one point where we
enter the world-switch. Show me that, at this point, PSTATE.UAO *before*
the call is different from PSTATE.UAO after the call. Give me the exact
sequence of event that leads to this situation. Show me a stack trace.

Until you do this, I will ignore any further comment coming from you on
this subject.

Thanks,

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