Re: [PATCH v2 7/7] x86/hap: Increase the number of initial mempool_size to 1024 pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2024 18:52, Petr Beneš wrote:
> From: Petr Beneš <w1benny@xxxxxxxxx>
> 
> This change anticipates scenarios where `max_altp2m` is set to its maximum
> supported value (i.e., 512), ensuring sufficient memory is allocated upfront
> to accommodate all altp2m tables without initialization failure.

And guests with fewer or even no altp2m-s still need the same bump? You
know the number of altp2m-s upon domain creation, so why bump by any more
than what's strictly needed for that?

> The necessity for this increase arises from the current mechanism where altp2m
> tables are allocated at initialization, requiring one page from the mempool
> for each altp2m view.

So that's the p2m_alloc_table() out of hap_enable()? If you're permitting
up to 512 altp2m-s, I think it needs considering to not waste up to 2Mb
without knowing how many of the altp2m-s are actually going to be used.
How complicate on-demand allocation would be I can't tell though, I have
to admit.

> --- a/tools/tests/paging-mempool/test-paging-mempool.c
> +++ b/tools/tests/paging-mempool/test-paging-mempool.c
> @@ -35,7 +35,7 @@ static struct xen_domctl_createdomain create = {
>  
>  static uint64_t default_mempool_size_bytes =
>  #if defined(__x86_64__) || defined(__i386__)
> -    256 << 12; /* Only x86 HAP for now.  x86 Shadow needs more work. */
> +    1024 << 12; /* Only x86 HAP for now.  x86 Shadow needs more work. */

I also can't derive from the description why we'd need to go from 256 to
1024 here and ...

> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -468,7 +468,7 @@ int hap_enable(struct domain *d, u32 mode)
>      if ( old_pages == 0 )
>      {
>          paging_lock(d);
> -        rv = hap_set_allocation(d, 256, NULL);
> +        rv = hap_set_allocation(d, 1024, NULL);

... here. You talk of (up to) 512 pages there only.

Also isn't there at least one more place where the tool stack (libxl I
think) would need changing, where Dom0 ballooning needs are calculated?
And/or doesn't the pool size have a default calculation in the tool
stack, too?

Jan