[v4,33/40] KVM: arm64: Configure c15, PMU, and debug register traps on cpu load/put for VHE
diff mbox

Message ID 20180215210332.8648-34-christoffer.dall@linaro.org
State New
Headers show

Commit Message

Christoffer Dall Feb. 15, 2018, 9:03 p.m. UTC
We do not have to change the c15 trap setting on each switch to/from the
guest on VHE systems, because this setting only affects EL0.

The PMU and debug trap configuration can also be done on vcpu load/put
instead, because they don't affect how the host kernel can access the
debug registers while executing KVM kernel code.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm64/include/asm/kvm_hyp.h |  3 +++
 arch/arm64/kvm/hyp/switch.c      | 31 ++++++++++++++++++++++---------
 arch/arm64/kvm/hyp/sysreg-sr.c   |  4 ++++
 3 files changed, 29 insertions(+), 9 deletions(-)

Comments

Marc Zyngier Feb. 21, 2018, 6:20 p.m. UTC | #1
On Thu, 15 Feb 2018 21:03:25 +0000,
Christoffer Dall wrote:
> 
> We do not have to change the c15 trap setting on each switch to/from the
> guest on VHE systems, because this setting only affects EL0.

Did you mean EL1 instead?

> 
> The PMU and debug trap configuration can also be done on vcpu load/put
> instead, because they don't affect how the host kernel can access the
> debug registers while executing KVM kernel code.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm64/include/asm/kvm_hyp.h |  3 +++
>  arch/arm64/kvm/hyp/switch.c      | 31 ++++++++++++++++++++++---------
>  arch/arm64/kvm/hyp/sysreg-sr.c   |  4 ++++
>  3 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index 2b1fda90dde4..949f2e77ae58 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -147,6 +147,9 @@ void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
>  void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
>  bool __fpsimd_enabled(void);
>  
> +void activate_traps_vhe_load(struct kvm_vcpu *vcpu);
> +void deactivate_traps_vhe_put(void);
> +
>  u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
>  void __noreturn __hyp_do_panic(unsigned long, ...);
>  
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 9c40e203bd09..5e94955b89ea 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -101,6 +101,8 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
>  {
>  	u64 val;
>  
> +	__activate_traps_common(vcpu);
> +
>  	val = CPTR_EL2_DEFAULT;
>  	val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;
>  	write_sysreg(val, cptr_el2);
> @@ -120,20 +122,12 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
>  		write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2);
>  
>  	__activate_traps_fpsimd32(vcpu);
> -	__activate_traps_common(vcpu);
>  	__activate_traps_arch()(vcpu);
>  }
>  
>  static void __hyp_text __deactivate_traps_vhe(void)
>  {
>  	extern char vectors[];	/* kernel exception vectors */
> -	u64 mdcr_el2 = read_sysreg(mdcr_el2);
> -
> -	mdcr_el2 &= MDCR_EL2_HPMN_MASK |
> -		    MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT |
> -		    MDCR_EL2_TPMS;
> -
> -	write_sysreg(mdcr_el2, mdcr_el2);
>  	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
>  	write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1);
>  	write_sysreg(vectors, vbar_el1);
> @@ -143,6 +137,8 @@ static void __hyp_text __deactivate_traps_nvhe(void)
>  {
>  	u64 mdcr_el2 = read_sysreg(mdcr_el2);
>  
> +	__deactivate_traps_common();
> +
>  	mdcr_el2 &= MDCR_EL2_HPMN_MASK;
>  	mdcr_el2 |= MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT;
>  
> @@ -166,10 +162,27 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu)
>  	if (vcpu->arch.hcr_el2 & HCR_VSE)
>  		vcpu->arch.hcr_el2 = read_sysreg(hcr_el2);
>  
> -	__deactivate_traps_common();
>  	__deactivate_traps_arch()();
>  }
>  
> +void activate_traps_vhe_load(struct kvm_vcpu *vcpu)
> +{
> +	__activate_traps_common(vcpu);
> +}
> +
> +void deactivate_traps_vhe_put(void)
> +{
> +	u64 mdcr_el2 = read_sysreg(mdcr_el2);
> +
> +	mdcr_el2 &= MDCR_EL2_HPMN_MASK |
> +		    MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT |
> +		    MDCR_EL2_TPMS;
> +
> +	write_sysreg(mdcr_el2, mdcr_el2);
> +
> +	__deactivate_traps_common();
> +}
> +
>  static void __hyp_text __activate_vm(struct kvm *kvm)
>  {
>  	write_sysreg(kvm->arch.vttbr, vttbr_el2);
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index aacba4636871..b3894df6bf1a 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -254,6 +254,8 @@ void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu)
>  	__sysreg_restore_el1_state(guest_ctxt);
>  
>  	vcpu->arch.sysregs_loaded_on_cpu = true;
> +
> +	activate_traps_vhe_load(vcpu);
>  }
>  
>  /**
> @@ -275,6 +277,8 @@ void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
>  	if (!has_vhe())
>  		return;
>  
> +	deactivate_traps_vhe_put();
> +
>  	__sysreg_save_el1_state(guest_ctxt);
>  	__sysreg_save_user_state(guest_ctxt);
>  	__sysreg32_save_state(vcpu);
> -- 
> 2.14.2
> 

I must admit that I find these two layers of trap configuration mildly
confusing. I can see why it is done like this (there is hardly any
other way), but still...

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
Andrew Jones Feb. 22, 2018, 3:51 p.m. UTC | #2
On Thu, Feb 15, 2018 at 10:03:25PM +0100, Christoffer Dall wrote:
> We do not have to change the c15 trap setting on each switch to/from the
> guest on VHE systems, because this setting only affects EL0.
> 
> The PMU and debug trap configuration can also be done on vcpu load/put
> instead, because they don't affect how the host kernel can access the
> debug registers while executing KVM kernel code.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm64/include/asm/kvm_hyp.h |  3 +++
>  arch/arm64/kvm/hyp/switch.c      | 31 ++++++++++++++++++++++---------
>  arch/arm64/kvm/hyp/sysreg-sr.c   |  4 ++++
>  3 files changed, 29 insertions(+), 9 deletions(-)
>

The last patch 32/40 probably should have been closer to 13/40 in
this series. That would allow this patch to directly follow it's
preparatory patch 31/40, but I'm just whiny reviewer...

Reviewed-by: Andrew Jones <drjones@redhat.com>
Christoffer Dall Feb. 22, 2018, 6:57 p.m. UTC | #3
On Wed, Feb 21, 2018 at 06:20:54PM +0000, Marc Zyngier wrote:
> On Thu, 15 Feb 2018 21:03:25 +0000,
> Christoffer Dall wrote:
> > 
> > We do not have to change the c15 trap setting on each switch to/from the
> > guest on VHE systems, because this setting only affects EL0.
> 
> Did you mean EL1 instead?
> 

Not sure what I meant, but HSTR_EL2 appears to affect EL1 and EL0, and
the PMU configuration we can do on vcpu_load on VHE systems is only
about EL0 as far as I can tell.

> > 
> > The PMU and debug trap configuration can also be done on vcpu load/put
> > instead, because they don't affect how the host kernel can access the
> > debug registers while executing KVM kernel code.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  arch/arm64/include/asm/kvm_hyp.h |  3 +++
> >  arch/arm64/kvm/hyp/switch.c      | 31 ++++++++++++++++++++++---------
> >  arch/arm64/kvm/hyp/sysreg-sr.c   |  4 ++++
> >  3 files changed, 29 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> > index 2b1fda90dde4..949f2e77ae58 100644
> > --- a/arch/arm64/include/asm/kvm_hyp.h
> > +++ b/arch/arm64/include/asm/kvm_hyp.h
> > @@ -147,6 +147,9 @@ void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
> >  void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
> >  bool __fpsimd_enabled(void);
> >  
> > +void activate_traps_vhe_load(struct kvm_vcpu *vcpu);
> > +void deactivate_traps_vhe_put(void);
> > +
> >  u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
> >  void __noreturn __hyp_do_panic(unsigned long, ...);
> >  
> > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > index 9c40e203bd09..5e94955b89ea 100644
> > --- a/arch/arm64/kvm/hyp/switch.c
> > +++ b/arch/arm64/kvm/hyp/switch.c
> > @@ -101,6 +101,8 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
> >  {
> >  	u64 val;
> >  
> > +	__activate_traps_common(vcpu);
> > +
> >  	val = CPTR_EL2_DEFAULT;
> >  	val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;
> >  	write_sysreg(val, cptr_el2);
> > @@ -120,20 +122,12 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
> >  		write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2);
> >  
> >  	__activate_traps_fpsimd32(vcpu);
> > -	__activate_traps_common(vcpu);
> >  	__activate_traps_arch()(vcpu);
> >  }
> >  
> >  static void __hyp_text __deactivate_traps_vhe(void)
> >  {
> >  	extern char vectors[];	/* kernel exception vectors */
> > -	u64 mdcr_el2 = read_sysreg(mdcr_el2);
> > -
> > -	mdcr_el2 &= MDCR_EL2_HPMN_MASK |
> > -		    MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT |
> > -		    MDCR_EL2_TPMS;
> > -
> > -	write_sysreg(mdcr_el2, mdcr_el2);
> >  	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
> >  	write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1);
> >  	write_sysreg(vectors, vbar_el1);
> > @@ -143,6 +137,8 @@ static void __hyp_text __deactivate_traps_nvhe(void)
> >  {
> >  	u64 mdcr_el2 = read_sysreg(mdcr_el2);
> >  
> > +	__deactivate_traps_common();
> > +
> >  	mdcr_el2 &= MDCR_EL2_HPMN_MASK;
> >  	mdcr_el2 |= MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT;
> >  
> > @@ -166,10 +162,27 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu)
> >  	if (vcpu->arch.hcr_el2 & HCR_VSE)
> >  		vcpu->arch.hcr_el2 = read_sysreg(hcr_el2);
> >  
> > -	__deactivate_traps_common();
> >  	__deactivate_traps_arch()();
> >  }
> >  
> > +void activate_traps_vhe_load(struct kvm_vcpu *vcpu)
> > +{
> > +	__activate_traps_common(vcpu);
> > +}
> > +
> > +void deactivate_traps_vhe_put(void)
> > +{
> > +	u64 mdcr_el2 = read_sysreg(mdcr_el2);
> > +
> > +	mdcr_el2 &= MDCR_EL2_HPMN_MASK |
> > +		    MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT |
> > +		    MDCR_EL2_TPMS;
> > +
> > +	write_sysreg(mdcr_el2, mdcr_el2);
> > +
> > +	__deactivate_traps_common();
> > +}
> > +
> >  static void __hyp_text __activate_vm(struct kvm *kvm)
> >  {
> >  	write_sysreg(kvm->arch.vttbr, vttbr_el2);
> > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> > index aacba4636871..b3894df6bf1a 100644
> > --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> > @@ -254,6 +254,8 @@ void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu)
> >  	__sysreg_restore_el1_state(guest_ctxt);
> >  
> >  	vcpu->arch.sysregs_loaded_on_cpu = true;
> > +
> > +	activate_traps_vhe_load(vcpu);
> >  }
> >  
> >  /**
> > @@ -275,6 +277,8 @@ void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
> >  	if (!has_vhe())
> >  		return;
> >  
> > +	deactivate_traps_vhe_put();
> > +
> >  	__sysreg_save_el1_state(guest_ctxt);
> >  	__sysreg_save_user_state(guest_ctxt);
> >  	__sysreg32_save_state(vcpu);
> > -- 
> > 2.14.2
> > 
> 
> I must admit that I find these two layers of trap configuration mildly
> confusing. I can see why it is done like this (there is hardly any
> other way), but still...

Perhaps the naming could be improved.

Right now we have:

_traps_common:        Same code for non-VHE/VHE.  Called:
                         non-VHE: on every switch.
		         VHE: on load/put.
_traps:               Same code for non-VHE/VHE.  Called:
                         VHE/non-VHE: On every switch.
_traps_nvhe:          Code specific to non-VHE system.  Called:
                         non-VHE: on every switch
_traps_vhe:           Code specific to VHE system.  Called:
                         VHE: on every switch
_traps_vhe_load/put:  Code  specific to VHE system.  Called:
                         VHE: on vcpu load/put

We could simplify this at the cost of code duplication to:

_traps_nvhe
_traps_vhe
_traps_vhe_load/put

Thoughts?

> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 

Thanks,
-Christoffer

Patch
diff mbox

diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index 2b1fda90dde4..949f2e77ae58 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -147,6 +147,9 @@  void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
 void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
 bool __fpsimd_enabled(void);
 
+void activate_traps_vhe_load(struct kvm_vcpu *vcpu);
+void deactivate_traps_vhe_put(void);
+
 u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
 void __noreturn __hyp_do_panic(unsigned long, ...);
 
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 9c40e203bd09..5e94955b89ea 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -101,6 +101,8 @@  static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
 {
 	u64 val;
 
+	__activate_traps_common(vcpu);
+
 	val = CPTR_EL2_DEFAULT;
 	val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;
 	write_sysreg(val, cptr_el2);
@@ -120,20 +122,12 @@  static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
 		write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2);
 
 	__activate_traps_fpsimd32(vcpu);
-	__activate_traps_common(vcpu);
 	__activate_traps_arch()(vcpu);
 }
 
 static void __hyp_text __deactivate_traps_vhe(void)
 {
 	extern char vectors[];	/* kernel exception vectors */
