Received-SPF: pass (google.com: domain of linux-kernel+bounces-137659-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240403080452.1007601-1-atishp@rivosinc.com> <20240403080452.1007601-21-atishp@rivosinc.com>
 <20240405-d1a4cb9a441a05a9d2f8b1c8@orel> <976411ab-6ddf-4b10-8e13-1575928415ce@rivosinc.com>
 <20240409-dd055c3d08e027cf2a5cb4dc@orel>
In-Reply-To: <20240409-dd055c3d08e027cf2a5cb4dc@orel>
From: Atish Kumar Patra <atishp@rivosinc.com>
Date: Tue, 9 Apr 2024 15:11:36 -0700
Message-ID: <CAHBxVyEh0K5b0SdN-asrOuuggBztZ-mjCoOR=EC067pURRg3aA@mail.gmail.com>
Subject: Re: [PATCH v5 20/22] KVM: riscv: selftests: Add SBI PMU selftest
To: Andrew Jones <ajones@ventanamicro.com>
Cc: linux-kernel@vger.kernel.org, Anup Patel <anup@brainfault.org>, 
	Ajay Kaher <akaher@vmware.com>, Alexandre Ghiti <alexghiti@rivosinc.com>, 
	Alexey Makhalov <amakhalov@vmware.com>, Conor Dooley <conor.dooley@microchip.com>, 
	Juergen Gross <jgross@suse.com>, kvm-riscv@lists.infradead.org, kvm@vger.kernel.org, 
	linux-kselftest@vger.kernel.org, linux-riscv@lists.infradead.org, 
	Mark Rutland <mark.rutland@arm.com>, Palmer Dabbelt <palmer@dabbelt.com>, 
	Paolo Bonzini <pbonzini@redhat.com>, Paul Walmsley <paul.walmsley@sifive.com>, 
	Shuah Khan <shuah@kernel.org>, virtualization@lists.linux.dev, 
	VMware PV-Drivers Reviewers <pv-drivers@vmware.com>, Will Deacon <will@kernel.org>, x86@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 9, 2024 at 1:01=E2=80=AFAM Andrew Jones <ajones@ventanamicro.co=
m> wrote:
>
> On Mon, Apr 08, 2024 at 05:37:19PM -0700, Atish Patra wrote:
> > On 4/5/24 05:50, Andrew Jones wrote:
> > > On Wed, Apr 03, 2024 at 01:04:49AM -0700, Atish Patra wrote:
> > > ...
> > > > +static void test_pmu_basic_sanity(void)
> > > > +{
> > > > + long out_val =3D 0;
> > > > + bool probe;
> > > > + struct sbiret ret;
> > > > + int num_counters =3D 0, i;
> > > > + union sbi_pmu_ctr_info ctrinfo;
> > > > +
> > > > + probe =3D guest_sbi_probe_extension(SBI_EXT_PMU, &out_val);
> > > > + GUEST_ASSERT(probe && out_val =3D=3D 1);
> > > > +
> > > > + num_counters =3D get_num_counters();
> > > > +
> > > > + for (i =3D 0; i < num_counters; i++) {
> > > > +         ret =3D sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_GET_IN=
FO, i,
> > > > +                         0, 0, 0, 0, 0);
> > > > +
> > > > +         /* There can be gaps in logical counter indicies*/
> > > > +         if (ret.error)
> > > > +                 continue;
> > > > +         GUEST_ASSERT_NE(ret.value, 0);
> > > > +
> > > > +         ctrinfo.value =3D ret.value;
> > > > +
> > > > +         /**
> > > > +          * Accesibillity check of hardware and read capability of=
 firmware counters.
> > >
> > > Accessibility
> > >
> >
> > Fixed it.
> >
> > > > +          * The spec doesn't mandate any initial value. No need to=
 check any value.
> > > > +          */
> > > > +         read_counter(i, ctrinfo);
> > > > + }
> > > > +
> > > > + GUEST_DONE();
> > > > +}
> > > > +
> > > > +static void run_vcpu(struct kvm_vcpu *vcpu)
> > > > +{
> > > > + struct ucall uc;
> > > > +
> > > > + vcpu_run(vcpu);
> > > > + switch (get_ucall(vcpu, &uc)) {
> > > > + case UCALL_ABORT:
> > > > +         REPORT_GUEST_ASSERT(uc);
> > > > +         break;
> > > > + case UCALL_DONE:
> > > > + case UCALL_SYNC:
> > > > +         break;
> > > > + default:
> > > > +         TEST_FAIL("Unknown ucall %lu", uc.cmd);
> > > > +         break;
> > > > + }
> > > > +}
> > > > +
> > > > +void test_vm_destroy(struct kvm_vm *vm)
> > > > +{
> > > > + memset(ctrinfo_arr, 0, sizeof(union sbi_pmu_ctr_info) * RISCV_MAX=
_PMU_COUNTERS);
> > > > + counter_mask_available =3D 0;
> > > > + kvm_vm_free(vm);
> > > > +}
> > > > +
> > > > +static void test_vm_basic_test(void *guest_code)
> > > > +{
> > > > + struct kvm_vm *vm;
> > > > + struct kvm_vcpu *vcpu;
> > > > +
> > > > + vm =3D vm_create_with_one_vcpu(&vcpu, guest_code);
> > > > + __TEST_REQUIRE(__vcpu_has_sbi_ext(vcpu, KVM_RISCV_SBI_EXT_PMU),
> > > > +                            "SBI PMU not available, skipping test"=
);
> > > > + vm_init_vector_tables(vm);
> > > > + /* Illegal instruction handler is required to verify read access =
without configuration */
> > > > + vm_install_exception_handler(vm, EXC_INST_ILLEGAL, guest_illegal_=
exception_handler);
> > >
> > > I still don't see where the "verify" part is. The handler doesn't rec=
ord
> > > that it had to handle anything.
> > >
> >
> > The objective of the test is to ensure that we get an illegal instructi=
on
> > without configuration.
>
> This part I guessed.
>
> > The presence of the registered exception handler is
> > sufficient for that.
>
> This part I disagree with. The handler may not be necessary and not run i=
f
> we don't get the ILL. Usually when I write tests like these I set a
> boolean in the handler and check it after the instruction which should
> have sent us there to make sure we did indeed go there.
>

Ahh I got your point now. That makes sense. Since it was just a sanity test=
,
I hadn't put the boolean check earlier. But you are correct about bugs
in kvm code which wouldn't
generate an expected ILL .

I have added it. Thanks for the suggestion :)

