Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

[gengdongjiu]

For UAO, if not save/restore PSTATE.UAO, we can use below fixing.

diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 9341376..c3dd761 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -21,6 +21,8 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_hyp.h>

+#include <asm/exec.h>
+
 /* Yes, this does nothing, on purpose */
 static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }

@@ -121,8 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt)
 	write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr);
 }

+static void __hyp_text __sysreg_restore_state_vhe(struct kvm_cpu_context *ctxt)
+{
+    uao_thread_switch(current);
+}
+
 static hyp_alternate_select(__sysreg_call_restore_host_state,
-			    __sysreg_restore_state, __sysreg_do_nothing,
+			    __sysreg_restore_state, __sysreg_restore_state_vhe,
 			    ARM64_HAS_VIRT_HOST_EXTN);

 void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)



On 2017/9/6 17:32, gengdongjiu wrote:
> Hi Marc,
> 
> On 2017/9/6 16:17, Marc Zyngier wrote:
>> On 05/09/17 19:58, gengdongjiu wrote:
>>> when exit from guest, some host PSTATE bits may be lost, such as
>>> PSTATE.PAN or PSTATE.UAO. It is because host and hypervisor all run
>>> in the EL2, host PSTATE value cannot be saved and restored via
>>> SPSR_EL2. So if guest has changed the PSTATE, host continues with
>>> a wrong value guest has set.
>>>
>>> Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx>
>>> Signed-off-by: Haibin Zhang <zhanghaibin7@xxxxxxxxxx>
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h |  8 +++++++
>>>  arch/arm64/include/asm/kvm_hyp.h  |  2 ++
>>>  arch/arm64/include/asm/sysreg.h   | 23 +++++++++++++++++++
>>>  arch/arm64/kvm/hyp/entry.S        |  2 --
>>>  arch/arm64/kvm/hyp/switch.c       | 24 ++++++++++++++++++--
>>>  arch/arm64/kvm/hyp/sysreg-sr.c    | 48 ++++++++++++++++++++++++++++++++++++---
>>>  6 files changed, 100 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index e923b58..cba7d3e 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -193,6 +193,12 @@ struct kvm_cpu_context {
>>>  	};
>>>  };
>>>  
>>> +struct kvm_cpu_host_pstate {
>>> +	u64 daif;
>>> +	u64 uao;
>>> +	u64 pan;
>>> +};
>>
>> I love it. This is the most expensive way of saving/restoring a single
>> 32bit value.
>>
>> More seriously, please see the discussion between James and Christoffer
>> there[1]. I expect James to address the PAN/UAO states together with the
>> debug state in the next iteration of his patch.
> 
> I roughly see the discussion between James and Christoffer, Seems Christoffer does not suggest save and
> restore it, and suggest to do below, and UAO/PAN may not use the same ways.
> 
>       __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>       {
>           if (has_vhe())
>               asm("msr     daifset, #0xf");
> 
>  	...
>          exit_code = __guest_enter(vcpu, host_ctxt);
>  	...
> 
>  	if (has_vhe())
>               asm("msr     daifclr, #0xd");
>       }
> 
> 
> If not save/restore them, the KVM will set them according to the CPU capability. For example below fixing, it will check CPU capability, If CPU supports PAN,
> the kvm will always enable the PAN for the host. But in fact, the host may be not enable the PAN.
> Of course for the UAO, we can use the similar fixing if Marc or Christoffer is agreed. but seems not make sense.
> 
> commit cb96408da4e11698674abd04aeac941c1bed2038
> Author: Vladimir Murzin <vladimir.murzin@xxxxxxx>
> Date:   Thu Sep 1 15:29:03 2016 +0100
> 
>     arm64: KVM: VHE: reset PSTATE.PAN on entry to EL2
> 
>     SCTLR_EL2.SPAN bit controls what happens with the PSTATE.PAN bit on an
>     exception. However, this bit has no effect on the PSTATE.PAN when
>     HCR_EL2.E2H or HCR_EL2.TGE is unset. Thus when VHE is used and
>     exception taken from a guest PSTATE.PAN bit left unchanged and we
>     continue with a value guest has set.
> 
>     To address that always reset PSTATE.PAN on entry from EL1.
> 
>     Fixes: 1f364c8c48a0 ("arm64: VHE: Add support for running Linux in EL2 mode")
> 
>     Signed-off-by: Vladimir Murzin <vladimir.murzin@xxxxxxx>
>     Reviewed-by: James Morse <james.morse@xxxxxxx>
>     Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>     Cc: <stable@xxxxxxxxxxxxxxx> # v4.6+
>     Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> 
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 3967c231..b5926ee 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -96,6 +96,8 @@ 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)]
> 
> 
>>
>> Thanks,
>>
>> 	M.
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg599798.html
>>