[PATCH v2 0/3] Do basic initialization things

[PATCH v2 0/3] Do basic initialization things

[Oleksii Kurochko]

The patch series groups and updates the following patches:
1. xen/riscv: disable fpu
2. xen/riscv: initialize .bss section
3. xen/riscv: read/save hart_id and dtb_base passed by bootloader

---
Changes since v1:
 * initialization of .bss was moved to head.S
 * read/save/pass of hart_id and  dtb_base passed by a bootloader
   were moved to head.S. Also, it was updated start_xen() arguments
   to recieve hard_id & dtb_base
---

Oleksii Kurochko (3):
  xen/riscv: read/save hart_id and dtb_base passed by bootloader
  xen/riscv: initialize .bss section
  xen/riscv: disable fpu

 xen/arch/riscv/include/asm/asm.h |  4 ++++
 xen/arch/riscv/riscv64/head.S    | 33 ++++++++++++++++++++++++++++++++
 xen/arch/riscv/setup.c           | 11 ++++++++++-
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.39.0

[PATCH v2 1/3] xen/riscv: read/save hart_id and dtb_base passed by bootloader

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes since v1:
 * read/save/pass of hart_id and  dtb_base passed by a bootloader
   were moved to head.S. 
 * Update start_xen() to recieve hard_id & dtb_base
---
 xen/arch/riscv/riscv64/head.S | 24 ++++++++++++++++++++++++
 xen/arch/riscv/setup.c        |  3 ++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index ffd95f9f89..851b4691a5 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -1,3 +1,4 @@
+#include <asm/asm.h>
 #include <asm/riscv_encoding.h>
 
         .section .text.header, "ax", %progbits
@@ -6,8 +7,31 @@ ENTRY(start)
         /* Mask all interrupts */
         csrw    CSR_SIE, zero
 
+        /* Save HART ID and DTB base */
+        lla     a6, _bootcpu_id
+        REG_S   a0, (a6)
+        lla     a6, _dtb_base
+        REG_S   a1, (a6)
+
         la      sp, cpu0_boot_stack
         li      t0, STACK_SIZE
         add     sp, sp, t0
 
+        lla     a6, _bootcpu_id
+        REG_L   a0, (a6)
+        lla     a6, _dtb_base
+        REG_L   a1, (a6)
+
         tail    start_xen
+
+        /*
+         * Boot cpu id is passed by a bootloader
+         */
+_bootcpu_id:
+        RISCV_PTR 0x0
+
+        /*
+         * DTB base is passed by a bootloader
+         */
+_dtb_base:
+        RISCV_PTR 0x0
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 1c87899e8e..d9723fe1c0 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -7,7 +7,8 @@
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
     __aligned(STACK_SIZE);
 
