* [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest @ 2016-12-06 14:56 ` Marc Zyngier 0 siblings, 0 replies; 12+ messages in thread From: Marc Zyngier @ 2016-12-06 14:56 UTC (permalink / raw) To: Christoffer Dall Cc: Itaru Kitayama, Catalin Marinas, Hoan Tran, Will Deacon, kvmarm, linux-arm-kernel, Tai Tri Nguyen The ARMv8 architecture allows the cycle counter to be configured by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0, hence accessing PMCCFILTR_EL0. But it disallows the use of PMSELR_EL0.SEL==0x1f to access the cycle counter itself through PMXEVCNTR_EL0. Linux itself doesn't violate this rule, but we may end up with PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1, despite the guest not having done anything wrong. In order to avoid this unfortunate course of events (haha!), let's sanitize PMSELR_EL0 on guest entry. This ensures that the guest won't explode unexpectedly. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- This is another approach to fix this issue, this time nuking PMSELR_EL0 on guest entry instead of relying on perf not to clobber the register. Tested on v4.9-rc8 with a Rev A3 X-Gene. arch/arm64/kvm/hyp/switch.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 83037cd..3b7cfbd 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) write_sysreg(val, hcr_el2); /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ write_sysreg(1 << 15, hstr_el2); - /* Make sure we trap PMU access from EL0 to EL2 */ + /* + * Make sure we trap PMU access from EL0 to EL2. Also sanitize + * PMSELR_EL0 to make sure it never contains the cycle + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. + */ + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) + write_sysreg(0, pmselr_el0); write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); __activate_traps_arch()(); -- 2.1.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest @ 2016-12-06 14:56 ` Marc Zyngier 0 siblings, 0 replies; 12+ messages in thread From: Marc Zyngier @ 2016-12-06 14:56 UTC (permalink / raw) To: linux-arm-kernel The ARMv8 architecture allows the cycle counter to be configured by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0, hence accessing PMCCFILTR_EL0. But it disallows the use of PMSELR_EL0.SEL==0x1f to access the cycle counter itself through PMXEVCNTR_EL0. Linux itself doesn't violate this rule, but we may end up with PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1, despite the guest not having done anything wrong. In order to avoid this unfortunate course of events (haha!), let's sanitize PMSELR_EL0 on guest entry. This ensures that the guest won't explode unexpectedly. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- This is another approach to fix this issue, this time nuking PMSELR_EL0 on guest entry instead of relying on perf not to clobber the register. Tested on v4.9-rc8 with a Rev A3 X-Gene. arch/arm64/kvm/hyp/switch.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 83037cd..3b7cfbd 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) write_sysreg(val, hcr_el2); /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ write_sysreg(1 << 15, hstr_el2); - /* Make sure we trap PMU access from EL0 to EL2 */ + /* + * Make sure we trap PMU access from EL0 to EL2. Also sanitize + * PMSELR_EL0 to make sure it never contains the cycle + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. + */ + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) + write_sysreg(0, pmselr_el0); write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); __activate_traps_arch()(); -- 2.1.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest 2016-12-06 14:56 ` Marc Zyngier @ 2016-12-06 15:27 ` Will Deacon -1 siblings, 0 replies; 12+ messages in thread From: Will Deacon @ 2016-12-06 15:27 UTC (permalink / raw) To: Marc Zyngier Cc: Itaru Kitayama, Catalin Marinas, Hoan Tran, linux-arm-kernel, kvmarm, Tai Tri Nguyen On Tue, Dec 06, 2016 at 02:56:50PM +0000, Marc Zyngier wrote: > The ARMv8 architecture allows the cycle counter to be configured > by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0, > hence accessing PMCCFILTR_EL0. But it disallows the use of > PMSELR_EL0.SEL==0x1f to access the cycle counter itself through > PMXEVCNTR_EL0. > > Linux itself doesn't violate this rule, but we may end up with > PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that > guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1, > despite the guest not having done anything wrong. > > In order to avoid this unfortunate course of events (haha!), let's > sanitize PMSELR_EL0 on guest entry. This ensures that the guest > won't explode unexpectedly. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > This is another approach to fix this issue, this time nuking PMSELR_EL0 > on guest entry instead of relying on perf not to clobber the register. > > Tested on v4.9-rc8 with a Rev A3 X-Gene. > > arch/arm64/kvm/hyp/switch.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 83037cd..3b7cfbd 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > write_sysreg(val, hcr_el2); > /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > write_sysreg(1 << 15, hstr_el2); > - /* Make sure we trap PMU access from EL0 to EL2 */ > + /* > + * Make sure we trap PMU access from EL0 to EL2. Also sanitize > + * PMSELR_EL0 to make sure it never contains the cycle > + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. "UNDEF at EL1, as opposed to trapping to EL2" might be clearer, but up to you. > + */ > + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) > + write_sysreg(0, pmselr_el0); > write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); Curious, but why do you check MDCR.HPMN for PMSELR_EL0, but not for PMUSERENR_EL0? Anyway: Acked-by: Will Deacon <will.deacon@arm.com> Thanks, Will ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest @ 2016-12-06 15:27 ` Will Deacon 0 siblings, 0 replies; 12+ messages in thread From: Will Deacon @ 2016-12-06 15:27 UTC (permalink / raw) To: linux-arm-kernel On Tue, Dec 06, 2016 at 02:56:50PM +0000, Marc Zyngier wrote: > The ARMv8 architecture allows the cycle counter to be configured > by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0, > hence accessing PMCCFILTR_EL0. But it disallows the use of > PMSELR_EL0.SEL==0x1f to access the cycle counter itself through > PMXEVCNTR_EL0. > > Linux itself doesn't violate this rule, but we may end up with > PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that > guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1, > despite the guest not having done anything wrong. > > In order to avoid this unfortunate course of events (haha!), let's > sanitize PMSELR_EL0 on guest entry. This ensures that the guest > won't explode unexpectedly. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > This is another approach to fix this issue, this time nuking PMSELR_EL0 > on guest entry instead of relying on perf not to clobber the register. > > Tested on v4.9-rc8 with a Rev A3 X-Gene. > > arch/arm64/kvm/hyp/switch.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 83037cd..3b7cfbd 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > write_sysreg(val, hcr_el2); > /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > write_sysreg(1 << 15, hstr_el2); > - /* Make sure we trap PMU access from EL0 to EL2 */ > + /* > + * Make sure we trap PMU access from EL0 to EL2. Also sanitize > + * PMSELR_EL0 to make sure it never contains the cycle > + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. "UNDEF at EL1, as opposed to trapping to EL2" might be clearer, but up to you. > + */ > + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) > + write_sysreg(0, pmselr_el0); > write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); Curious, but why do you check MDCR.HPMN for PMSELR_EL0, but not for PMUSERENR_EL0? Anyway: Acked-by: Will Deacon <will.deacon@arm.com> Thanks, Will ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest 2016-12-06 15:27 ` Will Deacon @ 2016-12-06 15:42 ` Marc Zyngier -1 siblings, 0 replies; 12+ messages in thread From: Marc Zyngier @ 2016-12-06 15:42 UTC (permalink / raw) To: Will Deacon Cc: Itaru Kitayama, Catalin Marinas, Hoan Tran, linux-arm-kernel, kvmarm, Tai Tri Nguyen On 06/12/16 15:27, Will Deacon wrote: > On Tue, Dec 06, 2016 at 02:56:50PM +0000, Marc Zyngier wrote: >> The ARMv8 architecture allows the cycle counter to be configured >> by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0, >> hence accessing PMCCFILTR_EL0. But it disallows the use of >> PMSELR_EL0.SEL==0x1f to access the cycle counter itself through >> PMXEVCNTR_EL0. >> >> Linux itself doesn't violate this rule, but we may end up with >> PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that >> guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1, >> despite the guest not having done anything wrong. >> >> In order to avoid this unfortunate course of events (haha!), let's >> sanitize PMSELR_EL0 on guest entry. This ensures that the guest >> won't explode unexpectedly. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> This is another approach to fix this issue, this time nuking PMSELR_EL0 >> on guest entry instead of relying on perf not to clobber the register. >> >> Tested on v4.9-rc8 with a Rev A3 X-Gene. >> >> arch/arm64/kvm/hyp/switch.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >> index 83037cd..3b7cfbd 100644 >> --- a/arch/arm64/kvm/hyp/switch.c >> +++ b/arch/arm64/kvm/hyp/switch.c >> @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) >> write_sysreg(val, hcr_el2); >> /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ >> write_sysreg(1 << 15, hstr_el2); >> - /* Make sure we trap PMU access from EL0 to EL2 */ >> + /* >> + * Make sure we trap PMU access from EL0 to EL2. Also sanitize >> + * PMSELR_EL0 to make sure it never contains the cycle >> + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. > > "UNDEF at EL1, as opposed to trapping to EL2" might be clearer, but up to > you. Sure, that's a useful clarification. >> + */ >> + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) >> + write_sysreg(0, pmselr_el0); >> write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > > Curious, but why do you check MDCR.HPMN for PMSELR_EL0, but not for > PMUSERENR_EL0? Why would PMUSERENR_EL0 be constrained by the number of counters available to the guest? > > Anyway: > > Acked-by: Will Deacon <will.deacon@arm.com> Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest @ 2016-12-06 15:42 ` Marc Zyngier 0 siblings, 0 replies; 12+ messages in thread From: Marc Zyngier @ 2016-12-06 15:42 UTC (permalink / raw) To: linux-arm-kernel On 06/12/16 15:27, Will Deacon wrote: > On Tue, Dec 06, 2016 at 02:56:50PM +0000, Marc Zyngier wrote: >> The ARMv8 architecture allows the cycle counter to be configured >> by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0, >> hence accessing PMCCFILTR_EL0. But it disallows the use of >> PMSELR_EL0.SEL==0x1f to access the cycle counter itself through >> PMXEVCNTR_EL0. >> >> Linux itself doesn't violate this rule, but we may end up with >> PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that >> guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1, >> despite the guest not having done anything wrong. >> >> In order to avoid this unfortunate course of events (haha!), let's >> sanitize PMSELR_EL0 on guest entry. This ensures that the guest >> won't explode unexpectedly. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> This is another approach to fix this issue, this time nuking PMSELR_EL0 >> on guest entry instead of relying on perf not to clobber the register. >> >> Tested on v4.9-rc8 with a Rev A3 X-Gene. >> >> arch/arm64/kvm/hyp/switch.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >> index 83037cd..3b7cfbd 100644 >> --- a/arch/arm64/kvm/hyp/switch.c >> +++ b/arch/arm64/kvm/hyp/switch.c >> @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) >> write_sysreg(val, hcr_el2); >> /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ >> write_sysreg(1 << 15, hstr_el2); >> - /* Make sure we trap PMU access from EL0 to EL2 */ >> + /* >> + * Make sure we trap PMU access from EL0 to EL2. Also sanitize >> + * PMSELR_EL0 to make sure it never contains the cycle >> + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. > > "UNDEF at EL1, as opposed to trapping to EL2" might be clearer, but up to > you. Sure, that's a useful clarification. >> + */ >> + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) >> + write_sysreg(0, pmselr_el0); >> write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > > Curious, but why do you check MDCR.HPMN for PMSELR_EL0, but not for > PMUSERENR_EL0? Why would PMUSERENR_EL0 be constrained by the number of counters available to the guest? > > Anyway: > > Acked-by: Will Deacon <will.deacon@arm.com> Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest 2016-12-06 14:56 ` Marc Zyngier @ 2016-12-06 16:29 ` Christoffer Dall -1 siblings, 0 replies; 12+ messages in thread From: Christoffer Dall @ 2016-12-06 16:29 UTC (permalink / raw) To: Marc Zyngier Cc: Itaru Kitayama, Catalin Marinas, Hoan Tran, Will Deacon, kvmarm, linux-arm-kernel, Tai Tri Nguyen On Tue, Dec 06, 2016 at 02:56:50PM +0000, Marc Zyngier wrote: > The ARMv8 architecture allows the cycle counter to be configured > by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0, > hence accessing PMCCFILTR_EL0. But it disallows the use of > PMSELR_EL0.SEL==0x1f to access the cycle counter itself through > PMXEVCNTR_EL0. > > Linux itself doesn't violate this rule, but we may end up with > PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that > guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1, > despite the guest not having done anything wrong. > > In order to avoid this unfortunate course of events (haha!), let's I actually did find this funny. > sanitize PMSELR_EL0 on guest entry. This ensures that the guest > won't explode unexpectedly. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > This is another approach to fix this issue, this time nuking PMSELR_EL0 > on guest entry instead of relying on perf not to clobber the register. > > Tested on v4.9-rc8 with a Rev A3 X-Gene. > > arch/arm64/kvm/hyp/switch.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 83037cd..3b7cfbd 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > write_sysreg(val, hcr_el2); > /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > write_sysreg(1 << 15, hstr_el2); > - /* Make sure we trap PMU access from EL0 to EL2 */ > + /* > + * Make sure we trap PMU access from EL0 to EL2. Also sanitize > + * PMSELR_EL0 to make sure it never contains the cycle > + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. > + */ > + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) > + write_sysreg(0, pmselr_el0); I'm a bit confused about how we use the HPMN field. This value is always set to the full number of counters available on the system and never modified by the guest, right? So this is in essence a check that says 'do you have any performance counters, then make sure accesses don't undef to el1 instead of trapping to el2', but then my question is, why not just set pmselr_el0 to zero unconditionally, because in the case where (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) == 0, it means we have no counters, which we'll have exposed to the guest anyhow, and we should undef at el1 in the guest, or did I get this completely wrong (like everything else today)? > write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > __activate_traps_arch()(); > -- > 2.1.4 > Thanks, -Christoffer ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest @ 2016-12-06 16:29 ` Christoffer Dall 0 siblings, 0 replies; 12+ messages in thread From: Christoffer Dall @ 2016-12-06 16:29 UTC (permalink / raw) To: linux-arm-kernel On Tue, Dec 06, 2016 at 02:56:50PM +0000, Marc Zyngier wrote: > The ARMv8 architecture allows the cycle counter to be configured > by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0, > hence accessing PMCCFILTR_EL0. But it disallows the use of > PMSELR_EL0.SEL==0x1f to access the cycle counter itself through > PMXEVCNTR_EL0. > > Linux itself doesn't violate this rule, but we may end up with > PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that > guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1, > despite the guest not having done anything wrong. > > In order to avoid this unfortunate course of events (haha!), let's I actually did find this funny. > sanitize PMSELR_EL0 on guest entry. This ensures that the guest > won't explode unexpectedly. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > This is another approach to fix this issue, this time nuking PMSELR_EL0 > on guest entry instead of relying on perf not to clobber the register. > > Tested on v4.9-rc8 with a Rev A3 X-Gene. > > arch/arm64/kvm/hyp/switch.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 83037cd..3b7cfbd 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > write_sysreg(val, hcr_el2); > /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > write_sysreg(1 << 15, hstr_el2); > - /* Make sure we trap PMU access from EL0 to EL2 */ > + /* > + * Make sure we trap PMU access from EL0 to EL2. Also sanitize > + * PMSELR_EL0 to make sure it never contains the cycle > + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. > + */ > + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) > + write_sysreg(0, pmselr_el0); I'm a bit confused about how we use the HPMN field. This value is always set to the full number of counters available on the system and never modified by the guest, right? So this is in essence a check that says 'do you have any performance counters, then make sure accesses don't undef to el1 instead of trapping to el2', but then my question is, why not just set pmselr_el0 to zero unconditionally, because in the case where (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) == 0, it means we have no counters, which we'll have exposed to the guest anyhow, and we should undef at el1 in the guest, or did I get this completely wrong (like everything else today)? > write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > __activate_traps_arch()(); > -- > 2.1.4 > Thanks, -Christoffer ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest 2016-12-06 16:29 ` Christoffer Dall @ 2016-12-06 16:37 ` Will Deacon -1 siblings, 0 replies; 12+ messages in thread From: Will Deacon @ 2016-12-06 16:37 UTC (permalink / raw) To: Christoffer Dall Cc: Marc Zyngier, Catalin Marinas, Hoan Tran, Itaru Kitayama, kvmarm, linux-arm-kernel, Tai Tri Nguyen On Tue, Dec 06, 2016 at 05:29:18PM +0100, Christoffer Dall wrote: > On Tue, Dec 06, 2016 at 02:56:50PM +0000, Marc Zyngier wrote: > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > > index 83037cd..3b7cfbd 100644 > > --- a/arch/arm64/kvm/hyp/switch.c > > +++ b/arch/arm64/kvm/hyp/switch.c > > @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > > write_sysreg(val, hcr_el2); > > /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > > write_sysreg(1 << 15, hstr_el2); > > - /* Make sure we trap PMU access from EL0 to EL2 */ > > + /* > > + * Make sure we trap PMU access from EL0 to EL2. Also sanitize > > + * PMSELR_EL0 to make sure it never contains the cycle > > + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. > > + */ > > + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) > > + write_sysreg(0, pmselr_el0); > > I'm a bit confused about how we use the HPMN field. This value is > always set to the full number of counters available on the system and > never modified by the guest, right? So this is in essence a check that > says 'do you have any performance counters, then make sure accesses > don't undef to el1 instead of trapping to el2', but then my question is, > why not just set pmselr_el0 to zero unconditionally, because in the case > where (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) == 0, it means we have > no counters, which we'll have exposed to the guest anyhow, and we should > undef at el1 in the guest, or did I get this completely wrong (like > everything else today)? I think Marc and I came to the same conclusion a few minutes ago. The check you might want is "Have I instantiated a virtual PMU for this device?", but that's probably a micro-optimisation. Will ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest @ 2016-12-06 16:37 ` Will Deacon 0 siblings, 0 replies; 12+ messages in thread From: Will Deacon @ 2016-12-06 16:37 UTC (permalink / raw) To: linux-arm-kernel On Tue, Dec 06, 2016 at 05:29:18PM +0100, Christoffer Dall wrote: > On Tue, Dec 06, 2016 at 02:56:50PM +0000, Marc Zyngier wrote: > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > > index 83037cd..3b7cfbd 100644 > > --- a/arch/arm64/kvm/hyp/switch.c > > +++ b/arch/arm64/kvm/hyp/switch.c > > @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > > write_sysreg(val, hcr_el2); > > /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > > write_sysreg(1 << 15, hstr_el2); > > - /* Make sure we trap PMU access from EL0 to EL2 */ > > + /* > > + * Make sure we trap PMU access from EL0 to EL2. Also sanitize > > + * PMSELR_EL0 to make sure it never contains the cycle > > + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. > > + */ > > + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) > > + write_sysreg(0, pmselr_el0); > > I'm a bit confused about how we use the HPMN field. This value is > always set to the full number of counters available on the system and > never modified by the guest, right? So this is in essence a check that > says 'do you have any performance counters, then make sure accesses > don't undef to el1 instead of trapping to el2', but then my question is, > why not just set pmselr_el0 to zero unconditionally, because in the case > where (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) == 0, it means we have > no counters, which we'll have exposed to the guest anyhow, and we should > undef at el1 in the guest, or did I get this completely wrong (like > everything else today)? I think Marc and I came to the same conclusion a few minutes ago. The check you might want is "Have I instantiated a virtual PMU for this device?", but that's probably a micro-optimisation. Will ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest 2016-12-06 16:29 ` Christoffer Dall @ 2016-12-06 17:17 ` Marc Zyngier -1 siblings, 0 replies; 12+ messages in thread From: Marc Zyngier @ 2016-12-06 17:17 UTC (permalink / raw) To: Christoffer Dall Cc: Itaru Kitayama, Catalin Marinas, Hoan Tran, Will Deacon, kvmarm, linux-arm-kernel, Tai Tri Nguyen On 06/12/16 16:29, Christoffer Dall wrote: > On Tue, Dec 06, 2016 at 02:56:50PM +0000, Marc Zyngier wrote: >> The ARMv8 architecture allows the cycle counter to be configured >> by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0, >> hence accessing PMCCFILTR_EL0. But it disallows the use of >> PMSELR_EL0.SEL==0x1f to access the cycle counter itself through >> PMXEVCNTR_EL0. >> >> Linux itself doesn't violate this rule, but we may end up with >> PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that >> guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1, >> despite the guest not having done anything wrong. >> >> In order to avoid this unfortunate course of events (haha!), let's > > I actually did find this funny. > >> sanitize PMSELR_EL0 on guest entry. This ensures that the guest >> won't explode unexpectedly. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> This is another approach to fix this issue, this time nuking PMSELR_EL0 >> on guest entry instead of relying on perf not to clobber the register. >> >> Tested on v4.9-rc8 with a Rev A3 X-Gene. >> >> arch/arm64/kvm/hyp/switch.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >> index 83037cd..3b7cfbd 100644 >> --- a/arch/arm64/kvm/hyp/switch.c >> +++ b/arch/arm64/kvm/hyp/switch.c >> @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) >> write_sysreg(val, hcr_el2); >> /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ >> write_sysreg(1 << 15, hstr_el2); >> - /* Make sure we trap PMU access from EL0 to EL2 */ >> + /* >> + * Make sure we trap PMU access from EL0 to EL2. Also sanitize >> + * PMSELR_EL0 to make sure it never contains the cycle >> + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. >> + */ >> + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) >> + write_sysreg(0, pmselr_el0); > > I'm a bit confused about how we use the HPMN field. This value is > always set to the full number of counters available on the system and > never modified by the guest, right? So this is in essence a check that > says 'do you have any performance counters, then make sure accesses > don't undef to el1 instead of trapping to el2', but then my question is, > why not just set pmselr_el0 to zero unconditionally, because in the case > where (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) == 0, it means we have > no counters, which we'll have exposed to the guest anyhow, and we should > undef at el1 in the guest, or did I get this completely wrong (like > everything else today)? Yeah, that's probably the best course of action. If the guest does something silly, tough. I'll drop the test and repost the thing. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest @ 2016-12-06 17:17 ` Marc Zyngier 0 siblings, 0 replies; 12+ messages in thread From: Marc Zyngier @ 2016-12-06 17:17 UTC (permalink / raw) To: linux-arm-kernel On 06/12/16 16:29, Christoffer Dall wrote: > On Tue, Dec 06, 2016 at 02:56:50PM +0000, Marc Zyngier wrote: >> The ARMv8 architecture allows the cycle counter to be configured >> by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0, >> hence accessing PMCCFILTR_EL0. But it disallows the use of >> PMSELR_EL0.SEL==0x1f to access the cycle counter itself through >> PMXEVCNTR_EL0. >> >> Linux itself doesn't violate this rule, but we may end up with >> PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that >> guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1, >> despite the guest not having done anything wrong. >> >> In order to avoid this unfortunate course of events (haha!), let's > > I actually did find this funny. > >> sanitize PMSELR_EL0 on guest entry. This ensures that the guest >> won't explode unexpectedly. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> This is another approach to fix this issue, this time nuking PMSELR_EL0 >> on guest entry instead of relying on perf not to clobber the register. >> >> Tested on v4.9-rc8 with a Rev A3 X-Gene. >> >> arch/arm64/kvm/hyp/switch.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >> index 83037cd..3b7cfbd 100644 >> --- a/arch/arm64/kvm/hyp/switch.c >> +++ b/arch/arm64/kvm/hyp/switch.c >> @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) >> write_sysreg(val, hcr_el2); >> /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ >> write_sysreg(1 << 15, hstr_el2); >> - /* Make sure we trap PMU access from EL0 to EL2 */ >> + /* >> + * Make sure we trap PMU access from EL0 to EL2. Also sanitize >> + * PMSELR_EL0 to make sure it never contains the cycle >> + * counter, which could make a PMXEVCNTR_EL0 access UNDEF. >> + */ >> + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) >> + write_sysreg(0, pmselr_el0); > > I'm a bit confused about how we use the HPMN field. This value is > always set to the full number of counters available on the system and > never modified by the guest, right? So this is in essence a check that > says 'do you have any performance counters, then make sure accesses > don't undef to el1 instead of trapping to el2', but then my question is, > why not just set pmselr_el0 to zero unconditionally, because in the case > where (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) == 0, it means we have > no counters, which we'll have exposed to the guest anyhow, and we should > undef at el1 in the guest, or did I get this completely wrong (like > everything else today)? Yeah, that's probably the best course of action. If the guest does something silly, tough. I'll drop the test and repost the thing. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-12-06 17:17 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-06 14:56 [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest Marc Zyngier 2016-12-06 14:56 ` Marc Zyngier 2016-12-06 15:27 ` Will Deacon 2016-12-06 15:27 ` Will Deacon 2016-12-06 15:42 ` Marc Zyngier 2016-12-06 15:42 ` Marc Zyngier 2016-12-06 16:29 ` Christoffer Dall 2016-12-06 16:29 ` Christoffer Dall 2016-12-06 16:37 ` Will Deacon 2016-12-06 16:37 ` Will Deacon 2016-12-06 17:17 ` Marc Zyngier 2016-12-06 17:17 ` Marc Zyngier
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.