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

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

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?

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.

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.

Best regards

Heinrich