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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 6/24/26 9:32 AM, Orzel, Michal wrote:
> 
> 
> 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.

I suggested a rewording of the commit message in reply to Andrew's 
comment. Could you please review my response?

> 
>>
>> 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@suse.com>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>> 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.
> 

Agree, it should be updated. I'll apply your suggestion in the next 
patch version.

>> + * 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).

Makes sense. Also, it will address the issue that, based on your comment 
above, the comment should be updated here and before the prototype.

> 
>> -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.

I will add __must_check.

Thanks.

~ Oleksii