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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-03-09 at 10:46 +0100, Jan Beulich wrote:
> 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).

Thanks for the clarification.

I will look at xen.efi & xen.gz, but I think I will follow scenario 1
or similar to it ( as I did in the path diff with the introduction of
force variable) for now, it is required only to make addresses relative
to phys_offset in one array.

After that, I will enable MMU, and it won't be necessary to add
phys_offset or make similar changes to the introduction of force
variable mentioned in the patch diff.

~ Oleksii