Re: [PATCH v4] xen/riscv: Increase XEN_VIRT_SIZE

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 17:56, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -31,20 +31,30 @@ 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.
>   * 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
> + * separately. (look at stage1_pgtbl_root)
>   *
> - * 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 identity mapping.
>   */
> -#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)) + 1)

I'm still struggling here. In the original expression, the +1 was to cover
the root page table, wasn't it? By now subtracting 2 from
CONFIG_PAGING_LEVELS, you reduce the overall result by 2. One of these two
is accounted for by the Xen size calculation now. Where's the other one?
Or are you suggesting that for some (not obvious to me) reason we
previously calculated one too many?

> +/* Changing these checks can require an update of PGTBL_INITIAL_COUNT */
> +_Static_assert(IS_ALIGNED(XEN_VIRT_START, GB(1)),
> +               "XEN_VIRT_START should be 1gb aligned");
> +_Static_assert(IS_ALIGNED(XEN_VIRT_SIZE, MB(2)),
> +               "XEN_VIRT_SIZE should be 2mb aligned");

Why would you need to open-code BUILD_BUG_ON() here? See the various
build_assertions() functions we've accumulated.

Jan