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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.03.2023 15:54, Oleksii wrote:
> Actually after my latest experiments it looks that we don't need to
> calculate that things at all because for RISC-V it is  used everywhere
> PC-relative access.
> Thereby it doesn't matter what is an address where Xen was loaded and
> linker address.
> Right now I found only to cases which aren't PC-relative.
> Please look at the patch below:
> diff --git a/xen/arch/riscv/include/asm/config.h
> b/xen/arch/riscv/include/asm/config.h
> index 763a922a04..e1ba613d81 100644
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -39,7 +39,7 @@
>    name:
>  #endif
>  
> -#define XEN_VIRT_START  _AT(UL, 0x80200000)
> +#define XEN_VIRT_START  _AT(UL, 0x00200000)

I think this wants to remain the address where Xen actually runs, and
where Xen is linked to. This ...

> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -123,8 +123,14 @@ int do_bug_frame(const struct cpu_user_regs *regs,
> vaddr_t pc)
>      const char *filename, *predicate;
>      int lineno;
>  
> -    static const struct bug_frame* bug_frames[] = {
> -        &__start_bug_frames[0],
> +    /*
> +     * force fill bug_frames array using auipc/addi instructions to
> +     * make addresses in bug_frames PC-relative.
> +    */
> +    const struct bug_frame * force = (const struct bug_frame *)
> &__start_bug_frames[0];
> +
> +    const struct bug_frame* bug_frames[] = {
> +        force,
>          &__stop_bug_frames_0[0],
>          &__stop_bug_frames_1[0],
>          &__stop_bug_frames_2[0],

... array would better be static anyway, and ...

> The changes related to <asm/config.h> are  only to make linker_addr !=
> load_address. So:
> 1. The first issue with cpu0_boot_stack in the head.S file. When we do:
>       la      sp, cpu0_boot_stack
>    Pseudo-instruction la will be transformed to auipc/addi OR
> auipc/l{w|d}.
>    It depends on an option: nopic, pic. [1]
>    
>    So the solution can be the following:
>    * As it is done in the patch: add to the start of head.S ".option  
> nopic"
>    * Change la to lla thereby it will be always generated "auipc/addi"
> to get an address of variable.
> 
> 2. The second issue is with the code in do_bug_frame() with bug_frames
> array:
>    const struct bug_frame* bug_frames[] = {
>         &__start_bug_frames[0],
>         &__stop_bug_frames_0[0],
>         &__stop_bug_frames_1[0],
>         &__stop_bug_frames_2[0],
>         &__stop_bug_frames_3[0],
>     };
>   In this case &{start,stop}bug_frames{,{0-3}} will be changed to     
> linker address. In case of when load_addr is 0x80200000 and linker_addr
> is 0x00200000 then &{start,stop}bug_frames{,{0-3}} will be equal to
> 0x00200000 + X.

... this "solution" to a problem you introduce by wrongly modifying
the linked address would then need applying to any other similar code
pattern found in Xen. Which is (I hope obviously) not a viable route.
Instead code running before address translation is enable needs to be
extra careful in what code and data items it accesses, and how.

Jan

>     To force using addresses related to load_addr  in bug_frames, it is
> necessary to declare a variable with getting an address of
> &__{start,stop}bug_frames{,{0-3}} thereby it will generate the code:
>         2002c2:       00001797                auipc   a5,0x1
>       2002c6:       d3e78793                addi    a5,a5,-706 #
> 201000 <__start_bug_frames>
>       2002ca:       faf43c23                sd      a5,-72(s0)
>       2002ce:       00001797                auipc   a5,0x1
>       2002d2:       d3a78793                addi    a5,a5,-710 #
> 201008 <__stop_bug_frames_