| Message ID | 20231011081649.3226792-3-oliver.upton@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vPMU fixes for NV/EL2 | expand |
On 11/10/2023 09:16, Oliver Upton wrote: > Prevent the guest from setting the NSH bit, which enables event counting > while the PE is in EL2. kvm_pmu_create_perf_event() never wired up the > bit, nor does it make any sense in the context of a guest without NV. > > While at it, build the event type mask using explicit field definitions > instead of relying on ARMV8_PMU_EVTYPE_MASK. KVM probably should've been > doing this in the first place, as it avoids changes to the > aforementioned mask affecting sysreg emulation. > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > --- > arch/arm64/kvm/pmu-emul.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 0666212c0c15..087764435390 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -663,8 +663,7 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data, > if (!kvm_vcpu_has_pmu(vcpu)) > return; > > - mask = ARMV8_PMU_EVTYPE_MASK; > - mask &= ~ARMV8_PMU_EVTYPE_EVENT; > + mask = ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0; > mask |= kvm_pmu_event_mask(vcpu->kvm); > The change looks good to me and complies with what we do. However, I think we are missing the support for a guest using the combination of PMEVTYPER.NS{K/U} instead of the PMEVTYPER.{P/U} for filtering the events. As per Arm ARM, it is permitted to use the PMEVTYPER.NSK/U (leaving PMEVTYPER.{P,U} == 0) for filtering in Non-Secure EL1. It is true that, Linux guests uses P/U, but another OS/entity could use NSK/NSU. Anyways, for this patch: Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com? Suzuki > reg = counter_index_to_evtreg(pmc->idx);
On Wed, Oct 11, 2023 at 01:33:16PM +0100, Suzuki K Poulose wrote: [...] > However, I think we are missing the support for a guest using the > combination of PMEVTYPER.NS{K/U} instead of the PMEVTYPER.{P/U} for > filtering the events. As per Arm ARM, it is permitted to use the > PMEVTYPER.NSK/U (leaving PMEVTYPER.{P,U} == 0) for filtering in Non-Secure > EL1. Ah, good eye. The pseudocode is easy enough to rip off, something like the below diff would get things going. There's an extra step of making these bits RES0 if EL3 isn't present in the guest's ID register values, but not a huge deal. > Anyways, for this patch: > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com? Thanks! diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c index 087764435390..b6df9ba39940 100644 --- a/arch/arm64/kvm/pmu-emul.c +++ b/arch/arm64/kvm/pmu-emul.c @@ -585,6 +585,7 @@ static void kvm_pmu_create_perf_event(struct kvm_pmc *pmc) struct perf_event *event; struct perf_event_attr attr; u64 eventsel, reg, data; + bool p, u, nsk, nsu; reg = counter_index_to_evtreg(pmc->idx); data = __vcpu_sys_reg(vcpu, reg); @@ -611,13 +612,18 @@ static void kvm_pmu_create_perf_event(struct kvm_pmc *pmc) !test_bit(eventsel, vcpu->kvm->arch.pmu_filter)) return; + p = data & ARMV8_PMU_EXCLUDE_EL1; + u = data & ARMV8_PMU_EXCLUDE_EL0; + nsk = data & ARMV8_PMU_EXCLUDE_NS_EL1; + nsu = data & ARMV8_PMU_EXCLUDE_NS_EL0; + memset(&attr, 0, sizeof(struct perf_event_attr)); attr.type = arm_pmu->pmu.type; attr.size = sizeof(attr); attr.pinned = 1; attr.disabled = !kvm_pmu_counter_is_enabled(pmc); - attr.exclude_user = data & ARMV8_PMU_EXCLUDE_EL0 ? 1 : 0; - attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0; + attr.exclude_user = (u != nsu); + attr.exclude_kernel = (p != nsk); attr.exclude_hv = 1; /* Don't count EL2 events */ attr.exclude_host = 1; /* Don't count host events */ attr.config = eventsel; @@ -663,7 +669,8 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data, if (!kvm_vcpu_has_pmu(vcpu)) return; - mask = ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0; + mask = ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0 | + ARMV8_PMU_EXCLUDE_NS_EL1 | ARMV8_PMU_EXCLUDE_NS_EL0; mask |= kvm_pmu_event_mask(vcpu->kvm); reg = counter_index_to_evtreg(pmc->idx); diff --git a/include/linux/perf/arm_pmuv3.h b/include/linux/perf/arm_pmuv3.h index 753f8dbd9d10..872119cc2bac 100644 --- a/include/linux/perf/arm_pmuv3.h +++ b/include/linux/perf/arm_pmuv3.h @@ -235,9 +235,11 @@ /* * Event filters for PMUv3 */ -#define ARMV8_PMU_EXCLUDE_EL1 (1U << 31) -#define ARMV8_PMU_EXCLUDE_EL0 (1U << 30) -#define ARMV8_PMU_INCLUDE_EL2 (1U << 27) +#define ARMV8_PMU_EXCLUDE_EL1 (1U << 31) +#define ARMV8_PMU_EXCLUDE_EL0 (1U << 30) +#define ARMV8_PMU_EXCLUDE_NS_EL1 (1U << 29) +#define ARMV8_PMU_EXCLUDE_NS_EL0 (1U << 28) +#define ARMV8_PMU_INCLUDE_EL2 (1U << 27) /* * PMUSERENR: user enable reg
On 11/10/2023 09:16, Oliver Upton wrote: > Prevent the guest from setting the NSH bit, which enables event counting > while the PE is in EL2. kvm_pmu_create_perf_event() never wired up the > bit, nor does it make any sense in the context of a guest without NV. > > While at it, build the event type mask using explicit field definitions > instead of relying on ARMV8_PMU_EVTYPE_MASK. KVM probably should've been > doing this in the first place, as it avoids changes to the > aforementioned mask affecting sysreg emulation. > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > --- > arch/arm64/kvm/pmu-emul.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 0666212c0c15..087764435390 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -663,8 +663,7 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data, > if (!kvm_vcpu_has_pmu(vcpu)) > return; > > - mask = ARMV8_PMU_EVTYPE_MASK; ARMV8_PMU_EVTYPE_MASK is still used in access_pmu_evtyper() and reset_pmevtyper(), although it's not really an issue if you can't set the bits in the first place. But it probably makes sense to use the same mask everywhere. > - mask &= ~ARMV8_PMU_EVTYPE_EVENT; > + mask = ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0; > mask |= kvm_pmu_event_mask(vcpu->kvm); > > reg = counter_index_to_evtreg(pmc->idx);
On Thu, Oct 12, 2023 at 10:43:30AM +0100, James Clark wrote: > ARMV8_PMU_EVTYPE_MASK is still used in access_pmu_evtyper() and > reset_pmevtyper(), although it's not really an issue if you can't set > the bits in the first place. But it probably makes sense to use the same > mask everywhere. Agreed. Well, the masking done for reads in access_pmu_evtyper() is pointless since we sanitise the value when written. I'll update reset_pmevtyper() though.
On 11/10/2023 17:17, Oliver Upton wrote: > On Wed, Oct 11, 2023 at 01:33:16PM +0100, Suzuki K Poulose wrote: > > [...] > >> However, I think we are missing the support for a guest using the >> combination of PMEVTYPER.NS{K/U} instead of the PMEVTYPER.{P/U} for >> filtering the events. As per Arm ARM, it is permitted to use the >> PMEVTYPER.NSK/U (leaving PMEVTYPER.{P,U} == 0) for filtering in Non-Secure >> EL1. > > Ah, good eye. The pseudocode is easy enough to rip off, something like > the below diff would get things going. There's an extra step of making > these bits RES0 if EL3 isn't present in the guest's ID register values, > but not a huge deal. True, the change below looks good to me. Thanks for addressing this. Suzuki > >> Anyways, for this patch: >> >> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com? > > Thanks! > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 087764435390..b6df9ba39940 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -585,6 +585,7 @@ static void kvm_pmu_create_perf_event(struct kvm_pmc *pmc) > struct perf_event *event; > struct perf_event_attr attr; > u64 eventsel, reg, data; > + bool p, u, nsk, nsu; > > reg = counter_index_to_evtreg(pmc->idx); > data = __vcpu_sys_reg(vcpu, reg); > @@ -611,13 +612,18 @@ static void kvm_pmu_create_perf_event(struct kvm_pmc *pmc) > !test_bit(eventsel, vcpu->kvm->arch.pmu_filter)) > return; > > + p = data & ARMV8_PMU_EXCLUDE_EL1; > + u = data & ARMV8_PMU_EXCLUDE_EL0; > + nsk = data & ARMV8_PMU_EXCLUDE_NS_EL1; > + nsu = data & ARMV8_PMU_EXCLUDE_NS_EL0; > + > memset(&attr, 0, sizeof(struct perf_event_attr)); > attr.type = arm_pmu->pmu.type; > attr.size = sizeof(attr); > attr.pinned = 1; > attr.disabled = !kvm_pmu_counter_is_enabled(pmc); > - attr.exclude_user = data & ARMV8_PMU_EXCLUDE_EL0 ? 1 : 0; > - attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0; > + attr.exclude_user = (u != nsu); > + attr.exclude_kernel = (p != nsk); > attr.exclude_hv = 1; /* Don't count EL2 events */ > attr.exclude_host = 1; /* Don't count host events */ > attr.config = eventsel; > @@ -663,7 +669,8 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data, > if (!kvm_vcpu_has_pmu(vcpu)) > return; > > - mask = ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0; > + mask = ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0 | > + ARMV8_PMU_EXCLUDE_NS_EL1 | ARMV8_PMU_EXCLUDE_NS_EL0; > mask |= kvm_pmu_event_mask(vcpu->kvm); > > reg = counter_index_to_evtreg(pmc->idx); > diff --git a/include/linux/perf/arm_pmuv3.h b/include/linux/perf/arm_pmuv3.h > index 753f8dbd9d10..872119cc2bac 100644 > --- a/include/linux/perf/arm_pmuv3.h > +++ b/include/linux/perf/arm_pmuv3.h > @@ -235,9 +235,11 @@ > /* > * Event filters for PMUv3 > */ > -#define ARMV8_PMU_EXCLUDE_EL1 (1U << 31) > -#define ARMV8_PMU_EXCLUDE_EL0 (1U << 30) > -#define ARMV8_PMU_INCLUDE_EL2 (1U << 27) > +#define ARMV8_PMU_EXCLUDE_EL1 (1U << 31) > +#define ARMV8_PMU_EXCLUDE_EL0 (1U << 30) > +#define ARMV8_PMU_EXCLUDE_NS_EL1 (1U << 29) > +#define ARMV8_PMU_EXCLUDE_NS_EL0 (1U << 28) > +#define ARMV8_PMU_INCLUDE_EL2 (1U << 27) > > /* > * PMUSERENR: user enable reg
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c index 0666212c0c15..087764435390 100644 --- a/arch/arm64/kvm/pmu-emul.c +++ b/arch/arm64/kvm/pmu-emul.c @@ -663,8 +663,7 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data, if (!kvm_vcpu_has_pmu(vcpu)) return; - mask = ARMV8_PMU_EVTYPE_MASK; - mask &= ~ARMV8_PMU_EVTYPE_EVENT; + mask = ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0; mask |= kvm_pmu_event_mask(vcpu->kvm); reg = counter_index_to_evtreg(pmc->idx);
Prevent the guest from setting the NSH bit, which enables event counting while the PE is in EL2. kvm_pmu_create_perf_event() never wired up the bit, nor does it make any sense in the context of a guest without NV. While at it, build the event type mask using explicit field definitions instead of relying on ARMV8_PMU_EVTYPE_MASK. KVM probably should've been doing this in the first place, as it avoids changes to the aforementioned mask affecting sysreg emulation. Signed-off-by: Oliver Upton <oliver.upton@linux.dev> --- arch/arm64/kvm/pmu-emul.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)