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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-02-27 at 17:45 +0000, Julien Grall wrote:
> Hi,
> 
> On 27/02/2023 17:17, Oleksii wrote:
> > On Sat, 2023-02-25 at 18:05 +0000, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 24/02/2023 15:06, Oleksii Kurochko wrote:
> > > > Calculate load and linker linker image addresses and
> > > > setup initial pagetables.
> > > > 
> > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > > > ---
> > > >    xen/arch/riscv/setup.c | 11 +++++++++++
> > > >    1 file changed, 11 insertions(+)
> > > > 
> > > > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> > > > index b7cd438a1d..f69bc278bb 100644
> > > > --- a/xen/arch/riscv/setup.c
> > > > +++ b/xen/arch/riscv/setup.c
> > > > @@ -1,9 +1,11 @@
> > > >    #include <xen/bug.h>
> > > >    #include <xen/compile.h>
> > > >    #include <xen/init.h>
> > > > +#include <xen/kernel.h>
> > > >    
> > > >    #include <asm/csr.h>
> > > >    #include <asm/early_printk.h>
> > > > +#include <asm/mm.h>
> > > >    #include <asm/traps.h>
> > > >    
> > > >    /* Xen stack for bringing up the first CPU. */
> > > > @@ -43,6 +45,11 @@ static void __init disable_fpu(void)
> > > >    
> > > >    void __init noreturn start_xen(void)
> > > >    {
> > > > +    unsigned long load_start    = (unsigned long)start;
> > > > +    unsigned long load_end      = load_start + (unsigned
> > > > long)(_end - _start);
> > > 
> > > I am a bit puzzled, on top of load_addr() and linker_addr(), you
> > > wrote
> > > it can't use global variable/function. But here... you are using
> > > them.
> > > So how is this different?
> > I don't use load_addr() and linker_addr() macros here.
> 
> I understand that. But my comment was related to:
> 
> /*
>   * WARNING: load_addr() and linker_addr() are to be called only when
> the MMU is
>   * disabled and only when executed by the primary CPU.  They cannot 
> refer to
>   * any global variable or functions.
>   */
> 
> _start and _end are global variables. So why can you use them here
> but 
> not there?
> 
> If you could use them in load_addr() then you could simplify a lot
> your 
> logic.
> 
I experimented with it and it seems to me that the macros can be used
with functions&global variables so the comment is incorrect.
> > > 
> > > > +    unsigned long linker_start  = (unsigned long)_start;
> > > > +    unsigned long linker_end    = (unsigned long)_end;
> > > 
> > > I am a bit confused with how you define the start/end for both
> > > the
> > > linker and load. In one you use _start and the other _end.
> > > 
> > > Both are fixed at compile time, so I assume the values will be a
> > > linked
> > > address rather than the load address. So how is this meant to
> > > how?
> > > 
> > _start, _end - it is label from linker script so I use them to
> > define
> > linker_start and linker_end addresses.
> > 
> > load_start is defined as an address of start() function from head.S
> > and
> > load_end is the load_start + the size  (_end - _start)
> 
> I think you misunderstood my comment. I understand what the variables
> are for. But I don't understand the computation because Xen could be 
> loaded at a different address than the runtime address.
> 
What do you mean here by the runtime address? Do you mean an address
where boot loader put Xen? Or an address after relocation, or something
else?

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)
 
 #define SMP_CACHE_BYTES (1 << 6)
 
diff --git a/xen/arch/riscv/riscv64/head.S
b/xen/arch/riscv/riscv64/head.S
index ffd95f9f89..87842632e9 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -1,5 +1,7 @@
 #include <asm/riscv_encoding.h>
 
+        .option nopic
+
         .section .text.header, "ax", %progbits
 
 ENTRY(start)
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index d87a9cfd2c..cd0acdee51 100644
--- 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],

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.
   
    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_

> > 
> > > Furthermore, I would expect linker_start and load_start to point
> > > to
> > > the
> > > same symbol (the only different is one store the virtual address
> > > whereas
> > > the other the physical address). But here you are technically
> > > using
> > > two
> > > different symbol. Can you explain why?
> > It is used to make identity mapping for the range [load_addr,
> > load_end]
> > and [linker_addr, linker_end]. It was done so because in Bobby's
> > patches in the linker script XEN_VIRT_START is defined as
> > _AT(vaddr_t,0x00200000) but bootloader loads Xen at 0x80200000 and
> > so
> > in this case loadr_addr != linker_addr.
> > But I have changed XEN_VIRT_START to 0x8020...00 so they are equal
> > now.
> 
> So this will be broken as soon as this code will be tested on an 
> hardware where there is no RAM at 0x8020...00. I would strongly 
> recommend for you to test your code with XEN_VIRT_START != load
> address.
I've added the check to the new version of the patch but please the
comments above.
> 
> > > 
> > > > +
> > > >        /*
> > > >         * The following things are passed by bootloader:
> > > >         *   a0 -> hart_id
> > > > @@ -65,6 +72,10 @@ void __init noreturn start_xen(void)
> > > >    
> > > >        test_macros_from_bug_h();
> > > >    
> > > > +    setup_initial_pagetables(load_start, load_end,
> > > > linker_start,
> > > > linker_end);
> > > 
> > > Shouldn't this happen earlier in start_xen()?
> > It can. If to be honest I don't know if it should. I added at the
> > end
> > only because it was the last thing I worked on...
> 
> I think we should enable the MMU and switch to the runtime mapping as
> early as possible.
I realized that during the mentioned above experiments.

~ Oleksii