[PATCH 0/3] KVM: x86: Intel PERF_CAPABILITIES fix and cleanups

[PATCH 0/3] KVM: x86: Intel PERF_CAPABILITIES fix and cleanups

[Sean Christopherson]

Fix a bug where KVM doesn't refresh the vPMU after userspace writes
PERF_CAPABILITIES, and then leverage that fix to avoiding checking
guest_cpuid_has(X86_FEATURE_PDCM) during VM-Enter, which is slow enough
that it shows up in perf traces[*].

[*] https://gist.github.com/avagin/f50a6d569440c9ae382281448c187f4e

Sean Christopherson (3):
  KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES
  KVM: VMX: Use proper type-safe functions for vCPU => LBRs helpers
  KVM: VMX: Adjust number of LBR records for PERF_CAPABILITIES at
    refresh

 arch/x86/kvm/vmx/pmu_intel.c | 12 +++---------
 arch/x86/kvm/vmx/vmx.h       | 29 ++++++++++++++++++++---------
 arch/x86/kvm/x86.c           |  4 ++--
 3 files changed, 25 insertions(+), 20 deletions(-)


base-commit: 1a4d88a361af4f2e91861d632c6a1fe87a9665c2
-- 
2.37.1.359.gd136c6c3e2-goog

[PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES

[Sean Christopherson]

Refresh the PMU if userspace modifies MSR_IA32_PERF_CAPABILITIES.  KVM
consumes the vCPU's PERF_CAPABILITIES when enumerating PEBS support, but
relies on CPUID updates to refresh the PMU.  I.e. KVM will do the wrong
thing if userspace stuffs PERF_CAPABILITIES _after_ setting guest CPUID.

Opportunistically fix a curly-brace indentation.

Fixes: c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS")
Cc: Like Xu <like.xu.linux@gmail.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5366f884e9a7..362c538285db 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3543,9 +3543,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return 1;
 
 		vcpu->arch.perf_capabilities = data;
-
+		kvm_pmu_refresh(vcpu);
 		return 0;
-		}
+	}
 	case MSR_EFER:
 		return set_efer(vcpu, msr_info);
 	case MSR_K7_HWCR:
-- 
2.37.1.359.gd136c6c3e2-goog

[PATCH 2/3] KVM: VMX: Use proper type-safe functions for vCPU => LBRs helpers

[Sean Christopherson]

Turn vcpu_to_lbr_desc() and vcpu_to_lbr_records() into functions in order
to provide type safety, to document exactly what they return, and to
allow consuming the helpers in vmx.h.  Move the definitions as necessary
(the macros "reference" to_vmx() before its definition).

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.h | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 286c88e285ea..690421b7d26c 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -6,6 +6,7 @@
 
 #include <asm/kvm.h>
 #include <asm/intel_pt.h>
+#include <asm/perf_event.h>
 
 #include "capabilities.h"
 #include "kvm_cache_regs.h"
@@ -91,15 +92,6 @@ union vmx_exit_reason {
 	u32 full;
 };
 
-#define vcpu_to_lbr_desc(vcpu) (&to_vmx(vcpu)->lbr_desc)
-#define vcpu_to_lbr_records(vcpu) (&to_vmx(vcpu)->lbr_desc.records)
-
-void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu);
-bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu);
-
-int intel_pmu_create_guest_lbr_event(struct kvm_vcpu *vcpu);
-void vmx_passthrough_lbr_msrs(struct kvm_vcpu *vcpu);
-
 struct lbr_desc {
 	/* Basic info about guest LBR records. */
 	struct x86_pmu_lbr records;
@@ -524,6 +516,22 @@ static inline struct vcpu_vmx *to_vmx(struct kvm_vcpu *vcpu)
 	return container_of(vcpu, struct vcpu_vmx, vcpu);
 }
 
+static inline struct lbr_desc *vcpu_to_lbr_desc(struct kvm_vcpu *vcpu)
+{
+	return &to_vmx(vcpu)->lbr_desc;
+}
+
+static inline struct x86_pmu_lbr *vcpu_to_lbr_records(struct kvm_vcpu *vcpu)
+{
+	return &vcpu_to_lbr_desc(vcpu)->records;
+}
+
+void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu);
+bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu);
+
+int intel_pmu_create_guest_lbr_event(struct kvm_vcpu *vcpu);
+void vmx_passthrough_lbr_msrs(struct kvm_vcpu *vcpu);
+
 static inline unsigned long vmx_get_exit_qual(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
-- 
2.37.1.359.gd136c6c3e2-goog

[PATCH 3/3] KVM: VMX: Adjust number of LBR records for PERF_CAPABILITIES at refresh

[Sean Christopherson]

Now that the PMU is refreshed when MSR_IA32_PERF_CAPABILITIES is written
by host userspace, zero out the number of LBR records for a vCPU during
PMU refresh if PMU_CAP_LBR_FMT is not set in PERF_CAPABILITIES instead of
handling the check at run-time.

guest_cpuid_has() is expensive due to the linear search of guest CPUID
entries, intel_pmu_lbr_is_enabled() is checked on every VM-Enter, _and_
simply enumerating the same "Model" as the host causes KVM to set the
number of LBR records to a non-zero value.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/pmu_intel.c | 12 +++---------
 arch/x86/kvm/vmx/vmx.h       |  7 +++++--
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index cfcb590afaa7..d111dc0d86df 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -171,13 +171,6 @@ static inline struct kvm_pmc *get_fw_gp_pmc(struct kvm_pmu *pmu, u32 msr)
 	return get_gp_pmc(pmu, msr, MSR_IA32_PMC0);
 }
 
-bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu)
-{
-	struct x86_pmu_lbr *lbr = vcpu_to_lbr_records(vcpu);
-
-	return lbr->nr && (vcpu_get_perf_capabilities(vcpu) & PMU_CAP_LBR_FMT);
-}
-
 static bool intel_pmu_is_valid_lbr_msr(struct kvm_vcpu *vcpu, u32 index)
 {
 	struct x86_pmu_lbr *records = vcpu_to_lbr_records(vcpu);
@@ -590,7 +583,9 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
 	bitmap_set(pmu->all_valid_pmc_idx,
 		INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
 
-	if (cpuid_model_is_consistent(vcpu))
+	perf_capabilities = vcpu_get_perf_capabilities(vcpu);
+	if (cpuid_model_is_consistent(vcpu) &&
+	    (perf_capabilities & PMU_CAP_LBR_FMT))
 		x86_perf_get_lbr(&lbr_desc->records);
 	else
 		lbr_desc->records.nr = 0;
@@ -598,7 +593,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
 	if (lbr_desc->records.nr)
 		bitmap_set(pmu->all_valid_pmc_idx, INTEL_PMC_IDX_FIXED_VLBR, 1);
 
-	perf_capabilities = vcpu_get_perf_capabilities(vcpu);
 	if (perf_capabilities & PERF_CAP_PEBS_FORMAT) {
 		if (perf_capabilities & PERF_CAP_PEBS_BASELINE) {
 			pmu->pebs_enable_mask = counter_mask;
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 690421b7d26c..c05e302fe2b1 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -526,9 +526,12 @@ static inline struct x86_pmu_lbr *vcpu_to_lbr_records(struct kvm_vcpu *vcpu)
 	return &vcpu_to_lbr_desc(vcpu)->records;
 }
 
+static inline bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu)
+{
+	return !!vcpu_to_lbr_records(vcpu)->nr;
+}
+
 void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu);
-bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu);
-
 int intel_pmu_create_guest_lbr_event(struct kvm_vcpu *vcpu);
 void vmx_passthrough_lbr_msrs(struct kvm_vcpu *vcpu);
 
-- 
2.37.1.359.gd136c6c3e2-goog

Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES

[Like Xu]

On 28/7/2022 7:34 am, Sean Christopherson wrote:
> Refresh the PMU if userspace modifies MSR_IA32_PERF_CAPABILITIES.  KVM
> consumes the vCPU's PERF_CAPABILITIES when enumerating PEBS support, but
> relies on CPUID updates to refresh the PMU.  I.e. KVM will do the wrong
> thing if userspace stuffs PERF_CAPABILITIES _after_ setting guest CPUID.

Unwise userspace should reap its consequences if it does not break KVM or host.

When a guest feature can be defined/controlled by multiple KVM APIs entries,
(such as SET_CPUID2, msr_feature, KVM_CAP, module_para), should KVM
define the priority of these APIs (e.g. whether they can override each other) ?

Removing this ambiguity ensures consistency in the architecture and behavior of 
all KVM features.
Any further performance optimizations can be based on these finalized values as 
you do.

> 
> Opportunistically fix a curly-brace indentation.
> 
> Fixes: c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS")
> Cc: Like Xu <like.xu.linux@gmail.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/x86.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5366f884e9a7..362c538285db 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3543,9 +3543,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   			return 1;
>   
>   		vcpu->arch.perf_capabilities = data;
> -
> +		kvm_pmu_refresh(vcpu);

I had proposed this diff but was met with silence.

>   		return 0;
> -		}
> +	}
>   	case MSR_EFER:
>   		return set_efer(vcpu, msr_info);
>   	case MSR_K7_HWCR:

Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES

[Sean Christopherson]

On Thu, Jul 28, 2022, Like Xu wrote:
> On 28/7/2022 7:34 am, Sean Christopherson wrote:
> > Refresh the PMU if userspace modifies MSR_IA32_PERF_CAPABILITIES.  KVM
> > consumes the vCPU's PERF_CAPABILITIES when enumerating PEBS support, but
> > relies on CPUID updates to refresh the PMU.  I.e. KVM will do the wrong
> > thing if userspace stuffs PERF_CAPABILITIES _after_ setting guest CPUID.
> 
> Unwise userspace should reap its consequences if it does not break KVM or host.

I don't think this is a case of userspace being weird or unwise.  IMO, setting
CPUID before MSRs is perfectly logical and intuitive.

> When a guest feature can be defined/controlled by multiple KVM APIs entries,
> (such as SET_CPUID2, msr_feature, KVM_CAP, module_para), should KVM
> define the priority of these APIs (e.g. whether they can override each other) ?

KVM does have "rules" in the sense that it has an established ABI for things
like KVM_CAP and module params, though documentation may be lacking in some cases.
The CPUID and MSR ioctls don't have a prescribe ordering though.

> Removing this ambiguity ensures consistency in the architecture and behavior
> of all KVM features.

Agreed, but the CPUID and MSR ioctls (among many others) have existed for quite
some time.  KVM likely can't retroactively force a specific order without breaking
one userspace or another.

