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_
