Re: [PATCH v12 07/11] KVM: vmx/pmu: Unmask LBR fields in the MSR_IA32_DEBUGCTLMSR emualtion

[Xiaoyao Li]

On 7/8/2020 4:21 AM, Sean Christopherson wrote:
> On Sat, Jun 13, 2020 at 05:42:50PM +0800, Xu, Like wrote:
> > On 2020/6/13 17:14, Xiaoyao Li wrote:
> > > On 6/13/2020 4:09 PM, Like Xu wrote:
[...]
> > > > @@ -237,6 +238,9 @@ static int intel_pmu_get_msr(struct kvm_vcpu *vcpu,
> > > > struct msr_data *msr_info)
> > > >  ÂÂÂÂÂÂÂÂÂÂÂÂÂ return 1;
> > > >  ÂÂÂÂÂÂÂÂÂ msr_info->data = vcpu->arch.perf_capabilities;
> > > >  ÂÂÂÂÂÂÂÂÂ return 0;
> > > > +ÂÂÂ case MSR_IA32_DEBUGCTLMSR:
> > > > +ÂÂÂÂÂÂÂ msr_info->data = vmcs_read64(GUEST_IA32_DEBUGCTL);
> > >
> > > Can we put the emulation of MSR_IA32_DEBUGCTLMSR in vmx_{get/set})_msr().
> > > AFAIK, MSR_IA32_DEBUGCTLMSR is not a pure PMU related MSR that there is
> > > bit 2 to enable #DB for bus lock.
> > We already have "case MSR_IA32_DEBUGCTLMSR" handler in the vmx_set_msr()
> > and you may apply you bus lock changes in that handler.
>
> Hrm, but that'd be weird dependency as vmx_set_msr() would need to check for
> #DB bus lock support but not actually write GUEST_IA32_DEBUGCTL, or we'd end
> up writing it twice when both bus lock and LBR are supported.

Yeah. That's what I concerned as well.

> I don't see anything in the series that takes action on writes to
> MSR_IA32_DEBUGCTLMSR beyond updating the VMCS, i.e. AFAICT there isn't any
> reason to call into the PMU, VMX can simply query vmx_get_perf_capabilities()
> to check if it's legal to enable DEBUGCTLMSR_LBR_MASK.
>
> A question for both LBR and bus lock: would it make sense to cache the
> guest's value in vcpu_vmx so that querying the guest value doesn't require
> a VMREAD?  I don't have a good feel for how frequently it would be accessed.

Cache the guest's value is OK, even though #DB bus lock bit wouldn't be 
toggled frequently in a normal OS.