Re: [PATCH v2 05/15] KVM: vmx/pmu: Emulate MSR_ARCH_LBR_DEPTH for guest Arch LBR

Yang, Weijiang
2023-01-30T11:46:57Z

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;
>> +}
>> + …