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

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 <redacted>
Signed-off-by: Haibin Zhang <redacted>
---
 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

CC Catalin


On 2017/9/6 2:58, gengdongjiu wrote:

On 05/09/17 19:58, gengdongjiu wrote:

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.

Thanks,

	M.

[1] https://www.spinics.net/lists/arm-kernel/msg599798.html
-- 
Jazz is not dead. It just smells funny...

Hi Marc,

On 2017/9/6 16:17, Marc Zyngier wrote:

On 05/09/17 19:58, gengdongjiu wrote:

quoted

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 <redacted>
Signed-off-by: Haibin Zhang <redacted>
---
 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 [off-list ref]
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 [off-list ref]
    Reviewed-by: James Morse [off-list ref]
    Acked-by: Marc Zyngier [off-list ref]
    Cc: [off-list ref] # v4.6+
    Signed-off-by: Christoffer Dall [off-list ref]

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

On 06/09/17 10:32, gengdongjiu wrote:

Hi Marc,

On 2017/9/6 16:17, Marc Zyngier wrote:

quoted

On 05/09/17 19:58, gengdongjiu wrote:

quoted

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 <redacted>
Signed-off-by: Haibin Zhang <redacted>
---
 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.

Can you please elaborate on cases where PAN is not enabled?

Thanks
Vladimir

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:

Vladimir,

On 2017/9/6 17:41, Vladimir Murzin wrote:

Can you please elaborate on cases where PAN is not enabled?

I mean the informal private usage, For example, he disabled the PAN dynamically to let kernel space to access the user space.
After he dynamic disabled the PAN, then switched to guest OS. after return to host. he found the PAN stage is modified.
Of cause this is not a formal usage, in our host kernel, it is always enabled, no dynamic change, but I means it may exist such cases.

On 06/09/17 11:35, gengdongjiu wrote:

Vladimir,

On 2017/9/6 17:41, Vladimir Murzin wrote:

quoted

Can you please elaborate on cases where PAN is not enabled?

I mean the informal private usage, For example, he disabled the PAN dynamically to let kernel space to access the user space.
After he dynamic disabled the PAN, then switched to guest OS. after return to host. he found the PAN stage is modified.
Of cause this is not a formal usage, in our host kernel, it is always enabled, no dynamic change, but I means it may exist such cases.

So, in short, there is no real issue with PAN, right? What about UAO?

Cheers
Vladimir

On 2017/9/6 20:00, Vladimir Murzin wrote:

On 06/09/17 11:35, gengdongjiu wrote:

quoted

Vladimir,

On 2017/9/6 17:41, Vladimir Murzin wrote:

quoted

Can you please elaborate on cases where PAN is not enabled?

I mean the informal private usage, For example, he disabled the PAN dynamically to let kernel space to access the user space.
After he dynamic disabled the PAN, then switched to guest OS. after return to host. he found the PAN stage is modified.
Of cause this is not a formal usage, in our host kernel, it is always enabled, no dynamic change, but I means it may exist such cases.

So, in short, there is no real issue with PAN, right? What about UAO?

For the PAN, if host OS dynamically enable/disable PAN should have issue.
Do you think that is not a issue as above description?

"host OS dynamically disable the PAN, but after go back from the guest OS, The PAN is unexpectedly enabled"

Cheers
Vladimir

.

On 06/09/17 13:14, gengdongjiu wrote:

On 2017/9/6 20:00, Vladimir Murzin wrote:

quoted

On 06/09/17 11:35, gengdongjiu wrote:

quoted

Vladimir,

On 2017/9/6 17:41, Vladimir Murzin wrote:

quoted

Can you please elaborate on cases where PAN is not enabled?

I mean the informal private usage, For example, he disabled the PAN dynamically to let kernel space to access the user space.
After he dynamic disabled the PAN, then switched to guest OS. after return to host. he found the PAN stage is modified.
Of cause this is not a formal usage, in our host kernel, it is always enabled, no dynamic change, but I means it may exist such cases.

So, in short, there is no real issue with PAN, right? What about UAO?

For the PAN, if host OS dynamically enable/disable PAN should have issue.
Do you think that is not a issue as above description?

