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(-)
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
--
2.40.1On 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 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 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
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, -- Julien Grall
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
© 2016 - 2026 Red Hat, Inc.