| Message ID | 83444f8f90cf2adf431762d919ba958a25ff8ce4.1677250203.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | enable MMU for RISC-V | expand |
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@gmail.com> > --- > 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? > + 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? 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? > + > /* > * 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()? Cheers,
On 25.02.2023 19:05, Julien Grall wrote: > On 24/02/2023 15:06, Oleksii Kurochko wrote: >> @@ -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 guess "use" means "access" (i.e. call a function or read/write a variable). I suppose it does not mean "take the address of". Jan
On 27/02/2023 15:17, Jan Beulich wrote: > On 25.02.2023 19:05, Julien Grall wrote: >> On 24/02/2023 15:06, Oleksii Kurochko wrote: >>> @@ -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 guess "use" means "access" (i.e. call a function or read/write a > variable). I suppose it does not mean "take the address of". If so, then I don't understand why we need to pass linker_start, linker_end in parameters for setup_initial_pages(). Cheers,
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@gmail.com> > > --- > > 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. > > > + 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) > 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. > > > + > > /* > > * 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... > > Cheers, > ~ Oleksii
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@gmail.com> >>> --- >>> 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. >> >>> + 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. > >> 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. >> >>> + >>> /* >>> * 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. Cheers,
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@gmail.com> > > > > --- > > > > 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
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_
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
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
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
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); + unsigned long linker_start = (unsigned long)_start; + unsigned long linker_end = (unsigned long)_end; + /* * 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); + + early_printk("MMU has been enabled\n"); + for ( ;; ) asm volatile ("wfi");
Calculate load and linker linker image addresses and setup initial pagetables. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/setup.c | 11 +++++++++++ 1 file changed, 11 insertions(+)