Re: [PATCH v2 1/1] xen/arm: populate a default P2M pool for new domains

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 23-Oct-26 15:26, Julian Vetter wrote:
> On x86, a domain can be populated directly after the
> XEN_DOMCTL_createdomain call. Sizing the pool is thus an optional tuning
> step for the toolstack (libxl: "the Xen minimum is for safety, not
> performance").
> 
> On ARM however, the P2M pool of a non-hardware domain starts empty. So,
> any toolstack must size the pool before populating the domain's physmap,
> otherwise no P2M tables can be allocated. This is an ARM specific step,
> which every toolstack has to know about.
> 
> Instead, populate a default pool in p2m_init(). Ideally, it would match
> the formula used by dom0less (domain_p2m_pages()) and libxl
> (libxl__get_required_paging_memory()), but that depends on the amount of
> RAM for the given domain, which isn't known at domain creation. So, use
> the calculation but without depending on the amount of RAM for the
> domain, i.e., 1MByte per vCPU, plus 512KBytes for the extended regions.
> Then dom0less and libxl set the full size, as they do today.
> 
> Skip the hardware domain, which has no dedicated P2M pool. Also skip LLC
> colored setups, as the domain's colors are only configured after domain
> creation, so the pool pages would be allocated with the default colors.
> 
> Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
> ---
> Changes in v2:
> - Size the default pool based on the exisiting formula
> - Rewrite commit message to explain why a default pool is desirable
> ---
>  xen/arch/arm/mmu/p2m.c                  | 24 ++++++++++++++++++++++++
>  xen/common/device-tree/dom0less-build.c |  7 +------
>  xen/include/xen/domain.h                | 13 +++++++++++++
>  3 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
> index 2cf35d8a37..d4971ba8d9 100644
> --- a/xen/arch/arm/mmu/p2m.c
> +++ b/xen/arch/arm/mmu/p2m.c
> @@ -3,7 +3,10 @@
>  #include <xen/domain_page.h>
>  #include <xen/ioreq.h>
>  #include <xen/lib.h>
> +#include <xen/llc-coloring.h>
> +#include <xen/pfn.h>
>  #include <xen/sched.h>
> +#include <xen/sizes.h>
>  #include <xen/softirq.h>
>  
>  #include <asm/alternative.h>
> @@ -1541,6 +1544,27 @@ int p2m_init(struct domain *d)
>      if ( rc )
>          return rc;
>  
> +    /*
> +     * Populate the P2M pages pool here. So, the domain is usable without the
> +     * pool being sized first, as on x86. The toolstack, or dom0less, still
> +     * sets the full size later.
> +     *
> +     * Skip the hardware domain, which has no dedicated P2M pool, and LLC
> +     * colored setups, as the domain's colors are only configured after 
> domain
> +     * creation.
> +     */
> +    if ( !is_hardware_domain(d) && !llc_coloring_enabled )
> +    {
> +        unsigned long pages =
> +            PFN_DOWN(domain_p2m_default_kb(0, d->max_vcpus) * SZ_1K);
domain_p2m_default_kb helper would better convert to pages right away, given
that all the callers eventually need pages, not size in kb.

> +
> +        spin_lock(&d->arch.paging.lock);
> +        rc = p2m_set_allocation(d, pages, /* can_preempt */ false);
I'm not sure if this design is correct (I know I asked "why can't we set the
allocation right away" but at that time I did not consider the worst case). In
the domain creation happening at runtime, this call can take a lot of time in
the worst case (e.g. 128 vCPUs) and is not preemptable unlike
XEN_DOMCTL_set_paging_mempool_size. Maybe indeed it's better to come up with a
sane default, given that toolstacks still have to size it up to take into
account the RAM size. FWICS x86 HAP uses 256 pages. We could use this value too.
Does other Arm maintainers have any opinion here?

> +        spin_unlock(&d->arch.paging.lock);
> +        if ( rc )
> +            return rc;
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/xen/common/device-tree/dom0less-build.c 
> b/xen/common/device-tree/dom0less-build.c
> index fcb438c50a..21a13f9370 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -737,12 +737,7 @@ static void __init domain_vcpu_affinity(struct domain *d,
>  static unsigned long __init domain_p2m_pages(unsigned long maxmem_kb,
>                                               unsigned int smp_cpus)
>  {
> -    /*
> -     * Keep in sync with libxl__get_required_paging_memory().
> -     * 256 pages (1MB) per vcpu, plus 1 page per MiB of RAM for the P2M map,
> -     * plus 128 pages to cover extended regions.
> -     */
> -    unsigned long memkb = 4 * (256 * smp_cpus + (maxmem_kb / 1024) + 128);
> +    unsigned long memkb = domain_p2m_default_kb(maxmem_kb, smp_cpus);
>  
>      BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
>  
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index aeb8b36ad1..74dee32fb9 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -132,6 +132,19 @@ int default_initialise_vcpu(struct vcpu *v, 
> XEN_GUEST_HANDLE_PARAM(void) arg);
>  int arch_get_paging_mempool_size(struct domain *d, uint64_t *size /* bytes 
> */);
>  int arch_set_paging_mempool_size(struct domain *d, uint64_t size /* bytes 
> */);
>  
> +/*
> + * Default size of the P2M pool, in KBytes, for a domain with the given 
> amount
> + * of RAM and number of vCPUs.
> + * Keep in sync with libxl__get_required_paging_memory().
> + * 256 pages (1MB) per vCPU, plus 1 page per MiB of RAM for the P2M map,
> + * plus 128 pages to cover extended regions.
This is Arm formula but the header is common.

~Michal