All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits
@ 2017-09-06 14:10 ` gengdongjiu
  0 siblings, 0 replies; 62+ messages in thread
From: gengdongjiu @ 2017-09-06 14:10 UTC (permalink / raw)
  To: Vladimir Murzin, Marc Zyngier, christoffer.dall, pbonzini,
	rkrcmar, linux-arm-kernel, kvmarm, kvm, linux-kernel,
	suzuki.poulose, mark.rutland, catalin.marinas
  Cc: James Morse, Zhanghaibin (Euler), Huangshaoyu

Hi, Vladimir

> >> Do you see effect of "PAN is unexpectedly enabled"?
> > In fact I did not encounter this case, but I think it can exist.
> > I think if host OS dynamically disable PAN, it wants the host kernel access the user space address space not through copy_to/from_user
> API.
> > Now if it is unexpectedly enabled, when host kernel still accesses the user space address,  it will happen MMU fault exception.
> 
> And this is expected! The only allowed channel for kernel <-> user is uaccess API.
> 
> I guess that you have test (and that great!) which violates that rule (for testing purpose, why not?) and now you are trying to fit kernel into
> it...


If you think that makes sense for it, we do not consider the paste.PAN in the world-switch.
For the pstate.UAO issue, do you agree my fixing or you have other suggestion?  Also to other reviewer. Thanks.

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)


> 
> Cheers
> Vladimir
> 
> >
> >
> >>
> >> Cheers
> >> Vladimir
> >>
> >>>
> >>>>
> >>>> Cheers
> >>>> Vladimir
> >>>>
> >>>> .
> >>>>
> >>>
> >>>
> >>
> >>
> >> .
> >>
> >
> >

^ permalink raw reply related	[flat|nested] 62+ messages in thread
* re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits
@ 2017-09-06 22:09 ` gengdongjiu
  0 siblings, 0 replies; 62+ messages in thread
From: gengdongjiu @ 2017-09-06 22:09 UTC (permalink / raw)
  To: vladimir.murzin, marc.zyngier, christoffer.dall, pbonzini,
	rkrcmar, linux-arm-kernel, kvmarm, kvm, linux-kernel,
	suzuki.poulose, mark.rutland, Catalin Marinas
  Cc: Huangshaoyu, Zhanghaibin (Euler)


[-- Attachment #1.1: Type: text/plain, Size: 3860 bytes --]

The negative effect is that kernel can not access kernel space adress through copy_to/from_user even KERNEL_DS is set
发件人：vladimir.murzin
收件人：耿东久,marc.zyngier,christoffer.dall,pbonzini,rkrcmar,linux-arm-kernel,kvmarm,kvm,linux-kernel,suzuki.poulose,mark.rutland,Catalin Marinas
抄送：James.Morse,张海斌,黄韶宇
时间：2017-09-06 23:19:38
主题:Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

On 06/09/17 16:08, gengdongjiu wrote:
> It is similar with the PAN，when the guest traps to el2，it will reset the pstate.PAN <http://pstate.PAN> to 0, and continue run。In fact the host pstate.UAO <http://pstate.UAO> can be 1, but guest change it to 0 when trap to EL2。so after swich to host，need to check whether set pstate.UAO <http://pstate.UAO> again。

What negative effect do you see with change UAO to 0 (i.e. do not override)?

P. S.
Please, avoid HTML and top posting

Vladimir

> *发件人：*Vladimir Murzin
> *收件人：*耿东久,marc.zyngier,christoffer.dall@linaro.org,pbonzini@redhat.com,rkrcmar@redhat.com,linux-arm-kernel@lists.infradead.org,kvmarm@lists.cs.columbia.edu,kvm@vger.kernel.org,linux-kernel,suzuki.poulose@arm.com,mark.rutland@arm.com,Catalin Marinas
> *抄送：*James Morse,张海斌,黄韶宇
> *时间：*2017-09-06 22:41:09
> *主题:*Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits
>
> On 06/09/17 15:10, gengdongjiu wrote:
>> Hi, Vladimir
>>
>>>>> Do you see effect of "PAN is unexpectedly enabled"?
>>>> In fact I did not encounter this case, but I think it can exist.
>>>> I think if host OS dynamically disable PAN, it wants the host kernel access the user space address space not through copy_to/from_user
>>> API.
>>>> Now if it is unexpectedly enabled, when host kernel still accesses the user space address,  it will happen MMU fault exception.
>>>
>>> And this is expected! The only allowed channel for kernel <-> user is uaccess API.
>>>
>>> I guess that you have test (and that great!) which violates that rule (for testing purpose, why not?) and now you are trying to fit kernel into
>>> it...
>>
>>
>> If you think that makes sense for it, we do not consider the paste.PAN in the world-switch.
>> For the pstate.UAO issue, do you agree my fixing or you have other suggestion?  Also to other reviewer. Thanks.
>
> It would help if you give precise description on "pstate.UAO issue".
>
> Thanks
> Vladimir
>
>>
>> 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)
>>
>>
>>>
>>> Cheers
>>> Vladimir
>>>
>>>>
>>>>
>>>>>
>>>>> Cheers
>>>>> Vladimir
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Cheers
>>>>>>> Vladimir
>>>>>>>
>>>>>>> .
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>
>