> Any further performance optimizations can be based on these finalized values
> as you do.
> 
> > 
> > Opportunistically fix a curly-brace indentation.
> > 
> > Fixes: c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS")
> > Cc: Like Xu <like.xu.linux@gmail.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >   arch/x86/kvm/x86.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 5366f884e9a7..362c538285db 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3543,9 +3543,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >   			return 1;
> >   		vcpu->arch.perf_capabilities = data;
> > -
> > +		kvm_pmu_refresh(vcpu);
> 
> I had proposed this diff but was met with silence.

My apologies, I either missed it or didn't connect the dots.

Re: [PATCH 2/3] KVM: VMX: Use proper type-safe functions for vCPU => LBRs helpers

[Like Xu]

On 28/7/2022 7:34 am, Sean Christopherson wrote:
> Turn vcpu_to_lbr_desc() and vcpu_to_lbr_records() into functions in order
> to provide type safety, to document exactly what they return, and to

Considering the prevalence of similar practices, perhaps we (at least me)
need doc more benefits of "type safety" or the risks of not doing so.

> allow consuming the helpers in vmx.h.  Move the definitions as necessary
> (the macros "reference" to_vmx() before its definition).
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/vmx/vmx.h | 26 +++++++++++++++++---------
>   1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 286c88e285ea..690421b7d26c 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -6,6 +6,7 @@
>   
>   #include <asm/kvm.h>
>   #include <asm/intel_pt.h>
> +#include <asm/perf_event.h>
>   
>   #include "capabilities.h"
>   #include "kvm_cache_regs.h"
> @@ -91,15 +92,6 @@ union vmx_exit_reason {
>   	u32 full;
>   };
>   
> -#define vcpu_to_lbr_desc(vcpu) (&to_vmx(vcpu)->lbr_desc)
> -#define vcpu_to_lbr_records(vcpu) (&to_vmx(vcpu)->lbr_desc.records)

More targets can be found in the arch/x86/kvm/pmu.h:

#define vcpu_to_pmu(vcpu) (&(vcpu)->arch.pmu)
#define pmu_to_vcpu(pmu)  (container_of((pmu), struct kvm_vcpu, arch.pmu))
#define pmc_to_pmu(pmc)   (&(pmc)->vcpu->arch.pmu)

> -
> -void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu);
> -bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu);
> -
> -int intel_pmu_create_guest_lbr_event(struct kvm_vcpu *vcpu);
> -void vmx_passthrough_lbr_msrs(struct kvm_vcpu *vcpu);

Unrelated move, but no reason not to do so opportunistically.

> -
>   struct lbr_desc {
>   	/* Basic info about guest LBR records. */
>   	struct x86_pmu_lbr records;
> @@ -524,6 +516,22 @@ static inline struct vcpu_vmx *to_vmx(struct kvm_vcpu *vcpu)
>   	return container_of(vcpu, struct vcpu_vmx, vcpu);
>   }
>   
> +static inline struct lbr_desc *vcpu_to_lbr_desc(struct kvm_vcpu *vcpu)
> +{
> +	return &to_vmx(vcpu)->lbr_desc;
> +}
> +
> +static inline struct x86_pmu_lbr *vcpu_to_lbr_records(struct kvm_vcpu *vcpu)
> +{
> +	return &vcpu_to_lbr_desc(vcpu)->records;
> +}
> +
> +void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu);
> +bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu);
> +
> +int intel_pmu_create_guest_lbr_event(struct kvm_vcpu *vcpu);
> +void vmx_passthrough_lbr_msrs(struct kvm_vcpu *vcpu);
> +
>   static inline unsigned long vmx_get_exit_qual(struct kvm_vcpu *vcpu)
>   {
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);

Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES

[Like Xu]

On 28/7/2022 11:27 pm, Sean Christopherson wrote:
> On Thu, Jul 28, 2022, Like Xu wrote:
>> On 28/7/2022 7:34 am, Sean Christopherson wrote:
>>> Refresh the PMU if userspace modifies MSR_IA32_PERF_CAPABILITIES.  KVM
>>> consumes the vCPU's PERF_CAPABILITIES when enumerating PEBS support, but
>>> relies on CPUID updates to refresh the PMU.  I.e. KVM will do the wrong
>>> thing if userspace stuffs PERF_CAPABILITIES _after_ setting guest CPUID.
>>
>> Unwise userspace should reap its consequences if it does not break KVM or host.
> 
> I don't think this is a case of userspace being weird or unwise.  IMO, setting
> CPUID before MSRs is perfectly logical and intuitive.

The concern is whether to allow changing the semantically featured MSR value
(as an alternative to CPUID or KVM_CAP.) from user space after the guest CPUID
is finalized or the guest has run for a while.

Changing the presence semantics of related CPUID via a post-written msr-feature,
or vice versa, is seen as a user-space ill-advisedness. Based on the ill-advisedness
of the user space input, KVM's strange behaviour is to be expected. Right ?

A wise user space should take care of both PEBS CPUID and PEBS fields
in the PERF_CAPABILITIES, in whatever time order they are passed to KVM.
KVM implementation should treat them as equivalent for any availability check
(regardless of performance issue, it's my bad to traverse CPUID rathe than 
perf_cap).

If two or more settings cannot be coordinated with each other in the user space 
level,
KVM must choose to rely on one setting or another or check all settings (more 
expensive).

> 
>> When a guest feature can be defined/controlled by multiple KVM APIs entries,
>> (such as SET_CPUID2, msr_feature, KVM_CAP, module_para), should KVM
>> define the priority of these APIs (e.g. whether they can override each other) ?
> 
> KVM does have "rules" in the sense that it has an established ABI for things
> like KVM_CAP and module params, though documentation may be lacking in some cases.
> The CPUID and MSR ioctls don't have a prescribe ordering though.

