[v1] KVM: x86/pmu: Make top-down.slots event unavailable in supported leaf

[PATCH RESEND] KVM: x86/pmu: Make top-down.slots event unavailable in supported leaf

Posted by Like Xu 4 years, 5 months ago

From: Like Xu <likexu@tencent.com>

When we choose to disable the fourth fixed counter TOPDOWN.SLOTS,
we need to also reduce the length of the 0AH.EBX bit vector, which
enumerates architecture performance monitoring events, and set
0AH.EBX.[bit 7] to 1 if the new value of EAX[31:24] is still > 7.

Fixes: 2e8cd7a3b8287 ("kvm: x86: limit the maximum number of vPMU fixed counters to 3")
Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/cpuid.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 0b920e12bb6d..1f0131145296 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -782,6 +782,21 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		eax.split.mask_length = cap.events_mask_len;
 
 		edx.split.num_counters_fixed = min(cap.num_counters_fixed, MAX_FIXED_COUNTERS);
+
+		/*
+		 * The 8th Intel pre-defined architectural event (Topdown Slots) will be supported
+		 * if the 4th fixed counter exists && EAX[31:24] > 7 && EBX[7] = 0.
+		 *
+		 * Currently, KVM needs to set EAX[31:24] < 8 or EBX[7] == 1
+		 * to make this event unavailable in a consistent way.
+		 */
+		if (edx.split.num_counters_fixed < 4) {
+			if (eax.split.mask_length > 7)
+				eax.split.mask_length--;
+			if (eax.split.mask_length > 7)
+				cap.events_mask |= BIT_ULL(7);
+		}
+
 		edx.split.bit_width_fixed = cap.bit_width_fixed;
 		if (cap.version)
 			edx.split.anythread_deprecated = 1;
-- 
2.33.1

Re: [PATCH RESEND] KVM: x86/pmu: Make top-down.slots event unavailable in supported leaf

Posted by Sean Christopherson 4 years, 5 months ago

On Wed, Jan 05, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> When we choose to disable the fourth fixed counter TOPDOWN.SLOTS,
> we need to also reduce the length of the 0AH.EBX bit vector, which
> enumerates architecture performance monitoring events, and set
> 0AH.EBX.[bit 7] to 1 if the new value of EAX[31:24] is still > 7.
> 
> Fixes: 2e8cd7a3b8287 ("kvm: x86: limit the maximum number of vPMU fixed counters to 3")
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>  arch/x86/kvm/cpuid.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 0b920e12bb6d..1f0131145296 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -782,6 +782,21 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		eax.split.mask_length = cap.events_mask_len;
>  
>  		edx.split.num_counters_fixed = min(cap.num_counters_fixed, MAX_FIXED_COUNTERS);
> +
> +		/*
> +		 * The 8th Intel pre-defined architectural event (Topdown Slots) will be supported
> +		 * if the 4th fixed counter exists && EAX[31:24] > 7 && EBX[7] = 0.

Please wrap at ~80 chars.

> +		 *
> +		 * Currently, KVM needs to set EAX[31:24] < 8 or EBX[7] == 1
> +		 * to make this event unavailable in a consistent way.
> +		 */
> +		if (edx.split.num_counters_fixed < 4) {
> +			if (eax.split.mask_length > 7)
> +				eax.split.mask_length--;

This will break if there's a bit>7 enumerated in EBX (events_mask) that KVM wants
to expose to the guest.  It doesn't cause problems today because bits 31:8 are all
reserved, but that will not always be the case.

We could do

		if (edx.split.num_counters_fixed < 4) {
			if (eax.split.mask_length == 7)
				eax.split.mask_length--;
			else
				cap.events_mask |= BIT_ULL(7);
		}

but I don't see any reason to make this more complex than:

		if (edx.split.num_counters_fixed < 4 &&
		    eax.split.mask_length > 7)
			cap.events_mask |= BIT_ULL(7);

> +			if (eax.split.mask_length > 7)
> +				cap.events_mask |= BIT_ULL(7);
> +		}
> +
>  		edx.split.bit_width_fixed = cap.bit_width_fixed;
>  		if (cap.version)
>  			edx.split.anythread_deprecated = 1;
> -- 
> 2.33.1
>

Re: [PATCH RESEND] KVM: x86/pmu: Make top-down.slots event unavailable in supported leaf

Posted by Paolo Bonzini 4 years, 5 months ago

On 1/5/22 06:07, Like Xu wrote:
> +		/*
> +		 * The 8th Intel pre-defined architectural event (Topdown Slots) will be supported
> +		 * if the 4th fixed counter exists && EAX[31:24] > 7 && EBX[7] = 0.
> +		 *
> +		 * Currently, KVM needs to set EAX[31:24] < 8 or EBX[7] == 1
> +		 * to make this event unavailable in a consistent way.
> +		 */
> +		if (edx.split.num_counters_fixed < 4) {
> +			if (eax.split.mask_length > 7)
> +				eax.split.mask_length--;
> +			if (eax.split.mask_length > 7)
> +				cap.events_mask |= BIT_ULL(7);
> +		}
> +

The first "> 7" is wrong; it should be == 8, shouldn't it?  Something like

if (edx.split.num_counters_fixed < 4 && eax.split.mask_length >= 8) {
	if (eax.split.mask_length == 8)
		eax.split.mask_length--;
	else
		cap.events_mask |= BIT_ULL(7);
}

is what you mean, I think?

Paolo