[PATCH 0/2] KVM: arm64: vPMU fixes for NV/EL2

[PATCH 0/2] KVM: arm64: vPMU fixes for NV/EL2

[Oliver Upton]

KVM allows userspace to select both the vPMU and NV feature flags, which
is an absolute no-go since we do not handle any of the EL2 controls.
Furthermore, our sysreg emulation allows the guest to set the NSH bit
responsible for counting events at EL2 despite the fact it does
absolutely nothing.

Series to address the both of these issues. While neither of them are
really a big deal at the moment, the second patch is relevant to James
C's PMU event threshold series [*].

[*] https://lore.kernel.org/kvmarm/20231010141551.2262059-1-james.clark@arm.com/

Oliver Upton (2):
  KVM: arm64: Disallow vPMU for NV guests
  KVM: arm64: Treat PMEVTYPER<n>_EL0.NSH as RES0

 arch/arm64/kvm/arm.c      | 5 +++--
 arch/arm64/kvm/pmu-emul.c | 3 +--
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.42.0.609.gbb76f46606-goog

[PATCH 1/2] KVM: arm64: Disallow vPMU for NV guests

[Oliver Upton]

The existing PMU emulation code is inadequate for use with nested
virt. Disable the feature altogether with NV until the hypervisor
controls are handled correctly.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/arm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 1bfdd583b261..356b7eec3c93 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1238,9 +1238,10 @@ static int kvm_vcpu_init_check_features(struct kvm_vcpu *vcpu,
 	    test_bit(KVM_ARM_VCPU_PTRAUTH_GENERIC, &features))
 		return -EINVAL;
 
-	/* Disallow NV+SVE for the time being */
+	/* Disallow PMU and SVE with NV for the time being */
 	if (test_bit(KVM_ARM_VCPU_HAS_EL2, &features) &&
-	    test_bit(KVM_ARM_VCPU_SVE, &features))
+	    (test_bit(KVM_ARM_VCPU_SVE, &features) ||
+	     test_bit(KVM_ARM_VCPU_PMU_V3, &features)))
 		return -EINVAL;
 
 	if (!test_bit(KVM_ARM_VCPU_EL1_32BIT, &features))
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH 1/2] KVM: arm64: Disallow vPMU for NV guests

[Marc Zyngier]

On 2023-10-11 09:16, Oliver Upton wrote:
> The existing PMU emulation code is inadequate for use with nested
> virt. Disable the feature altogether with NV until the hypervisor
> controls are handled correctly.

Could you at least mention *what* is missing? Most of the handling
should identical, and the couple of bits what would need to be
handled (such as MDCR_EL2) are not covered by this disabling.

As it is stands, I'm not there is much to be gained from this.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] KVM: arm64: Disallow vPMU for NV guests

[Oliver Upton]

On Wed, Oct 11, 2023 at 04:54:49PM +0100, Marc Zyngier wrote:
> On 2023-10-11 09:16, Oliver Upton wrote:
> > The existing PMU emulation code is inadequate for use with nested
> > virt. Disable the feature altogether with NV until the hypervisor
> > controls are handled correctly.
> 
> Could you at least mention *what* is missing? Most of the handling
> should identical, and the couple of bits what would need to be
> handled (such as MDCR_EL2) are not covered by this disabling.

Heh, I could've spelled it out a bit more :)

The part that caught my attention is that we don't honor the NSH bit
(hence the next patch), and doing that correctly isn't going to be
trivial. In cases where event filtering is mismatched between vEL2
and EL1 I think we need to reprogram the associated perf events on
nested transitions. We could probably optimize this by using two sets of
perf events to make the switch a bit faster, but that's beside the
point.

Looks like MDCR_EL2.{HPMN,HPME} aren't handled yet either. These are all
easy enough to work on (and my interest is certainly piqued), but it
seems to me PMU+NV isn't going to work out of the gate. It'd be nice to
permit the combination only when we're confident the feature is
complete.

I haven't any strong opinions here though, and you're the one carrying
the whole NV pile in the first place. Up to you what to do here.

-- 
Thanks,
Oliver

[PATCH 2/2] KVM: arm64: Treat PMEVTYPER<n>_EL0.NSH as RES0

[Oliver Upton]

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);
 
 	reg = counter_index_to_evtreg(pmc->idx);
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH 2/2] KVM: arm64: Treat PMEVTYPER<n>_EL0.NSH as RES0

[Suzuki K Poulose]

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);

Re: [PATCH 2/2] KVM: arm64: Treat PMEVTYPER<n>_EL0.NSH as RES0

[Oliver Upton]

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
-- 
Best,
Oliver

Re: [PATCH 2/2] KVM: arm64: Treat PMEVTYPER<n>_EL0.NSH as RES0

[Suzuki K Poulose]

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

Re: [PATCH 2/2] KVM: arm64: Treat PMEVTYPER<n>_EL0.NSH as RES0

[James Clark]

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);

Re: [PATCH 2/2] KVM: arm64: Treat PMEVTYPER<n>_EL0.NSH as RES0

[Oliver Upton]

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.

-- 
Thanks,
Oliver