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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.03.2023 17:16, Oleksii wrote:
> On Wed, 2023-03-08 at 16:17 +0100, Jan Beulich wrote:
>> 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.
>>
> I modified the linked address only for the experiment ( when load_addr
> != linker_addr to emulate situation Julien told me about), so it's not
> something I planned to send as a part of the final patch, and I
> probably forgot to mention that in my previous mail.
> 
> It is only one place where we have to do a kind of 'force' and is
> needed to make the current state of RISC-V Xen work in case we don't
> have MMU enabled yet and linker_addr != load_addr. All other cases
> where it is used something from the range (linker_start, linker_end)
> will be managed by MMU.
> 
> If we can't use mentioned above solution, we still need to handle the
> situation when linker_addr != load_addr and MMU isn't enabled yet.
> Other options to do that:
> 1. add phys_offset ( | load_addr - linker_addr | ) everywhere where
> bug_frames array is used: bug_frames[id] + phys_offset

Well, that again special cases a certain data structure. As said before,
you need to be very careful with any C code involved before translation
is enabled. Unless you want to retain relocations (so you can "move"
from load-time to link time addresses alongside enabling translation,
like we do on x86 in xen.efi), you want to constrain code paths as much
as possible. One approach is to move enabling of translation to early
assembly code (like we do on x86 for xen.gz). The other is to amend
involved code paths with something like what you say above.

> 2. Check somewhere at the start if linker_addr != load_addr, then throw
> an error and panic().

That's not really an option if the boot loader isn't required to place
the image at its linked address (which would be odd if translation
isn't expected to be enabled yet at that point). Plus no matter what
linked address you choose, I guess there may be systems where that
address range simply isn't (fully) populated with RAM.

> Other options might exist. So I would appreciate it if you could
> suggest me some.
> 
> Could you let me know if any options are suitable for handling a case
> when linker_addr?

Main question is how tied you are to doing this in C. x86 and both
Arm flavors do it in assembly, with - as said - the exception of
x86's xen.efi where instead we retain (or generate) and process base
relocations (see efi_arch_relocate_image(), called by
efi_arch_post_exit_boot() immediately before switching to the "real"
page tables).

Jan