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

[Yang Weijiang]

On Mon, Aug 09, 2021 at 09:36:49PM +0800, Like Xu wrote:
> On 6/8/2021 3:42 pm, Yang Weijiang wrote:
> >From: Like Xu <like.xu@xxxxxxxxxxxxxxx>
> >
> >Arch LBRs are enabled by setting MSR_ARCH_LBR_CTL.LBREn to 1. A new guest
> >state field named "Guest IA32_LBR_CTL" is added to enhance guest LBR usage.
> >When guest Arch LBR is enabled, a guest LBR event will be created like the
> >model-specific LBR does. Clear guest LBR enable bit on host PMI handling so
> >guest can see expected config.
> >
> >On processors that support Arch LBR, MSR_IA32_DEBUGCTLMSR[bit 0] has no
> >meaning. It can be written to 0 or 1, but reads will always return 0.
> >Like IA32_DEBUGCTL, IA32_ARCH_LBR_CTL msr is also preserved on INIT.
> >
> >Regardless of the Arch LBR or legacy LBR, when the LBR_EN bit 0 of the
> >corresponding control MSR is set to 1, LBR recording will be enabled.
> >
> >Signed-off-by: Like Xu <like.xu@xxxxxxxxxxxxxxx>
> >Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> >---
> >  arch/x86/events/intel/lbr.c      |  2 --
> >  arch/x86/include/asm/msr-index.h |  1 +
> >  arch/x86/include/asm/vmx.h       |  2 ++
> >  arch/x86/kvm/vmx/pmu_intel.c     | 48 ++++++++++++++++++++++++++++----
> >  arch/x86/kvm/vmx/vmx.c           |  9 ++++++
> >  5 files changed, 55 insertions(+), 7 deletions(-)
> >

[...]

> >+static bool arch_lbr_ctl_is_valid(struct kvm_vcpu *vcpu, u64 ctl)
> >+{
> >+	unsigned int eax, ebx, ecx, edx;
> >+
> >+	if (!kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
> >+		return false;
> >+
> >+	cpuid_count(0x1c, 0, &eax, &ebx, &ecx, &edx);
> >+	if (!(ebx & BIT(0)) && (ctl & ARCH_LBR_CTL_CPL))
> >+		return false;
> >+	if (!(ebx & BIT(2)) && (ctl & ARCH_LBR_CTL_STACK))
> >+		return false;
> >+	if (!(ebx & BIT(1)) && (ctl & ARCH_LBR_CTL_BRN_MASK))
> >+		return false;
> >+
> >+	return !(ctl & ~KVM_ARCH_LBR_CTL_MASK);
> >+}
> 
> Please check it with the *guest* cpuid entry.
If KVM "trusts" user-space, then check with guest cpuid is OK.
But if user-space enable excessive controls, then check against guest
cpuid could make things mess.

> 
> And it should remove the bits that are not supported by x86_pmu.lbr_ctl_mask before
> vmcs_write64(...) if the guest value is a superset of the host value with
> warning message.
Then I think it makes more sense to check against x86_pmu.lbr_xxx masks in above function
for compatibility. What do you think of it?
> 
> >+
> >  static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  {
> >  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> >@@ -392,6 +414,9 @@ static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  	case MSR_ARCH_LBR_DEPTH:
> >  		msr_info->data = lbr_desc->records.nr;
> >  		return 0;
> >+	case MSR_ARCH_LBR_CTL:
> >+		msr_info->data = vmcs_read64(GUEST_IA32_LBR_CTL);
> >+		return 0;
> >  	default:
> >  		if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) ||
> >  		    (pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) {
> >  		vmcs_write64(GUEST_IA32_DEBUGCTL, data);

[...]

> >  		if (intel_pmu_lbr_is_enabled(vcpu) && !to_vmx(vcpu)->lbr_desc.event &&
> >  		    (data & DEBUGCTLMSR_LBR))
> >@@ -4441,6 +4448,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> >  		vmcs_writel(GUEST_SYSENTER_ESP, 0);
> >  		vmcs_writel(GUEST_SYSENTER_EIP, 0);
> >  		vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
> >+		if (static_cpu_has(X86_FEATURE_ARCH_LBR))
> >+			vmcs_write64(GUEST_IA32_LBR_CTL, 0);
> 
> Please update dump_vmcs() to dump GUEST_IA32_LBR_CTL as well.
OK, will add it.
> 
> How about update the load_vmcs12_host_state() for GUEST_IA32_LBR_CTL
> since you enabled the nested case in this patch set ?
No, I didn't enable nested Arch LBR but unblocked some issues for nested case.
> 
> >  	}
> >  	kvm_set_rflags(vcpu, X86_EFLAGS_FIXED);
> >