"host OS dynamically disable the PAN, but after go back from the guest OS, The PAN is unexpectedly enabled"

Do you see effect of "PAN is unexpectedly enabled"?

Cheers
Vladimir

quoted

Cheers
Vladimir

.

On 2017/9/6 20:00, Vladimir Murzin wrote:

On 06/09/17 11:35, gengdongjiu wrote:

quoted

Vladimir,

On 2017/9/6 17:41, Vladimir Murzin wrote:

quoted

Can you please elaborate on cases where PAN is not enabled?

I mean the informal private usage, For example, he disabled the PAN dynamically to let kernel space to access the user space.
After he dynamic disabled the PAN, then switched to guest OS. after return to host. he found the PAN stage is modified.
Of cause this is not a formal usage, in our host kernel, it is always enabled, no dynamic change, but I means it may exist such cases.

So, in short, there is no real issue with PAN, right? What about UAO?

For the pstate.UAO, current code has issue from my test. Because after switching from guest os, it does not set pstate.UAO again.
PAN is set again in your previous patch when switch to host, but UAO is not.
If you have concern about the save/restore PSTATE bits, may be we can use below modification to fix UAO issue.

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

.

On 2017/9/6 20:30, Vladimir Murzin wrote:

On 06/09/17 13:14, gengdongjiu wrote:

quoted

On 2017/9/6 20:00, Vladimir Murzin wrote:

quoted

On 06/09/17 11:35, gengdongjiu wrote:

quoted

Vladimir,

On 2017/9/6 17:41, Vladimir Murzin wrote:

quoted

Can you please elaborate on cases where PAN is not enabled?

I mean the informal private usage, For example, he disabled the PAN dynamically to let kernel space to access the user space.
After he dynamic disabled the PAN, then switched to guest OS. after return to host. he found the PAN stage is modified.
Of cause this is not a formal usage, in our host kernel, it is always enabled, no dynamic change, but I means it may exist such cases.

So, in short, there is no real issue with PAN, right? What about UAO?

For the PAN, if host OS dynamically enable/disable PAN should have issue.
Do you think that is not a issue as above description?

"host OS dynamically disable the PAN, but after go back from the guest OS, The PAN is unexpectedly enabled"

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.

Cheers
Vladimir

quoted

quoted

Cheers
Vladimir

.

.

On 06/09/17 13:44, gengdongjiu wrote:

On 2017/9/6 20:30, Vladimir Murzin wrote:

quoted

On 06/09/17 13:14, gengdongjiu wrote:

quoted

On 2017/9/6 20:00, Vladimir Murzin wrote:

quoted

On 06/09/17 11:35, gengdongjiu wrote:

quoted

Vladimir,

On 2017/9/6 17:41, Vladimir Murzin wrote:

quoted

Can you please elaborate on cases where PAN is not enabled?

I mean the informal private usage, For example, he disabled the PAN dynamically to let kernel space to access the user space.
After he dynamic disabled the PAN, then switched to guest OS. after return to host. he found the PAN stage is modified.
Of cause this is not a formal usage, in our host kernel, it is always enabled, no dynamic change, but I means it may exist such cases.

So, in short, there is no real issue with PAN, right? What about UAO?

For the PAN, if host OS dynamically enable/disable PAN should have issue.
Do you think that is not a issue as above description?

"host OS dynamically disable the PAN, but after go back from the guest OS, The PAN is unexpectedly enabled"

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...

Cheers
Vladimir

quoted

Cheers
Vladimir

quoted

quoted

Cheers
Vladimir

.

.

Hi gengdongjiu,

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on v4.13 next-20170906]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/gengdongjiu/arm64-KVM-VHE-save-and-restore-some-PSTATE-bits/20170907-013418
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   /tmp/ccT6UpbP.s: Assembler messages:

quoted

/tmp/ccT6UpbP.s:1415: Error: constant expression required

   /tmp/ccT6UpbP.s:1439: Error: constant expression required

quoted

/tmp/ccT6UpbP.s:1420: Error: attempt to move .org backwards

   /tmp/ccT6UpbP.s:1444: Error: attempt to move .org backwards

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 36347 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170907/e1a08923/attachment-0001.gz>