From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E37B3EC874B for ; Thu, 7 Sep 2023 19:01:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=frUmgym4n9lqQ2w+5RghQypJWJcHaTdHrZD7PRclLRE=; b=NfL/XDAUq7CTwOaEku7s2FS4i2 ksQ6GWUIpZTh5WLw7dkw8llaPZgyW4dDz1sw4RPh7l3y0w3Rbi3xdutQhEDmwd8E3gOu3CrkRiZnS K7ZDpqCWA6QtlcnipF71A2g1+NOM7yz6zGuu6tPF06Hd92+blx63qlKUG+GBTSt6cSEXpL8hwW9ps KEmrnAoWCCNoUoeJ3Xfjf5RHfzwVfGXLfjBek+5BoNkclUVwiOuGvQnjQ1hKpLiaKVCMv9OLvU6Fw /FhrQXNljCLyLXRDuRAim7Exr3JNbO3rbzkV8p+JhY1cYWu+zUkLWafLYb2uaDNOudK6p4b1m20th xydTdhFA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qeKFj-00Caj4-1h; Thu, 07 Sep 2023 19:01:19 +0000 Received: from mail-wm1-x334.google.com ([2a00:1450:4864:20::334]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qeKFf-00CahJ-0W for linux-arm-kernel@lists.infradead.org; Thu, 07 Sep 2023 19:01:18 +0000 Received: by mail-wm1-x334.google.com with SMTP id 5b1f17b1804b1-401b5516104so14252405e9.2 for ; Thu, 07 Sep 2023 12:01:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1694113272; x=1694718072; darn=lists.infradead.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=eociOZShIIhf6uzSUtfcCoJa3P8brx2laWW2HO9Th3Y=; b=BfOAJNar+6lTVXJSg4O1uVDed832rvxE/bKSuNUpmNKhNmk+Lcb/k4GQ5sVbsteoLX omoLeA/1YiwT1AZaR1OTdFTflQpn58MMYLQcNKhcgLKxOTASsqHSU01c0n1J7ygT/+vX LlpFnLL5SS+HpqH2+f/B5q0TwY9JaWBqmxmviDV6FbRxEPRkhHu8Inmq6Hs4PZRukJf4 8S4xeKPdw2ERRaav7ymrNSHJ+VsBAsIzIh5+IyCp6thUYxZZ4f0DnvvgycUbARjDvEHR HARFiq1UMHpDlZGyavXCwUWZpWb7UjESYJ31/vqFTNgjZ6TeJ4DnqP272dTNfAtN2BuG cW/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694113272; x=1694718072; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=eociOZShIIhf6uzSUtfcCoJa3P8brx2laWW2HO9Th3Y=; b=PH2Z+eWAYihggi4qVUY1xwJnXA02sLbEXXoy1lKeEBf+bRaCrY8ZyEpR0DI0XGkyW9 DKEhAxrzI/QNAhVT+bVYQntSB/SFp8NHUJWyFimv5XZ38tbXP7hYbroxgeTdx7//fkAo TdEMb1MdSBTrmxgHgJprvjLG6YTlKn4TkKPePr6kYu5atnEGYwBr6JfijM1Yr8uTGjsp CUQHcKZZwy5JGmT292vRpjn3Fxbx2gY+S3wZQV3D5VDx+mbSLDU3kIxtgW1rLsXjEW2I tTy3JH6W93JgpyvEHYRdzXmB0a/EuF1s3gv6ug476Iism2LITw7ArTDEma97atc8ZfFk 7bvA== X-Gm-Message-State: AOJu0YxgJnyf39uL8LO6L4OabT00BT8NUn/1jk9ohk72CBXADwOKdjqf N0mRzWLNlUa62DmdYOaCWIIWlw== X-Google-Smtp-Source: AGHT+IGhEgKQHBGkqLlsP4F/DGv1jyf9duZnERJIUkqYRa1UCO/fiKxyNhLBIfZMVQ0b4/1jnwd8TQ== X-Received: by 2002:a05:600c:3b1f:b0:401:b204:3b85 with SMTP id m31-20020a05600c3b1f00b00401b2043b85mr392330wms.36.1694113272221; Thu, 07 Sep 2023 12:01:12 -0700 (PDT) Received: from localhost (cst2-173-16.cust.vodafone.cz. [31.30.173.16]) by smtp.gmail.com with ESMTPSA id q10-20020adfea0a000000b0031ae2a7adb5sm109334wrm.85.2023.09.07.12.01.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Sep 2023 12:01:11 -0700 (PDT) Date: Thu, 7 Sep 2023 21:01:10 +0200 From: Andrew Jones To: Haibo Xu Cc: Haibo Xu , Paul Walmsley , Palmer Dabbelt , Albert Ou , Paolo Bonzini , Shuah Khan , Marc Zyngier , Oliver Upton , James Morse , Suzuki K Poulose , Zenghui Yu , Anup Patel , Atish Patra , Guo Ren , Conor Dooley , Greentime Hu , wchen , Daniel Henrique Barboza , Sean Christopherson , Ricardo Koller , Vishal Annapurve , David Matlack , Aaron Lewis , Mingwei Zhang , Vitaly Kuznetsov , Ackerley Tng , Vipin Sharma , Maxim Levitsky , Peter Gonda , Philippe =?utf-8?Q?Mathieu-Daud=C3=A9?= , Thomas Huth , Like Xu , David Woodhouse , Michal Luczaj , zhang songyi , 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 Message-ID: <20230907-5d3da2b6dca23bedb31b33a0@orel> References: <20230904-a69c6a228bf4553cc58bca17@orel> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="moqzehiniedyglk4" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230907_120115_203696_0111843E X-CRM114-Status: GOOD ( 37.14 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --moqzehiniedyglk4 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Thu, Sep 07, 2023 at 12:20:29PM +0800, Haibo Xu wrote: > On Mon, Sep 4, 2023 at 10:58 PM Andrew Jones 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 > > > --- > > > > 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, drew --moqzehiniedyglk4 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="simple-riscv-timer-test.c" #include #define CONFIG_64BIT #include "kvm_util.h" #include "riscv/arch_timer.h" static unsigned long timer_freq; static unsigned int irq_fired; void mdelay(unsigned long msecs) { while (msecs--) udelay(1000); } static void guest_irq_handler(struct ex_regs *regs) { GUEST_PRINTF("%s\n", __func__); GUEST_ASSERT_EQ(regs->cause & ~CAUSE_IRQ_FLAG, IRQ_S_TIMER); irq_fired = 1; csr_write(CSR_STIMECMP, -1); while (csr_read(CSR_SIP) & IE_TIE) cpu_relax(); } static void guest_code(void) { GUEST_PRINTF("stage 1\n"); mdelay(1000); GUEST_ASSERT_EQ(irq_fired, 0); GUEST_PRINTF("stage 2\n"); timer_set_next_cval_ms(500); mdelay(1000); GUEST_ASSERT_EQ(irq_fired, 1); irq_fired = 0; GUEST_PRINTF("stage 3\n"); csr_clear(CSR_SIE, IE_TIE); timer_set_next_cval_ms(500); mdelay(1000); GUEST_ASSERT_EQ(irq_fired, 0); GUEST_PRINTF("stage 4\n"); csr_set(CSR_SIE, IE_TIE); mdelay(1); GUEST_ASSERT_EQ(irq_fired, 1); irq_fired = 0; GUEST_PRINTF("stage 5\n"); csr_clear(CSR_SSTATUS, SR_IE); timer_set_next_cval_ms(500); mdelay(1000); GUEST_ASSERT_EQ(irq_fired, 0); GUEST_PRINTF("stage 6\n"); csr_set(CSR_SSTATUS, SR_IE); mdelay(1); GUEST_ASSERT_EQ(irq_fired, 1); irq_fired = 0; GUEST_PRINTF("guest done\n"); GUEST_DONE(); } int main(void) { struct kvm_vcpu *vcpu; struct kvm_vm *vm; struct ucall uc; uint64_t val; int done = 0; vm = vm_create_with_one_vcpu(&vcpu, guest_code); vm_init_vector_tables(vm); vm_install_interrupt_handler(vm, guest_irq_handler); vcpu_init_vector_tables(vcpu); vcpu_set_reg(vcpu, RISCV_CSR_REG(sstatus), SR_IE); vcpu_set_reg(vcpu, RISCV_CSR_REG(sie), IE_TIE); vcpu_get_reg(vcpu, RISCV_TIMER_REG(frequency), &timer_freq); sync_global_to_guest(vm, timer_freq); vcpu_get_reg(vcpu, RISCV_TIMER_REG(compare), &val); assert(val == -1); while (!done) { vcpu_run(vcpu); switch (get_ucall(vcpu, &uc)) { case UCALL_PRINTF: printf("%s", uc.buffer); break; case UCALL_DONE: printf("Done.\n"); done = 1; break; } } } --moqzehiniedyglk4 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --moqzehiniedyglk4--