-void __init noreturn start_xen(void)
+void __init noreturn start_xen(unsigned long bootcpu_id,
+                               unsigned long dtb_base)
 {
     early_printk("Hello from C env\n");
 
-- 
2.39.0

Re: [PATCH v2 1/3] xen/riscv: read/save hart_id and dtb_base passed by bootloader

[Andrew Cooper]

On 02/03/2023 1:23 pm, Oleksii Kurochko wrote:
> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
> index ffd95f9f89..851b4691a5 100644
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -6,8 +7,31 @@ ENTRY(start)
>          /* Mask all interrupts */
>          csrw    CSR_SIE, zero
>  
> +        /* Save HART ID and DTB base */
> +        lla     a6, _bootcpu_id
> +        REG_S   a0, (a6)
> +        lla     a6, _dtb_base
> +        REG_S   a1, (a6)
> +
>          la      sp, cpu0_boot_stack
>          li      t0, STACK_SIZE
>          add     sp, sp, t0
>  
> +        lla     a6, _bootcpu_id
> +        REG_L   a0, (a6)
> +        lla     a6, _dtb_base
> +        REG_L   a1, (a6)

This is overkill.

Put a comment at start identifying which parameters are in which
registers, and just make sure not to clobber them - RISCV has plenty of
registers.

Right now, we don't touch the a registers at all, which is why your v1
patch worked.  (a0 and a1 still have the same value when we get into C).

If we do need to start calling more complex things here (and I'm not
sure that we do), we could either store the parameters in s2-11, or
spill them onto the stack; both of which are preferable to spilling to
global variables like this.

> +
>          tail    start_xen
> +
> +        /*
> +         * Boot cpu id is passed by a bootloader
> +         */
> +_bootcpu_id:
> +        RISCV_PTR 0x0

Just a note (as you want to delete this anyway), this isn't a PTR, it's
a LONG.

> +
> +        /*
> +         * DTB base is passed by a bootloader
> +         */
> +_dtb_base:
> +        RISCV_PTR 0x0
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index 1c87899e8e..d9723fe1c0 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -7,7 +7,8 @@
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
> -void __init noreturn start_xen(void)
> +void __init noreturn start_xen(unsigned long bootcpu_id,
> +                               unsigned long dtb_base)

To be clear, this change should be this hunk exactly as it is, and a
comment immediately ahead of ENTRY(start) describing the entry ABI.

There is no need currently to change any of the asm code.

~Andrew

Re: [PATCH v2 1/3] xen/riscv: read/save hart_id and dtb_base passed by bootloader

[Oleksii]

On Thu, 2023-03-02 at 14:02 +0000, Andrew Cooper wrote:
> On 02/03/2023 1:23 pm, Oleksii Kurochko wrote:
> > diff --git a/xen/arch/riscv/riscv64/head.S
> > b/xen/arch/riscv/riscv64/head.S
> > index ffd95f9f89..851b4691a5 100644
> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -6,8 +7,31 @@ ENTRY(start)
> >          /* Mask all interrupts */
> >          csrw    CSR_SIE, zero
> >  
> > +        /* Save HART ID and DTB base */
> > +        lla     a6, _bootcpu_id
> > +        REG_S   a0, (a6)
> > +        lla     a6, _dtb_base
> > +        REG_S   a1, (a6)
> > +
> >          la      sp, cpu0_boot_stack
> >          li      t0, STACK_SIZE
> >          add     sp, sp, t0
> >  
> > +        lla     a6, _bootcpu_id
> > +        REG_L   a0, (a6)
> > +        lla     a6, _dtb_base
> > +        REG_L   a1, (a6)
> 
> This is overkill.
> 
> Put a comment at start identifying which parameters are in which
> registers, and just make sure not to clobber them - RISCV has plenty
> of
> registers.
> 
> Right now, we don't touch the a registers at all, which is why your
> v1
> patch worked.  (a0 and a1 still have the same value when we get into
> C).
> 
> If we do need to start calling more complex things here (and I'm not
> sure that we do), we could either store the parameters in s2-11, or
> spill them onto the stack; both of which are preferable to spilling
> to
> global variables like this.
> 
> > +
> >          tail    start_xen
> > +
> > +        /*
> > +         * Boot cpu id is passed by a bootloader
> > +         */
> > +_bootcpu_id:
> > +        RISCV_PTR 0x0
> 
> Just a note (as you want to delete this anyway), this isn't a PTR,
> it's
> a LONG.
> 
> > +
> > +        /*
> > +         * DTB base is passed by a bootloader
> > +         */
> > +_dtb_base:
> > +        RISCV_PTR 0x0
> > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> > index 1c87899e8e..d9723fe1c0 100644
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -7,7 +7,8 @@
> >  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
> >      __aligned(STACK_SIZE);
> >  
> > -void __init noreturn start_xen(void)
> > +void __init noreturn start_xen(unsigned long bootcpu_id,
> > +                               unsigned long dtb_base)
> 
> To be clear, this change should be this hunk exactly as it is, and a
> comment immediately ahead of ENTRY(start) describing the entry ABI.
> 
> There is no need currently to change any of the asm code.
I think that I'll use s2 and s3 to save bootcpu_id.

But I am unsure I understand why the asm code shouldn't be changed.

I mean that a0-7 are used as function arguments, a0-1 are used for
return value so they are expected to be changed. That is why we have to
save them somewhere.

If I understand you correctly I can write in a comment ahead of
ENTRY(start) that a0, and a1 are reserved for hart_id and dtb_base
which are passed from a bootloader but it will work only if start_xen
will be only C function called from head.S.

I probably misunderstand you...

~ Oleksii

Re: [PATCH v2 1/3] xen/riscv: read/save hart_id and dtb_base passed by bootloader

[Andrew Cooper]

On 02/03/2023 2:53 pm, Oleksii wrote:
> On Thu, 2023-03-02 at 14:02 +0000, Andrew Cooper wrote:
>> On 02/03/2023 1:23 pm, Oleksii Kurochko wrote:
>>> +
>>> +        /*
>>> +         * DTB base is passed by a bootloader
>>> +         */
>>> +_dtb_base:
>>> +        RISCV_PTR 0x0
>>> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
>>> index 1c87899e8e..d9723fe1c0 100644
>>> --- a/xen/arch/riscv/setup.c
>>> +++ b/xen/arch/riscv/setup.c
>>> @@ -7,7 +7,8 @@
>>>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>>>      __aligned(STACK_SIZE);
>>>  
>>> -void __init noreturn start_xen(void)
>>> +void __init noreturn start_xen(unsigned long bootcpu_id,
>>> +                               unsigned long dtb_base)
>> To be clear, this change should be this hunk exactly as it is, and a
>> comment immediately ahead of ENTRY(start) describing the entry ABI.
>>
>> There is no need currently to change any of the asm code.
> I think that I'll use s2 and s3 to save bootcpu_id.
>
> But I am unsure I understand why the asm code shouldn't be changed.

Because nothing in the asm code (right now) touches any of the a registers.

Therefore the parameters that OpenSBI prepared for start() (i.e. a0 and
a1 here) are still correct for start_xen().

If, and only if, we need to modify a* for other reasons in start() do we
need to preserve their values somehow.

> If I understand you correctly I can write in a comment ahead of
> ENTRY(start) that a0, and a1 are reserved for hart_id and dtb_base
> which are passed from a bootloader but it will work only if start_xen
> will be only C function called from head.S.

Not quite.  You want a comment explaining what the OpenSBI -> start()
ABI is.  So people know what a0/etc is at ENTRY(start).

Here is an example from a different project:
https://github.com/TrenchBoot/secure-kernel-loader/blob/master/head.S#L52-L68


There is no need to do unnecessary work (i.e. preserving them right
now), until you find a reason to need to spill them.  Right now, there's
not need, and this isn't obviously going to change in the short term.

~Andrew

Re: [PATCH v2 1/3] xen/riscv: read/save hart_id and dtb_base passed by bootloader

[Oleksii]

On Thu, 2023-03-02 at 18:58 +0000, Andrew Cooper wrote:
> On 02/03/2023 2:53 pm, Oleksii wrote:
> > On Thu, 2023-03-02 at 14:02 +0000, Andrew Cooper wrote:
> > > On 02/03/2023 1:23 pm, Oleksii Kurochko wrote:
> > > > +
> > > > +        /*
> > > > +         * DTB base is passed by a bootloader
> > > > +         */
> > > > +_dtb_base:
> > > > +        RISCV_PTR 0x0
> > > > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> > > > index 1c87899e8e..d9723fe1c0 100644
> > > > --- a/xen/arch/riscv/setup.c
> > > > +++ b/xen/arch/riscv/setup.c
> > > > @@ -7,7 +7,8 @@
> > > >  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
> > > >      __aligned(STACK_SIZE);
> > > >  
> > > > -void __init noreturn start_xen(void)
> > > > +void __init noreturn start_xen(unsigned long bootcpu_id,
> > > > +                               unsigned long dtb_base)
> > > To be clear, this change should be this hunk exactly as it is,
> > > and a
> > > comment immediately ahead of ENTRY(start) describing the entry
> > > ABI.
> > > 
> > > There is no need currently to change any of the asm code.
> > I think that I'll use s2 and s3 to save bootcpu_id.
> > 
> > But I am unsure I understand why the asm code shouldn't be changed.
> 
> Because nothing in the asm code (right now) touches any of the a
> registers.
> 
> Therefore the parameters that OpenSBI prepared for start() (i.e. a0
> and
> a1 here) are still correct for start_xen().
> 
> If, and only if, we need to modify a* for other reasons in start() do
> we
> need to preserve their values somehow.
> 
> > If I understand you correctly I can write in a comment ahead of
> > ENTRY(start) that a0, and a1 are reserved for hart_id and dtb_base
> > which are passed from a bootloader but it will work only if
> > start_xen
> > will be only C function called from head.S.
> 
> Not quite.  You want a comment explaining what the OpenSBI -> start()
> ABI is.  So people know what a0/etc is at ENTRY(start).
> 
> Here is an example from a different project:
> https://github.com/TrenchBoot/secure-kernel-loader/blob/master/head.S#L52-L68
> 
> 
> There is no need to do unnecessary work (i.e. preserving them right
> now), until you find a reason to need to spill them.  Right now,
> there's
> not need, and this isn't obviously going to change in the short term.
> 
Thanks for clarification.

I will document ABI for start and that's it for now
~ Oleksii

[PATCH v2 2/3] xen/riscv: initialize .bss section

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes since v1:
 * initialization of .bss was moved to head.S
---
 xen/arch/riscv/include/asm/asm.h | 4 ++++
 xen/arch/riscv/riscv64/head.S    | 9 +++++++++
 2 files changed, 13 insertions(+)

diff --git a/xen/arch/riscv/include/asm/asm.h b/xen/arch/riscv/include/asm/asm.h
index 6d426ecea7..5208529cb4 100644
--- a/xen/arch/riscv/include/asm/asm.h
+++ b/xen/arch/riscv/include/asm/asm.h
@@ -26,14 +26,18 @@
 #if __SIZEOF_POINTER__ == 8
 #ifdef __ASSEMBLY__
 #define RISCV_PTR		.dword
+#define RISCV_SZPTR		8
 #else
 #define RISCV_PTR		".dword"
+#define RISCV_SZPTR		8
 #endif
 #elif __SIZEOF_POINTER__ == 4
 #ifdef __ASSEMBLY__
 #define RISCV_PTR		.word
+#define RISCV_SZPTR		4
 #else
 #define RISCV_PTR		".word"
+#define RISCV_SZPTR		4
 #endif
 #else
 #error "Unexpected __SIZEOF_POINTER__"
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 851b4691a5..b139976b7a 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -13,6 +13,15 @@ ENTRY(start)
         lla     a6, _dtb_base
         REG_S   a1, (a6)
 
+        la      a3, __bss_start
+        la      a4, __bss_end
+        ble     a4, a3, clear_bss_done
+clear_bss:
+        REG_S   zero, (a3)
+        add     a3, a3, RISCV_SZPTR
+        blt     a3, a4, clear_bss
+clear_bss_done:
+
         la      sp, cpu0_boot_stack
         li      t0, STACK_SIZE
         add     sp, sp, t0
-- 
2.39.0

Re: [PATCH v2 2/3] xen/riscv: initialize .bss section

[Andrew Cooper]

On 02/03/2023 1:23 pm, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes since v1:
>  * initialization of .bss was moved to head.S
> ---
>  xen/arch/riscv/include/asm/asm.h | 4 ++++
>  xen/arch/riscv/riscv64/head.S    | 9 +++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/xen/arch/riscv/include/asm/asm.h b/xen/arch/riscv/include/asm/asm.h
> index 6d426ecea7..5208529cb4 100644
> --- a/xen/arch/riscv/include/asm/asm.h
> +++ b/xen/arch/riscv/include/asm/asm.h
> @@ -26,14 +26,18 @@
>  #if __SIZEOF_POINTER__ == 8
>  #ifdef __ASSEMBLY__
>  #define RISCV_PTR		.dword
> +#define RISCV_SZPTR		8
>  #else
>  #define RISCV_PTR		".dword"
> +#define RISCV_SZPTR		8
>  #endif
>  #elif __SIZEOF_POINTER__ == 4
>  #ifdef __ASSEMBLY__
>  #define RISCV_PTR		.word
> +#define RISCV_SZPTR		4
>  #else
>  #define RISCV_PTR		".word"
> +#define RISCV_SZPTR		4

This an extremely verbose way of saying that __SIZEOF_POINTER__ is the
right value to use...

Just drop the change here.  The code is better without this indirection.

>  #endif
>  #else
>  #error "Unexpected __SIZEOF_POINTER__"
> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
> index 851b4691a5..b139976b7a 100644
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -13,6 +13,15 @@ ENTRY(start)
>          lla     a6, _dtb_base
>          REG_S   a1, (a6)
>  

/* Clear the BSS */

The comments (even just oneliners) will become increasingly useful as
the logic here grows.

> +        la      a3, __bss_start
> +        la      a4, __bss_end
> +        ble     a4, a3, clear_bss_done
> +clear_bss:
> +        REG_S   zero, (a3)
> +        add     a3, a3, RISCV_SZPTR
> +        blt     a3, a4, clear_bss
> +clear_bss_done:

You should use t's here, not a's.  What we are doing here is temporary
and not constructing arguments to a function.  Furthermore we want to
preserve the a's where possible to avoid spilling the parameters.

Finally, the symbols should have an .L_ prefix to make the local symbols.

It really doesn't matter now, but will when you're retrofitting elf
metadata to asm code to make livepatching work.  (I'm doing this on x86
and it would have been easier if people had written the code nicely the
first time around.)

~Andrew

Re: [PATCH v2 2/3] xen/riscv: initialize .bss section

[Oleksii]

On Thu, 2023-03-02 at 14:12 +0000, Andrew Cooper wrote:
> On 02/03/2023 1:23 pm, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes since v1:
> >  * initialization of .bss was moved to head.S
> > ---
> >  xen/arch/riscv/include/asm/asm.h | 4 ++++
> >  xen/arch/riscv/riscv64/head.S    | 9 +++++++++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/xen/arch/riscv/include/asm/asm.h
> > b/xen/arch/riscv/include/asm/asm.h
> > index 6d426ecea7..5208529cb4 100644
> > --- a/xen/arch/riscv/include/asm/asm.h
> > +++ b/xen/arch/riscv/include/asm/asm.h
> > @@ -26,14 +26,18 @@
> >  #if __SIZEOF_POINTER__ == 8
> >  #ifdef __ASSEMBLY__
> >  #define RISCV_PTR              .dword
> > +#define RISCV_SZPTR            8
> >  #else
> >  #define RISCV_PTR              ".dword"
> > +#define RISCV_SZPTR            8
> >  #endif
> >  #elif __SIZEOF_POINTER__ == 4
> >  #ifdef __ASSEMBLY__
> >  #define RISCV_PTR              .word
> > +#define RISCV_SZPTR            4
> >  #else
> >  #define RISCV_PTR              ".word"
> > +#define RISCV_SZPTR            4
> 
> This an extremely verbose way of saying that __SIZEOF_POINTER__ is
> the
> right value to use...
> 
> Just drop the change here.  The code is better without this
> indirection.
> 
> >  #endif
> >  #else
> >  #error "Unexpected __SIZEOF_POINTER__"
> > diff --git a/xen/arch/riscv/riscv64/head.S
> > b/xen/arch/riscv/riscv64/head.S
> > index 851b4691a5..b139976b7a 100644
> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -13,6 +13,15 @@ ENTRY(start)
> >          lla     a6, _dtb_base
> >          REG_S   a1, (a6)
> >  
> 
> /* Clear the BSS */
> 
> The comments (even just oneliners) will become increasingly useful as
> the logic here grows.
> 
> > +        la      a3, __bss_start
> > +        la      a4, __bss_end
> > +        ble     a4, a3, clear_bss_done
> > +clear_bss:
> > +        REG_S   zero, (a3)
> > +        add     a3, a3, RISCV_SZPTR
> > +        blt     a3, a4, clear_bss
> > +clear_bss_done:
> 
> You should use t's here, not a's.  What we are doing here is
> temporary
> and not constructing arguments to a function.  Furthermore we want to
> preserve the a's where possible to avoid spilling the parameters.
> 
> Finally, the symbols should have an .L_ prefix to make the local
> symbols.
> 
> It really doesn't matter now, but will when you're retrofitting elf
> metadata to asm code to make livepatching work.  (I'm doing this on
> x86
> and it would have been easier if people had written the code nicely
> the
> first time around.)
Thanks. I'll update the code.
> 
> ~Andrew

Re: [PATCH v2 2/3] xen/riscv: initialize .bss section

[Jan Beulich]

On 02.03.2023 14:23, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -13,6 +13,15 @@ ENTRY(start)
>          lla     a6, _dtb_base
>          REG_S   a1, (a6)
>  
> +        la      a3, __bss_start
> +        la      a4, __bss_end
> +        ble     a4, a3, clear_bss_done

While it may be that .bss is indeed empty right now, even short term
it won't be, and never will. I'd drop this conditional (and in
particular the label), inserting a transient item into .bss for the
time being. As soon as your patch introducing page tables has landed,
there will be multiple pages worth of .bss.

Also are this and ...

> +clear_bss:
> +        REG_S   zero, (a3)
> +        add     a3, a3, RISCV_SZPTR
> +        blt     a3, a4, clear_bss

... this branch actually the correct ones? I'd expect the unsigned
flavors to be used when comparing addresses. It may not matter here
and/or right now, but it'll set a bad precedent unless you expect
to only ever work on addresses which have the sign bit clear.

Jan

> +clear_bss_done:
> +
>          la      sp, cpu0_boot_stack
>          li      t0, STACK_SIZE
>          add     sp, sp, t0

Re: [PATCH v2 2/3] xen/riscv: initialize .bss section

[Oleksii]

On Thu, 2023-03-02 at 15:22 +0100, Jan Beulich wrote:
> On 02.03.2023 14:23, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -13,6 +13,15 @@ ENTRY(start)
> >          lla     a6, _dtb_base
> >          REG_S   a1, (a6)
> >  
> > +        la      a3, __bss_start
> > +        la      a4, __bss_end
> > +        ble     a4, a3, clear_bss_done
> 
> While it may be that .bss is indeed empty right now, even short term
> it won't be, and never will. I'd drop this conditional (and in
> particular the label), inserting a transient item into .bss for the
> time being. As soon as your patch introducing page tables has landed,
> there will be multiple pages worth of .bss.
If I understand you correctly you suggested declare some variable:
   int dummy_bss __attribute__((unused));

Then .bss won't be zero:
   $ riscv64-linux-gnu-objdump -x xen/xen-syms | grep -i dummy_bss    
   0000000080205000 g     O .bss   0000000000000004 .hidden dummy_bss

And when page tables will be ready it will be needed to remove
dummy_bss.

Another one option is to update linker script ( looks better then
previous one ):
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -140,6 +140,7 @@ SECTIONS
         . = ALIGN(SMP_CACHE_BYTES);
         __per_cpu_data_end = .;
         *(.bss .bss.*)
+        . = . + 1;
         . = ALIGN(POINTER_ALIGN);
         __bss_end = .;
     } :text

