Re: [PATCH 1/1] target/riscv: enable floating point unit

[Andrew Jones <ajones@ventanamicro.com>]

On Tue, Sep 17, 2024 at 03:28:42PM GMT, Heinrich Schuchardt wrote:
> On 17.09.24 14:13, Andrew Jones wrote:
> > On Mon, Sep 16, 2024 at 08:16:33PM GMT, Heinrich Schuchardt wrote:
> > > OpenSBI enables the floating point in mstatus. For consistency QEMU/KVM
> > > should do the same.
> > > 
> > > 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.
> > > 
> > > Additionally to this patch EDK II should be changed to make no assumptions
> > > about the state of the floating point unit.
> > > 
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > >   target/riscv/cpu.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 4bda754b01..c32e2721d4 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -923,6 +923,13 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
> > >       if (mcc->parent_phases.hold) {
> > >           mcc->parent_phases.hold(obj, type);
> > >       }
> > > +    if (riscv_has_ext(env, RVF) || riscv_has_ext(env, RVD)) {
> > > +        env->mstatus = set_field(env->mstatus, MSTATUS_FS, env->misa_mxl);
> > > +        for (int regnr = 0; regnr < 32; ++regnr) {
> > > +            env->fpr[regnr] = 0;
> > > +        }
> > > +        riscv_csrrw(env, CSR_FCSR, NULL, 0, -1);
> > > +    }
> > 
> > If this is only fixing KVM, then I think it belongs in
> > kvm_riscv_reset_vcpu(). But, I feel like we're working around an issue
> > with KVM synchronization with this, as well as with the "clear CSR values"
> > part of commit 8633951530cc ("target/riscv: Clear CSR values at reset and
> > sync MPSTATE with host"). KVM knows how to reset VCPUs. It does so on
> > VCPU creation and for any secondaries started with SBI HSM start. KVM's
> > reset would set sstatus.FS to 1 ("Initial") and zero out all the fp
> > registers and fcsr. So it seems like we're either synchronizing prior to
> > KVM resetting the boot VCPU, not synchronizing at all, or KVM isn't doing
> > the reset of the boot VCPU.
> > 
> > Thanks,
> > drew
> 
> Hello Drew,
> 
> Thanks for reviewing.
> 
> Concerning the question whether kvm_riscv_reset_vcpu() would be a better
> place for the change:
> 
> Is there any specification prescribing what the state of the FS bits should
> be when entering M-mode and when entering S-mode?

I didn't see anything in the spec, so I think 0 (or 1 when all fp
registers are also reset) is reasonable for an implementation to
choose.

> 
> Patch 8633951530cc seems not to touch the status register in QEMU's
> kvm_riscv_reset_vcpu(). So it is not obvious that this patch could have
> caused the problem.

I don't think 8633951530cc caused this problem. It was solving its own
problem in the same way, which is to add some more reset for the VCPU.
I think both it and this patch are working around a problem with KVM or
with a problem synchronizing with KVM. If that's the case, and we fix
KVM or the synchronization with KVM, then I would revert the reset parts
of 8633951530cc too.

> 
> Looking at the call sequences in Linux gives some ideas where to debug:
> 
> kvm_arch_vcpu_create calls kvm_riscv_reset_vcpu which calls
> kvm_riscv_vcpu_fp_reset.
> 
> riscv_vcpu_set_isa_ext_single and kvm_riscv_vcpu_set_reg_config
> only call kvm_riscv_vcpu_fp_reset if !vcpu->arch.ran_atleast_once.
> 
> kvm_riscv_vcpu_fp_reset sets FS bits to "initial"
> if CONFIG_FPU=y and extension F or D is available.
> 
> It seems that in KVM only the creation of a vcpu will set the FS bits but
> rebooting will not.

If KVM never resets the boot VCPU on reboot, then maybe it should or needs
QEMU to inform it to do so. I'd rather just one of the two (KVM or QEMU)
decide what needs to be reset and to which values, rather than both having
their own ideas. For example, with this patch, the boot hart will have its
sstatus.FS set to 3, but, iiuc, all secondaries will be brought up
with their sstatus.FS set to 1.

Thanks,
drew