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)?
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