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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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





[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux