Re: [PATCH v2 3/3] xen/riscv: disable fpu

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-03-02 at 14:20 +0000, Andrew Cooper wrote:
> On 02/03/2023 1:23 pm, Oleksii Kurochko wrote:
> > Disable FPU to detect illegal usage of floating point in kernel
> > space.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes since v1:
> >  * Rebase on top of two previous patches.
> > ---
> 
> Apologies - I meant to ask these on the previous series, but didn't
> get
> around to it.
> 
> Why do we disable interrupts at the very start of start(), but only
> disable the FPU at the start of C ?
I decided to do at the start of start_xen() as it's the first C
function and before that there is only assembler where we can control
not to use FPU.

But to be 100% sure we can move to the start() function.
Could you please share your thoughts about?
> 
> To start with, doesn't OpenSBI have a starting ABI spec?  What does
> that
> say on the matter of the enablement of these features on entry into
> the
> environment?
I tried to find specific OpenSBI ABI spec before and, unfortunately, i
didn't find any. Only docs in their repo:
https://github.com/riscv-software-src/opensbi/blob/master/docs/firmware/fw.md
My expactation was that such information should be part of RISC-V
SBI/ABI which OpenSBI implements but it is mentioned only SBI functions
that should be implemented.

I look at OpenSBI code and it looks like it disables interrupts before
jump to hypervisor:
https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L805
But it doesn't do anything with FPU.

Thereby I can't be sure that it's mandatory or not for OpenSBI to
disable/enable interrupts, FPU and so on.

If you have or saw the OpenSBI starting ABI please let me know.

> 
> Either way, my gut feeling is that these disables (if necessary to
> begin
> with) should be together, rather than split like this.
> 
> 
> That aside, while I can see the value of checking this now, won't we
> have to delete this again in order to allow for context switching a
> vCPUs FPU register state?
Not really.

My expectation that we will have the function similar to:
void cpu_vcpu_fp_init(...)
{
        riscv_regs(vcpu)->sstatus &= ~SSTATUS_FS;
        if (riscv_isa_extension_available(riscv_priv(vcpu)->isa, f) ||
            riscv_isa_extension_available(riscv_priv(vcpu)->isa, d))
                riscv_regs(vcpu)->sstatus |= SSTATUS_FS_INITIAL;
        else
                ....
        memset(&riscv_priv(vcpu)->fp, 0, sizeof(riscv_priv(vcpu)-
>fp));
}


~ Oleksii