[PATCH 0/2] KVM vPMU code refine

[PATCH 0/2] KVM vPMU code refine

[Dapeng Mi]

This small patchset refines KVM vPMU code and relevant selftests.
Patch 1/2 defines new macro KVM_PMC_MAX_GENERIC to avoid the Intel
specific macro KVM_INTEL_PMC_MAX_GENERIC to be used in vPMU x86 common
code. Patch 2/2 reduces the verbosity of "Random seed" messages to avoid
the hugh number of messages to flood the regular output of selftests.

No logic change in this patchset and Kselftests/KUT PMU related tests
passed with this patchset.

Dapeng Mi (2):
  KVM: x86/pmu: Define KVM_PMC_MAX_GENERIC for platform independence
  selftests: kvm: Reduce verbosity of "Random seed" messages

 arch/x86/include/asm/kvm_host.h            | 9 +++++----
 arch/x86/kvm/svm/pmu.c                     | 2 +-
 arch/x86/kvm/vmx/pmu_intel.c               | 2 ++
 tools/testing/selftests/kvm/lib/kvm_util.c | 2 +-
 4 files changed, 9 insertions(+), 6 deletions(-)


base-commit: 0ce958282e66b3d1882e2bb2f503a5e2cebcc3ef
-- 
2.34.1

[PATCH 1/2] KVM: x86/pmu: Define KVM_PMC_MAX_GENERIC for platform independence

[Dapeng Mi]

The existing macro, KVM_INTEL_PMC_MAX_GENERIC, ambiguously represents the
maximum supported General Purpose (GP) counter number for both Intel and
AMD platforms. This could lead to issues if AMD begins to support more GP
counters than Intel.

To resolve this, a new platform-independent macro, KVM_PMC_MAX_GENERIC,
is introduced to represent the maximum GP counter number across all x86
platforms.

