| Message ID | f84bdc5ad9f10f864d070f7581dce663ccc9cb53.1687178053.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/riscv: introduce identity mapping | expand |
On 19.06.2023 15:34, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/riscv64/head.S > +++ b/xen/arch/riscv/riscv64/head.S > @@ -29,6 +29,8 @@ ENTRY(start) > > jal reset_stack > > + jal calc_phys_offset > + > tail start_xen > > .section .text, "ax", %progbits Since you call a C function, the code to save/restore a0/a1 needs to move here (from patch 4). Jan
On Thu, 2023-07-06 at 13:18 +0200, Jan Beulich wrote: > On 19.06.2023 15:34, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/riscv64/head.S > > +++ b/xen/arch/riscv/riscv64/head.S > > @@ -29,6 +29,8 @@ ENTRY(start) > > > > jal reset_stack > > > > + jal calc_phys_offset > > + > > tail start_xen > > > > .section .text, "ax", %progbits > > Since you call a C function, the code to save/restore a0/a1 needs to > move here (from patch 4). Thanks. It makes sense. It would be better to move save/restore a0/a1 ( from patch 4 )code here. The only one reason I didn't do that before that calc_phys_offset doesn't touch that and it is guaranteed that it will not ( as it doesn't have arguments ) ~ Oleksii
On 07/07/2023 10:12, Oleksii wrote: > On Thu, 2023-07-06 at 13:18 +0200, Jan Beulich wrote: >> On 19.06.2023 15:34, Oleksii Kurochko wrote: >>> --- a/xen/arch/riscv/riscv64/head.S >>> +++ b/xen/arch/riscv/riscv64/head.S >>> @@ -29,6 +29,8 @@ ENTRY(start) >>> >>> jal reset_stack >>> >>> + jal calc_phys_offset >>> + >>> tail start_xen >>> >>> .section .text, "ax", %progbits >> >> Since you call a C function, the code to save/restore a0/a1 needs to >> move here (from patch 4). > Thanks. It makes sense. > It would be better to move save/restore a0/a1 ( from patch 4 )code > here. > > The only one reason I didn't do that before that calc_phys_offset > doesn't touch that and it is guaranteed that it will not ( as it > doesn't have arguments ) IIUC, the calling convention requires a0/a1 to be caller saved. So even if they are not used for arguments, such callee is still free to use them for internal purpose. Cheers,
On 07.07.2023 11:12, Oleksii wrote: > On Thu, 2023-07-06 at 13:18 +0200, Jan Beulich wrote: >> On 19.06.2023 15:34, Oleksii Kurochko wrote: >>> --- a/xen/arch/riscv/riscv64/head.S >>> +++ b/xen/arch/riscv/riscv64/head.S >>> @@ -29,6 +29,8 @@ ENTRY(start) >>> >>> jal reset_stack >>> >>> + jal calc_phys_offset >>> + >>> tail start_xen >>> >>> .section .text, "ax", %progbits >> >> Since you call a C function, the code to save/restore a0/a1 needs to >> move here (from patch 4). > Thanks. It makes sense. > It would be better to move save/restore a0/a1 ( from patch 4 )code > here. > > The only one reason I didn't do that before that calc_phys_offset > doesn't touch that and it is guaranteed that it will not ( as it > doesn't have arguments ) How does a function not having parameters guarantee that registers used for parameter passing aren't touched? Inside a function, the compiler is free to use argument-passing registers just like other temporary ones; their values don't need preserving, from all I know (otherwise the RISC-V ABI would be different to all other ABIs I know of). Jan
On Fri, 2023-07-07 at 10:17 +0100, Julien Grall wrote: > > > On 07/07/2023 10:12, Oleksii wrote: > > On Thu, 2023-07-06 at 13:18 +0200, Jan Beulich wrote: > > > On 19.06.2023 15:34, Oleksii Kurochko wrote: > > > > --- a/xen/arch/riscv/riscv64/head.S > > > > +++ b/xen/arch/riscv/riscv64/head.S > > > > @@ -29,6 +29,8 @@ ENTRY(start) > > > > > > > > jal reset_stack > > > > > > > > + jal calc_phys_offset > > > > + > > > > tail start_xen > > > > > > > > .section .text, "ax", %progbits > > > > > > Since you call a C function, the code to save/restore a0/a1 needs > > > to > > > move here (from patch 4). > > Thanks. It makes sense. > > It would be better to move save/restore a0/a1 ( from patch 4 )code > > here. > > > > The only one reason I didn't do that before that calc_phys_offset > > doesn't touch that and it is guaranteed that it will not ( as it > > doesn't have arguments ) > > IIUC, the calling convention requires a0/a1 to be caller saved. So > even > if they are not used for arguments, such callee is still free to use > them for internal purpose. You are right. I haven't seen that compiler use them if 'void' is passed to function as an argument. But I agree that we have to follow the calling convention to be sure that all is fine. ~ Oleksii
On Fri, 2023-07-07 at 11:35 +0200, Jan Beulich wrote: > On 07.07.2023 11:12, Oleksii wrote: > > On Thu, 2023-07-06 at 13:18 +0200, Jan Beulich wrote: > > > On 19.06.2023 15:34, Oleksii Kurochko wrote: > > > > --- a/xen/arch/riscv/riscv64/head.S > > > > +++ b/xen/arch/riscv/riscv64/head.S > > > > @@ -29,6 +29,8 @@ ENTRY(start) > > > > > > > > jal reset_stack > > > > > > > > + jal calc_phys_offset > > > > + > > > > tail start_xen > > > > > > > > .section .text, "ax", %progbits > > > > > > Since you call a C function, the code to save/restore a0/a1 needs > > > to > > > move here (from patch 4). > > Thanks. It makes sense. > > It would be better to move save/restore a0/a1 ( from patch 4 )code > > here. > > > > The only one reason I didn't do that before that calc_phys_offset > > doesn't touch that and it is guaranteed that it will not ( as it > > doesn't have arguments ) > > How does a function not having parameters guarantee that registers > used for parameter passing aren't touched? Inside a function, the > compiler is free to use argument-passing registers just like other > temporary ones; their values don't need preserving, from all I know > (otherwise the RISC-V ABI would be different to all other ABIs I > know of). Well, you are right that it doesn't guarantee and the calling convention tells that arg registers should be saved/restored before/after function call. But I haven't seen yet that compiler touch arg registers if function accepts 'void' as an function argument. So 'guarantee' isn't correct word. Thanks for the note. ~ Oleksii
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h index 64293eacee..996041ce81 100644 --- a/xen/arch/riscv/include/asm/mm.h +++ b/xen/arch/riscv/include/asm/mm.h @@ -11,4 +11,6 @@ void setup_initial_pagetables(void); void enable_mmu(void); void cont_after_mmu_is_enabled(void); +void calc_phys_offset(void); + #endif /* _ASM_RISCV_MM_H */ diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c index 8ceed445cf..570033f17f 100644 --- a/xen/arch/riscv/mm.c +++ b/xen/arch/riscv/mm.c @@ -1,3 +1,4 @@ +#include <xen/cache.h> #include <xen/compiler.h> #include <xen/init.h> #include <xen/kernel.h> @@ -19,9 +20,10 @@ struct mmu_desc { extern unsigned char cpu0_boot_stack[STACK_SIZE]; -#define PHYS_OFFSET ((unsigned long)_start - XEN_VIRT_START) -#define LOAD_TO_LINK(addr) ((addr) - PHYS_OFFSET) -#define LINK_TO_LOAD(addr) ((addr) + PHYS_OFFSET) +unsigned long __ro_after_init phys_offset; + +#define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset) +#define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset) /* * It is expected that Xen won't be more then 2 MB. @@ -273,3 +275,13 @@ void __init noreturn noinline enable_mmu() switch_stack_and_jump((unsigned long)cpu0_boot_stack + STACK_SIZE, cont_after_mmu_is_enabled); } + +/* + * calc_phys_offset() should be used before MMU is enabled because access to + * start() is PC-relative and in case when load_addr != linker_addr phys_offset + * will have an incorrect value + */ +void __init calc_phys_offset(void) +{ + phys_offset = (unsigned long)start - XEN_VIRT_START; +} diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S index 2c0304646a..850fbb58a7 100644 --- a/xen/arch/riscv/riscv64/head.S +++ b/xen/arch/riscv/riscv64/head.S @@ -29,6 +29,8 @@ ENTRY(start) jal reset_stack + jal calc_phys_offset + tail start_xen .section .text, "ax", %progbits
The function was introduced to calculate and save physical offset before MMU is enabled because access to start() is PC-relative and in case of linker_addr != load_addr it will result in incorrect value in phys_offset. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V2: - add __ro_after_init for phys_offset variable. - remove double blank lines. - add __init for calc_phys_offset(). - update the commit message. - declaring variable phys_off as non static as it will be used in head.S. --- xen/arch/riscv/include/asm/mm.h | 2 ++ xen/arch/riscv/mm.c | 18 +++++++++++++++--- xen/arch/riscv/riscv64/head.S | 2 ++ 3 files changed, 19 insertions(+), 3 deletions(-)