Re: [PATCH v1 2/3] xen/riscv: setup initial pagetables

[Julien Grall <julien@xxxxxxx>]

Hi,

On 24/02/2023 15:06, Oleksii Kurochko wrote:
> Calculate load and linker linker image addresses and
> setup initial pagetables.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
>   xen/arch/riscv/setup.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index b7cd438a1d..f69bc278bb 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,9 +1,11 @@
>   #include <xen/bug.h>
>   #include <xen/compile.h>
>   #include <xen/init.h>
> +#include <xen/kernel.h>
>   
>   #include <asm/csr.h>
>   #include <asm/early_printk.h>
> +#include <asm/mm.h>
>   #include <asm/traps.h>
>   
>   /* Xen stack for bringing up the first CPU. */
> @@ -43,6 +45,11 @@ static void __init disable_fpu(void)
>   
>   void __init noreturn start_xen(void)
>   {
> +    unsigned long load_start    = (unsigned long)start;
> +    unsigned long load_end      = load_start + (unsigned long)(_end - _start);
I am a bit puzzled, on top of load_addr() and linker_addr(), you wrote 
it can't use global variable/function. But here... you are using them. 
So how is this different?
> +    unsigned long linker_start  = (unsigned long)_start;
> +    unsigned long linker_end    = (unsigned long)_end;
I am a bit confused with how you define the start/end for both the 
linker and load. In one you use _start and the other _end.
Both are fixed at compile time, so I assume the values will be a linked 
address rather than the load address. So how is this meant to how?
Furthermore, I would expect linker_start and load_start to point to the 
same symbol (the only different is one store the virtual address whereas 
the other the physical address). But here you are technically using two 
different symbol. Can you explain why?
> +
>       /*
>        * The following things are passed by bootloader:
>        *   a0 -> hart_id
> @@ -65,6 +72,10 @@ void __init noreturn start_xen(void)
>   
>       test_macros_from_bug_h();
>   
> +    setup_initial_pagetables(load_start, load_end, linker_start, linker_end);
Shouldn't this happen earlier in start_xen()?

Cheers,

--
Julien Grall