Re: [PATCH v13 021/113] KVM: TDX: Make pmu_intel.c ignore guest TD case

[Like Xu <like.xu.linux@xxxxxxxxx>]

On 2/4/2023 4:50 pm, Zhi Wang wrote:
> Hi Like:
>
> Would you mind to take a look on this patch? It would be nice to have
> a r-b also from you. :)
>
> On Sun, 12 Mar 2023 10:55:45 -0700
> isaku.yamahata@xxxxxxxxx wrote:
>
> > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> >
> > Because TDX KVM doesn't support PMU yet (it's future work of TDX KVM
> > support as another patch series) and pmu_intel.c touches vmx specific

It would be nice to have pmu support for tdx-guest from the very beginning.
If you guys are more open on the tdx development model, I'd like to help on 
those features.

> > structure in vcpu initialization, as workaround add dummy structure to
> > struct vcpu_tdx and pmu_intel.c can ignore TDX case.

If the target is not to provide a workaround, how about other variants:
	- struct lbr_desc lbr_desc;
	- pebs ds_buffer;
?

We also need tdx selftest to verify the unavailability of these features.
Also, it would be great to have TDX's "System Profiling Mode" featue back in the 
specification.

> > Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > ---
> >   arch/x86/kvm/vmx/pmu_intel.c | 46 +++++++++++++++++++++++++++++++++++-
> >   arch/x86/kvm/vmx/pmu_intel.h | 28 ++++++++++++++++++++++
> >   arch/x86/kvm/vmx/tdx.h       |  8 ++++++-
> >   arch/x86/kvm/vmx/vmx.c       |  2 +-
> >   arch/x86/kvm/vmx/vmx.h       | 32 +------------------------
> >   5 files changed, 82 insertions(+), 34 deletions(-)
> >   create mode 100644 arch/x86/kvm/vmx/pmu_intel.h
> >
> > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> > index e8a3be0b9df9..df1f4ddfa72d 100644
> > --- a/arch/x86/kvm/vmx/pmu_intel.c
> > +++ b/arch/x86/kvm/vmx/pmu_intel.c
> > @@ -19,6 +19,7 @@
> >   #include "lapic.h"
> >   #include "nested.h"
> >   #include "pmu.h"
> > +#include "tdx.h"
> >   
> >   #define MSR_PMC_FULL_WIDTH_BIT      (MSR_IA32_PMC0 - MSR_IA32_PERFCTR0)
> >   
> > @@ -40,6 +41,26 @@ static struct {
> >   /* mapping between fixed pmc index and intel_arch_events array */
> >   static int fixed_pmc_events[] = {1, 0, 7};
> >   
> > +struct lbr_desc *vcpu_to_lbr_desc(struct kvm_vcpu *vcpu)
> > +{
> > +#ifdef CONFIG_INTEL_TDX_HOST
> > +	if (is_td_vcpu(vcpu))
> > +		return &to_tdx(vcpu)->lbr_desc;
> > +#endif
> > +
> > +	return &to_vmx(vcpu)->lbr_desc;
> > +}
> > +
> > +struct x86_pmu_lbr *vcpu_to_lbr_records(struct kvm_vcpu *vcpu)
> > +{
> > +#ifdef CONFIG_INTEL_TDX_HOST
> > +	if (is_td_vcpu(vcpu))
> > +		return &to_tdx(vcpu)->lbr_desc.records;
> > +#endif
> > +
> > +	return &to_vmx(vcpu)->lbr_desc.records;
> > +}
> > +
> >   static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
> >   {
> >   	struct kvm_pmc *pmc;
> > @@ -172,6 +193,23 @@ 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_compatible(struct kvm_vcpu *vcpu)
> > +{
> > +	if (is_td_vcpu(vcpu))
> > +		return false;
> > +	return cpuid_model_is_consistent(vcpu);
> > +}
> > +
> > +bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu)
> > +{
> > +	struct x86_pmu_lbr *lbr = vcpu_to_lbr_records(vcpu);
> > +
> > +	if (is_td_vcpu(vcpu))
> > +		return false;
> > +
> > +	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);
> > @@ -282,6 +320,9 @@ int intel_pmu_create_guest_lbr_event(struct kvm_vcpu *vcpu)
> >   					PERF_SAMPLE_BRANCH_USER,
> >   	};
> >   
> > +	if (WARN_ON_ONCE(is_td_vcpu(vcpu)))
> > +		return 0;
> > +
> >   	if (unlikely(lbr_desc->event)) {
> >   		__set_bit(INTEL_PMC_IDX_FIXED_VLBR, pmu->pmc_in_use);
> >   		return 0;
> > @@ -591,7 +632,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> >   		INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
> >   
> >   	perf_capabilities = vcpu_get_perf_capabilities(vcpu);
> > -	if (cpuid_model_is_consistent(vcpu) &&
> > +	if (intel_pmu_lbr_is_compatible(vcpu) &&
> >   	    (perf_capabilities & PMU_CAP_LBR_FMT))
> >   		x86_perf_get_lbr(&lbr_desc->records);
> >   	else
> > @@ -647,6 +688,9 @@ static void intel_pmu_reset(struct kvm_vcpu *vcpu)
> >   	struct kvm_pmc *pmc = NULL;
> >   	int i;
> >   
> > +	if (is_td_vcpu(vcpu))
> > +		return;
> > +
> >   	for (i = 0; i < KVM_INTEL_PMC_MAX_GENERIC; i++) {
> >   		pmc = &pmu->gp_counters[i];
> >   
> > diff --git a/arch/x86/kvm/vmx/pmu_intel.h b/arch/x86/kvm/vmx/pmu_intel.h
> > new file mode 100644
> > index 000000000000..66bba47c1269
> > --- /dev/null
> > +++ b/arch/x86/kvm/vmx/pmu_intel.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __KVM_X86_VMX_PMU_INTEL_H
> > +#define  __KVM_X86_VMX_PMU_INTEL_H
> > +
> > +struct lbr_desc *vcpu_to_lbr_desc(struct kvm_vcpu *vcpu);
> > +struct x86_pmu_lbr *vcpu_to_lbr_records(struct kvm_vcpu *vcpu);
> > +
> > +bool intel_pmu_lbr_is_compatible(struct kvm_vcpu *vcpu);
> > +bool intel_pmu_lbr_is_enabled(struct kvm_vcpu *vcpu);
> > +int intel_pmu_create_guest_lbr_event(struct kvm_vcpu *vcpu);
> > +
> > +struct lbr_desc {
> > +	/* Basic info about guest LBR records. */
> > +	struct x86_pmu_lbr records;
> > +
> > +	/*
> > +	 * Emulate LBR feature via passthrough LBR registers when the
> > +	 * per-vcpu guest LBR event is scheduled on the current pcpu.
> > +	 *
> > +	 * The records may be inaccurate if the host reclaims the LBR.
> > +	 */
> > +	struct perf_event *event;
> > +
> > +	/* True if LBRs are marked as not intercepted in the MSR bitmap */
> > +	bool msr_passthrough;
> > +};
> > +
> > +#endif /* __KVM_X86_VMX_PMU_INTEL_H */
> > diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> > index 1e00e75b1c5e..5728820fed5e 100644
> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -4,6 +4,7 @@
> >   
> >   #ifdef CONFIG_INTEL_TDX_HOST
> >   
> > +#include "pmu_intel.h"
> >   #include "tdx_ops.h"
> >   
> >   struct kvm_tdx {
> > @@ -21,7 +22,12 @@ struct kvm_tdx {
> >   
> >   struct vcpu_tdx {
> >   	struct kvm_vcpu	vcpu;
> > -	/* TDX specific members follow. */
> > +
> > +	/*
> > +	 * Dummy to make pmu_intel not corrupt memory.
> > +	 * TODO: Support PMU for TDX.  Future work.
> > +	 */
> > +	struct lbr_desc lbr_desc;
> >   };
> >   
> >   static inline bool is_td(struct kvm *kvm)
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index d23830d92f61..f9e9fd7fde2c 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -2432,7 +2432,7 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >   			if ((data & PMU_CAP_LBR_FMT) !=
> >   			    (kvm_caps.supported_perf_cap & PMU_CAP_LBR_FMT))
> >   				return 1;
> > -			if (!cpuid_model_is_consistent(vcpu))
> > +			if (!intel_pmu_lbr_is_compatible(vcpu))
> >   				return 1;
> >   		}
> >   		if (data & PERF_CAP_PEBS_FORMAT) {
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 2acdc54bc34b..1d15c3c2751b 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -11,6 +11,7 @@
> >   #include "capabilities.h"
> >   #include "../kvm_cache_regs.h"
> >   #include "posted_intr.h"
> > +#include "pmu_intel.h"
> >   #include "vmcs.h"
> >   #include "vmx_ops.h"
> >   #include "../cpuid.h"
> > @@ -105,22 +106,6 @@ static inline bool intel_pmu_has_perf_global_ctrl(struct kvm_pmu *pmu)
> >   	return pmu->version > 1;
> >   }
> >   
> > -struct lbr_desc {
> > -	/* Basic info about guest LBR records. */
> > -	struct x86_pmu_lbr records;
> > -
> > -	/*
> > -	 * Emulate LBR feature via passthrough LBR registers when the
> > -	 * per-vcpu guest LBR event is scheduled on the current pcpu.
> > -	 *
> > -	 * The records may be inaccurate if the host reclaims the LBR.
> > -	 */
> > -	struct perf_event *event;
> > -
> > -	/* True if LBRs are marked as not intercepted in the MSR bitmap */
> > -	bool msr_passthrough;
> > -};
> > -
> >   /*
> >    * The nested_vmx structure is part of vcpu_vmx, and holds information we need
> >    * for correct emulation of VMX (i.e., nested VMX) on this vcpu.
> > @@ -650,21 +635,6 @@ static __always_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;
> > -}
> > -
> > -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);
> >   int intel_pmu_create_guest_lbr_event(struct kvm_vcpu *vcpu);
> >   void vmx_passthrough_lbr_msrs(struct kvm_vcpu *vcpu);