On 1/28/2023 4:25 AM, Sean Christopherson wrote:
On Thu, Nov 24, 2022, Yang Weijiang wrote:
[...]
+++ b/arch/x86/include/asm/kvm_host.h
@@ -571,6 +571,9 @@ struct kvm_pmu {
* redundant check before cleanup if guest don't use vPMU at all.
*/
u8 event_count;
+
+ /* Guest arch lbr depth supported by KVM. */
+ u64 kvm_arch_lbr_depth;
There is zero reason to store this separately. KVM already records the allowed
depth in kvm_vcpu.lbr_desc.records.nr.
kvm_vcpu.lbr_desc.records.nr alone cannot tell whether it's legacy lbr or arch-lbr unless
binding host arch-lbr checking.
};
struct kvm_pmu_ops;
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 905673228932..0c78cb4b72be 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -178,6 +178,10 @@ static bool intel_pmu_is_valid_lbr_msr(struct kvm_vcpu *vcpu, u32 index)
(index == MSR_LBR_SELECT || index == MSR_LBR_TOS))
return true;
+ if (index == MSR_ARCH_LBR_DEPTH)
+ return kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR) &&
Like the previous patch, since intel_pmu_lbr_is_enabled() effectively serves as
a generic kvm_cpu_cap_has(LBRS) check, this can be distilled to:
if (cpu_feature_enabled(X86_FEATURE_ARCH_LBR)) {
if (index == MSR_ARCH_LBR_DEPTH || index == MSR_ARCH_LBR_CTL)
return true;
} else {
if (index == MSR_LBR_SELECT || index == MSR_LBR_TOS))
return true;
}
yes, exactly, thanks!
+ guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR);
+
if ((index >= records->from && index < records->from + records->nr) ||
(index >= records->to && index < records->to + records->nr))
return true;
@@ -345,6 +349,7 @@ static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
{
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
struct kvm_pmc *pmc;
+ struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
u32 msr = msr_info->index;
switch (msr) {
@@ -369,6 +374,9 @@ static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
case MSR_PEBS_DATA_CFG:
msr_info->data = pmu->pebs_data_cfg;
return 0;
+ case MSR_ARCH_LBR_DEPTH:
+ msr_info->data = lbr_desc->records.nr;
+ return 0;
default:
if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) ||
(pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) {
@@ -395,6 +403,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
{
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
struct kvm_pmc *pmc;
+ struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
u32 msr = msr_info->index;
u64 data = msr_info->data;
u64 reserved_bits, diff;
@@ -456,6 +465,24 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
return 0;
}
break;
+ case MSR_ARCH_LBR_DEPTH:
+ if (!pmu->kvm_arch_lbr_depth && !msr_info->host_initiated)
Don't invent a new check, just prevent KVM from reaching this path via the
existing intel_pmu_lbr_is_enabled().
intel_pmu_lbr_is_enabled() only indicates LBR is on(either legacy or
arch-lbr), but
MSR_ARCH_LBR_DEPTH is only for arch-lbr.
+ return 1;
+ /*
+ * When guest/host depth are different, the handling would be tricky,
+ * so only max depth is supported for both host and guest.
+ */
This semi-arbitrary restriction is fine because Intel's architecture allows KVM
to enumerate support for a single depth, but somewhere in the changelog and/or
code that actually needs to be state. This blurb
In the first generation of Arch LBR, max entry size is 32,
host configures the max size and guest always honors the setting.
makes it sound like KVM is relying on the guest to do the right thing, and this
code looks like KVM is making up it's own behavior.
Will modify the change log.
+ if (data != pmu->kvm_arch_lbr_depth)
+ return 1;
+
+ lbr_desc->records.nr = data;
+ /*
+ * Writing depth MSR from guest could either setting the
+ * MSR or resetting the LBR records with the side-effect.
+ */
+ if (kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
Another check, really? KVM shouldn't reach this point if KVM doesn't support
Arch LBRs. And if that isn't guarantee (honestly forgot what this series actually
proposed at this point), then that's a bug, full stop.
Right, this check is unnecessary.
+ wrmsrl(MSR_ARCH_LBR_DEPTH, lbr_desc->records.nr);
IIUC, this is subtly broken. Piecing together all of the undocumented bits, my
understanding is that arch LBRs piggyback KVM's existing LBR support, i.e. use a
"virtual" perf event.
Yes.
And like traditional LBR support, the host can steal control
of the LBRs in IRQ context by disabling the perf event via IPI. And since writes
to MSR_ARCH_LBR_DEPTH purge LBR records, this needs to be treated as if it were a
write to an LBR record, i.e. belongs in the IRQs disabled section of
intel_pmu_handle_lbr_msrs_access().
I assume you're referring to host events preempt guest events. In that
case, it's possible
guest operations interfere host events/data. But this series
implementation focus on
"guest only" mode, i.e., it sets {Load|Clear}_LBR_CTL at VM entry/exit,
that way, we don't
need to care about host preempt, the event data is saved/restored at
event sched_{out|in}.
If for some magical reason it's safe to access arch LBR MSRs without disabling IRQs
and confirming perf event ownership, I want to see a very detailed changelog
explaining exactly how that magic works.
Will change the commit log to explain more.
+ return 0;
default:
if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) ||
(pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) {
@@ -506,6 +533,32 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
}
}
+static bool cpuid_enable_lbr(struct kvm_vcpu *vcpu)
+{
+ struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
+ struct kvm_cpuid_entry2 *entry;
+ int depth_bit;
+
+ if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
+ return !static_cpu_has(X86_FEATURE_ARCH_LBR) &&
+ cpuid_model_is_consistent(vcpu);
+
+ pmu->kvm_arch_lbr_depth = 0;
+ if (!guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR))
+ return false;
+
+ entry = kvm_find_cpuid_entry(vcpu, 0x1C);
+ if (!entry)
+ return false;
+
+ depth_bit = fls(cpuid_eax(0x1C) & 0xff);
This is unnecessarily fragile. Get the LBR depth from perf, don't read CPUID and
assume perf will always configured the max depth.,
Make sense, will refactor the function in next version.
This enabling also belongs at the tail end of the series, i.e. KVM shouldn't let
userspace enable LBRs until all the support pieces are in place.
OK.
+ if ((entry->eax & 0xff) != (1 << (depth_bit - 1)))
+ return false;
+
+ pmu->kvm_arch_lbr_depth = depth_bit * 8;
+ return true;
+}
+
[...]
