All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Jones <ajones@ventanamicro.com>
To: kvm-riscv@lists.infradead.org
Subject: [PATCH 1/1] target/riscv: enable floating point unit
Date: Wed, 18 Sep 2024 15:56:58 +0200	[thread overview]
Message-ID: <20240918-4e2df3f0cabdb8002d7315d9@orel> (raw)
In-Reply-To: <f1e41b95-c499-4e06-91cb-006dcd9d29e6@canonical.com>

On Wed, Sep 18, 2024 at 03:49:39PM GMT, Heinrich Schuchardt wrote:
> On 18.09.24 15:12, Peter Maydell wrote:
> > On Wed, 18 Sept 2024 at 14:06, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > > Thanks Peter for looking into this.
> > > 
> > > QEMU's cpu_synchronize_all_post_init() and
> > > do_kvm_cpu_synchronize_post_reset() both end up in
> > > kvm_arch_put_registers() and that is long after Linux
> > > kvm_arch_vcpu_create() has been setting some FPU state. See the output
> > > below.
> > > 
> > > kvm_arch_put_registers() copies the CSRs by calling
> > > kvm_riscv_put_regs_csr(). Here we can find:
> > > 
> > >       KVM_RISCV_SET_CSR(cs, env, sstatus, env->mstatus);
> > > 
> > > This call enables or disables the FPU according to the value of
> > > env->mstatus.
> > > 
> > > So we need to set the desired state of the floating point unit in QEMU.
> > > And this is what the current patch does both for TCG and KVM.
> > 
> > If it does this for both TCG and KVM then I don't understand
> > this bit from the commit message:
> > 
> > # Without this patch EDK II with TLS enabled crashes when hitting the first
> > # floating point instruction while running QEMU with --accel kvm and runs
> > # fine with --accel tcg.
> > 
> > Shouldn't this guest crash the same way with both KVM and TCG without
> > this patch, because the FPU state is the same for both?
> > 
> > -- PMM
> 
> By default `qemu-system-riscv64 --accel tcg` runs OpenSBI as firmware which
> enables the FPU.
> 
> If you would choose a different SBI implementation which does not enable the
> FPU you could experience the same crash.
> 

Thanks Heinrich, I had also forgotten that distinction. So the last
question is whether or not we want to reset mstatus.FS to 1 instead of 3,
as is done in this patch.

Thanks,
drew


WARNING: multiple messages have this Message-ID (diff)
From: Andrew Jones <ajones@ventanamicro.com>
To: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	 Palmer Dabbelt <palmer@dabbelt.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	 Bin Meng <bmeng.cn@gmail.com>, Weiwei Li <liwei1518@gmail.com>,
	 Daniel Henrique Barboza <dbarboza@ventanamicro.com>,
	Liu Zhiwei <zhiwei_liu@linux.alibaba.com>,
	qemu-riscv@nongnu.org,  qemu-devel@nongnu.org,
	Anup Patel <anup@brainfault.org>,
	 Atish Patra <atishp@atishpatra.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	 Albert Ou <aou@eecs.berkeley.edu>,
	kvm@vger.kernel.org, kvm-riscv@lists.infradead.org,
	 linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] target/riscv: enable floating point unit
Date: Wed, 18 Sep 2024 15:56:58 +0200	[thread overview]
Message-ID: <20240918-4e2df3f0cabdb8002d7315d9@orel> (raw)
In-Reply-To: <f1e41b95-c499-4e06-91cb-006dcd9d29e6@canonical.com>

On Wed, Sep 18, 2024 at 03:49:39PM GMT, Heinrich Schuchardt wrote:
> On 18.09.24 15:12, Peter Maydell wrote:
> > On Wed, 18 Sept 2024 at 14:06, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > > Thanks Peter for looking into this.
> > > 
> > > QEMU's cpu_synchronize_all_post_init() and
> > > do_kvm_cpu_synchronize_post_reset() both end up in
> > > kvm_arch_put_registers() and that is long after Linux
> > > kvm_arch_vcpu_create() has been setting some FPU state. See the output
> > > below.
> > > 
> > > kvm_arch_put_registers() copies the CSRs by calling
> > > kvm_riscv_put_regs_csr(). Here we can find:
> > > 
> > >       KVM_RISCV_SET_CSR(cs, env, sstatus, env->mstatus);
> > > 
> > > This call enables or disables the FPU according to the value of
> > > env->mstatus.
> > > 
> > > So we need to set the desired state of the floating point unit in QEMU.
> > > And this is what the current patch does both for TCG and KVM.
> > 
> > If it does this for both TCG and KVM then I don't understand
> > this bit from the commit message:
> > 
> > # Without this patch EDK II with TLS enabled crashes when hitting the first
> > # floating point instruction while running QEMU with --accel kvm and runs
> > # fine with --accel tcg.
> > 
> > Shouldn't this guest crash the same way with both KVM and TCG without
> > this patch, because the FPU state is the same for both?
> > 
> > -- PMM
> 
> By default `qemu-system-riscv64 --accel tcg` runs OpenSBI as firmware which
> enables the FPU.
> 
> If you would choose a different SBI implementation which does not enable the
> FPU you could experience the same crash.
> 

Thanks Heinrich, I had also forgotten that distinction. So the last
question is whether or not we want to reset mstatus.FS to 1 instead of 3,
as is done in this patch.

Thanks,
drew

