[v2,10/18] arm64: KVM/debug: use EL1&0 stage 1 translation regime

Message ID 20191220143025.33853-11-andrew.murray@arm.com
State New
Headers show
Series
  • [v2,01/18] dt-bindings: ARM SPE: highlight the need for PPI partitions on heterogeneous systems
Related show

Commit Message

Andrew Murray Dec. 20, 2019, 2:30 p.m.
From: Sudeep Holla <sudeep.holla@arm.com>


Now that we have all the save/restore mechanism in place, lets enable
the translation regime used by buffer from EL2 stage 1 to EL1 stage 1
on VHE systems.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

[ Reword commit, don't trap to EL2 ]
Signed-off-by: Andrew Murray <andrew.murray@arm.com>

---
 arch/arm64/kvm/hyp/switch.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.21.0

Comments

Marc Zyngier Dec. 22, 2019, 10:34 a.m. | #1
On Fri, 20 Dec 2019 14:30:17 +0000,
Andrew Murray <andrew.murray@arm.com> wrote:
> 

> From: Sudeep Holla <sudeep.holla@arm.com>

> 

> Now that we have all the save/restore mechanism in place, lets enable

> the translation regime used by buffer from EL2 stage 1 to EL1 stage 1

> on VHE systems.

> 

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> [ Reword commit, don't trap to EL2 ]


Not trapping to EL2 for the case where we don't allow SPE in the
guest is not acceptable.

> Signed-off-by: Andrew Murray <andrew.murray@arm.com>

> ---

>  arch/arm64/kvm/hyp/switch.c | 2 ++

>  1 file changed, 2 insertions(+)

> 

> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c

> index 67b7c160f65b..6c153b79829b 100644

> --- a/arch/arm64/kvm/hyp/switch.c

> +++ b/arch/arm64/kvm/hyp/switch.c

> @@ -100,6 +100,7 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)

>  

>  	write_sysreg(val, cpacr_el1);

>  

> +	write_sysreg(vcpu->arch.mdcr_el2 | 3 << MDCR_EL2_E2PB_SHIFT, mdcr_el2);

>  	write_sysreg(kvm_get_hyp_vector(), vbar_el1);

>  }

>  NOKPROBE_SYMBOL(activate_traps_vhe);

> @@ -117,6 +118,7 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)

>  		__activate_traps_fpsimd32(vcpu);

>  	}

>  

> +	write_sysreg(vcpu->arch.mdcr_el2 | 3 << MDCR_EL2_E2PB_SHIFT, mdcr_el2);


There is a _MASK macro that can replace this '3', and is in keeping
with the rest of the code.

It still remains that it looks like the wrong place to do this, and
vcpu_load seems much better. Why should you write to mdcr_el2 on each
entry to the guest, since you know whether it has SPE enabled at the
point where it gets scheduled?

	M.

-- 
Jazz is not dead, it just smells funny.
Andrew Murray Dec. 24, 2019, 11:11 a.m. | #2
On Sun, Dec 22, 2019 at 10:34:55AM +0000, Marc Zyngier wrote:
> On Fri, 20 Dec 2019 14:30:17 +0000,

> Andrew Murray <andrew.murray@arm.com> wrote:

> > 

> > From: Sudeep Holla <sudeep.holla@arm.com>

> > 

> > Now that we have all the save/restore mechanism in place, lets enable

> > the translation regime used by buffer from EL2 stage 1 to EL1 stage 1

> > on VHE systems.

> > 

> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> > [ Reword commit, don't trap to EL2 ]

> 

> Not trapping to EL2 for the case where we don't allow SPE in the

> guest is not acceptable.


Yes understood (because of this I had meant to send the series as RFC btw).


> 

> > Signed-off-by: Andrew Murray <andrew.murray@arm.com>

> > ---

> >  arch/arm64/kvm/hyp/switch.c | 2 ++

> >  1 file changed, 2 insertions(+)

> > 

> > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c

> > index 67b7c160f65b..6c153b79829b 100644

> > --- a/arch/arm64/kvm/hyp/switch.c

> > +++ b/arch/arm64/kvm/hyp/switch.c

> > @@ -100,6 +100,7 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)

> >  

> >  	write_sysreg(val, cpacr_el1);

> >  

> > +	write_sysreg(vcpu->arch.mdcr_el2 | 3 << MDCR_EL2_E2PB_SHIFT, mdcr_el2);

> >  	write_sysreg(kvm_get_hyp_vector(), vbar_el1);

> >  }

> >  NOKPROBE_SYMBOL(activate_traps_vhe);

> > @@ -117,6 +118,7 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)

> >  		__activate_traps_fpsimd32(vcpu);

> >  	}

> >  

> > +	write_sysreg(vcpu->arch.mdcr_el2 | 3 << MDCR_EL2_E2PB_SHIFT, mdcr_el2);

> 

> There is a _MASK macro that can replace this '3', and is in keeping

> with the rest of the code.


OK.


> 

> It still remains that it looks like the wrong place to do this, and

> vcpu_load seems much better. Why should you write to mdcr_el2 on each

> entry to the guest, since you know whether it has SPE enabled at the

> point where it gets scheduled?


Yes OK, I'll move what I can to vcpu_load.

Thanks,

Andrew Murray


> 

> 	M.

> 

> -- 

> Jazz is not dead, it just smells funny.

Patch

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 67b7c160f65b..6c153b79829b 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -100,6 +100,7 @@  static void activate_traps_vhe(struct kvm_vcpu *vcpu)
 
 	write_sysreg(val, cpacr_el1);
 
+	write_sysreg(vcpu->arch.mdcr_el2 | 3 << MDCR_EL2_E2PB_SHIFT, mdcr_el2);
 	write_sysreg(kvm_get_hyp_vector(), vbar_el1);
 }
 NOKPROBE_SYMBOL(activate_traps_vhe);
@@ -117,6 +118,7 @@  static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
 		__activate_traps_fpsimd32(vcpu);
 	}
 
+	write_sysreg(vcpu->arch.mdcr_el2 | 3 << MDCR_EL2_E2PB_SHIFT, mdcr_el2);
 	write_sysreg(val, cptr_el2);
 
 	if (cpus_have_const_cap(ARM64_WORKAROUND_1319367)) {