Re: [PATCH v2 3/8] xen/riscv: introduce stack stuff

[Julien Grall <julien@xxxxxxx>]

Hi,

On 09/01/2023 15:46, Oleksii Kurochko wrote:
> The patch introduces and sets up a stack in order to go to C environment
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
It looks like the comments from Andrew were missed.

> ---
> Changes in V2:
>      - introduce STACK_SIZE define.
>      - use consistent padding between instruction mnemonic and operand(s)
> ---
>   xen/arch/riscv/Makefile             | 1 +
>   xen/arch/riscv/include/asm/config.h | 2 ++
>   xen/arch/riscv/riscv64/head.S       | 8 +++++++-
>   xen/arch/riscv/setup.c              | 6 ++++++
>   4 files changed, 16 insertions(+), 1 deletion(-)
>   create mode 100644 xen/arch/riscv/setup.c
>
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 248f2cbb3e..5a67a3f493 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,4 +1,5 @@
>   obj-$(CONFIG_RISCV_64) += riscv64/
> +obj-y += setup.o
>   
>   $(TARGET): $(TARGET)-syms
>         $(OBJCOPY) -O binary -S $< $@
> diff --git a/xen/arch/riscv/include/asm/config.h 
> b/xen/arch/riscv/include/asm/config.h
> index 0370f865f3..bdd2237f01 100644
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -43,6 +43,8 @@
>   
>   #define SMP_CACHE_BYTES (1 << 6)
>   
> +#define STACK_SIZE (PAGE_SIZE)
> +
>   #endif /* __RISCV_CONFIG_H__ */
>   /*
>    * Local variables:
> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
> index 990edb70a0..c1f33a1934 100644
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -1,4 +1,10 @@
>           .section .text.header, "ax", %progbits
>   
>   ENTRY(start)
> -        j  start
> +        la      sp, cpu0_boot_stack
> +        li      t0, STACK_SIZE
> +        add     sp, sp, t0
> +
> +_start_hang:
> +        wfi
> +        j       _start_hang
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> new file mode 100644
> index 0000000000..41ef4912bd
> --- /dev/null
> +++ b/xen/arch/riscv/setup.c
> @@ -0,0 +1,6 @@
> +#include <xen/init.h>
> +#include <xen/compile.h>
Why do you need to include <xen/compile.h>?

In any case, please order the include alphabetically. I haven't look at 
the rest of the series. But please go through the series and check that 
generic comments (like this one) are addressed everywhere.
Cheers,

--
Julien Grall