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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/15/25 12:07 PM, Jan Beulich wrote:

> 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?
>     
+1 was added to cover the case where the Xen address is not 2MB-aligned.

For the root page table, we use a separate variable stage1_pgtbl_root.
Since the root page table is shared with the identity mapping, it is not
included in PGTBL_INITIAL_COUNT.

> 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?
>     
My calculation was as follows:
 [a] One shared root page table is needed for both the identity mapping and Xen itself.
 [b] (CONFIG_PAGING_LEVELS - 2) page tables are needed for identity mapping and Xen.
     The -2 accounts for:
     - No extra root page table (since we use a separate variable for it).
     - No extra L0 page table (as those will be calculated separately in the next step).
  [c] We also need (XEN_VIRT_SIZE >> XEN_PT_LEVEL_SHIFT(1)) L0s to cover Xen itself,
      plus one additional L0 page table for the identity mapping.
It might be better to calculate the total in the following way:
 ((CONFIG_PAGING_LEVELS - 1) * 2 + (XEN_VIRT_SIZE >> XEN_PT_LEVEL_SHIFT(1)) - 1)

Here’s the reasoning:
  - (CONFIG_PAGING_LEVELS - 1) excludes the root page table (provided separately via
    stage1_pgtbl_root), but we add back:
      - One L0 for Xen,
      - One L0 for the identity mapping.
Since the L0 used for identity mapping is now implicitly part of (CONFIG_PAGING_LEVELS - 1),
we can drop the +1 from PGTBL_INITIAL_COUNT.
Additionally, we need to subtract 1, because changing from (CONFIG_PAGING_LEVELS - 2) to
(CONFIG_PAGING_LEVELS - 1) adds one extra L0 for Xen itself too.

Does it make sense?

> > +/* 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.
>     
I missed build_assertions() functions. I will do in a similar way.

Thanks.

~ Oleksii