WARNING: multiple messages have this Message-ID (diff)
From: Andrew Jones <ajones@ventanamicro.com>
To: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	 Palmer Dabbelt <palmer@dabbelt.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	 Bin Meng <bmeng.cn@gmail.com>, Weiwei Li <liwei1518@gmail.com>,
	 Daniel Henrique Barboza <dbarboza@ventanamicro.com>,
	Liu Zhiwei <zhiwei_liu@linux.alibaba.com>,
	qemu-riscv@nongnu.org,  qemu-devel@nongnu.org,
	Anup Patel <anup@brainfault.org>,
	 Atish Patra <atishp@atishpatra.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	 Albert Ou <aou@eecs.berkeley.edu>,
	kvm@vger.kernel.org, kvm-riscv@lists.infradead.org,
	 linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] target/riscv: enable floating point unit
Date: Wed, 18 Sep 2024 15:56:58 +0200	[thread overview]
Message-ID: <20240918-4e2df3f0cabdb8002d7315d9@orel> (raw)
In-Reply-To: <f1e41b95-c499-4e06-91cb-006dcd9d29e6@canonical.com>

On Wed, Sep 18, 2024 at 03:49:39PM GMT, Heinrich Schuchardt wrote:
> On 18.09.24 15:12, Peter Maydell wrote:
> > On Wed, 18 Sept 2024 at 14:06, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > > Thanks Peter for looking into this.
> > > 
> > > QEMU's cpu_synchronize_all_post_init() and
> > > do_kvm_cpu_synchronize_post_reset() both end up in
> > > kvm_arch_put_registers() and that is long after Linux
> > > kvm_arch_vcpu_create() has been setting some FPU state. See the output
> > > below.
> > > 
> > > kvm_arch_put_registers() copies the CSRs by calling
> > > kvm_riscv_put_regs_csr(). Here we can find:
> > > 
> > >       KVM_RISCV_SET_CSR(cs, env, sstatus, env->mstatus);
> > > 
> > > This call enables or disables the FPU according to the value of
> > > env->mstatus.
> > > 
> > > So we need to set the desired state of the floating point unit in QEMU.
> > > And this is what the current patch does both for TCG and KVM.
> > 
> > If it does this for both TCG and KVM then I don't understand
> > this bit from the commit message:
> > 
> > # Without this patch EDK II with TLS enabled crashes when hitting the first
> > # floating point instruction while running QEMU with --accel kvm and runs
> > # fine with --accel tcg.
> > 
> > Shouldn't this guest crash the same way with both KVM and TCG without
> > this patch, because the FPU state is the same for both?
> > 
> > -- PMM
> 
> By default `qemu-system-riscv64 --accel tcg` runs OpenSBI as firmware which
> enables the FPU.
> 
> If you would choose a different SBI implementation which does not enable the
> FPU you could experience the same crash.
> 

Thanks Heinrich, I had also forgotten that distinction. So the last
question is whether or not we want to reset mstatus.FS to 1 instead of 3,
as is done in this patch.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2024-09-18 13:56 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-09-16 18:16 [PATCH 1/1] target/riscv: enable floating point unit Heinrich Schuchardt
2024-09-17 12:13 ` Andrew Jones
2024-09-17 13:28   ` Heinrich Schuchardt
2024-09-17 14:49     ` Andrew Jones
2024-09-17 16:45       ` Heinrich Schuchardt
2024-09-17 16:45         ` Heinrich Schuchardt
2024-09-17 16:45         ` Heinrich Schuchardt
2024-09-18  6:05         ` Andrew Jones
2024-09-18  6:05           ` Andrew Jones
2024-09-18  6:05           ` Andrew Jones
2024-09-18 11:10           ` Peter Maydell
2024-09-18 11:10             ` Peter Maydell
2024-09-18 11:10             ` Peter Maydell
2024-09-18 13:06             ` Heinrich Schuchardt
2024-09-18 13:06               ` Heinrich Schuchardt
2024-09-18 13:06               ` Heinrich Schuchardt
2024-09-18 13:12               ` Peter Maydell
2024-09-18 13:12                 ` Peter Maydell
2024-09-18 13:12                 ` Peter Maydell
2024-09-18 13:49                 ` Heinrich Schuchardt
2024-09-18 13:49                   ` Heinrich Schuchardt
2024-09-18 13:49                   ` Heinrich Schuchardt
2024-09-18 13:56                   ` Andrew Jones [this message]
2024-09-18 13:56                     ` Andrew Jones
2024-09-18 13:56                     ` Andrew Jones
2024-09-18 15:49                     ` Heinrich Schuchardt
2024-09-18 15:49                       ` Heinrich Schuchardt
2024-09-18 15:49                       ` Heinrich Schuchardt
2024-09-18 15:32                   ` Peter Maydell
2024-09-18 15:32                     ` Peter Maydell
2024-09-18 15:32                     ` Peter Maydell
2024-09-18 16:27                     ` Heinrich Schuchardt
2024-09-18 16:27                       ` Heinrich Schuchardt
2024-09-18 16:27                       ` Heinrich Schuchardt
2024-10-08  0:45                     ` Alistair Francis
2024-10-08  0:45                       ` Alistair Francis
2024-10-08  0:45                       ` Alistair Francis
2024-09-18 14:38 ` Richard Henderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240918-4e2df3f0cabdb8002d7315d9@orel \
    --to=ajones@ventanamicro.com \
    --cc=kvm-riscv@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.