Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES

From: Like Xu <like.xu.linux@gmail.com>
To: Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES
Date: Thu, 28 Jul 2022 20:02:53 +0800	[thread overview]
Message-ID: <271bddfa-9e48-d5f6-6147-af346d7946bf@gmail.com> (raw)
In-Reply-To: <20220727233424.2968356-2-seanjc@google.com>

On 28/7/2022 7:34 am, Sean Christopherson wrote:
> Refresh the PMU if userspace modifies MSR_IA32_PERF_CAPABILITIES.  KVM
> consumes the vCPU's PERF_CAPABILITIES when enumerating PEBS support, but
> relies on CPUID updates to refresh the PMU.  I.e. KVM will do the wrong
> thing if userspace stuffs PERF_CAPABILITIES _after_ setting guest CPUID.

Unwise userspace should reap its consequences if it does not break KVM or host.

When a guest feature can be defined/controlled by multiple KVM APIs entries,
(such as SET_CPUID2, msr_feature, KVM_CAP, module_para), should KVM
define the priority of these APIs (e.g. whether they can override each other) ?

Removing this ambiguity ensures consistency in the architecture and behavior of 
all KVM features.
Any further performance optimizations can be based on these finalized values as 
you do.

> 
> Opportunistically fix a curly-brace indentation.
> 
> Fixes: c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS")
> Cc: Like Xu <like.xu.linux@gmail.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/x86.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5366f884e9a7..362c538285db 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3543,9 +3543,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   			return 1;
>   
>   		vcpu->arch.perf_capabilities = data;
> -
> +		kvm_pmu_refresh(vcpu);

I had proposed this diff but was met with silence.

>   		return 0;
> -		}
> +	}
>   	case MSR_EFER:
>   		return set_efer(vcpu, msr_info);
>   	case MSR_K7_HWCR: