Re: [PATCH] xen/arm: Don't create useless P2M pool for a hardware domain

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Michal,

Michal Orzel <michal.orzel@xxxxxxx> writes:

> On Arm, the hardware domain does not use the P2M pool. Its P2M pages are
> allocated directly from the heap, without any limit. However, when the
> hardware domain is created using dom0less, we still allocate a P2M pool
> for it, either of the default size or the one given in
> xen,domain-p2m-mem-mb. This memory is never used and is simply wasted.
> The same applies when the pool size is set for the hardware domain using
> XEN_DOMCTL_set_paging_mempool_size.
>
> Don't create the pool for a hardware domain. Make the get/set paging
> mempool domctls return -EOPNOTSUPP, skip the pool teardown and reject
> xen,domain-p2m-mem-mb for a hardware domain in dom0less.
>
> The dom0less check is Arm specific, because on RISC-V the hardware domain
> does use the P2M pool.
>
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>

> ---
>  docs/misc/arm/device-tree/booting.txt   |  4 +++-
>  xen/arch/arm/mmu/p2m.c                  | 14 ++++++++++++++
>  xen/common/device-tree/dom0less-build.c | 12 ++++++++++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index 3787d2e2b668..c3705ab7d913 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -277,7 +277,9 @@ with the following properties:
>      used for the domain P2M pool. This is in-sync with the shadow_memory
>      option in xl.cfg. Leaving this field empty in device tree will lead to
>      the default size of domain P2M pool, i.e. 1MB per guest vCPU plus 4KB
> -    per MB of guest RAM plus 512KB for guest extended regions.
> +    per MB of guest RAM plus 512KB for guest extended regions. On Arm, the
> +    hardware domain has no dedicated P2M pool, so specifying this property
> +    for it makes the domain fail to build.
>  
>  - max_grant_version
>  
> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
> index 2cf35d8a3709..70d7c9fed955 100644
> --- a/xen/arch/arm/mmu/p2m.c
> +++ b/xen/arch/arm/mmu/p2m.c
> @@ -61,6 +61,10 @@ static void p2m_free_page(struct domain *d, struct 
> page_info *pg)
>  /* Return the size of the pool, in bytes. */
>  int arch_get_paging_mempool_size(struct domain *d, uint64_t *size)
>  {
> +    /* No dedicated pool (no limit) for a hardware domain */
> +    if ( is_hardware_domain(d) )
> +        return -EOPNOTSUPP;
> +
>      *size = (uint64_t)ACCESS_ONCE(d->arch.paging.p2m_total_pages) << 
> PAGE_SHIFT;
>      return 0;
>  }
> @@ -69,6 +73,8 @@ int p2m_set_allocation(struct domain *d, unsigned long 
> pages, bool can_preempt)
>  {
>      struct page_info *pg;
>  
> +    /* No dedicated pool (no limit) for a hardware domain */
> +    ASSERT(!is_hardware_domain(d));
>      ASSERT(spin_is_locked(&d->arch.paging.lock));
>  
>      for ( ; ; )
> @@ -119,6 +125,10 @@ int arch_set_paging_mempool_size(struct domain *d, 
> uint64_t size)
>      unsigned long pages = size >> PAGE_SHIFT;
>      int rc;
>  
> +    /* No dedicated pool (no limit) for a hardware domain */
> +    if ( is_hardware_domain(d) )
> +        return -EOPNOTSUPP;
> +
>      if ( (size & ~PAGE_MASK) ||          /* Non page-sized request? */
>           pages != (size >> PAGE_SHIFT) ) /* 32-bit overflow? */
>          return -EINVAL;
> @@ -134,6 +144,10 @@ int p2m_teardown_allocation(struct domain *d)
>  {
>      int ret = 0;
>  
> +    /* No dedicated pool - nothing to tear down */
> +    if ( is_hardware_domain(d) )
> +        return 0;
> +
>      spin_lock(&d->arch.paging.lock);
>      if ( d->arch.paging.p2m_total_pages != 0 )
>      {
> diff --git a/xen/common/device-tree/dom0less-build.c 
> b/xen/common/device-tree/dom0less-build.c
> index fcb438c50a04..a877e695ad11 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -757,6 +757,18 @@ static int __init domain_p2m_set_allocation(struct 
> domain *d, uint64_t mem,
>      int rc;
>  
>      rc = dt_property_read_u32(node, "xen,domain-p2m-mem-mb", &p2m_mem_mb);
> +
> +    /* No dedicated pool (no limit) for a hardware domain on Arm */
> +    if ( IS_ENABLED(CONFIG_ARM) && is_hardware_domain(d) )
> +    {
> +        if ( !rc )
> +            return 0;
> +
> +        printk(XENLOG_ERR
> +               "xen,domain-p2m-mem-mb cannot be specified for a hardware 
> domain\n");
> +        return -EINVAL;
> +    }
> +
>      /* If xen,domain-p2m-mem-mb is not specified, use the default value. */
>      p2m_pages = rc ?
>                  p2m_mem_mb << (20 - PAGE_SHIFT) :

-- 
WBR, Volodymyr