mbox series

[v6,0/7] Introduce and test masked events

Message ID 20221021205105.1621014-1-aaronlewis@google.com (mailing list archive)
Headers show
Series Introduce and test masked events | expand

Message

Aaron Lewis Oct. 21, 2022, 8:50 p.m. UTC
This series introduces the concept of masked events to the pmu event
filter. Masked events can help reduce the number of events needed in the
pmu event filter by allowing a more generalized matching method to be
used for the unit mask when filtering guest events in the pmu.  With
masked events, if an event select should be restricted from the guest,
instead of having to add an entry to the pmu event filter for each
event select + unit mask pair, a masked event can be added to generalize
the unit mask values.

v5 -> v6
 - The following changes were based on Sean's feedback.
 - Patch #1, Use a const for EVENTSEL_EVENT rather than a callback.
 - Patch #2, s/invalid/impossible and removed helpers.
 - Patch #3, Ditched internal event struct.  Sticking with arch layout.
 - Patch #4, 
     - Switched masked events to follow arch layout.
     - Created an internal struct for the pmu event filter.
     - Track separate lists for include events and exclude events.
     - General refactors as a result of these changes.

v4 -> v5
 - Patch #3, Simplified the return logic in filter_contains_match(). [Jim]
 - Patch #4, Corrected documentation for errors and returns. [Jim]
 - Patch #6, Added TEST_REQUIRE for KVM_CAP_PMU_EVENT_MASKED_EVENTS. [Jim]
 - Patch #7,
     - Removed TEST_REQUIRE for KVM_CAP_PMU_EVENT_MASKED_EVENTS (moved it
       to patch #6).
     - Changed the assert to a branch in supports_event_mem_inst_retired().
       [Jim]
     - Added a check to make sure 3 GP counters are available. [Jim]

v3 -> v4
 - Patch #1, Fix the mask for the guest event select used in bsearch.
 - Patch #2, Remove invalid events from the pmu event filter.
 - Patch #3, Create an internal/common representation for filter events.
 - Patch #4,
     - Use a common filter event to simplify kernel code. [Jim]
     - s/invalid/exclude for masked events. More descriptive. [Sean]
     - Simplified masked event layout. There was no need to complicate it.
     - Add KVM_CAP_PMU_EVENT_MASKED_EVENTS.
 - Patch #7, Rewrote the masked events tests using MEM_INST_RETIRED (0xd0)
   on Intel and LS Dispatch (0x29) on AMD. They have multiple unit masks
   each which were leveraged for improved masked events testing.

v2 -> v3
 - Reworked and documented the invert flag usage.  It was possible to
   get ambiguous results when using it.  That should not be possible
   now.
 - Added testing for inverted masked events to validate the updated
   implementation.
 - Removed testing for inverted masked events from the masked events test.
   They were meaningless with the updated implementation.  More meaning
   tests were added separately.

v1 -> v2
 - Made has_invalid_event() static to fix warning.
 - Fixed checkpatch.pl errors and warnings.
 - Updated to account for KVM_X86_PMU_OP().

Aaron Lewis (7):
  kvm: x86/pmu: Correct the mask used in a pmu event filter lookup
  kvm: x86/pmu: Remove impossible events from the pmu event filter
  kvm: x86/pmu: prepare the pmu event filter for masked events
  kvm: x86/pmu: Introduce masked events to the pmu event filter
  selftests: kvm/x86: Add flags when creating a pmu event filter
  selftests: kvm/x86: Add testing for KVM_SET_PMU_EVENT_FILTER
  selftests: kvm/x86: Test masked events

 Documentation/virt/kvm/api.rst                |  77 +++-
 arch/x86/include/asm/kvm_host.h               |  14 +-
 arch/x86/include/uapi/asm/kvm.h               |  29 ++
 arch/x86/kvm/pmu.c                            | 241 +++++++++--
 arch/x86/kvm/pmu.h                            |   2 +
 arch/x86/kvm/svm/pmu.c                        |   1 +
 arch/x86/kvm/vmx/pmu_intel.c                  |   1 +
 arch/x86/kvm/x86.c                            |   1 +
 include/uapi/linux/kvm.h                      |   1 +
 .../kvm/x86_64/pmu_event_filter_test.c        | 387 +++++++++++++++++-
 10 files changed, 703 insertions(+), 51 deletions(-)

Comments

Sean Christopherson Oct. 27, 2022, 10:02 p.m. UTC | #1
On Fri, Oct 21, 2022, Aaron Lewis wrote:
> This series introduces the concept of masked events to the pmu event
> filter. Masked events can help reduce the number of events needed in the
> pmu event filter by allowing a more generalized matching method to be
> used for the unit mask when filtering guest events in the pmu.  With
> masked events, if an event select should be restricted from the guest,
> instead of having to add an entry to the pmu event filter for each
> event select + unit mask pair, a masked event can be added to generalize
> the unit mask values.

...

> Aaron Lewis (7):
>   kvm: x86/pmu: Correct the mask used in a pmu event filter lookup
>   kvm: x86/pmu: Remove impossible events from the pmu event filter
>   kvm: x86/pmu: prepare the pmu event filter for masked events
>   kvm: x86/pmu: Introduce masked events to the pmu event filter
>   selftests: kvm/x86: Add flags when creating a pmu event filter
>   selftests: kvm/x86: Add testing for KVM_SET_PMU_EVENT_FILTER
>   selftests: kvm/x86: Test masked events

One comment request in the last patch, but it's not the end of the world if it
doesn't get added right away.

An extra set of eyeballs from Paolo, Jim, and/or Like would be welcome as I don't
consider myself trustworthy when it comes to PMU code...

Reviewed-by: Sean Christopherson <seanjc@google.com>
Like Xu Nov. 9, 2022, 11:28 a.m. UTC | #2
On 28/10/2022 6:02 am, Sean Christopherson wrote:
>> Aaron Lewis (7):
>>    kvm: x86/pmu: Correct the mask used in a pmu event filter lookup
>>    kvm: x86/pmu: Remove impossible events from the pmu event filter
>>    kvm: x86/pmu: prepare the pmu event filter for masked events
>>    kvm: x86/pmu: Introduce masked events to the pmu event filter
>>    selftests: kvm/x86: Add flags when creating a pmu event filter
>>    selftests: kvm/x86: Add testing for KVM_SET_PMU_EVENT_FILTER
>>    selftests: kvm/x86: Test masked events
> One comment request in the last patch, but it's not the end of the world if it
> doesn't get added right away.
> 
> An extra set of eyeballs from Paolo, Jim, and/or Like would be welcome as I don't
> consider myself trustworthy when it comes to PMU code...
> 
> Reviewed-by: Sean Christopherson<seanjc@google.com>
> 

I'm not going to block these changes just because I don't use the 
pmu-event-filter feature very heavily.
One of my concern is the relatively lower test coverage of pmu-event-filter 
involved code, despite its predictable performance optimizations.

Maybe a rebase version would attract more attention (or at least mine).

Thanks,
Like Xu
Aaron Lewis Nov. 9, 2022, 5:41 p.m. UTC | #3
On Wed, Nov 9, 2022 at 11:28 AM Like Xu <like.xu.linux@gmail.com> wrote:
>
> On 28/10/2022 6:02 am, Sean Christopherson wrote:
> >> Aaron Lewis (7):
> >>    kvm: x86/pmu: Correct the mask used in a pmu event filter lookup
> >>    kvm: x86/pmu: Remove impossible events from the pmu event filter
> >>    kvm: x86/pmu: prepare the pmu event filter for masked events
> >>    kvm: x86/pmu: Introduce masked events to the pmu event filter
> >>    selftests: kvm/x86: Add flags when creating a pmu event filter
> >>    selftests: kvm/x86: Add testing for KVM_SET_PMU_EVENT_FILTER
> >>    selftests: kvm/x86: Test masked events
> > One comment request in the last patch, but it's not the end of the world if it
> > doesn't get added right away.
> >
> > An extra set of eyeballs from Paolo, Jim, and/or Like would be welcome as I don't
> > consider myself trustworthy when it comes to PMU code...
> >
> > Reviewed-by: Sean Christopherson<seanjc@google.com>
> >
>
> I'm not going to block these changes just because I don't use the
> pmu-event-filter feature very heavily.
> One of my concern is the relatively lower test coverage of pmu-event-filter
> involved code, despite its predictable performance optimizations.

Is there something else you are hoping to see as far as testing goes
other than the selftest?  Or is something missing in it?

>
> Maybe a rebase version would attract more attention (or at least mine).

Sure, I'll send out v7 rebased on top of kvm/queue.

>
> Thanks,
> Like Xu