Should we continue with this inter-dependence (as a silent feature) ?
The patch implies that it should be left as it is in order not to break any user 
space.

How we break out of this rut ?

> 
>> Removing this ambiguity ensures consistency in the architecture and behavior
>> of all KVM features.
> 
> Agreed, but the CPUID and MSR ioctls (among many others) have existed for quite
> some time.  KVM likely can't retroactively force a specific order without breaking
> one userspace or another.
> 
>> Any further performance optimizations can be based on these finalized values
>> as you do.
>>
>>>
>>> Opportunistically fix a curly-brace indentation.
>>>
>>> Fixes: c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS")
>>> Cc: Like Xu <like.xu.linux@gmail.com>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>    arch/x86/kvm/x86.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 5366f884e9a7..362c538285db 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -3543,9 +3543,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>    			return 1;
>>>    		vcpu->arch.perf_capabilities = data;
>>> -
>>> +		kvm_pmu_refresh(vcpu);
>>
>> I had proposed this diff but was met with silence.
> 
> My apologies, I either missed it or didn't connect the dots.

Re: [PATCH 3/3] KVM: VMX: Adjust number of LBR records for PERF_CAPABILITIES at refresh

[Like Xu]

On 28/7/2022 7:34 am, Sean Christopherson wrote:
> guest_cpuid_has() is expensive due to the linear search of guest CPUID
> entries, intel_pmu_lbr_is_enabled() is checked on every VM-Enter,_and_
> simply enumerating the same "Model" as the host causes KVM to set the
> number of LBR records to a non-zero value.

Before reconsidering vcpu->arch.perf_capabilities to reach a conclusion,
how about this minor inline change help reduce my sins ?

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 0ecbbae42976..06a21d66be13 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7039,7 +7039,8 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
  	pt_guest_enter(vmx);

  	atomic_switch_perf_msrs(vmx);
-	if (intel_pmu_lbr_is_enabled(vcpu))
+	if (vmx->lbr_desc.records.nr &&
+	    (vcpu->arch.perf_capabilities & PMU_CAP_LBR_FMT))
  		vmx_passthrough_lbr_msrs(vcpu);

  	if (enable_preemption_timer)

Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES

[Sean Christopherson]

On Fri, Jul 29, 2022, Like Xu wrote:
> On 28/7/2022 11:27 pm, Sean Christopherson wrote:
> > On Thu, Jul 28, 2022, Like Xu wrote:
> > > On 28/7/2022 7:34 am, Sean Christopherson wrote:
> > > > Refresh the PMU if userspace modifies MSR_IA32_PERF_CAPABILITIES.  KVM
> > > > consumes the vCPU's PERF_CAPABILITIES when enumerating PEBS support, but
> > > > relies on CPUID updates to refresh the PMU.  I.e. KVM will do the wrong
> > > > thing if userspace stuffs PERF_CAPABILITIES _after_ setting guest CPUID.
> > > 
> > > Unwise userspace should reap its consequences if it does not break KVM or host.
> > 
> > I don't think this is a case of userspace being weird or unwise.  IMO, setting
> > CPUID before MSRs is perfectly logical and intuitive.
> 
> The concern is whether to allow changing the semantically featured MSR value
> (as an alternative to CPUID or KVM_CAP.) from user space after the guest CPUID
> is finalized or the guest has run for a while.

Hrm, I forgot about that problem.

> > KVM does have "rules" in the sense that it has an established ABI for things
> > like KVM_CAP and module params, though documentation may be lacking in some cases.
> > The CPUID and MSR ioctls don't have a prescribe ordering though.
> 
> Should we continue with this inter-dependence (as a silent feature) ?
> The patch implies that it should be left as it is in order not to break any
> user space.
> 
> How we break out of this rut ?

The correct fix in KVM is to reject writes to feature MSRs after KVM_RUN.  KVM
already does this for CPUID.   I'm pretty sure KVM needs to allow writes with the
same value to support QEMU's hotplug behavior, but that's easy enough to handle
(in theory).  There are few enough feature MSRs that I think we can get away with
a linear walk, e.g.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5366f884e9a7..fffc57dea304 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2150,6 +2150,22 @@ static int do_get_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)

 static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 {
+       u64 current_val;
+       int i;
+
+       if (vcpu->arch.last_vmentry_cpu != -1 && index != MSR_IA32_UCODE_REV) {
+               for (i = 0; i < num_msr_based_features; i++) {
+                       if (index != msr_based_features[i])
+                               continue;
+
+                       if (do_get_msr(vcpu, index, &current_val) ||
+                           *data != current_val)
+                               return -EINVAL;
+
+                       return 0;
+               }
+       }
+
        return kvm_set_msr_ignored_check(vcpu, index, *data, true);
 }

Re: [PATCH 2/3] KVM: VMX: Use proper type-safe functions for vCPU => LBRs helpers

[Sean Christopherson]

On Fri, Jul 29, 2022, Like Xu wrote:
> On 28/7/2022 7:34 am, Sean Christopherson wrote:
> > Turn vcpu_to_lbr_desc() and vcpu_to_lbr_records() into functions in order
> > to provide type safety, to document exactly what they return, and to
> 
> Considering the prevalence of similar practices, perhaps we (at least me)
> need doc more benefits of "type safety" or the risks of not doing so.

