linux-kernel - Re: [PATCH v9 2/3] KVM: selftests: aarch64: Introduce pmu_event_filter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 17 Jun 2024 17:01:02 -0700
From: Raghavendra Rao Ananta <rananta@...gle.com>
To: Shaoqin Huang <shahuang@...hat.com>
Cc: Oliver Upton <oliver.upton@...ux.dev>, Marc Zyngier <maz@...nel.org>, kvmarm@...ts.linux.dev, 
	Paolo Bonzini <pbonzini@...hat.com>, Shuah Khan <shuah@...nel.org>, 
	James Morse <james.morse@....com>, Suzuki K Poulose <suzuki.poulose@....com>, 
	Zenghui Yu <yuzenghui@...wei.com>, linux-kernel@...r.kernel.org, kvm@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v9 2/3] KVM: selftests: aarch64: Introduce pmu_event_filter_test

Hi Shaoqin


On Thu, Jun 13, 2024 at 1:28 AM Shaoqin Huang <shahuang@...hat.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
>
>