Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <81c32ae2-ff21-131f-e498-f87b1e7fe3b5@gmail.com>
Date:   Wed, 12 Jul 2023 00:32:45 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.13.0
Subject: Re: [PATCH 4/4] KVM: x86/pmu: Move .hw_event_available() check out of
 PMC filter helper
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Aaron Lewis <aaronlewis@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
References: <20230607010206.1425277-1-seanjc@google.com>
 <20230607010206.1425277-5-seanjc@google.com>
From:   Like Xu <like.xu.linux@gmail.com>
In-Reply-To: <20230607010206.1425277-5-seanjc@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023/6/7 09:02, Sean Christopherson wrote:
> Move the call to kvm_x86_pmu.hw_event_available(), which has nothing to
> with the userspace PMU filter, out of check_pmu_event_filter() and into
> its sole caller pmc_event_is_allowed().  pmc_event_is_allowed() didn't
> exist when commit 7aadaa988c5e ("KVM: x86/pmu: Drop amd_event_mapping[]
> in the KVM context"), so presumably the motivation for invoking
> .hw_event_available() from check_pmu_event_filter() was to avoid having
> to add multiple call sites.

The event unavailability check based on intel cpuid is, in my opinion,
part of our pmu_event_filter mechanism. Unavailable events can be
defined either by KVM userspace or by architectural cpuid (if any).

The bigger issue here is what happens when the two rules conflict, and
the answer can be found more easily by putting the two parts in one
function (the architectural cpuid rule takes precedence).

> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/pmu.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 1690d41c1830..2a32dc6aa3f7 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -387,9 +387,6 @@ static bool check_pmu_event_filter(struct kvm_pmc *pmc)
>   	struct kvm_x86_pmu_event_filter *filter;
>   	struct kvm *kvm = pmc->vcpu->kvm;
>   
> -	if (!static_call(kvm_x86_pmu_hw_event_available)(pmc))
> -		return false;
> -
>   	filter = srcu_dereference(kvm->arch.pmu_event_filter, &kvm->srcu);
>   	if (!filter)
>   		return true;
> @@ -403,6 +400,7 @@ static bool check_pmu_event_filter(struct kvm_pmc *pmc)
>   static bool pmc_event_is_allowed(struct kvm_pmc *pmc)
>   {
>   	return pmc_is_globally_enabled(pmc) && pmc_speculative_in_use(pmc) &&
> +	       static_call(kvm_x86_pmu_hw_event_available)(pmc) &&
>   	       check_pmu_event_filter(pmc);
>   }
>