If one of the options is fine then to be honest I am not sure that I
understand why it is better than have 3 instructions which will be
unnecessary when first bss variable will be introduced. And actually
the same will be with item in bss, it will become unnecessary when
something from bss will be introduced.

I am OK with one of the mentioned above options but still would like
to understand what are advantages.

> 
> Also are this and ...
> 
> > +clear_bss:
> > +        REG_S   zero, (a3)
> > +        add     a3, a3, RISCV_SZPTR
> > +        blt     a3, a4, clear_bss
> 
> ... this branch actually the correct ones? I'd expect the unsigned
> flavors to be used when comparing addresses. It may not matter here
> and/or right now, but it'll set a bad precedent unless you expect
> to only ever work on addresses which have the sign bit clear.
I'll change blt to bltu.

~ Oleksii

Re: [PATCH v2 2/3] xen/riscv: initialize .bss section

[Jan Beulich]

On 02.03.2023 16:55, Oleksii wrote:
> On Thu, 2023-03-02 at 15:22 +0100, Jan Beulich wrote:
>> On 02.03.2023 14:23, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/riscv64/head.S
>>> +++ b/xen/arch/riscv/riscv64/head.S
>>> @@ -13,6 +13,15 @@ ENTRY(start)
>>>          lla     a6, _dtb_base
>>>          REG_S   a1, (a6)
>>>  
>>> +        la      a3, __bss_start
>>> +        la      a4, __bss_end
>>> +        ble     a4, a3, clear_bss_done
>>
>> While it may be that .bss is indeed empty right now, even short term
>> it won't be, and never will. I'd drop this conditional (and in
>> particular the label), inserting a transient item into .bss for the
>> time being. As soon as your patch introducing page tables has landed,
>> there will be multiple pages worth of .bss.
> If I understand you correctly you suggested declare some variable:
>    int dummy_bss __attribute__((unused));
> 
> Then .bss won't be zero:
>    $ riscv64-linux-gnu-objdump -x xen/xen-syms | grep -i dummy_bss    
>    0000000080205000 g     O .bss   0000000000000004 .hidden dummy_bss
> 
> And when page tables will be ready it will be needed to remove
> dummy_bss.
> 
> Another one option is to update linker script ( looks better then
> previous one ):
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -140,6 +140,7 @@ SECTIONS
>          . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_data_end = .;
>          *(.bss .bss.*)
> +        . = . + 1;
>          . = ALIGN(POINTER_ALIGN);
>          __bss_end = .;
>      } :text

Right, I did think of this as an alternative solution as well. Either
is fine with me.

> If one of the options is fine then to be honest I am not sure that I
> understand why it is better than have 3 instructions which will be
> unnecessary when first bss variable will be introduced. And actually
> the same will be with item in bss, it will become unnecessary when
> something from bss will be introduced.
> 
> I am OK with one of the mentioned above options but still would like
> to understand what are advantages.

You could also remove the branch and the label once .bss is no longer
empty. It'll just raise needless questions if that's left in long
term. Plus - I'm not a maintainer, I'm only voicing suggestions ...

Jan

Re: [PATCH v2 2/3] xen/riscv: initialize .bss section

[Andrew Cooper]

On 02/03/2023 3:55 pm, Oleksii wrote:
> On Thu, 2023-03-02 at 15:22 +0100, Jan Beulich wrote:
>> On 02.03.2023 14:23, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/riscv64/head.S
>>> +++ b/xen/arch/riscv/riscv64/head.S
>>> @@ -13,6 +13,15 @@ ENTRY(start)
>>>          lla     a6, _dtb_base
>>>          REG_S   a1, (a6)
>>>  
>>> +        la      a3, __bss_start
>>> +        la      a4, __bss_end
>>> +        ble     a4, a3, clear_bss_done
>> While it may be that .bss is indeed empty right now, even short term
>> it won't be, and never will. I'd drop this conditional (and in
>> particular the label), inserting a transient item into .bss for the
>> time being. As soon as your patch introducing page tables has landed,
>> there will be multiple pages worth of .bss.
> If I understand you correctly you suggested declare some variable:
>    int dummy_bss __attribute__((unused));
>
> Then .bss won't be zero:
>    $ riscv64-linux-gnu-objdump -x xen/xen-syms | grep -i dummy_bss    
>    0000000080205000 g     O .bss   0000000000000004 .hidden dummy_bss
>
> And when page tables will be ready it will be needed to remove
> dummy_bss.

