Re: [PATCH v9 2/3] KVM: selftests: aarch64: Introduce pmu_event_filter_test

[Shaoqin Huang <shahuang@redhat.com>]

Hi Raghavendra,

Thanks for helping review this series.

On 6/18/24 08:01, Raghavendra Rao Ananta wrote:
> Hi Shaoqin
> 
> 
> On Thu, Jun 13, 2024 at 1:28 AM Shaoqin Huang <shahuang@redhat.com> wrote:
> 
>> +static void prepare_expected_pmce(struct kvm_pmu_event_filter *filter)
>> +{
>> +       struct pmu_common_event_ids pmce_mask = { ~0, ~0 };
>> +       bool first_filter = true;
>> +       int i;
>> +
>> +       while (filter && filter->nevents != 0) {
> Do you also want to add a check to ensure we aren't running over
> FILTER_NR (I'd expect a compiler warning/error though)?

The FILTER_NR is only used to assign the length of the filter array, if 
the defined filter array length is larger than the FILTER_NR, I believe 
there will be a compiling warning.

> 
>> +               if (first_filter) {
>> +                       if (filter->action == KVM_PMU_EVENT_ALLOW)
>> +                               memset(&pmce_mask, 0, sizeof(pmce_mask));
>> +                       first_filter = false;
>> +               }
> nit: Probably we can make the 'first_filter' part a little cleaner by
> checking this outside the loop.
> 
> if (filter && filter->action == KVM_PMU_EVENT_ALLOW)
>          memset(&pmce_mask, 0, sizeof(pmce_mask));
> 
> while (filter && filter->nevents != 0) {
>      ...
> }

Thanks, this looks much better and I will change the code to it.

> 
>> +static struct test_desc tests[] = {
>> +       {
>> +               .name = "without_filter",
>> +               .filter = {
>> +                       { 0 }
>> +               },
>> +       },
>> +       {
>> +               .name = "member_allow_filter",
>> +               .filter = {
>> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_SW_INCR, 0),
> In terms of readability, do you think it's better to use
> KVM_PMU_EVENT_{ALLOW|DENY}, instead of 0 and 1?
> 
> Or, if that's coming out to be too long, may be create another wrapper
> over DEFINE_FILTER, and simply use that in the array:
> 
> #define EVENT_ALLOW(event) DEFINE_FILTER(event, KVM_PMU_EVENT_ALLOW)
> #define EVENT_DENY(event) DEFINE_FILTER(event, KVM_PMU_EVENT_DENY)
> 
> .filter = {
>      EVENT_ALLOW(ARMV8_PMUV3_PERFCTR_SW_INCR),
> 

Pretty good idea. I will take your code which looks much clean.

>> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_INST_RETIRED, 0),
>> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_BR_RETIRED, 0),
>> +                       { 0 },
>> +               },
>> +       },
> 
>> +       {
>> +               .name = "cancel_filter",
>> +               .filter = {
>> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_CPU_CYCLES, 0),
>> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_CPU_CYCLES, 1),
>> +               },
> Since the initial filter map depends on the event being allowed or
> denied, do you think another "cancel_filter" case to first deny and
> then allow would also be better?

Yes. That would be better, I will add another test which first deny and 
then allow it.

> 
>> +       },
>> +       {
>> +               .name = "multiple_filter",
>> +               .filter = {
>> +                       __DEFINE_FILTER(0x0, 0x10, 0),
>> +                       __DEFINE_FILTER(0x6, 0x3, 1),
>> +               },
>> +       },
>> +       { 0 }
>> +};
>> +
>> +static void run_tests(void)
>> +{
>> +       struct test_desc *t;
>> +
>> +       for (t = &tests[0]; t->name; t++)
>> +               run_test(t);
>> +}
>> +
>> +int used_pmu_events[] = {
> nit: static int used_pmu_events[] = {
> 

Got it.

Thanks,
Shaoqin

> Thank you.
> Raghavendra
> 
> 
>> +       ARMV8_PMUV3_PERFCTR_BR_RETIRED,
>> +       ARMV8_PMUV3_PERFCTR_INST_RETIRED,
>> +       ARMV8_PMUV3_PERFCTR_CHAIN,
>> +};
>> +
>> +static bool kvm_pmu_support_events(void)
>> +{
>> +       struct pmu_common_event_ids used_pmce = { 0, 0 };
>> +
>> +       create_vpmu_vm(guest_get_pmceid);
>> +
>> +       memset(&max_pmce, 0, sizeof(max_pmce));
>> +       sync_global_to_guest(vpmu_vm.vm, max_pmce);
>> +       run_vcpu(vpmu_vm.vcpu);
>> +       sync_global_from_guest(vpmu_vm.vm, max_pmce);
>> +       destroy_vpmu_vm();
>> +
>> +       for (int i = 0; i < ARRAY_SIZE(used_pmu_events); i++)
>> +               set_pmce(&used_pmce, KVM_PMU_EVENT_ALLOW, used_pmu_events[i]);
>> +
>> +       return ((max_pmce.pmceid0 & used_pmce.pmceid0) == used_pmce.pmceid0) &&
>> +              ((max_pmce.pmceid1 & used_pmce.pmceid1) == used_pmce.pmceid1);
>> +}
>> +
>> +int main(void)
>> +{
>> +       TEST_REQUIRE(kvm_has_cap(KVM_CAP_ARM_PMU_V3));
>> +       TEST_REQUIRE(kvm_pmu_support_events());
>> +
>> +       run_tests();
>> +}
>> --
>> 2.40.1
>>
>>
> 

-- 
Shaoqin