Re: [PATCH v3 02/23] xen: arm: update p2m_set_allocation() prototype

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

On 17-Jun-26 13:17, Oleksii Kurochko wrote:
> p2m_set_allocation() signals preemption redundantly: via a bool *preempted
> out-argument (set to true) and via -ERESTART return code, both at the same
> time. This led to the caller-side ASSERT(preempted == (rc == -ERESTART))
> added solely to document their agreement.
> 
> Drop the out-argument entirely. The return code alone is sufficient to
> distinguish preemption (-ERESTART) from success (0) or other failures.
> Replace the pointer with a plain bool can_preempt that controls whether
> the preemption check is performed at all, making the previous
> NULL-to-suppress-preemption calling convention explicit and type-safe.
As Andrew said, this paragraph contradicts the first one i.e. we still need to
pass whether to perform the check or not, so it's not redundant.

> 
> Since p2m_set_allocation() is called by the common dom0less build code,
> move its declaration from the ARM-specific asm/p2m.h to xen/p2m-common.h.
> 
> Reported-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in v3:
>  - Nothing changed. Only rebase.
> ---
> Changes in v2:
>  - new patch
> ---
> ---
>  xen/arch/arm/include/asm/p2m.h          |  1 -
>  xen/arch/arm/mmu/p2m.c                  | 22 ++++++++--------------
>  xen/arch/riscv/include/asm/paging.h     |  2 +-
>  xen/arch/riscv/p2m.c                    |  7 ++++---
>  xen/arch/riscv/paging.c                 |  7 ++-----
>  xen/common/device-tree/dom0less-build.c |  2 +-
>  xen/include/xen/p2m-common.h            |  2 ++
>  7 files changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> index 4a4913716bdd..737da60dcf58 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -238,7 +238,6 @@ void p2m_restore_state(struct vcpu *n);
>  /* Print debugging/statistial info about a domain's p2m */
>  void p2m_dump_info(struct domain *d);
>  
> -int p2m_set_allocation(struct domain *d, unsigned long pages, bool 
> *preempted);
>  int p2m_teardown_allocation(struct domain *d);
>  
>  static inline void p2m_write_lock(struct p2m_domain *p2m)
> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
> index 51abf3504fcf..e5c6be7c0890 100644
> --- a/xen/arch/arm/mmu/p2m.c
> +++ b/xen/arch/arm/mmu/p2m.c
> @@ -67,10 +67,11 @@ int arch_get_paging_mempool_size(struct domain *d, 
> uint64_t *size)
>  
>  /*
>   * Set the pool of pages to the required number of pages.
> - * Returns 0 for success, non-zero for failure.
> + * Returns 0 for success, -ERESTART if preempted, or a negative error code on
It returns -ERESTART if preempted *AND* preemption is allowed - please clarify
in the description.

> + * failure.
>   * Call with d->arch.paging.lock held.
>   */
> -int p2m_set_allocation(struct domain *d, unsigned long pages, bool 
> *preempted)
> +int p2m_set_allocation(struct domain *d, unsigned long pages, bool 
> can_preempt)
>  {
>      struct page_info *pg;
>  
> @@ -112,11 +113,8 @@ int p2m_set_allocation(struct domain *d, unsigned long 
> pages, bool *preempted)
>              break;
>  
>          /* Check to see if we need to yield and try again */
> -        if ( preempted && general_preempt_check() )
> -        {
> -            *preempted = true;
> +        if ( can_preempt && general_preempt_check() )
>              return -ERESTART;
> -        }
>      }
>  
>      return 0;
> @@ -125,7 +123,6 @@ int p2m_set_allocation(struct domain *d, unsigned long 
> pages, bool *preempted)
>  int arch_set_paging_mempool_size(struct domain *d, uint64_t size)
>  {
>      unsigned long pages = size >> PAGE_SHIFT;
> -    bool preempted = false;
>      int rc;
>  
>      if ( (size & ~PAGE_MASK) ||          /* Non page-sized request? */
> @@ -133,27 +130,24 @@ int arch_set_paging_mempool_size(struct domain *d, 
> uint64_t size)
>          return -EINVAL;
>  
>      spin_lock(&d->arch.paging.lock);
> -    rc = p2m_set_allocation(d, pages, &preempted);
> +    rc = p2m_set_allocation(d, pages, true);
>      spin_unlock(&d->arch.paging.lock);
>  
> -    ASSERT(preempted == (rc == -ERESTART));
> -
>      return rc;
>  }
>  
>  int p2m_teardown_allocation(struct domain *d)
>  {
>      int ret = 0;
> -    bool preempted = false;
>  
>      spin_lock(&d->arch.paging.lock);
>      if ( d->arch.paging.p2m_total_pages != 0 )
>      {
> -        ret = p2m_set_allocation(d, 0, &preempted);
> -        if ( preempted )
> +        ret = p2m_set_allocation(d, 0, true);
> +        if ( ret == -ERESTART )
>          {
>              spin_unlock(&d->arch.paging.lock);
> -            return -ERESTART;
> +            return ret;
>          }
>          ASSERT(d->arch.paging.p2m_total_pages == 0);
>      }
> diff --git a/xen/arch/riscv/include/asm/paging.h 
> b/xen/arch/riscv/include/asm/paging.h
> index e487c89a4ccd..103384723dc5 100644
> --- a/xen/arch/riscv/include/asm/paging.h
> +++ b/xen/arch/riscv/include/asm/paging.h
> @@ -9,7 +9,7 @@ struct page_info;
>  int paging_domain_init(struct domain *d);
>  
>  int paging_freelist_adjust(struct domain *d, unsigned long pages,
> -                           bool *preempted);
> +                           bool can_preempt);
>  
>  int paging_ret_to_domheap(struct domain *d, unsigned int nr_pages);
>  int paging_refill_from_domheap(struct domain *d, unsigned int nr_pages);
> diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
> index 703b9f4d2540..41d6d3d5e699 100644
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -430,15 +430,16 @@ int p2m_init(struct domain *d, const struct 
> xen_domctl_createdomain *config)
>  
>  /*
>   * Set the pool of pages to the required number of pages.
> - * Returns 0 for success, non-zero for failure.
> + * Returns 0 for success, -ERESTART if preempted, or a negative error code on
> + * failure.
>   * Call with d->arch.paging.lock held.
>   */
Move the description where a prototype lives. This way you will avoid adding one
at each definition (remove from here and Arm then).

> -int p2m_set_allocation(struct domain *d, unsigned long pages, bool 
> *preempted)
> +int p2m_set_allocation(struct domain *d, unsigned long pages, bool 
> can_preempt)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      int rc;
>  
> -    if ( (rc = paging_freelist_adjust(d, pages, preempted)) )
> +    if ( (rc = paging_freelist_adjust(d, pages, can_preempt)) )
>          return rc;
>  
>      /*
> diff --git a/xen/arch/riscv/paging.c b/xen/arch/riscv/paging.c
> index 76a203edbb0c..35f572689a7c 100644
> --- a/xen/arch/riscv/paging.c
> +++ b/xen/arch/riscv/paging.c
> @@ -47,7 +47,7 @@ static int _paging_add_to_freelist(struct domain *d)
>  }
>  
>  int paging_freelist_adjust(struct domain *d, unsigned long pages,
> -                           bool *preempted)
> +                           bool can_preempt)
>  {
>      ASSERT(spin_is_locked(&d->arch.paging.lock));
>  
> @@ -66,11 +66,8 @@ int paging_freelist_adjust(struct domain *d, unsigned long 
> pages,
>              return rc;
>  
>          /* Check to see if we need to yield and try again */
> -        if ( preempted && general_preempt_check() )
> -        {
> -            *preempted = true;
> +        if ( can_preempt && general_preempt_check() )
>              return -ERESTART;
> -        }
>      }
>  
>      return 0;
> diff --git a/xen/common/device-tree/dom0less-build.c 
> b/xen/common/device-tree/dom0less-build.c
> index eacfd93087ae..c3818ffed45f 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -747,7 +747,7 @@ static int __init domain_p2m_set_allocation(struct domain 
> *d, uint64_t mem,
>                  domain_p2m_pages(mem, d->max_vcpus);
>  
>      spin_lock(&d->arch.paging.lock);
> -    rc = p2m_set_allocation(d, p2m_pages, NULL);
> +    rc = p2m_set_allocation(d, p2m_pages, false);
>      spin_unlock(&d->arch.paging.lock);
>  
>      return rc;
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> index f0bd9a6b9896..1b44ec8ce36c 100644
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -43,5 +43,7 @@ int __must_check check_get_page_from_gfn(struct domain *d, 
> gfn_t gfn,
>                                           bool readonly, p2m_type_t *p2mt_p,
>                                           struct page_info **page_p);
>  
> +int p2m_set_allocation(struct domain *d, unsigned long pages,
I think it's worth adding __must_check given that rc is now the only preemption
status checker.

~Michal