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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/10/25 10:48 AM, Jan Beulich wrote:

> 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.)
>     
Sure, I'll change to 'const char *p_ = (const char*)(unsigned long)(p)'.

> > --- 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)?
>     
It should be really dropped, no need for MB in the comment.

> >   * 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?
>     
L0 for identity mapping is really missed.

Thanks.

~ Oleksii