-	u64 mdcr_el2 = read_sysreg(mdcr_el2);
-
-	mdcr_el2 &= MDCR_EL2_HPMN_MASK |
-		    MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT |
-		    MDCR_EL2_TPMS;
-
-	write_sysreg(mdcr_el2, mdcr_el2);
 	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
 	write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1);
 	write_sysreg(vectors, vbar_el1);
@@ -143,6 +137,8 @@  static void __hyp_text __deactivate_traps_nvhe(void)
 {
 	u64 mdcr_el2 = read_sysreg(mdcr_el2);
 
+	__deactivate_traps_common();
+
 	mdcr_el2 &= MDCR_EL2_HPMN_MASK;
 	mdcr_el2 |= MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT;
 
@@ -166,10 +162,27 @@  static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu)
 	if (vcpu->arch.hcr_el2 & HCR_VSE)
 		vcpu->arch.hcr_el2 = read_sysreg(hcr_el2);
 
-	__deactivate_traps_common();
 	__deactivate_traps_arch()();
 }
 
+void activate_traps_vhe_load(struct kvm_vcpu *vcpu)
+{
+	__activate_traps_common(vcpu);
+}
+
+void deactivate_traps_vhe_put(void)
+{
+	u64 mdcr_el2 = read_sysreg(mdcr_el2);
+
+	mdcr_el2 &= MDCR_EL2_HPMN_MASK |
+		    MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT |
+		    MDCR_EL2_TPMS;
+
+	write_sysreg(mdcr_el2, mdcr_el2);
+
+	__deactivate_traps_common();
+}
+
 static void __hyp_text __activate_vm(struct kvm *kvm)
 {
 	write_sysreg(kvm->arch.vttbr, vttbr_el2);
diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index aacba4636871..b3894df6bf1a 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -254,6 +254,8 @@  void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu)
 	__sysreg_restore_el1_state(guest_ctxt);
 
 	vcpu->arch.sysregs_loaded_on_cpu = true;
+
+	activate_traps_vhe_load(vcpu);
 }
 
 /**
@@ -275,6 +277,8 @@  void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
 	if (!has_vhe())
 		return;
 
+	deactivate_traps_vhe_put();
+
 	__sysreg_save_el1_state(guest_ctxt);
 	__sysreg_save_user_state(guest_ctxt);
 	__sysreg32_save_state(vcpu);