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

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]
Message-ID: <CAJve8om1VibbkHBuG4B+KbKY2eug42wGk9wiYtCYeF-WDY8EgQ@mail.gmail.com>
Date:   Fri, 8 Sep 2023 09:19:32 +0800
From:   Haibo Xu <xiaobo55x@...il.com>
To:     Andrew Jones <ajones@...tanamicro.com>
Cc:     Haibo Xu <haibo1.xu@...el.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Shuah Khan <shuah@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Oliver Upton <oliver.upton@...ux.dev>,
        James Morse <james.morse@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Zenghui Yu <yuzenghui@...wei.com>,
        Anup Patel <anup@...infault.org>,
        Atish Patra <atishp@...shpatra.org>,
        Guo Ren <guoren@...nel.org>,
        Conor Dooley <conor.dooley@...rochip.com>,
        Greentime Hu <greentime.hu@...ive.com>,
        wchen <waylingii@...il.com>,
        Daniel Henrique Barboza <dbarboza@...tanamicro.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Ricardo Koller <ricarkol@...gle.com>,
        Vishal Annapurve <vannapurve@...gle.com>,
        David Matlack <dmatlack@...gle.com>,
        Aaron Lewis <aaronlewis@...gle.com>,
        Mingwei Zhang <mizhang@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Ackerley Tng <ackerleytng@...gle.com>,
        Vipin Sharma <vipinsh@...gle.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        Peter Gonda <pgonda@...gle.com>,
        Philippe Mathieu-Daudé <philmd@...aro.org>,
        Thomas Huth <thuth@...hat.com>, Like Xu <likexu@...cent.com>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Michal Luczaj <mhal@...x.co>,
        zhang songyi <zhang.songyi@....com.cn>,
        linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
        kvm@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
        kvm-riscv@...ts.infradead.org
Subject: Re: [PATCH v2 8/8] KVM: riscv: selftests: Add sstc timer test

On Fri, Sep 8, 2023 at 3:01 AM Andrew Jones <ajones@...tanamicro.com> wrote:
>
> On Thu, Sep 07, 2023 at 12:20:29PM +0800, Haibo Xu wrote:
> > On Mon, Sep 4, 2023 at 10:58 PM Andrew Jones <ajones@...tanamicro.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@...el.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()).
>
> OK, whatever balances the reduction of duplicate code and avoidance of
> exporting helper functions. BTW, riscv may not need/want all the same
> helper functions as aarch64. Anyway, I guess I'll see how the next version
> turns out.
>
> >
> > > > +
> > > > +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.
>
> We should ensure we know the exact, expected state of the vcpu before,
> during, and after the test. If a state doesn't match expectations,
> then the test should assert and we should go investigate the test code
> to see if setup/checking is correct. If it is, then we've found a bug
> in KVM that we need to go investigate.
>
> For Sstc, a pending timer interrupt completely depends on stimecmp, so
> we need to watch that closely. Take a look at the attached simple timer
> test I pulled together to illustrate how stimecmp, timer interrupt enable,
> and all interrupt enable interact. You may want to use it to help port
> the arch_timer.
>

Thanks for sharing the test codes. Will have an investigation on it.

> Thanks,
> drew