[v2,3/3] xen/riscv: disable fpu

Commit Message

Oleksii Kurochko March 2, 2023, 1:23 p.m. UTC

Disable FPU to detect illegal usage of floating point in kernel
space.

Signed-off-by: Oleksii Kurochko <[e-mail]>
---
Changes since v1:
 * Rebase on top of two previous patches.
---
 xen/arch/riscv/setup.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Andrew Cooper March 2, 2023, 2:20 p.m. UTC | #1

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 <[e-mail]>
> ---
> 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 ?

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?

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?

~Andrew

Oleksii Kurochko March 2, 2023, 5:11 p.m. UTC | #2

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 <[e-mail]>
> > ---
> > 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

Patch

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index d9723fe1c0..23d60ed7f0 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,15 +1,23 @@ 
 #include <xen/compile.h>
 #include <xen/init.h>
 
+#include <asm/csr.h>
 #include <asm/early_printk.h>
 
 /* Xen stack for bringing up the first CPU. */
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
     __aligned(STACK_SIZE);
 
+static void __init disable_fpu(void)
+{
+    csr_write(CSR_SSTATUS, SSTATUS_FS);
+}
+
 void __init noreturn start_xen(unsigned long bootcpu_id,
                                unsigned long dtb_base)
 {
+    disable_fpu();
+
     early_printk("Hello from C env\n");
 
     early_printk("All set up\n");