There already is documentation, see "#ifdef and preprocessor use in general" in
Documentation/process/4.Coding.rst:

  C preprocessor macros present a number of hazards, including possible
  multiple evaluation of expressions with side effects and no type safety.
  If you are tempted to define a macro, consider creating an inline function
  instead.  The code which results will be the same, but inline functions are
  easier to read, do not evaluate their arguments multiple times, and allow
  the compiler to perform type checking on the arguments and return value.

To elaborate on type safety, or lack thereof, let's pretend that struct kvm_vcpu's
"mutex" was actually named "lock", i.e. that both struct kvm_vcpu and struct kvm had:

	struct mutex lock;

And for whatever reason, we wanted to add helpers to lock/unlock.  If the helpers
were implemented via macros, e.g.

  	#define kvm_lock_vm(kvm) mutex_lock(&(kvm)->lock)

then this ends up being 100% legal code from the compilers perspective

	kvm_lock_vm(vcpu);

i.e. it will compile cleanly and only cause problems at run time because the
preprocesor expands that to:

	mutex_lock(&vcpu->lock);

A function on the other hand provides type safety because even though both kvm.lock
and kvm_vcpu.lock exist, the compiler will yell due to attempting to pass a
"struct kvm_vcpu *" into a function that takes a "struct kvm *kvm".

	static inline void kvm_lock_vm(struct kvm *kvm)
	{
		mutex_lock(&kvm->lock);
	}

There are instances where a macro is used because it's deemed to be the lesser
evil.  E.g. KVM x86 does

  #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0)

to allow inlining without creating a circular dependency.  include/linux/kvm_host.h
uses kvm_arch_vcpu_memslots_id() and so it needs to be fully defined by
arch/x86/include/asm/kvm_host.h.  But because linux/kvm_host.h includes asm/kvm_host.h,
asm/kvm_host.h can't deference @vcpu due to "struct kvm_vcpu" not yet being defined.

The macro trick works for this case because the preprocessor doesn't compile or check
anything, it just expands the macro in its user.  I.e. the derefence of @vcpu occurs
in kvm_vcpu_memslots() from the compilers perspective, and at that point "struct kvm_vcpu"
is fully defined and everyone is happy.

There's still some amount of risk in misuing kvm_arch_vcpu_memslots_id(), but in
this case we've decided that the benefits of inlining outweigh the risk of misuse.

> > allow consuming the helpers in vmx.h.  Move the definitions as necessary
> > (the macros "reference" to_vmx() before its definition).
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >   arch/x86/kvm/vmx/vmx.h | 26 +++++++++++++++++---------
> >   1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 286c88e285ea..690421b7d26c 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -6,6 +6,7 @@
> >   #include <asm/kvm.h>
> >   #include <asm/intel_pt.h>
> > +#include <asm/perf_event.h>
> >   #include "capabilities.h"
> >   #include "kvm_cache_regs.h"
> > @@ -91,15 +92,6 @@ union vmx_exit_reason {
> >   	u32 full;
> >   };
> > -#define vcpu_to_lbr_desc(vcpu) (&to_vmx(vcpu)->lbr_desc)
> > -#define vcpu_to_lbr_records(vcpu) (&to_vmx(vcpu)->lbr_desc.records)
> 
> More targets can be found in the arch/x86/kvm/pmu.h:
> 
> #define vcpu_to_pmu(vcpu) (&(vcpu)->arch.pmu)
> #define pmu_to_vcpu(pmu)  (container_of((pmu), struct kvm_vcpu, arch.pmu))
> #define pmc_to_pmu(pmc)   (&(pmc)->vcpu->arch.pmu)

We should definitely convert those, assuming it's not too painful and not
completely impossible due to circular dependencies.

Re: [PATCH 3/3] KVM: VMX: Adjust number of LBR records for PERF_CAPABILITIES at refresh

[Sean Christopherson]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

On Fri, Jul 29, 2022, Like Xu wrote:
> On 28/7/2022 7:34 am, Sean Christopherson wrote:
> > guest_cpuid_has() is expensive due to the linear search of guest CPUID
> > entries, intel_pmu_lbr_is_enabled() is checked on every VM-Enter,_and_
> > simply enumerating the same "Model" as the host causes KVM to set the
> > number of LBR records to a non-zero value.
> 
> Before reconsidering vcpu->arch.perf_capabilities to reach a conclusion,
> how about this minor inline change help reduce my sins ?
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 0ecbbae42976..06a21d66be13 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7039,7 +7039,8 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  	pt_guest_enter(vmx);
> 
>  	atomic_switch_perf_msrs(vmx);
> -	if (intel_pmu_lbr_is_enabled(vcpu))
> +	if (vmx->lbr_desc.records.nr &&
> +	    (vcpu->arch.perf_capabilities & PMU_CAP_LBR_FMT))

That doesn't do the right thing if X86_FEATURE_PDCM is cleared in guest CPUID.
It doesn't even require odd userspace behavior since intel_pmu_init() does:

	vcpu->arch.perf_capabilities = vmx_get_perf_capabilities();

E.g. older userspace that doesn't set MSR_IA32_PERF_CAPABILITIES will clear PDCM
without touching the vCPU's MSR value.

In the unlikely scenario we can't figure out a solution for PERF_CAPABILITIES,
the alternative I tried first is to implement a generic CPUID feature "caching"
scheme and use it to expedite the PDCM lookup.  I scrapped that approach when I
realized that KVM really should be able to consume PERF_CAPABILITIES during PMU
refresh.

