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)?
> + 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) {
...
}
> +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),
> + 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?
> + },
> + {
> + .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[] = {
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
>
>
