Re: [MINI-OS PATCH 1/2] mm: provide a way to do very early page table allocations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.07.2025 08:37, Juergen Gross wrote:
> Add a small pool of statically allocated memory pages to be handed out
> for very early page table allocations.
> 
> This will make it possible to do virtual allocations e.g. for mapping
> the shared info page.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>  arch/x86/mm.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
> index bdff38fd..3f5c7ea7 100644
> --- a/arch/x86/mm.c
> +++ b/arch/x86/mm.c
> @@ -640,12 +640,17 @@ void change_readonly(bool readonly)
>   * return a valid PTE for a given virtual address. If PTE does not exist,
>   * allocate page-table pages.
>   */
> +#define N_PTS 4

Wouldn't it be prudent to have a comment here mentioning how this number
was derived, i.e. what's known to be covered? (To map the shared info
page I expect you really only need 3? Hence without a comment things may
remain unclear.)

> +static char early_pt[PAGE_SIZE * N_PTS] __attribute__((aligned(PAGE_SIZE)));

Maybe better early_pt[N_PTS][PAGE_SIZE], simplifying the allocation
code below a little?

> +static unsigned long n_early_pt = N_PTS;

unsigned int would do, I expect? With the suggestion above this could
then also use ARRAY_SIZE(), at which point there would be no real need
for N_PTS anymore.

>  static int need_pgt_func(unsigned long va, unsigned int lvl, bool is_leaf,
>                           pgentry_t *pte, void *par)
>  {
>      pgentry_t **result = par;
>      unsigned long pt_mfn;
>      unsigned long pt_pfn;
> +    unsigned long pt_addr;
>      unsigned int idx;
>  
>      if ( !is_leaf )
> @@ -664,7 +669,16 @@ static int need_pgt_func(unsigned long va, unsigned int 
> lvl, bool is_leaf,
>      }
>  
>      pt_mfn = virt_to_mfn(pte);
> -    pt_pfn = virt_to_pfn(alloc_page());
> +    if ( n_early_pt )
> +    {
> +        n_early_pt--;
> +        pt_addr = (unsigned long)&early_pt[n_early_pt * PAGE_SIZE];
> +    }
> +    else
> +    {
> +        pt_addr = alloc_page();
> +    }

The failure pattern when one fails to increase early_pt[] is likely going
to be problematic. Wouldn't it be better to check for failure here?

Jan