Re: [RFC PATCH v3 26/58] KVM: x86/pmu: Manage MSR interception for IA32_PERF_GLOBAL_CTRL

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 11/20/2024 2:16 AM, Sean Christopherson wrote:
> On Thu, Aug 01, 2024, Mingwei Zhang wrote:
>> ---
>>  arch/x86/include/asm/vmx.h |   1 +
>>  arch/x86/kvm/vmx/vmx.c     | 117 +++++++++++++++++++++++++++++++------
>>  arch/x86/kvm/vmx/vmx.h     |   3 +-
>>  3 files changed, 103 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
>> index d77a31039f24..5ed89a099533 100644
>> --- a/arch/x86/include/asm/vmx.h
>> +++ b/arch/x86/include/asm/vmx.h
>> @@ -106,6 +106,7 @@
>>  #define VM_EXIT_CLEAR_BNDCFGS                   0x00800000
>>  #define VM_EXIT_PT_CONCEAL_PIP			0x01000000
>>  #define VM_EXIT_CLEAR_IA32_RTIT_CTL		0x02000000
>> +#define VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL      0x40000000
> Please add a helper in capabilities.h:
>
> static inline bool cpu_has_save_perf_global_ctrl(void)
> {
> 	return vmcs_config.vmexit_ctrl & VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL;
> }

Sure.


