| Message ID | 19817eca0b7d4e8dee7eb5d5e7d3812133925eb3.1690191480.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/riscv: introduce identity mapping | expand |
On 24.07.2023 11:42, Oleksii Kurochko wrote: > @@ -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; While Misra compliance is distant future for RISC-V, there's a problem here with there not being any declaration for this global variable. Adding a declaration isn't the only solution though: Patch 2 also only uses the variable in assembly code. Therefore the variable here could be made static, with ... > @@ -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; > +} ... this function (being invoked by the same assembly code function) returning the value alongside storing it. FTAOD I wouldn't insist on this being taken care of right away, so if you get a maintainer ack this way, I'd be happy to commit as is. Jan
On Mon, 2023-07-24 at 15:40 +0200, Jan Beulich wrote: > On 24.07.2023 11:42, Oleksii Kurochko wrote: > > @@ -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; > > While Misra compliance is distant future for RISC-V, there's a > problem here with there not being any declaration for this global > variable. Adding a declaration isn't the only solution though: > Patch 2 also only uses the variable in assembly code. Therefore > the variable here could be made static, with ... > > > @@ -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; > > +} > > ... this function (being invoked by the same assembly code > function) returning the value alongside storing it. Thanks for explanation about MISRA compliance. > > FTAOD I wouldn't insist on this being taken care of right away, > so if you get a maintainer ack this way, I'd be happy to commit as > is. If I don't get an ACK, I'll update the code with the next patch version. ~ Oleksii
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..a28714e0ef 100644 --- a/xen/arch/riscv/riscv64/head.S +++ b/xen/arch/riscv/riscv64/head.S @@ -29,6 +29,19 @@ ENTRY(start) jal reset_stack + /* + * save hart_id ( bootcpu_id ) and dtb_base as a0 and a1 register can + * be used by C code + */ + mv s0, a0 + mv s1, a1 + + jal calc_phys_offset + + /* restore hart_id ( bootcpu_id ) and dtb address */ + mv a0, s0 + mv a1, s1 + 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 V4: - update the comment messages in head.S related to save/restore of a0/a1 regs. --- 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 | 13 +++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-)