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