>
>>  #define VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR	0x00036dff
>>  
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 339742350b7a..34a420fa98c5 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -4394,6 +4394,97 @@ static u32 vmx_pin_based_exec_ctrl(struct vcpu_vmx *vmx)
>>  	return pin_based_exec_ctrl;
>>  }
>>  
>> +static void vmx_set_perf_global_ctrl(struct vcpu_vmx *vmx)
> This is a misleading and inaccurate name.  It does far more than "set" PERF_GLOBAL_CTRL,
> it arguably doesn't ever "set" the MSR, and it gets the VMWRITE for the guest field
> wrong too.
>
>> +{
>> +	u32 vmentry_ctrl = vm_entry_controls_get(vmx);
>> +	u32 vmexit_ctrl = vm_exit_controls_get(vmx);
>> +	struct vmx_msrs *m;
>> +	int i;
>> +
>> +	if (cpu_has_perf_global_ctrl_bug() ||
> Note, cpu_has_perf_global_ctrl_bug() broken and needs to be purged:
> https://lore.kernel.org/all/20241119011433.1797921-1-seanjc@google.com
>
> Note #2, as mentioned earlier, the mediated PMU should take a hard depenency on
> the load/save controls.
>
> On to this code, it fails to enable the load/save controls, e.g. if userspace
> does KVM_SET_CPUID2 without a PMU, then KVM_SET_CPUID2 with a PMU.  In that case,
> KVM will fail to set the control bits, and will fallback to the slow MSR load/save
> lists.
>
> With all of the above and other ideas combined, something like so:
>
> 	bool set = enable_mediated_pmu && kvm_pmu_has_perf_global_ctrl();
>
> 	vm_entry_controls_changebit(vmx, VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL, set);
> 	vm_exit_controls_changebit(vmx,
> 				   VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL |
> 				   VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL, set);
>
>
> And I vote to put this in intel_pmu_refresh(); that avoids needing to figure out
> a name for the helper, while giving more flexibililty on the local variable name.
>
> Oh!  Definitely put it in intel_pmu_refresh(), because the RDPMC and MSR
> interception logic needs to be there.  E.g. toggling CPU_BASED_RDPMC_EXITING
> based solely on CPUID won't do the right thing if KVM ends up making the behavior
> depend on PERF_CAPABILITIES.
>
> Ditto for MSRs.  Though until my patch/series that drops kvm_pmu_refresh() from
> kvm_pmu_init() lands[*], trying to update MSR intercepts during refresh() will hit
> a NULL pointer deref as it's currently called before vmcs01 is allocated :-/
>
> I expect to land that series before mediated PMU, but I don't think it makes sense
> to take an explicit dependency for this series.  To fudge around the issue, maybe
> do this for the next version?
>
> static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> {
> 	__intel_pmu_refresh(vcpu);
>
> 	/*
> 	 * FIXME: Drop the MSR bitmap check if/when kvm_pmu_init() no longer
> 	 *        calls kvm_pmu_refresh(), i.e. when KVM refreshes the PMU only
> 	 *        after vmcs01 is allocated.
> 	 */
> 	if (to_vmx(vcpu)->vmcs01.msr_bitmap)
> 		intel_update_msr_intercepts(vcpu);
>
> 	vm_entry_controls_changebit(vmx, VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
> 				    enable_mediated_pmu && kvm_pmu_has_perf_global_ctrl());
>
> 	vm_exit_controls_changebit(vmx,
> 				   VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL |
> 				   VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL,
> 				   enable_mediated_pmu && kvm_pmu_has_perf_global_ctrl());
> }
>
> or with a local variable for "enable_mediated_pmu && kvm_pmu_has_perf_global_ctrl()".
> I can't come up with a decent name. :-)
>
> [*] https://lore.kernel.org/all/20240517173926.965351-10-seanjc@google.com

Sure. This looks better.


>
>> +	    !is_passthrough_pmu_enabled(&vmx->vcpu)) {
>> +		vmentry_ctrl &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
>> +		vmexit_ctrl &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
>> +		vmexit_ctrl &= ~VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL;
>> +	}
>> +
>> +	if (is_passthrough_pmu_enabled(&vmx->vcpu)) {
>> +		/*
>> +		 * Setup auto restore guest PERF_GLOBAL_CTRL MSR at vm entry.
>> +		 */
>> +		if (vmentry_ctrl & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL) {
>> +			vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL, 0);
> This incorrectly clobbers the guest's value.  A simple way to handle this is to
> always propagate writes to PERF_GLOBAL_CTRL to the VMCS, if the write is allowed
> and enable_mediated_pmu.  I.e. ensure GUEST_IA32_PERF_GLOBAL_CTRL is up-to-date
> regardless of whether or not it's configured to be loaded.  Then there's no need
> to write it here.

For mediated vPMU, I think we can move this into intel_pmu_refresh() as
well, and just after the vm_entry_controls_changebit() helpers. the value
should be supported gp counters mask which respects the behavior of
PERF_GLOBAL_CTRL reset.


>
>> +		} else {
>> +			m = &vmx->msr_autoload.guest;
>> +			i = vmx_find_loadstore_msr_slot(m, MSR_CORE_PERF_GLOBAL_CTRL);
>> +			if (i < 0) {
>> +				i = m->nr++;
>> +				vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, m->nr);
>> +			}
>> +			m->val[i].index = MSR_CORE_PERF_GLOBAL_CTRL;
>> +			m->val[i].value = 0;
>> +		}
>> +		/*
>> +		 * Setup auto clear host PERF_GLOBAL_CTRL msr at vm exit.
>> +		 */
>> +		if (vmexit_ctrl & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
>> +			vmcs_write64(HOST_IA32_PERF_GLOBAL_CTRL, 0);
> This should be unnecessary.  KVM should clear HOST_IA32_PERF_GLOBAL_CTRL in
> vmx_set_constant_host_state() if enable_mediated_pmu is true.  Arguably, it might
> make sense to clear it unconditionally, but with a comment explaining that it's
> only actually constant for the mediated PMU.

Sure. would move this clearing into vmx_set_constant_host_state(). Yeah, I
suppose it can be cleared unconditionally since
VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL bit never be set for legacy emulated vPMU.


>
> And if the mediated PMU requires the VMCS knobs, then all of the load/store list
> complexity goes away.
>
>>  static u32 vmx_vmentry_ctrl(void)
>>  {
>>  	u32 vmentry_ctrl = vmcs_config.vmentry_ctrl;
>> @@ -4401,17 +4492,10 @@ static u32 vmx_vmentry_ctrl(void)
>>  	if (vmx_pt_mode_is_system())
>>  		vmentry_ctrl &= ~(VM_ENTRY_PT_CONCEAL_PIP |
>>  				  VM_ENTRY_LOAD_IA32_RTIT_CTL);
>> -	/*
>> -	 * IA32e mode, and loading of EFER and PERF_GLOBAL_CTRL are toggled dynamically.
>> -	 */
>> -	vmentry_ctrl &= ~(VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL |
>> -			  VM_ENTRY_LOAD_IA32_EFER |
>> -			  VM_ENTRY_IA32E_MODE);
>> -
>> -	if (cpu_has_perf_global_ctrl_bug())
>> -		vmentry_ctrl &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
>> -
>> -	return vmentry_ctrl;
>> +	 /*
>> +	  * IA32e mode, and loading of EFER is toggled dynamically.
>> +	  */
>> +	return vmentry_ctrl &= ~(VM_ENTRY_LOAD_IA32_EFER | VM_ENTRY_IA32E_MODE);
> With my above suggestion, these changes are unnecessary.  If enable_mediated_pmu
> is false, or the vCPU doesn't have a PMU, clearing the controls is correct.  And
> when the vCPU is gifted a PMU, KVM will explicitly enabled the controls.
Yes.

>
> To discourage incorrect usage of these helpers maybe rename them to
> vmx_get_initial_{vmentry,vmexit}_ctrl()?

Sure.


>
>>  }
>>  
>>  static u32 vmx_vmexit_ctrl(void)
>> @@ -4429,12 +4513,8 @@ static u32 vmx_vmexit_ctrl(void)
>>  		vmexit_ctrl &= ~(VM_EXIT_PT_CONCEAL_PIP |
>>  				 VM_EXIT_CLEAR_IA32_RTIT_CTL);
>>  
>> -	if (cpu_has_perf_global_ctrl_bug())
>> -		vmexit_ctrl &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
>> -
>> -	/* Loading of EFER and PERF_GLOBAL_CTRL are toggled dynamically */
>> -	return vmexit_ctrl &
>> -		~(VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL | VM_EXIT_LOAD_IA32_EFER);
> But this code needs to *add* clearing of VM_EXIT_SAVE_IA32_PERF_GLOBAL_CTRL.

Sure.