> >
> > The verify part is that the test doesn't end up in a illegal instructio=
n
> > exception when you try to access a counter without configuring.
> >
> > Let me know if you think we should more verbose comment to explain the
> > scenario.
> >
>
> With a boolean the test code will be mostly self documenting, but a short
> comment saying why we expect the boolean to be set would be good too.
>
> Thanks,
> drew
>
> >
> > > > +
> > > > + vcpu_init_vector_tables(vcpu);
> > > > + run_vcpu(vcpu);
> > > > +
> > > > + test_vm_destroy(vm);
> > > > +}
> > > > +
> > > > +static void test_vm_events_test(void *guest_code)
> > > > +{
> > > > + struct kvm_vm *vm =3D NULL;
> > > > + struct kvm_vcpu *vcpu =3D NULL;
> > > > +
> > > > + vm =3D vm_create_with_one_vcpu(&vcpu, guest_code);
> > > > + __TEST_REQUIRE(__vcpu_has_sbi_ext(vcpu, KVM_RISCV_SBI_EXT_PMU),
> > > > +                            "SBI PMU not available, skipping test"=
);
> > > > + run_vcpu(vcpu);
> > > > +
> > > > + test_vm_destroy(vm);
> > > > +}
> > > > +
> > > > +int main(void)
> > > > +{
> > > > + test_vm_basic_test(test_pmu_basic_sanity);
> > > > + pr_info("SBI PMU basic test : PASS\n");
> > > > +
> > > > + test_vm_events_test(test_pmu_events);
> > > > + pr_info("SBI PMU event verification test : PASS\n");
> > > > +
> > > > + return 0;
> > > > +}
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > Thanks,
> > > drew
> >