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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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
2. Check somewhere at the start if linker_addr != load_addr, then throw
an error and panic().

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?
> >     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_
> 
~ Oleksii