diff mbox series

[4/4] KVM: x86/pmu: Move .hw_event_available() check out of PMC filter helper

Message ID 20230607010206.1425277-5-seanjc@google.com
State New
Headers
Series KVM: x86/pmu: Clean up arch/hw event handling |

Commit Message

Sean Christopherson June 7, 2023, 1:02 a.m. UTC 
  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.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/pmu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Like Xu July 11, 2023, 4:32 p.m. UTC | #1 
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);
>   }
>
Sean Christopherson July 12, 2023, 4:11 p.m. UTC | #2 
On Wed, Jul 12, 2023, Like Xu wrote:
> 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).

I want to clearly differentiate between what KVM allows and what userspace allows,
and specifically I want to use "filter" only to describe userspace intervention as
much as possible.

Outside of kvm_get_filtered_xcr0(), which I would classify as userspace-defined
behavior (albeit rather indirectly), and a few architecturally defined "filter"
terms from Intel and AMD, we don't use "filter" in KVM to describe KVM behavior.

IMO, there's a lot of value in being able to associate "filter" with userspace
desires, e.g. just mentioning "filtering" immediately frames a conversation as
dealing with userspace's wants, not internal KVM behavior.
diff mbox series

Patch

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);
 }