[v2,02/18] arm64: KVM: reset E2PB correctly in MDCR_EL2 when exiting the guest(VHE)

Message ID 20191220143025.33853-3-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>


On VHE systems, the reset value for MDCR_EL2.E2PB=b00 which defaults
to profiling buffer using the EL2 stage 1 translations. However if the
guest are allowed to use profiling buffers changing E2PB settings, we
need to ensure we resume back MDCR_EL2.E2PB=b00. Currently we just
do bitwise '&' with MDCR_EL2_E2PB_MASK which will retain the value.

So fix it by clearing all the bits in E2PB.

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

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

---
 arch/arm64/kvm/hyp/switch.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

-- 
2.21.0

Comments

Marc Zyngier Dec. 21, 2019, 1:12 p.m. | #1
On Fri, 20 Dec 2019 14:30:09 +0000
Andrew Murray <andrew.murray@arm.com> wrote:

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

> 

> On VHE systems, the reset value for MDCR_EL2.E2PB=b00 which defaults

> to profiling buffer using the EL2 stage 1 translations. 


Does the reset value actually matter here? I don't see it being
specific to VHE systems, and all we're trying to achieve is to restore
the SPE configuration to a state where it can be used by the host.

> However if the

> guest are allowed to use profiling buffers changing E2PB settings, we


How can the guest be allowed to change E2PB settings? Or do you mean
here that allowing the guest to use SPE will mandate changes of the
E2PB settings, and that we'd better restore the hypervisor state once
we exit?

> need to ensure we resume back MDCR_EL2.E2PB=b00. Currently we just

> do bitwise '&' with MDCR_EL2_E2PB_MASK which will retain the value.

> 

> So fix it by clearing all the bits in E2PB.

> 

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

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

> ---

>  arch/arm64/kvm/hyp/switch.c | 4 +---

>  1 file changed, 1 insertion(+), 3 deletions(-)

> 

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

> index 72fbbd86eb5e..250f13910882 100644

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

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

> @@ -228,9 +228,7 @@ 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;

> +	mdcr_el2 &= MDCR_EL2_HPMN_MASK | MDCR_EL2_TPMS;

>  

>  	write_sysreg(mdcr_el2, mdcr_el2);

>  


I'm OK with this change, but I believe the commit message could use
some tidying up.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
Andrew Murray Dec. 24, 2019, 10:29 a.m. | #2
On Sat, Dec 21, 2019 at 01:12:14PM +0000, Marc Zyngier wrote:
> On Fri, 20 Dec 2019 14:30:09 +0000

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

> 

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

> > 

> > On VHE systems, the reset value for MDCR_EL2.E2PB=b00 which defaults

> > to profiling buffer using the EL2 stage 1 translations. 

> 

> Does the reset value actually matter here? I don't see it being

> specific to VHE systems, and all we're trying to achieve is to restore

> the SPE configuration to a state where it can be used by the host.

> 

> > However if the

> > guest are allowed to use profiling buffers changing E2PB settings, we

> 

> How can the guest be allowed to change E2PB settings? Or do you mean

> here that allowing the guest to use SPE will mandate changes of the

> E2PB settings, and that we'd better restore the hypervisor state once

> we exit?

> 

> > need to ensure we resume back MDCR_EL2.E2PB=b00. Currently we just

> > do bitwise '&' with MDCR_EL2_E2PB_MASK which will retain the value.

> > 

> > So fix it by clearing all the bits in E2PB.

> > 

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

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

> > ---

> >  arch/arm64/kvm/hyp/switch.c | 4 +---

> >  1 file changed, 1 insertion(+), 3 deletions(-)

> > 

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

> > index 72fbbd86eb5e..250f13910882 100644

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

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

> > @@ -228,9 +228,7 @@ 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;

> > +	mdcr_el2 &= MDCR_EL2_HPMN_MASK | MDCR_EL2_TPMS;

> >  

> >  	write_sysreg(mdcr_el2, mdcr_el2);

> >  

> 

> I'm OK with this change, but I believe the commit message could use

> some tidying up.


No problem, I'll update the commit message.

Thanks,

Andrew Murray

> 

> Thanks,

> 

> 	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 72fbbd86eb5e..250f13910882 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -228,9 +228,7 @@  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;
+	mdcr_el2 &= MDCR_EL2_HPMN_MASK | MDCR_EL2_TPMS;
 
 	write_sysreg(mdcr_el2, mdcr_el2);