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