Re: [PATCH v2 8/8] KVM: riscv: selftests: Add sstc timer test

From: Haibo Xu <xiaobo55x@gmail.com>
To: Andrew Jones <ajones@ventanamicro.com>
Cc: "Haibo Xu" <haibo1.xu@intel.com>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Shuah Khan" <shuah@kernel.org>, "Marc Zyngier" <maz@kernel.org>,
	"Oliver Upton" <oliver.upton@linux.dev>,
	"James Morse" <james.morse@arm.com>,
	"Suzuki K Poulose" <suzuki.poulose@arm.com>,
	"Zenghui Yu" <yuzenghui@huawei.com>,
	"Anup Patel" <anup@brainfault.org>,
	"Atish Patra" <atishp@atishpatra.org>,
	"Guo Ren" <guoren@kernel.org>,
	"Conor Dooley" <conor.dooley@microchip.com>,
	"Greentime Hu" <greentime.hu@sifive.com>,
	wchen <waylingii@gmail.com>,
	"Daniel Henrique Barboza" <dbarboza@ventanamicro.com>,
	"Sean Christopherson" <seanjc@google.com>,
	"Ricardo Koller" <ricarkol@google.com>,
	"Vishal Annapurve" <vannapurve@google.com>,
	"David Matlack" <dmatlack@google.com>,
	"Aaron Lewis" <aaronlewis@google.com>,
	"Mingwei Zhang" <mizhang@google.com>,
	"Vitaly Kuznetsov" <vkuznets@redhat.com>,
	"Ackerley Tng" <ackerleytng@google.com>,
	"Vipin Sharma" <vipinsh@google.com>,
	"Maxim Levitsky" <mlevitsk@redhat.com>,
	"Peter Gonda" <pgonda@google.com>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Thomas Huth" <thuth@redhat.com>, "Like Xu" <likexu@tencent.com>,
	"David Woodhouse" <dwmw@amazon.co.uk>,
	"Michal Luczaj" <mhal@rbox.co>,
	"zhang songyi" <zhang.songyi@zte.com.cn>,
	linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
	kvm@vger.kernel.org, linux-kselftest@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	kvm-riscv@lists.infradead.org
Subject: Re: [PATCH v2 8/8] KVM: riscv: selftests: Add sstc timer test
Date: Thu, 7 Sep 2023 12:20:29 +0800	[thread overview]
Message-ID: <CAJve8ok03P-t7pTA9mH=5vvkUy4NtxHac_Z_NM88cs55YQZATg@mail.gmail.com> (raw)
In-Reply-To: <20230904-a69c6a228bf4553cc58bca17@orel>

On Mon, Sep 4, 2023 at 10:58 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Sat, Sep 02, 2023 at 08:59:30PM +0800, Haibo Xu wrote:
> > Add a KVM selftest to validate the Sstc timer functionality.
> > The test was ported from arm64 arch timer test.
> >
> > Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
> > ---

> > diff --git a/tools/testing/selftests/kvm/riscv/arch_timer.c b/tools/testing/selftests/kvm/riscv/arch_timer.c
> > new file mode 100644
> > index 000000000000..c50a33c1e4f9
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/riscv/arch_timer.c
> > @@ -0,0 +1,130 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * arch_timer.c - Tests the riscv64 sstc timer IRQ functionality
> > + *
> > + * The test validates the sstc timer IRQs using vstimecmp registers.
> > + * It's ported from the aarch64 arch_timer test.
> > + *

> guest_run[_stage]() can be shared with aarch64, we just have a single
> stage=0 for riscv.
>

Yes, we can. But if we share the guest_run[_stage]() by moving it to
kvm/arch_timer.c
or kvm/include/timer_test.h, we need to declare extra sub-functions
somewhere in a
header file(etc. guest_configure_timer_action()).

> > +
> > +static void guest_code(void)
> > +{
> > +     uint32_t cpu = guest_get_vcpuid();
> > +     struct test_vcpu_shared_data *shared_data = &vcpu_shared_data[cpu];
> > +
> > +     local_irq_disable();
> > +     timer_irq_disable();
> > +     local_irq_enable();
>
> I don't think we need to disable all interrupts when disabling the timer
> interrupt.
>

There is no local_irq_disable() protection during the initial debug
phase, but the test always
fail with below error messages:

Guest assert failed,  vcpu 0; stage; 0; iter: 0
==== Test Assertion Failure ====
  riscv/arch_timer.c:78: config_iter + 1 == irq_iter
  pid=585 tid=586 errno=4 - Interrupted system call
  (stack trace empty)
  0x1 != 0x0 (config_iter + 1 != irq_iter)

To be frank, I am not quite sure why the local_irq_disable/enable() matters.
One possible reason may be some timer irq was triggered before we set up the
timecmp register.

> > +
> > +     guest_run(shared_data);
> > +
> > +     GUEST_DONE();
> > +}
> > +
> > +void test_vm_cleanup(struct kvm_vm *vm)
> > +{
> > +     kvm_vm_free(vm);
> > +}
> > --
> > 2.34.1
> >
>
> Thanks,
> drew