I'm hesitant to even suggest a generic caching implementation because I suspect 
most performance critical uses of guest CPUID will be similar to PDMC, i.e. can
be captured during KVM_SET_CPUID2 without requiring an explicit cache.  And for
PERF_CAPABILITIES, IMO a robust implementation is a must have, i.e. we've failed
if we can't handle it during PMU refresh.

[-- Attachment #2: 0001-KVM-x86-Add-CPUID-cache-for-frequent-X86_FEATURE_-gu.patch --]
[-- Type: text/x-diff, Size: 3793 bytes --]

From 2b5621e0a504c821125d24475ee83d9e1cf24e96 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 26 Jul 2022 09:20:37 -0700
Subject: [PATCH 1/2] KVM: x86: Add CPUID cache for frequent X86_FEATURE_*
 guest lookups

Implement a small "cache" for expediting guest_cpuid_has() lookups of
frequently used features.  Guest CPUID lookups are slow, especially if
the associated leaf has no entry, as KVM uses a linear walk of all CPUID
entries to find the associated leaf, e.g. adding a guest_cpuid_has()
lookup in the VM-Enter path is slow enough that it shows up on perf
traces.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/cpuid.c            |  9 +++++++++
 arch/x86/kvm/cpuid.h            | 28 ++++++++++++++++++++++++++--
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e8281d64a431..8cdb5c46815d 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -759,6 +759,7 @@ struct kvm_vcpu_arch {
 	int cpuid_nent;
 	struct kvm_cpuid_entry2 *cpuid_entries;
 	u32 kvm_cpuid_base;
+	u32 kvm_cpuid_x86_feature_cache;
 
 	u64 reserved_gpa_bits;
 	int maxphyaddr;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 75dcf7a72605..27b25fdb4335 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -377,6 +377,12 @@ u64 kvm_vcpu_reserved_gpa_bits_raw(struct kvm_vcpu *vcpu)
 	return rsvd_bits(cpuid_maxphyaddr(vcpu), 63);
 }
 
+#define kvm_cpuid_cache_update(__vcpu, x86_feature)						\
+do {												\
+	if (__guest_cpuid_has(__vcpu, x86_feature))						\
+		(__vcpu)->arch.kvm_cpuid_x86_feature_cache |= BIT(KVM_CACHED_ ## x86_feature);	\
+} while (0)
+
 static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
                         int nent)
 {
@@ -412,6 +418,9 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 	vcpu->arch.cpuid_entries = e2;
 	vcpu->arch.cpuid_nent = nent;
 
+	/* Update the cache before doing anything else. */
+	vcpu->arch.kvm_cpuid_x86_feature_cache = 0;
+
 	kvm_update_kvm_cpuid_base(vcpu);
 	kvm_vcpu_after_set_cpuid(vcpu);
 
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index b1658c0de847..49009d16022a 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -85,8 +85,21 @@ static __always_inline u32 *guest_cpuid_get_register(struct kvm_vcpu *vcpu,
 	return __cpuid_entry_get_reg(entry, cpuid.reg);
 }
 
-static __always_inline bool guest_cpuid_has(struct kvm_vcpu *vcpu,
-					    unsigned int x86_feature)
+enum kvm_cpuid_cached_feature {
+	NR_KVM_CACHED_X86_FEATURES,
+};
+
+static __always_inline int guest_cpuid_get_cache_bit(unsigned int x86_feature)
+{
+	int cache_bytes = sizeof((struct kvm_vcpu_arch *)0)->kvm_cpuid_x86_feature_cache;
+
+	BUILD_BUG_ON(NR_KVM_CACHED_X86_FEATURES > cache_bytes * BITS_PER_BYTE);
+
+	return NR_KVM_CACHED_X86_FEATURES;
+}
+
+static __always_inline bool __guest_cpuid_has(struct kvm_vcpu *vcpu,
+					      unsigned int x86_feature)
 {
 	u32 *reg;
 
@@ -97,6 +110,17 @@ static __always_inline bool guest_cpuid_has(struct kvm_vcpu *vcpu,
 	return *reg & __feature_bit(x86_feature);
 }
 
+static __always_inline bool guest_cpuid_has(struct kvm_vcpu *vcpu,
+					    unsigned int x86_feature)
+{
+	int cache_bit = guest_cpuid_get_cache_bit(x86_feature);
+
+	if (cache_bit != NR_KVM_CACHED_X86_FEATURES)
+		return vcpu->arch.kvm_cpuid_x86_feature_cache & BIT(cache_bit);
+
+	return __guest_cpuid_has(vcpu, x86_feature);
+}
+
 static __always_inline void guest_cpuid_clear(struct kvm_vcpu *vcpu,
 					      unsigned int x86_feature)
 {

base-commit: 1a4d88a361af4f2e91861d632c6a1fe87a9665c2
-- 
2.37.1.455.g008518b4e5-goog


[-- Attachment #3: 0002-KVM-x86-Cache-guest-CPUID-s-PDMC-for-fast-lookup.patch --]
[-- Type: text/x-diff, Size: 1767 bytes --]

From fd2d041407ce8c3c8c643d9d64c63a8a05ba85af Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 26 Jul 2022 09:37:49 -0700
Subject: [PATCH 2/2] KVM: x86: Cache guest CPUID's PDMC for fast lookup

Add a cache entry for X86_FEATURE_PDCM to allow for expedited lookups.
For all intents and purposes, VMX queries X86_FEATURE_PDCM by default on
every VM-Enter.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c | 1 +
 arch/x86/kvm/cpuid.h | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 27b25fdb4335..fd32fddd7bc1 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -420,6 +420,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 
 	/* Update the cache before doing anything else. */
 	vcpu->arch.kvm_cpuid_x86_feature_cache = 0;
+	kvm_cpuid_cache_update(vcpu, X86_FEATURE_PDCM);
 
 	kvm_update_kvm_cpuid_base(vcpu);
 	kvm_vcpu_after_set_cpuid(vcpu);
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 49009d16022a..65114cf7742e 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -86,6 +86,7 @@ static __always_inline u32 *guest_cpuid_get_register(struct kvm_vcpu *vcpu,
 }
 
 enum kvm_cpuid_cached_feature {
+	KVM_CACHED_X86_FEATURE_PDCM,
 	NR_KVM_CACHED_X86_FEATURES,
 };
 
@@ -95,6 +96,10 @@ static __always_inline int guest_cpuid_get_cache_bit(unsigned int x86_feature)
 
 	BUILD_BUG_ON(NR_KVM_CACHED_X86_FEATURES > cache_bytes * BITS_PER_BYTE);
 
+	/* Use a "dumb" if statement, this is all resolved at compile time. */
+	if (x86_feature == X86_FEATURE_PDCM)
+		return KVM_CACHED_X86_FEATURE_PDCM;
+
 	return NR_KVM_CACHED_X86_FEATURES;
 }
 
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH 3/3] KVM: VMX: Adjust number of LBR records for PERF_CAPABILITIES at refresh

[Like Xu]

On 28/7/2022 7:34 am, Sean Christopherson wrote:
> -bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu)
> -{
> -	struct x86_pmu_lbr *lbr = vcpu_to_lbr_records(vcpu);
> -
> -	return lbr->nr && (vcpu_get_perf_capabilities(vcpu) & PMU_CAP_LBR_FMT);
> -}
> -
>   static bool intel_pmu_is_valid_lbr_msr(struct kvm_vcpu *vcpu, u32 index)
>   {
>   	struct x86_pmu_lbr *records = vcpu_to_lbr_records(vcpu);
> @@ -590,7 +583,9 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>   	bitmap_set(pmu->all_valid_pmc_idx,
>   		INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
>   
> -	if (cpuid_model_is_consistent(vcpu))
> +	perf_capabilities = vcpu_get_perf_capabilities(vcpu);
> +	if (cpuid_model_is_consistent(vcpu) &&
> +	    (perf_capabilities & PMU_CAP_LBR_FMT))
>   		x86_perf_get_lbr(&lbr_desc->records);

As one of evil source to add CPUID walk in the critical path:

The x86_perf_get_lbr() is one of the perf interfaces, KVM cannot always trust
that the number of returned lbr_desc->records.nr is always > 0,  and if not,
we have to tweak perf_capabilities inside KVM which violates user input again.

Do you have more inputs to address this issue ?

>   	else
>   		lbr_desc->records.nr = 0;

Re: [PATCH 3/3] KVM: VMX: Adjust number of LBR records for PERF_CAPABILITIES at refresh

[Sean Christopherson]

On Tue, Aug 02, 2022, Like Xu wrote:
> On 28/7/2022 7:34 am, Sean Christopherson wrote:
> > -bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu)
> > -{
> > -	struct x86_pmu_lbr *lbr = vcpu_to_lbr_records(vcpu);
> > -
> > -	return lbr->nr && (vcpu_get_perf_capabilities(vcpu) & PMU_CAP_LBR_FMT);
> > -}
> > -
> >   static bool intel_pmu_is_valid_lbr_msr(struct kvm_vcpu *vcpu, u32 index)
> >   {
> >   	struct x86_pmu_lbr *records = vcpu_to_lbr_records(vcpu);
> > @@ -590,7 +583,9 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >   	bitmap_set(pmu->all_valid_pmc_idx,
> >   		INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
> > -	if (cpuid_model_is_consistent(vcpu))
> > +	perf_capabilities = vcpu_get_perf_capabilities(vcpu);
> > +	if (cpuid_model_is_consistent(vcpu) &&
> > +	    (perf_capabilities & PMU_CAP_LBR_FMT))
> >   		x86_perf_get_lbr(&lbr_desc->records);
> 
> As one of evil source to add CPUID walk in the critical path:
> 
> The x86_perf_get_lbr() is one of the perf interfaces, KVM cannot always trust
> that the number of returned lbr_desc->records.nr is always > 0,  and if not,
> we have to tweak perf_capabilities inside KVM which violates user input again.
> 
> Do you have more inputs to address this issue ?

First, drop the unnecessary stub and return value from x86_perf_get_lbr().  KVM
selects PERF_EVENTS, so the stub and thus error path can't be hit.  I'll add
patches to the series to do this.

Second, check the number of perf LBRs in vmx_get_perf_capabilities() and advertise
PMU_CAP_LBR_FMT iff perf fully supports LBRs.

---
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 2 Aug 2022 07:45:33 -0700
Subject: [PATCH] KVM: VMX: Advertise PMU LBRs if and only if perf supports
 LBRs

Advertise LBR support to userspace via MSR_IA32_PERF_CAPABILITIES if and
only if perf fully supports LBRs.  Perf may disable LBRs (by zeroing the
number of LBRs) even on platforms the allegedly support LBRs, e.g. if
probing any LBR MSRs during setup fails.

Fixes: be635e34c284 ("KVM: vmx/pmu: Expose LBR_FMT in the MSR_IA32_PERF_CAPABILITIES")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/capabilities.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
index c5e5dfef69c7..d2fdaf888d33 100644
--- a/arch/x86/kvm/vmx/capabilities.h
+++ b/arch/x86/kvm/vmx/capabilities.h
@@ -404,6 +404,7 @@ static inline bool vmx_pebs_supported(void)
 static inline u64 vmx_get_perf_capabilities(void)
 {
 	u64 perf_cap = PMU_CAP_FW_WRITES;
+	struct x86_pmu_lbr lbr;
 	u64 host_perf_cap = 0;

 	if (!enable_pmu)
@@ -412,7 +413,9 @@ static inline u64 vmx_get_perf_capabilities(void)
 	if (boot_cpu_has(X86_FEATURE_PDCM))
 		rdmsrl(MSR_IA32_PERF_CAPABILITIES, host_perf_cap);

-	perf_cap |= host_perf_cap & PMU_CAP_LBR_FMT;
+	x86_perf_get_lbr(&lbr);
+	if (lbr.nr)
+		perf_cap |= host_perf_cap & PMU_CAP_LBR_FMT;

 	if (vmx_pebs_supported()) {
 		perf_cap |= host_perf_cap & PERF_CAP_PEBS_MASK;

base-commit: 1f011a0755c2135b035cdee3b54e3adc426ec95c
--

Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES

[Like Xu]

On 28/7/2022 7:34 am, Sean Christopherson wrote:
> Refresh the PMU if userspace modifies MSR_IA32_PERF_CAPABILITIES.  KVM
> consumes the vCPU's PERF_CAPABILITIES when enumerating PEBS support, but
> relies on CPUID updates to refresh the PMU.  I.e. KVM will do the wrong
> thing if userspace stuffs PERF_CAPABILITIES _after_ setting guest CPUID.
> 
> Opportunistically fix a curly-brace indentation.
> 
> Fixes: c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS")

Shouldn't it be: Fixes: 27461da31089 ("KVM: x86/pmu: Support full width counting") ?

Now, all the dots have been connected. As punishment, I'd like to cook this 
patch set more
with trackable tests so that you have more time for other things that are not 
housekeeping.

Thank you, Sean.

> Cc: Like Xu <like.xu.linux@gmail.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/x86.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5366f884e9a7..362c538285db 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3543,9 +3543,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   			return 1;
>   
>   		vcpu->arch.perf_capabilities = data;
> -
> +		kvm_pmu_refresh(vcpu);
>   		return 0;
> -		}
> +	}
>   	case MSR_EFER:
>   		return set_efer(vcpu, msr_info);
>   	case MSR_K7_HWCR:

Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES

[Sean Christopherson]

On Wed, Aug 03, 2022, Like Xu wrote:
> On 28/7/2022 7:34 am, Sean Christopherson wrote:
> > Refresh the PMU if userspace modifies MSR_IA32_PERF_CAPABILITIES.  KVM
> > consumes the vCPU's PERF_CAPABILITIES when enumerating PEBS support, but
> > relies on CPUID updates to refresh the PMU.  I.e. KVM will do the wrong
> > thing if userspace stuffs PERF_CAPABILITIES _after_ setting guest CPUID.
> > 
> > Opportunistically fix a curly-brace indentation.
> > 
> > Fixes: c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS")
> 
> Shouldn't it be: Fixes: 27461da31089 ("KVM: x86/pmu: Support full width counting") ?

Strictly speaking, I don't think so?  fw_writes_is_enabled() returns false if
guest CPUID doesn't have X86_FEATURE_PDCM, and AFAICT there are no other side
effects that are handled by intel_pmu_refresh().

> Now, all the dots have been connected. As punishment, I'd like to cook this
> patch set more with trackable tests so that you have more time for other
> things that are not housekeeping.

Let me post v2, I've already done all the work and testing.  If there's more to
be done, we can figure out next steps from there.

Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES

[Sean Christopherson]

On Wed, Aug 03, 2022, Sean Christopherson wrote:
> On Wed, Aug 03, 2022, Like Xu wrote:
> > Now, all the dots have been connected. As punishment, I'd like to cook this
> > patch set more with trackable tests so that you have more time for other
> > things that are not housekeeping.
> 
> Let me post v2, I've already done all the work and testing.  If there's more to
> be done, we can figure out next steps from there.

I partially take that back, I forgot about the "disallow writes to feature MSRs
after KVM_RUN".  I haven't addressed that and don't have bandwidth to work on it
for the foreseeable future.  If you can address the issue, that'd be awesome.

I'm still going to post v2, all of the PMU fixes/cleanups are valid regardless of
the KVM_RUN issue, i.e. they can go in sooner to fix slightly less theoretical
problems (I doubt there's a userspace that actually changes PERF_CAPABILITIES
after KVM_RUN).  I'll note in the changelog that KVM should disallow changing
feature MSRs after KVM_RUN.

Re: [PATCH 0/3] KVM: x86: Intel PERF_CAPABILITIES fix and cleanups

[Paolo Bonzini]

Queued, thanks.  The patches have been out for almost two weeks and,
based on the thread, any other cleanups/fixes can be one on top
(particularly the feature MSR one).

Paolo