Re: [PATCH v3 7/9] KVM: vmx/pmu: Add Arch LBR emulation and its VMCS field

From: Sean Christopherson <seanjc@google.com>
To: "Xu, Like" <like.xu@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	Kan Liang <kan.liang@linux.intel.com>,
	Dave Hansen <dave.hansen@intel.com>,
	wei.w.wang@intel.com, Borislav Petkov <bp@alien8.de>,
	kvm@vger.kernel.org, x86@kernel.org,
	linux-kernel@vger.kernel.org, Like Xu <like.xu@linux.intel.com>
Subject: Re: [PATCH v3 7/9] KVM: vmx/pmu: Add Arch LBR emulation and its VMCS field
Date: Thu, 4 Mar 2021 09:23:53 -0800	[thread overview]
Message-ID: <YEEXqf3b4uaSdNKv@google.com> (raw)
In-Reply-To: <267c408c-6999-649b-d733-8d64f9cf0594@intel.com>

On Thu, Mar 04, 2021, Xu, Like wrote:
> On 2021/3/4 1:26, Sean Christopherson wrote:
> > On Wed, Mar 03, 2021, Like Xu wrote:
> > > New VMX controls bits for Arch LBR are added. When bit 21 in vmentry_ctrl
> > > is set, VM entry will write the value from the "Guest IA32_LBR_CTL" guest
> > > state field to IA32_LBR_CTL. When bit 26 in vmexit_ctrl is set, VM exit
> > > will clear IA32_LBR_CTL after the value has been saved to the "Guest
> > > IA32_LBR_CTL" guest state field.
> > ...
> > 
> > > @@ -2529,7 +2532,8 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
> > >   	      VM_EXIT_LOAD_IA32_EFER |
> > >   	      VM_EXIT_CLEAR_BNDCFGS |
> > >   	      VM_EXIT_PT_CONCEAL_PIP |
> > > -	      VM_EXIT_CLEAR_IA32_RTIT_CTL;
> > > +	      VM_EXIT_CLEAR_IA32_RTIT_CTL |
> > > +	      VM_EXIT_CLEAR_IA32_LBR_CTL;
> > So, how does MSR_ARCH_LBR_CTL get restored on the host?  What if the host wants
> > to keep _its_ LBR recording active while the guest is running?
> 
> Thank you!
> 
> I will add "host_lbrctlmsr" field to "struct vcpu_vmx" and
> repeat the update/get_debugctlmsr() stuff.

I am not remotely confident that tracking LBRCTL via vcpu_vmx is correct, and
I'm far less confident that the existing DEBUGCTL logic is correct.  As Jim
pointed out[*], intel_pmu_handle_irq() can run at any time, and it's not at all
clear to me that the DEBUGCTL coming out of the NMI handler is guaranteed to be
the same value going in.  Ditto for LBRCTL.

Actually, NMIs aside, KVM's DEBUGCTL handling is provably broken since writing
/sys/devices/cpu/freeze_on_smi is propagated to other CPUs via IRQ, and KVM
snapshots DEBUCTL on vCPU load, i.e. runs with IRQs enabled long after grabbing
the value.

  WARNING: CPU: 5 PID: 0 at arch/x86/events/intel/core.c:4066 flip_smm_bit+0xb/0x30
  RIP: 0010:flip_smm_bit+0xb/0x30
  Call Trace:
   <IRQ>
   flush_smp_call_function_queue+0x118/0x1a0
   __sysvec_call_function+0x2c/0x90
   asm_call_irq_on_stack+0x12/0x20
   </IRQ>

So, rather than pile on more MSR handling that is at best dubious, and at worst
broken, I would like to see KVM properly integrate with perf to ensure KVM
restores the correct, fresh values of all MSRs that are owned by perf.  Or at
least add something that guarantees that intel_pmu_handle_irq() preserves the
MSRs.  As is, it's impossible to review these KVM changes without deep, deep
knowledge of what perf is doing.

https://lkml.kernel.org/r/20210209225653.1393771-1-jmattson@google.com