No logic changes are introduced in this patch.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 arch/x86/include/asm/kvm_host.h | 9 +++++----
 arch/x86/kvm/svm/pmu.c          | 2 +-
 arch/x86/kvm/vmx/pmu_intel.c    | 2 ++
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 57440bda4dc4..18137be6504a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -534,11 +534,12 @@ struct kvm_pmc {
 
 /* More counters may conflict with other existing Architectural MSRs */
 #define KVM_INTEL_PMC_MAX_GENERIC	8
-#define MSR_ARCH_PERFMON_PERFCTR_MAX	(MSR_ARCH_PERFMON_PERFCTR0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
-#define MSR_ARCH_PERFMON_EVENTSEL_MAX	(MSR_ARCH_PERFMON_EVENTSEL0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
+#define KVM_AMD_PMC_MAX_GENERIC	6
+#define KVM_PMC_MAX_GENERIC		KVM_INTEL_PMC_MAX_GENERIC
+#define MSR_ARCH_PERFMON_PERFCTR_MAX	(MSR_ARCH_PERFMON_PERFCTR0 + KVM_PMC_MAX_GENERIC - 1)
+#define MSR_ARCH_PERFMON_EVENTSEL_MAX	(MSR_ARCH_PERFMON_EVENTSEL0 + KVM_PMC_MAX_GENERIC - 1)
 #define KVM_PMC_MAX_FIXED	3
 #define MSR_ARCH_PERFMON_FIXED_CTR_MAX	(MSR_ARCH_PERFMON_FIXED_CTR0 + KVM_PMC_MAX_FIXED - 1)
-#define KVM_AMD_PMC_MAX_GENERIC	6
 
 struct kvm_pmu {
 	u8 version;
@@ -554,7 +555,7 @@ struct kvm_pmu {
 	u64 global_status_rsvd;
 	u64 reserved_bits;
 	u64 raw_event_mask;
-	struct kvm_pmc gp_counters[KVM_INTEL_PMC_MAX_GENERIC];
+	struct kvm_pmc gp_counters[KVM_PMC_MAX_GENERIC];
 	struct kvm_pmc fixed_counters[KVM_PMC_MAX_FIXED];
 
 	/*
diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
index 6e908bdc3310..2fca247798eb 100644
--- a/arch/x86/kvm/svm/pmu.c
+++ b/arch/x86/kvm/svm/pmu.c
@@ -218,7 +218,7 @@ static void amd_pmu_init(struct kvm_vcpu *vcpu)
 	int i;
 
 	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > AMD64_NUM_COUNTERS_CORE);
-	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > INTEL_PMC_MAX_GENERIC);
+	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > KVM_PMC_MAX_GENERIC);
 
 	for (i = 0; i < KVM_AMD_PMC_MAX_GENERIC ; i++) {
 		pmu->gp_counters[i].type = KVM_PMC_GP;
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index fb5cbd6cbeff..a4b0bee04596 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -570,6 +570,8 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
 	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
 	struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
 
+	BUILD_BUG_ON(KVM_INTEL_PMC_MAX_GENERIC > KVM_PMC_MAX_GENERIC);
+
 	for (i = 0; i < KVM_INTEL_PMC_MAX_GENERIC; i++) {
 		pmu->gp_counters[i].type = KVM_PMC_GP;
 		pmu->gp_counters[i].vcpu = vcpu;
-- 
2.34.1

Re: [PATCH 1/2] KVM: x86/pmu: Define KVM_PMC_MAX_GENERIC for platform independence

[Sean Christopherson]

On Thu, Jun 20, 2024, Dapeng Mi wrote:
> The existing macro, KVM_INTEL_PMC_MAX_GENERIC, ambiguously represents the
> maximum supported General Purpose (GP) counter number for both Intel and
> AMD platforms. This could lead to issues if AMD begins to support more GP
> counters than Intel.
> 
> To resolve this, a new platform-independent macro, KVM_PMC_MAX_GENERIC,
> is introduced to represent the maximum GP counter number across all x86
> platforms.
> 
> No logic changes are introduced in this patch.
> 
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 9 +++++----
>  arch/x86/kvm/svm/pmu.c          | 2 +-
>  arch/x86/kvm/vmx/pmu_intel.c    | 2 ++
>  3 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 57440bda4dc4..18137be6504a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -534,11 +534,12 @@ struct kvm_pmc {
>  
>  /* More counters may conflict with other existing Architectural MSRs */
>  #define KVM_INTEL_PMC_MAX_GENERIC	8
> -#define MSR_ARCH_PERFMON_PERFCTR_MAX	(MSR_ARCH_PERFMON_PERFCTR0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
> -#define MSR_ARCH_PERFMON_EVENTSEL_MAX	(MSR_ARCH_PERFMON_EVENTSEL0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
> +#define KVM_AMD_PMC_MAX_GENERIC	6
> +#define KVM_PMC_MAX_GENERIC		KVM_INTEL_PMC_MAX_GENERIC

Since we're changing the macro, maybe take the opportunity to use a better name?
E.g. KVM_MAX_NR_GP_COUNTERS?  And then in a follow-up patch, give fixed counters
the same treatment, e.g. KVM_MAX_NR_FIXED_COUNTERS.  Or maybe KVM_MAX_NR_GP_PMCS
and KVM_MAX_NR_FIXED_PMCS?

> +#define MSR_ARCH_PERFMON_PERFCTR_MAX	(MSR_ARCH_PERFMON_PERFCTR0 + KVM_PMC_MAX_GENERIC - 1)
> +#define MSR_ARCH_PERFMON_EVENTSEL_MAX	(MSR_ARCH_PERFMON_EVENTSEL0 + KVM_PMC_MAX_GENERIC - 1)

And I'm very, very tempted to say we should simply delete these two, along with
MSR_ARCH_PERFMON_FIXED_CTR_MAX, and just open code the "end" MSR in the one user.
Especially since "KVM" doesn't appear anyone in the name, i.e. because the names
misrepresent KVM's semi-arbitrary max as the *architectural* max.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6ad19d913d31..547dfe40d017 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7432,17 +7432,20 @@ static void kvm_probe_msr_to_save(u32 msr_index)
                     intel_pt_validate_hw_cap(PT_CAP_num_address_ranges) * 2))
                        return;
                break;
-       case MSR_ARCH_PERFMON_PERFCTR0 ... MSR_ARCH_PERFMON_PERFCTR_MAX:
+       case MSR_ARCH_PERFMON_PERFCTR0 ...
+            MSR_ARCH_PERFMON_PERFCTR0 + KVM_MAX_NR_GP_COUNTERS - 1:
                if (msr_index - MSR_ARCH_PERFMON_PERFCTR0 >=
                    kvm_pmu_cap.num_counters_gp)
                        return;
                break;
-       case MSR_ARCH_PERFMON_EVENTSEL0 ... MSR_ARCH_PERFMON_EVENTSEL_MAX:
+       case MSR_ARCH_PERFMON_EVENTSEL0 ...
+            MSR_ARCH_PERFMON_EVENTSEL0 + KVM_MAX_NR_GP_COUNTERS - 1:
                if (msr_index - MSR_ARCH_PERFMON_EVENTSEL0 >=
                    kvm_pmu_cap.num_counters_gp)
                        return;
                break;
-       case MSR_ARCH_PERFMON_FIXED_CTR0 ... MSR_ARCH_PERFMON_FIXED_CTR_MAX:
+       case MSR_ARCH_PERFMON_FIXED_CTR0 ...
+            MSR_ARCH_PERFMON_FIXED_CTR0 + KVM_MAR_NR_FIXED_COUNTERS - 1:
                if (msr_index - MSR_ARCH_PERFMON_FIXED_CTR0 >=
                    kvm_pmu_cap.num_counters_fixed)
                        return;

>  #define KVM_PMC_MAX_FIXED	3
>  #define MSR_ARCH_PERFMON_FIXED_CTR_MAX	(MSR_ARCH_PERFMON_FIXED_CTR0 + KVM_PMC_MAX_FIXED - 1)
> -#define KVM_AMD_PMC_MAX_GENERIC	6
>  
>  struct kvm_pmu {
>  	u8 version;
> @@ -554,7 +555,7 @@ struct kvm_pmu {
>  	u64 global_status_rsvd;
>  	u64 reserved_bits;
>  	u64 raw_event_mask;
> -	struct kvm_pmc gp_counters[KVM_INTEL_PMC_MAX_GENERIC];
> +	struct kvm_pmc gp_counters[KVM_PMC_MAX_GENERIC];
>  	struct kvm_pmc fixed_counters[KVM_PMC_MAX_FIXED];
>  
>  	/*
> diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
> index 6e908bdc3310..2fca247798eb 100644
> --- a/arch/x86/kvm/svm/pmu.c
> +++ b/arch/x86/kvm/svm/pmu.c
> @@ -218,7 +218,7 @@ static void amd_pmu_init(struct kvm_vcpu *vcpu)
>  	int i;
>  
>  	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > AMD64_NUM_COUNTERS_CORE);
> -	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > INTEL_PMC_MAX_GENERIC);
> +	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > KVM_PMC_MAX_GENERIC);
>  
>  	for (i = 0; i < KVM_AMD_PMC_MAX_GENERIC ; i++) {
>  		pmu->gp_counters[i].type = KVM_PMC_GP;
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index fb5cbd6cbeff..a4b0bee04596 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -570,6 +570,8 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
>  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>  	struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
>  
> +	BUILD_BUG_ON(KVM_INTEL_PMC_MAX_GENERIC > KVM_PMC_MAX_GENERIC);

Rather than BUILD_BUG_ON() for both Intel and AMD, can't we just do?

#define KVM_MAX_NR_GP_COUNTERS max(KVM_INTEL_PMC_MAX_GENERIC, KVM_AMD_PMC_MAX_GENERIC)

> +
>  	for (i = 0; i < KVM_INTEL_PMC_MAX_GENERIC; i++) {
>  		pmu->gp_counters[i].type = KVM_PMC_GP;
>  		pmu->gp_counters[i].vcpu = vcpu;
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] KVM: x86/pmu: Define KVM_PMC_MAX_GENERIC for platform independence

[Mi, Dapeng]

On 6/21/2024 12:16 AM, Sean Christopherson wrote:
> On Thu, Jun 20, 2024, Dapeng Mi wrote:
>> The existing macro, KVM_INTEL_PMC_MAX_GENERIC, ambiguously represents the
>> maximum supported General Purpose (GP) counter number for both Intel and
>> AMD platforms. This could lead to issues if AMD begins to support more GP
>> counters than Intel.
>>
>> To resolve this, a new platform-independent macro, KVM_PMC_MAX_GENERIC,
>> is introduced to represent the maximum GP counter number across all x86
>> platforms.
>>
>> No logic changes are introduced in this patch.
>>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h | 9 +++++----
>>  arch/x86/kvm/svm/pmu.c          | 2 +-
>>  arch/x86/kvm/vmx/pmu_intel.c    | 2 ++
>>  3 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 57440bda4dc4..18137be6504a 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -534,11 +534,12 @@ struct kvm_pmc {
>>  
>>  /* More counters may conflict with other existing Architectural MSRs */
>>  #define KVM_INTEL_PMC_MAX_GENERIC	8
>> -#define MSR_ARCH_PERFMON_PERFCTR_MAX	(MSR_ARCH_PERFMON_PERFCTR0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
>> -#define MSR_ARCH_PERFMON_EVENTSEL_MAX	(MSR_ARCH_PERFMON_EVENTSEL0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
>> +#define KVM_AMD_PMC_MAX_GENERIC	6
>> +#define KVM_PMC_MAX_GENERIC		KVM_INTEL_PMC_MAX_GENERIC
> Since we're changing the macro, maybe take the opportunity to use a better name?
> E.g. KVM_MAX_NR_GP_COUNTERS?  And then in a follow-up patch, give fixed counters
> the same treatment, e.g. KVM_MAX_NR_FIXED_COUNTERS.  Or maybe KVM_MAX_NR_GP_PMCS
> and KVM_MAX_NR_FIXED_PMCS?

Yeah, would change to KVM_MAX_NR_GP_COUNTERS and KVM_MAX_NR_FIXED_COUNTERS
in next version.


>
>> +#define MSR_ARCH_PERFMON_PERFCTR_MAX	(MSR_ARCH_PERFMON_PERFCTR0 + KVM_PMC_MAX_GENERIC - 1)
>> +#define MSR_ARCH_PERFMON_EVENTSEL_MAX	(MSR_ARCH_PERFMON_EVENTSEL0 + KVM_PMC_MAX_GENERIC - 1)
> And I'm very, very tempted to say we should simply delete these two, along with
> MSR_ARCH_PERFMON_FIXED_CTR_MAX, and just open code the "end" MSR in the one user.
> Especially since "KVM" doesn't appear anyone in the name, i.e. because the names
> misrepresent KVM's semi-arbitrary max as the *architectural* max.

Agree. MSR_ARCH_PERFMON_PERFCTR_MAX indeed brings some misleading which
make users think it's a HW's limitation instead of KVM's limitation.


>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6ad19d913d31..547dfe40d017 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7432,17 +7432,20 @@ static void kvm_probe_msr_to_save(u32 msr_index)
>                      intel_pt_validate_hw_cap(PT_CAP_num_address_ranges) * 2))
>                         return;
>                 break;
> -       case MSR_ARCH_PERFMON_PERFCTR0 ... MSR_ARCH_PERFMON_PERFCTR_MAX:
> +       case MSR_ARCH_PERFMON_PERFCTR0 ...
> +            MSR_ARCH_PERFMON_PERFCTR0 + KVM_MAX_NR_GP_COUNTERS - 1:
>                 if (msr_index - MSR_ARCH_PERFMON_PERFCTR0 >=
>                     kvm_pmu_cap.num_counters_gp)
>                         return;
>                 break;
> -       case MSR_ARCH_PERFMON_EVENTSEL0 ... MSR_ARCH_PERFMON_EVENTSEL_MAX:
> +       case MSR_ARCH_PERFMON_EVENTSEL0 ...
> +            MSR_ARCH_PERFMON_EVENTSEL0 + KVM_MAX_NR_GP_COUNTERS - 1:
>                 if (msr_index - MSR_ARCH_PERFMON_EVENTSEL0 >=
>                     kvm_pmu_cap.num_counters_gp)
>                         return;
>                 break;
> -       case MSR_ARCH_PERFMON_FIXED_CTR0 ... MSR_ARCH_PERFMON_FIXED_CTR_MAX:
> +       case MSR_ARCH_PERFMON_FIXED_CTR0 ...
> +            MSR_ARCH_PERFMON_FIXED_CTR0 + KVM_MAR_NR_FIXED_COUNTERS - 1:
>                 if (msr_index - MSR_ARCH_PERFMON_FIXED_CTR0 >=
>                     kvm_pmu_cap.num_counters_fixed)
>                         return;

Thanks.


>
>>  #define KVM_PMC_MAX_FIXED	3
>>  #define MSR_ARCH_PERFMON_FIXED_CTR_MAX	(MSR_ARCH_PERFMON_FIXED_CTR0 + KVM_PMC_MAX_FIXED - 1)
>> -#define KVM_AMD_PMC_MAX_GENERIC	6
>>  
>>  struct kvm_pmu {
>>  	u8 version;
>> @@ -554,7 +555,7 @@ struct kvm_pmu {
>>  	u64 global_status_rsvd;
>>  	u64 reserved_bits;
>>  	u64 raw_event_mask;
>> -	struct kvm_pmc gp_counters[KVM_INTEL_PMC_MAX_GENERIC];
>> +	struct kvm_pmc gp_counters[KVM_PMC_MAX_GENERIC];
>>  	struct kvm_pmc fixed_counters[KVM_PMC_MAX_FIXED];
>>  
>>  	/*
>> diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
>> index 6e908bdc3310..2fca247798eb 100644
>> --- a/arch/x86/kvm/svm/pmu.c
>> +++ b/arch/x86/kvm/svm/pmu.c
>> @@ -218,7 +218,7 @@ static void amd_pmu_init(struct kvm_vcpu *vcpu)
>>  	int i;
>>  
>>  	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > AMD64_NUM_COUNTERS_CORE);
>> -	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > INTEL_PMC_MAX_GENERIC);
>> +	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > KVM_PMC_MAX_GENERIC);
>>  
>>  	for (i = 0; i < KVM_AMD_PMC_MAX_GENERIC ; i++) {
>>  		pmu->gp_counters[i].type = KVM_PMC_GP;
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>> index fb5cbd6cbeff..a4b0bee04596 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -570,6 +570,8 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
>>  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>>  	struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
>>  
>> +	BUILD_BUG_ON(KVM_INTEL_PMC_MAX_GENERIC > KVM_PMC_MAX_GENERIC);
> Rather than BUILD_BUG_ON() for both Intel and AMD, can't we just do?
>
> #define KVM_MAX_NR_GP_COUNTERS max(KVM_INTEL_PMC_MAX_GENERIC, KVM_AMD_PMC_MAX_GENERIC)

Actually I tried this, but compiler would report the below error since
KVM_PMC_MAX_GENERIC would used to define the array
gp_counters[KVM_PMC_MAX_GENERIC];

./include/linux/minmax.h:48:50: error: braced-group within expression
allowed only inside a function

>
>> +
>>  	for (i = 0; i < KVM_INTEL_PMC_MAX_GENERIC; i++) {
>>  		pmu->gp_counters[i].type = KVM_PMC_GP;
>>  		pmu->gp_counters[i].vcpu = vcpu;
>> -- 
>> 2.34.1
>>

Re: [PATCH 1/2] KVM: x86/pmu: Define KVM_PMC_MAX_GENERIC for platform independence

[Sean Christopherson]

On Fri, Jun 21, 2024, Dapeng Mi wrote:
> >> diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
> >> index 6e908bdc3310..2fca247798eb 100644
> >> --- a/arch/x86/kvm/svm/pmu.c
> >> +++ b/arch/x86/kvm/svm/pmu.c
> >> @@ -218,7 +218,7 @@ static void amd_pmu_init(struct kvm_vcpu *vcpu)
> >>  	int i;
> >>  
> >>  	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > AMD64_NUM_COUNTERS_CORE);
> >> -	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > INTEL_PMC_MAX_GENERIC);
> >> +	BUILD_BUG_ON(KVM_AMD_PMC_MAX_GENERIC > KVM_PMC_MAX_GENERIC);
> >>  
> >>  	for (i = 0; i < KVM_AMD_PMC_MAX_GENERIC ; i++) {
> >>  		pmu->gp_counters[i].type = KVM_PMC_GP;
> >> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> >> index fb5cbd6cbeff..a4b0bee04596 100644
> >> --- a/arch/x86/kvm/vmx/pmu_intel.c
> >> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> >> @@ -570,6 +570,8 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
> >>  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> >>  	struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
> >>  
> >> +	BUILD_BUG_ON(KVM_INTEL_PMC_MAX_GENERIC > KVM_PMC_MAX_GENERIC);
> > Rather than BUILD_BUG_ON() for both Intel and AMD, can't we just do?
> >
> > #define KVM_MAX_NR_GP_COUNTERS max(KVM_INTEL_PMC_MAX_GENERIC, KVM_AMD_PMC_MAX_GENERIC)
> 
> Actually I tried this, but compiler would report the below error since
> KVM_PMC_MAX_GENERIC would used to define the array
> gp_counters[KVM_PMC_MAX_GENERIC];
> 
> ./include/linux/minmax.h:48:50: error: braced-group within expression
> allowed only inside a function

Oh, right, the min/max macros are super fancy to deal with types.  How about this
(getting there over 2-3 patches)?

I don't love the "#define MAX", but I don't see a better option.  I suppose maybe
we should use __MAX or KVM_MAX since kvm_host.h is likely included outside of KVM?

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5c0415899a07..ce0c9191eb85 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -532,13 +532,18 @@ struct kvm_pmc {
        u64 current_config;
 };
 
+#define MAX(a, b) ((a) >= (b) ? (a) : (b))
+
 /* More counters may conflict with other existing Architectural MSRs */
-#define KVM_INTEL_PMC_MAX_GENERIC      8
-#define MSR_ARCH_PERFMON_PERFCTR_MAX   (MSR_ARCH_PERFMON_PERFCTR0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
-#define MSR_ARCH_PERFMON_EVENTSEL_MAX  (MSR_ARCH_PERFMON_EVENTSEL0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
-#define KVM_PMC_MAX_FIXED      3
-#define MSR_ARCH_PERFMON_FIXED_CTR_MAX (MSR_ARCH_PERFMON_FIXED_CTR0 + KVM_PMC_MAX_FIXED - 1)
-#define KVM_AMD_PMC_MAX_GENERIC        6
+#define KVM_MAX_NR_INTEL_GP_COUNTERS   8
+#define KVM_MAX_NR_AMD_GP_COUNTERS     6
+#define KVM_MAX_NR_GP_COUNTERS         MAX(KVM_MAX_NR_INTEL_GP_COUNTERS, \
+                                           KVM_MAX_NR_AMD_GP_COUNTERS)
+
+#define KVM_MAX_NR_INTEL_FIXED_COUTNERS        3
+#define KVM_MAX_NR_AMD_FIXED_COUTNERS  0
+#define KVM_MAX_NR_FIXED_COUNTERS      MAX(KVM_MAX_NR_INTEL_FIXED_COUTNERS, \
+                                           KVM_MAX_NR_AMD_FIXED_COUTNERS)
 
 struct kvm_pmu {
        u8 version;
@@ -554,8 +559,8 @@ struct kvm_pmu {
        u64 global_status_rsvd;
        u64 reserved_bits;
        u64 raw_event_mask;
-       struct kvm_pmc gp_counters[KVM_INTEL_PMC_MAX_GENERIC];
-       struct kvm_pmc fixed_counters[KVM_PMC_MAX_FIXED];
+       struct kvm_pmc gp_counters[KVM_MAX_NR_GP_COUNTERS];
+       struct kvm_pmc fixed_counters[KVM_MAX_NR_FIXED_COUNTERS];
 
        /*
         * Overlay the bitmap with a 64-bit atomic so that all bits can be

[PATCH 2/2] selftests: kvm: Reduce verbosity of "Random seed" messages

[Dapeng Mi]

Huge number of "Random seed:" messages are printed when running
pmu_counters_test. It leads to the regular test output is totally
flooded by these over-verbose messages.

Downgrade "Random seed" message printing level to debug and prevent it
to be printed in normal case.

Reported-by: Yi Lai <yi1.lai@intel.com>
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/testing/selftests/kvm/lib/kvm_util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index ad00e4761886..8568c7d619c3 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -434,7 +434,7 @@ struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus,
 	slot0 = memslot2region(vm, 0);
 	ucall_init(vm, slot0->region.guest_phys_addr + slot0->region.memory_size);
 
-	pr_info("Random seed: 0x%x\n", guest_random_seed);
+	pr_debug("Random seed: 0x%x\n", guest_random_seed);
 	guest_rng = new_guest_random_state(guest_random_seed);
 	sync_global_to_guest(vm, guest_rng);
 
-- 
2.34.1

Re: [PATCH 2/2] selftests: kvm: Reduce verbosity of "Random seed" messages

[Sean Christopherson]

On Thu, Jun 20, 2024, Dapeng Mi wrote:
> Huge number of "Random seed:" messages are printed when running
> pmu_counters_test. It leads to the regular test output is totally
> flooded by these over-verbose messages.
> 
> Downgrade "Random seed" message printing level to debug and prevent it
> to be printed in normal case.

I completely agree this is annoying, but the whole point of printing the seed is
so that the seed is automatically captured if a test fails, e.g. so that the
failure can be reproduced if it is dependent on some random decision.

Rather than simply hiding the message, what if print the seed if and only if it
changes?

--
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 20 Jun 2024 10:29:53 -0700
Subject: [PATCH] KVM: selftests: Print the seed for the guest pRNG iff it has
 changed

Print the guest's random seed during VM creation if and only if the seed
has changed since the seed was last printed.  The vast majority of tests,
if not all tests at this point, set the seed during test initialization
and never change the seed, i.e. printing it every time a VM is created is
useless noise.

Snapshot and print the seed during early selftest init to play nice with
tests that use the kselftests harness, at the cost of printing an unused
seed for tests that change the seed during test-specific initialization,
e.g. dirty_log_perf_test.  The kselftests harness runs each testcase in a
separate process that is forked from the original process before creating
each testcase's VM, i.e. waiting until first VM creation will result in
the seed being printed by each testcase despite it never changing.  And
long term, the hope/goal is that setting the seed will be handled by the
core framework, i.e. that the dirty_log_perf_test wart will naturally go
away.

Reported-by: Yi Lai <yi1.lai@intel.com>
Reported-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/lib/kvm_util.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index ad00e4761886..56b170b725b3 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -21,6 +21,7 @@
 
 uint32_t guest_random_seed;
 struct guest_random_state guest_rng;
+static uint32_t last_guest_seed;
 
 static int vcpu_mmap_sz(void);
 
@@ -434,7 +435,10 @@ struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus,
 	slot0 = memslot2region(vm, 0);
 	ucall_init(vm, slot0->region.guest_phys_addr + slot0->region.memory_size);
 
-	pr_info("Random seed: 0x%x\n", guest_random_seed);
+	if (guest_random_seed != last_guest_seed) {
+		pr_info("Random seed: 0x%x\n", guest_random_seed);
+		last_guest_seed = guest_random_seed;
+	}
 	guest_rng = new_guest_random_state(guest_random_seed);
 	sync_global_to_guest(vm, guest_rng);
 
@@ -2319,7 +2323,8 @@ void __attribute((constructor)) kvm_selftest_init(void)
 	/* Tell stdout not to buffer its content. */
 	setbuf(stdout, NULL);
 
-	guest_random_seed = random();
+	guest_random_seed = last_guest_seed = random();
+	pr_info("Random seed: 0x%x\n", guest_random_seed);
 
 	kvm_selftest_arch_init();
 }

base-commit: c81b138d5075c6f5ba3419ac1d2a2e7047719c14
-- 

Re: [PATCH 2/2] selftests: kvm: Reduce verbosity of "Random seed" messages

[Mi, Dapeng]

On 6/21/2024 2:13 AM, Sean Christopherson wrote:
> On Thu, Jun 20, 2024, Dapeng Mi wrote:
>> Huge number of "Random seed:" messages are printed when running
>> pmu_counters_test. It leads to the regular test output is totally
>> flooded by these over-verbose messages.
>>
>> Downgrade "Random seed" message printing level to debug and prevent it
>> to be printed in normal case.
> I completely agree this is annoying, but the whole point of printing the seed is
> so that the seed is automatically captured if a test fails, e.g. so that the
> failure can be reproduced if it is dependent on some random decision.
>
> Rather than simply hiding the message, what if print the seed if and only if it
> changes?

Yeah, it's indeed better.

>
> --
> From: Sean Christopherson <seanjc@google.com>
> Date: Thu, 20 Jun 2024 10:29:53 -0700
> Subject: [PATCH] KVM: selftests: Print the seed for the guest pRNG iff it has
>  changed

s/iff/if/


>
> Print the guest's random seed during VM creation if and only if the seed
> has changed since the seed was last printed.  The vast majority of tests,
> if not all tests at this point, set the seed during test initialization
> and never change the seed, i.e. printing it every time a VM is created is
> useless noise.
>
> Snapshot and print the seed during early selftest init to play nice with
> tests that use the kselftests harness, at the cost of printing an unused
> seed for tests that change the seed during test-specific initialization,
> e.g. dirty_log_perf_test.  The kselftests harness runs each testcase in a
> separate process that is forked from the original process before creating
> each testcase's VM, i.e. waiting until first VM creation will result in
> the seed being printed by each testcase despite it never changing.  And
> long term, the hope/goal is that setting the seed will be handled by the
> core framework, i.e. that the dirty_log_perf_test wart will naturally go
> away.
>
> Reported-by: Yi Lai <yi1.lai@intel.com>
> Reported-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  tools/testing/selftests/kvm/lib/kvm_util.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index ad00e4761886..56b170b725b3 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -21,6 +21,7 @@
>  
>  uint32_t guest_random_seed;
>  struct guest_random_state guest_rng;
> +static uint32_t last_guest_seed;
>  
>  static int vcpu_mmap_sz(void);
>  
> @@ -434,7 +435,10 @@ struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus,
>  	slot0 = memslot2region(vm, 0);
>  	ucall_init(vm, slot0->region.guest_phys_addr + slot0->region.memory_size);
>  
> -	pr_info("Random seed: 0x%x\n", guest_random_seed);
> +	if (guest_random_seed != last_guest_seed) {
> +		pr_info("Random seed: 0x%x\n", guest_random_seed);
> +		last_guest_seed = guest_random_seed;
> +	}
>  	guest_rng = new_guest_random_state(guest_random_seed);
>  	sync_global_to_guest(vm, guest_rng);
>  
> @@ -2319,7 +2323,8 @@ void __attribute((constructor)) kvm_selftest_init(void)
>  	/* Tell stdout not to buffer its content. */
>  	setbuf(stdout, NULL);
>  
> -	guest_random_seed = random();
> +	guest_random_seed = last_guest_seed = random();
> +	pr_info("Random seed: 0x%x\n", guest_random_seed);
>  
>  	kvm_selftest_arch_init();
>  }
>
> base-commit: c81b138d5075c6f5ba3419ac1d2a2e7047719c14

Re: [PATCH 2/2] selftests: kvm: Reduce verbosity of "Random seed" messages

[Sean Christopherson]

On Fri, Jun 21, 2024, Dapeng Mi wrote:
> > --
> > From: Sean Christopherson <seanjc@google.com>
> > Date: Thu, 20 Jun 2024 10:29:53 -0700
> > Subject: [PATCH] KVM: selftests: Print the seed for the guest pRNG iff it has
> >  changed
> 
> s/iff/if/

"iff" is shorthand for "if and only if".  I try to write out the full "if and only
if" when possible, but use "iff" in shortlogs when I want to squeeze in more words.

Re: [PATCH 2/2] selftests: kvm: Reduce verbosity of "Random seed" messages

[Mi, Dapeng]

On 6/21/2024 9:29 PM, Sean Christopherson wrote:
> On Fri, Jun 21, 2024, Dapeng Mi wrote:
>>> --
>>> From: Sean Christopherson <seanjc@google.com>
>>> Date: Thu, 20 Jun 2024 10:29:53 -0700
>>> Subject: [PATCH] KVM: selftests: Print the seed for the guest pRNG iff it has
>>>  changed
>> s/iff/if/
> "iff" is shorthand for "if and only if".  I try to write out the full "if and only
> if" when possible, but use "iff" in shortlogs when I want to squeeze in more words.

Good to know. Thanks.

Re: [PATCH 0/2] KVM vPMU code refine

[Sean Christopherson]

On Thu, Jun 20, 2024, Dapeng Mi wrote:
> This small patchset refines KVM vPMU code and relevant selftests.
> Patch 1/2 defines new macro KVM_PMC_MAX_GENERIC to avoid the Intel
> specific macro KVM_INTEL_PMC_MAX_GENERIC to be used in vPMU x86 common
> code. Patch 2/2 reduces the verbosity of "Random seed" messages to avoid
> the hugh number of messages to flood the regular output of selftests.

In the future, please post these as separate patches, they are completely unrelated.

Re: [PATCH 0/2] KVM vPMU code refine

[Mi, Dapeng]

On 6/21/2024 12:07 AM, Sean Christopherson wrote:
> On Thu, Jun 20, 2024, Dapeng Mi wrote:
>> This small patchset refines KVM vPMU code and relevant selftests.
>> Patch 1/2 defines new macro KVM_PMC_MAX_GENERIC to avoid the Intel
>> specific macro KVM_INTEL_PMC_MAX_GENERIC to be used in vPMU x86 common
>> code. Patch 2/2 reduces the verbosity of "Random seed" messages to avoid
>> the hugh number of messages to flood the regular output of selftests.
> In the future, please post these as separate patches, they are completely unrelated.

Sure. Thanks.