Re: [REPOST PATCH 10/16] selftests: KVM: aarch64: Add KVM EVTYPE filter PMU test

[Raghavendra Rao Ananta]

On Mon, Mar 6, 2023 at 5:19 PM Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
>
> Hi Raghu,
>
> On Tue, Feb 14, 2023 at 5:07 PM Raghavendra Rao Ananta
> <rananta@xxxxxxxxxx> wrote:
> >
> > KVM doest't allow the guests to modify the filter types
> > such counting events in nonsecure/secure-EL2, EL3, and
> > so on. Validate the same by force-configuring the bits
> > in PMXEVTYPER_EL0, PMEVTYPERn_EL0, and PMCCFILTR_EL0
> > registers.
> >
> > The test extends further by trying to create an event
> > for counting only in EL2 and validates if the counter
> > is not progressing.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> > ---
> >  .../testing/selftests/kvm/aarch64/vpmu_test.c | 85 +++++++++++++++++++
> >  1 file changed, 85 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vpmu_test.c b/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> > index 3dfb770b538e9..5c166df245589 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> > @@ -15,6 +15,10 @@
> >   * of allowing or denying the events. The guest validates it by
> >   * checking if it's able to count only the events that are allowed.
> >   *
> > + * 3. KVM doesn't allow the guest to count the events attributed with
> > + * higher exception levels (EL2, EL3). Verify this functionality by
> > + * configuring and trying to count the events for EL2 in the guest.
> > + *
> >   * Copyright (c) 2022 Google LLC.
> >   *
> >   */
> > @@ -23,6 +27,7 @@
> >  #include <test_util.h>
> >  #include <vgic.h>
> >  #include <asm/perf_event.h>
> > +#include <linux/arm-smccc.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/bitmap.h>
> >
> > @@ -259,6 +264,7 @@ struct vpmu_vm {
> >  enum test_stage {
> >         TEST_STAGE_COUNTER_ACCESS = 1,
> >         TEST_STAGE_KVM_EVENT_FILTER,
> > +       TEST_STAGE_KVM_EVTYPE_FILTER,
> >  };
> >
> >  struct guest_data {
> > @@ -678,6 +684,70 @@ static void guest_event_filter_test(unsigned long *pmu_filter)
> >         }
> >  }
> >
> > +static void guest_evtype_filter_test(void)
> > +{
> > +       int i;
> > +       struct pmc_accessor *acc;
> > +       uint64_t typer, cnt;
> > +       struct arm_smccc_res res;
> > +
> > +       pmu_enable();
> > +
> > +       /*
> > +        * KVM blocks the guests from creating events for counting in Secure/Non-Secure Hyp (EL2),
> > +        * Monitor (EL3), and Multithreading configuration. It applies the mask
> > +        * ARMV8_PMU_EVTYPE_MASK against guest accesses to PMXEVTYPER_EL0, PMEVTYPERn_EL0,
> > +        * and PMCCFILTR_EL0 registers to prevent this. Check if KVM honors this using all possible
> > +        * ways to configure the EVTYPER.
> > +        */
>
> I would prefer to break long lines into multiple lines for these comments
> (or other comments in these patches), as "Linux kernel coding style"
> suggests.
> ---
> [https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings]
>
> The preferred limit on the length of a single line is 80 columns.
>
> Statements longer than 80 columns should be broken into sensible
> chunks, unless exceeding 80 columns significantly increases
> readability and does not hide information.
> ---
>
Sure, I'll fix it.
> > +       for (i = 0; i < ARRAY_SIZE(pmc_accessors); i++) {
> > +               acc = &pmc_accessors[i];
> > +
> > +               /* Set all filter bits (31-24), readback, and check against the mask */
> > +               acc->write_typer(0, 0xff000000);
> > +               typer = acc->read_typer(0);
> > +
> > +               GUEST_ASSERT_2((typer | ARMV8_PMU_EVTYPE_EVENT) == ARMV8_PMU_EVTYPE_MASK,
> > +                               typer | ARMV8_PMU_EVTYPE_EVENT, ARMV8_PMU_EVTYPE_MASK);
>
> It appears that bits[29:26] don't have to be zero depending on
> feature availability to the guest (Those bits needs to be zero
> only when relevant features are not available on the guest).
> So, the expected value must be changed depending on the feature
> availability if the test checks those bits.
> I have the same comment for the cycle counter.
>
But doesn't KVM (and the ARM PMU driver) ignore these bits upon write
using ARMV8_PMU_EVTYPE_MASK?
> > +
> > +               /*
> > +                * Regardless of ARMV8_PMU_EVTYPE_MASK, KVM sets perf attr.exclude_hv
> > +                * to not count NS-EL2 events. Verify this functionality by configuring
> > +                * a NS-EL2 event, for which the couunt shouldn't increment.
> > +                */
> > +               typer = ARMV8_PMUV3_PERFCTR_INST_RETIRED;
> > +               typer |= ARMV8_PMU_INCLUDE_EL2 | ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0;
> > +               acc->write_typer(0, typer);
> > +               acc->write_cntr(0, 0);
> > +               enable_counter(0);
> > +
> > +               /* Issue a hypercall to enter EL2 and return */
> > +               memset(&res, 0, sizeof(res));
> > +               smccc_hvc(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0, 0, 0, 0, 0, &res);
> > +
> > +               cnt = acc->read_cntr(0);
> > +               GUEST_ASSERT_3(cnt == 0, cnt, typer, i);
> > +       }
> > +
> > +       /* Check the same sequence for the Cycle counter */
> > +       write_pmccfiltr(0xff000000);
> > +       typer = read_pmccfiltr();
> > +       GUEST_ASSERT_2((typer | ARMV8_PMU_EVTYPE_EVENT) == ARMV8_PMU_EVTYPE_MASK,
> > +                               typer | ARMV8_PMU_EVTYPE_EVENT, ARMV8_PMU_EVTYPE_MASK);
> > +
> > +       typer = ARMV8_PMU_INCLUDE_EL2 | ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0;
> > +       write_pmccfiltr(typer);
> > +       reset_cycle_counter();
> > +       enable_cycle_counter();
> > +
> > +       /* Issue a hypercall to enter EL2 and return */
> > +       memset(&res, 0, sizeof(res));
> > +       smccc_hvc(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0, 0, 0, 0, 0, &res);
> > +
> > +       cnt = read_cycle_counter();
>
> Perhaps it's worth considering having the helpers for PMC registers
> (e.g. write_cntr()) accepting the cycle counter as the index==31
> to simplify the test code implementation ?
>
> Thank you,
> Reiji
>
> > +       GUEST_ASSERT_2(cnt == 0, cnt, typer);
> > +}
> > +
> >  static void guest_code(void)
> >  {
> >         switch (guest_data.test_stage) {
> > @@ -687,6 +757,9 @@ static void guest_code(void)
> >         case TEST_STAGE_KVM_EVENT_FILTER:
> >                 guest_event_filter_test(guest_data.pmu_filter);
> >                 break;
> > +       case TEST_STAGE_KVM_EVTYPE_FILTER:
> > +               guest_evtype_filter_test();
> > +               break;
> >         default:
> >                 GUEST_ASSERT_1(0, guest_data.test_stage);
> >         }
> > @@ -1014,10 +1087,22 @@ static void run_kvm_event_filter_test(void)
> >         run_kvm_event_filter_error_tests();
> >  }
> >
> > +static void run_kvm_evtype_filter_test(void)
> > +{
> > +       struct vpmu_vm *vpmu_vm;
> > +
> > +       guest_data.test_stage = TEST_STAGE_KVM_EVTYPE_FILTER;
> > +
> > +       vpmu_vm = create_vpmu_vm(guest_code, NULL);
> > +       run_vcpu(vpmu_vm->vcpu);
> > +       destroy_vpmu_vm(vpmu_vm);
> > +}
> > +
> >  static void run_tests(uint64_t pmcr_n)
> >  {
> >         run_counter_access_tests(pmcr_n);
> >         run_kvm_event_filter_test();
> > +       run_kvm_evtype_filter_test();
> >  }
> >
> >  /*
> > --
> > 2.39.1.581.gbfd45094c4-goog
> >