On 09.04.2025 21:01, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -9,6 +9,7 @@
> #include <xen/mm-frame.h>
> #include <xen/pdx.h>
> #include <xen/pfn.h>
> +#include <xen/sections.h>
> #include <xen/types.h>
>
> #include <asm/page-bits.h>
> @@ -35,6 +36,11 @@ static inline void *maddr_to_virt(paddr_t ma)
> return (void *)va;
> }
>
> +#define is_init_section(p) ({ \
> + char *p_ = (char *)(unsigned long)(p); \
> + (p_ >= __init_begin) && (p_ < __init_end); \
> +})
I think this wants to be put in xen/sections.h, next to where __init_{begin,end}
are declared. But first it wants making const-correct, to eliminate the
potential
of it indirectly casting away const-ness from the incoming argument.
(At some point related stuff wants moving from kernel.h to sections.h, I
suppose.
And at that point they will all want to have const added.)
> @@ -43,13 +49,21 @@ static inline void *maddr_to_virt(paddr_t ma)
> */
> static inline unsigned long virt_to_maddr(unsigned long va)
> {
> + const unsigned long xen_size = (unsigned long)(_end - _start);
> + const unsigned long xen_virt_start = _AC(XEN_VIRT_START, UL);
> + const unsigned long xen_virt_end = xen_virt_start + xen_size - 1;
> +
> if ((va >= DIRECTMAP_VIRT_START) &&
> (va <= DIRECTMAP_VIRT_END))
> return directmapoff_to_maddr(va - directmap_virt_start);
>
> - BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
> - ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
> - (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + PAGE_SHIFT)));
> + ASSERT((va >= xen_virt_start) && (va <= xen_virt_end));
> +
> + /*
> + * The .init* sections will be freed when Xen completes booting,
> + * so the [__init_begin, __init_end] range must be excluded.
[__init_begin, __init_end)
> + */
> + ASSERT((system_state != SYS_STATE_active) || !is_init_section(va));
system_state < SYS_STATE_active
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -31,20 +31,24 @@ unsigned long __ro_after_init phys_offset; /* =
> load_start - XEN_VIRT_START */
> #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
>
> /*
> - * It is expected that Xen won't be more then 2 MB.
> + * It is expected that Xen won't be more then XEN_VIRT_SIZE MB.
Why "MB" when the macro already expands to MB(16)?
> * The check in xen.lds.S guarantees that.
> - * At least 3 page tables (in case of Sv39 ) are needed to cover 2 MB.
> - * One for each page level table with PAGE_SIZE = 4 Kb.
> *
> - * One L0 page table can cover 2 MB(512 entries of one page table *
> PAGE_SIZE).
> + * Root page table is shared with the initial mapping and is declared
> + * separetely. (look at stage1_pgtbl_root)
Nit: separately
> *
> - * It might be needed one more page table in case when Xen load address
> - * isn't 2 MB aligned.
> + * An amount of page tables between root page table and L0 page table
> + * (in the case of Sv39 it covers L1 table):
> + * (CONFIG_PAGING_LEVELS - 2) are needed for an identity mapping and
> + * the same amount are needed for Xen.
> *
> - * CONFIG_PAGING_LEVELS page tables are needed for the identity mapping,
> - * except that the root page table is shared with the initial mapping
> + * An amount of L0 page tables:
> + * (512 entries of one L0 page table covers 2MB ==
> 1<<XEN_PT_LEVEL_SHIFT(1))
> + * XEN_VIRT_SIZE >> XEN_PT_LEVEL_SHIFT(1) are needed for Xen and
> + * one L0 is needed for indenity mapping.
Nit: identity
But more importantly, where's this one L0 ...
> */
> -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 + 1)
> +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 2) * 2 + \
> + (XEN_VIRT_SIZE >> XEN_PT_LEVEL_SHIFT(1)))
.... in this calculation?
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -178,4 +178,8 @@ ASSERT(!SIZEOF(.got.plt), ".got.plt non-empty")
> */
> ASSERT(_end - _start <= XEN_VIRT_SIZE, "Xen too large for early-boot
> assumptions")
>
> +/* Changing these ASSERTs can require an update of PGTBL_INITIAL_COUNT */
> +ASSERT(IS_ALIGNED(XEN_VIRT_START, GB(1)), "XEN_VIRT_START should be 1gb
> aligned")
> +ASSERT(IS_ALIGNED(XEN_VIRT_SIZE, MB(2)), "XEN_VIRT_SIZE should be 2mb
> aligned")
Such checking wants to happen as early as possible in the build. Deferring to
the linking step should be done only if some part of the expression only is
established when linking. Aiui both of these could be BUILD_BUG_ON() somewhere
near to where PGTBL_INITIAL_COUNT is defined or used.
Jan