[-- Attachment #1.2: Type: text/html, Size: 6823 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 62+ messages in thread
* [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits
@ 2017-09-05 18:58 ` gengdongjiu
  0 siblings, 0 replies; 62+ messages in thread
From: gengdongjiu @ 2017-09-05 18:58 UTC (permalink / raw)
  To: christoffer.dall, marc.zyngier, pbonzini, rkrcmar,
	vladimir.murzin, linux-arm-kernel, kvmarm, kvm, linux-kernel

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@huawei.com>
Signed-off-by: Haibin Zhang <zhanghaibin7@huawei.com>
---
 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;
+};
+
 typedef struct kvm_cpu_context kvm_cpu_context_t;
 
 struct kvm_vcpu_arch {
@@ -227,6 +233,8 @@ struct kvm_vcpu_arch {
 
 	/* Pointer to host CPU context */
 	kvm_cpu_context_t *host_cpu_context;
+	/* Host PSTATE value */
+	struct kvm_cpu_host_pstate host_pstate;
 	struct {
 		/* {Break,watch}point registers */
 		struct kvm_guest_debug_arch regs;
diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index 4572a9b..a75587a 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -134,6 +134,8 @@
 
 void __sysreg_save_host_state(struct kvm_cpu_context *ctxt);
 void __sysreg_restore_host_state(struct kvm_cpu_context *ctxt);
+void __sysreg_save_host_pstate(struct kvm_vcpu *vcpu);
+void __sysreg_restore_host_pstate(struct kvm_vcpu *vcpu);
 void __sysreg_save_guest_state(struct kvm_cpu_context *ctxt);
 void __sysreg_restore_guest_state(struct kvm_cpu_context *ctxt);
 void __sysreg32_save_state(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 248339e..efdcf40 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -295,6 +295,29 @@
 #define SYS_ICH_LR14_EL2		__SYS__LR8_EL2(6)
 #define SYS_ICH_LR15_EL2		__SYS__LR8_EL2(7)
 
+#define REG_PSTATE_PAN			sys_reg(3, 0, 4, 2, 3)
+#define REG_PSTATE_UAO			sys_reg(3, 0, 4, 2, 4)
+
+#define GET_PSTATE_PAN					\
+	({						\
+		u64 reg;				\
+		asm volatile(ALTERNATIVE("mov %0, #0",	\
+				"mrs_s %0, " __stringify(REG_PSTATE_PAN),\
+				ARM64_HAS_PAN)\
+				: "=r" (reg));\
+		reg;				\
+	})
+
+#define GET_PSTATE_UAO					\
+	({						\
+		u64 reg;					\
+		asm volatile(ALTERNATIVE("mov %0, #0",\
+				"mrs_s %0, " __stringify(REG_PSTATE_UAO),\
+				ARM64_HAS_UAO)\
+				: "=r" (reg));\
+		reg;			\
+	})
+
 /* Common SCTLR_ELx flags. */
 #define SCTLR_ELx_EE    (1 << 25)
 #define SCTLR_ELx_I	(1 << 12)
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 945e79c..9b380a1 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -278,6 +278,26 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
 	write_sysreg_el2(*vcpu_pc(vcpu), elr);
 }
 
+static void __hyp_text __save_host_state(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpu_context *host_ctxt;
+
+	host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
+
+	__sysreg_save_host_state(host_ctxt);
+	__sysreg_save_host_pstate(vcpu);
+}
+
+static void __hyp_text __restore_host_state(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpu_context *host_ctxt;
+
+	host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
+
+	__sysreg_restore_host_state(host_ctxt);
+	__sysreg_restore_host_pstate(vcpu);
+}
+
 int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
@@ -291,7 +311,7 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 	host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
 	guest_ctxt = &vcpu->arch.ctxt;
 
-	__sysreg_save_host_state(host_ctxt);
+	__save_host_state(vcpu);
 	__debug_cond_save_host_state(vcpu);
 
 	__activate_traps(vcpu);
@@ -374,7 +394,7 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 	__deactivate_traps(vcpu);
 	__deactivate_vm(vcpu);
 
-	__sysreg_restore_host_state(host_ctxt);
+	__restore_host_state(vcpu);
 
 	if (fp_enabled) {
 		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 9341376..ea8f437 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -22,7 +22,11 @@
 #include <asm/kvm_hyp.h>
 
 /* Yes, this does nothing, on purpose */
-static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }
+static void __hyp_text __sysreg_do_nothing_state(struct kvm_cpu_context *ctxt)
+{ }
+static void __hyp_text __sysreg_do_nothing_pstate(struct kvm_vcpu *vcpu)
+{ }
+
 
 /*
  * Non-VHE: Both host and guest must save everything.
@@ -69,7 +73,7 @@ static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt)
 }
 
 static hyp_alternate_select(__sysreg_call_save_host_state,
-			    __sysreg_save_state, __sysreg_do_nothing,
+			    __sysreg_save_state, __sysreg_do_nothing_state,
 			    ARM64_HAS_VIRT_HOST_EXTN);
 
 void __hyp_text __sysreg_save_host_state(struct kvm_cpu_context *ctxt)
@@ -122,7 +126,7 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt)
 }
 
 static hyp_alternate_select(__sysreg_call_restore_host_state,
-			    __sysreg_restore_state, __sysreg_do_nothing,
+			    __sysreg_restore_state, __sysreg_do_nothing_state,
 			    ARM64_HAS_VIRT_HOST_EXTN);
 
 void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)
@@ -137,6 +141,44 @@ void __hyp_text __sysreg_restore_guest_state(struct kvm_cpu_context *ctxt)
 	__sysreg_restore_common_state(ctxt);
 }
 
+static void __hyp_text __sysreg_save_pstate(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.host_pstate.daif = read_sysreg(daif);
+	vcpu->arch.host_pstate.pan = GET_PSTATE_PAN;
+	vcpu->arch.host_pstate.uao = GET_PSTATE_UAO;
+}
+
+static hyp_alternate_select(__sysreg_call_save_host_pstate,
+			    __sysreg_save_pstate, __sysreg_do_nothing_pstate,
+			    ARM64_HAS_VIRT_HOST_EXTN);
+
+void __hyp_text __sysreg_save_host_pstate(struct kvm_vcpu *vcpu)
+{
+	__sysreg_call_save_host_pstate()(vcpu);
+}
+
+static void __hyp_text __sysreg_restore_pstate(struct kvm_vcpu *vcpu)
+{
+	u8 value = !!(vcpu->arch.host_pstate.pan);
+
+	write_sysreg(vcpu->arch.host_pstate.daif, daif);
+	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(value), ARM64_HAS_PAN,
+			CONFIG_ARM64_PAN));
+
+	value = !!(vcpu->arch.host_pstate.uao);
+	asm(ALTERNATIVE("nop", SET_PSTATE_UAO(value), ARM64_HAS_UAO,
+			CONFIG_ARM64_UAO));
+}
+
+static hyp_alternate_select(__sysreg_call_restore_host_pstate,
+			    __sysreg_restore_pstate, __sysreg_do_nothing_pstate,
+			    ARM64_HAS_VIRT_HOST_EXTN);
+
+void __hyp_text __sysreg_restore_host_pstate(struct kvm_vcpu *vcpu)
+{
+	__sysreg_call_restore_host_pstate()(vcpu);
+}
+
 void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
 {
 	u64 *spsr, *sysreg;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 62+ messages in thread
end of thread, other threads:[~2017-09-06 22:11 UTC | newest]

Thread overview: 62+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06 14:10 [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits gengdongjiu
2017-09-06 14:10 ` gengdongjiu
2017-09-06 14:10 ` gengdongjiu
2017-09-06 14:40 ` Vladimir Murzin
2017-09-06 14:40   ` Vladimir Murzin
2017-09-06 14:40   ` Vladimir Murzin
2017-09-06 14:40   ` Vladimir Murzin
2017-09-06 15:08   ` gengdongjiu
2017-09-06 15:08     ` gengdongjiu
2017-09-06 15:19     ` Vladimir Murzin
2017-09-06 15:19       ` Vladimir Murzin
  -- strict thread matches above, loose matches on Subject: below --
2017-09-06 22:09 gengdongjiu
2017-09-06 22:09 ` gengdongjiu
2017-09-05 18:58 gengdongjiu
2017-09-05 18:58 ` gengdongjiu
2017-09-05 18:58 ` gengdongjiu
2017-09-06  5:26 ` gengdongjiu
2017-09-06  5:26   ` gengdongjiu
2017-09-06  5:26   ` gengdongjiu
2017-09-06  5:26   ` gengdongjiu
2017-09-06  8:17 ` Marc Zyngier
2017-09-06  8:17   ` Marc Zyngier
2017-09-06  8:17   ` Marc Zyngier
2017-09-06  9:32   ` gengdongjiu
2017-09-06  9:32     ` gengdongjiu
2017-09-06  9:32     ` gengdongjiu
2017-09-06  9:32     ` gengdongjiu
2017-09-06  9:41     ` Vladimir Murzin
2017-09-06  9:41       ` Vladimir Murzin
2017-09-06  9:41       ` Vladimir Murzin
2017-09-06 10:35       ` gengdongjiu
2017-09-06 10:35         ` gengdongjiu
2017-09-06 10:35         ` gengdongjiu
2017-09-06 10:35         ` gengdongjiu
2017-09-06 12:00         ` Vladimir Murzin
2017-09-06 12:00           ` Vladimir Murzin
2017-09-06 12:00           ` Vladimir Murzin
2017-09-06 12:14           ` gengdongjiu
2017-09-06 12:14             ` gengdongjiu
2017-09-06 12:14             ` gengdongjiu
2017-09-06 12:14             ` gengdongjiu
2017-09-06 12:30             ` Vladimir Murzin
2017-09-06 12:30               ` Vladimir Murzin
2017-09-06 12:30               ` Vladimir Murzin
2017-09-06 12:44               ` gengdongjiu
2017-09-06 12:44                 ` gengdongjiu
2017-09-06 12:44                 ` gengdongjiu
2017-09-06 12:44                 ` gengdongjiu
2017-09-06 13:00                 ` Vladimir Murzin
2017-09-06 13:00                   ` Vladimir Murzin
2017-09-06 13:00                   ` Vladimir Murzin
2017-09-06 12:32           ` gengdongjiu
2017-09-06 12:32             ` gengdongjiu
2017-09-06 12:32             ` gengdongjiu
2017-09-06 12:32             ` gengdongjiu
2017-09-06  9:49     ` gengdongjiu
2017-09-06  9:49       ` gengdongjiu
2017-09-06  9:49       ` gengdongjiu
2017-09-06  9:49       ` gengdongjiu
2017-09-06 20:49 ` kbuild test robot
2017-09-06 20:49   ` kbuild test robot
2017-09-06 20:49   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.