On 17.07.2023 16:40, Oleksii Kurochko wrote: > 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 <[email protected]> > --- > Changes in V3: > - save/restore of a0/a1 registers before C first function call. > --- > 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 | 14 ++++++++++++++ > 3 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h > index 5e3ac5cde3..d9c4205103 100644 > --- a/xen/arch/riscv/include/asm/mm.h > +++ b/xen/arch/riscv/include/asm/mm.h > @@ -15,4 +15,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 fddb3cd0bd..c84a8a7c3c 100644 > --- a/xen/arch/riscv/mm.c > +++ b/xen/arch/riscv/mm.c > @@ -1,5 +1,6 @@ > /* SPDX-License-Identifier: GPL-2.0-only */ > > +#include <xen/cache.h> > #include <xen/compiler.h> > #include <xen/init.h> > #include <xen/kernel.h> > @@ -19,9 +20,10 @@ struct mmu_desc { > pte_t *pgtbl_base; > }; > > -#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..9015d06233 100644 > --- a/xen/arch/riscv/riscv64/head.S > +++ b/xen/arch/riscv/riscv64/head.S > @@ -29,6 +29,20 @@ ENTRY(start) > > jal reset_stack > > + /* > + * save hart_id and dtb_base as a0 and a1 register can be used > + * by C code ( f.e. setup_initial_pagetables will update a0 and > + * a1 )
I'd recommend dropping the part in parentheses - for one the function doesn't exist yet, and then you're merely restating what the ABI has. > + */ > + mv s0, a0 > + mv s1, a1 > + > + jal calc_phys_offset > + > + /* restore bootcpu_id and dtb address */ Is the first name here intentionally different from that in the other comment (where it's "hart_id")? Jan > + mv a0, s0 > + mv a1, s1 > + > tail start_xen > > .section .text, "ax", %progbits