Well - to be deleted when the first real bss user appears, but yes -
that will probably be the pagetable series.

>
> Another one option is to update linker script ( looks better then
> previous one ):
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -140,6 +140,7 @@ SECTIONS
>          . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_data_end = .;
>          *(.bss .bss.*)
> +        . = . + 1;
>          . = ALIGN(POINTER_ALIGN);
>          __bss_end = .;
>      } :text
>
> If one of the options is fine then to be honest I am not sure that I
> understand why it is better than have 3 instructions which will be
> unnecessary when first bss variable will be introduced. And actually
> the same will be with item in bss, it will become unnecessary when
> something from bss will be introduced.
>
> I am OK with one of the mentioned above options but still would like
> to understand what are advantages.

A one-line delete in a C file deletion is most obviously-safe of the 3
options to be performed at some later date, when we've started
forgetting the specific details in this patch.

>> Also are this and ...
>>
>>> +clear_bss:
>>> +        REG_S   zero, (a3)
>>> +        add     a3, a3, RISCV_SZPTR
>>> +        blt     a3, a4, clear_bss
>> ... this branch actually the correct ones? I'd expect the unsigned
>> flavors to be used when comparing addresses. It may not matter here
>> and/or right now, but it'll set a bad precedent unless you expect
>> to only ever work on addresses which have the sign bit clear.
> I'll change blt to bltu.

This should indeed an unsigned compare.  It doesn't explode in practice
because paging is disabled and RISC-V's MAXPHYADDR is 56 bits so doesn't
set the sign bit.

~Andrew

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

[Oleksii Kurochko]

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

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

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");
-- 
2.39.0

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

[Andrew Cooper]

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@gmail.com>
> ---
> 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

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

[Oleksii]

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@gmail.com>
> > ---
> > 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