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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/23/26 1:29 PM, Andrew Cooper wrote:
> On 17/06/2026 12:17 pm, 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.
> Well no, it's not redundant.  A NULL pointer is used to signal that
> preemption is not permitted/available in the current context.
>
> You notice this below, but it does invalidate the description given in
> this paragraph.
I will reword commit message to:

```
p2m_set_allocation() uses a bool *preempted out-argument that overloads 
two meanings. When non-NULL, the value written back (true) duplicates 
information already carried by the -ERESTART return code — pure 
redundancy, which the caller-side ASSERT(preempted == (rc == -ERESTART)) 
only documents. Separately, a NULL pointer is an implicit calling 
convention meaning "preemption is not permitted in this context.
Replace the pointer with a plain bool can_preempt that explicitly 
controls whether the preemption check runs, making the NULL-to-suppress 
convention type-safe and self-documenting, and rely on the -ERESTART 
return code alone to report that preemption occurred.
...
```

Would this rewording work for you, Andrew and Michal?


> > 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
> > @@ -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);
> > 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);
> Passing booleans like this makes the code unnecessarily hard to follow.
>
> At least use ", /* can_preempt */ true);" so the context is available
> directly to the reader.
Did you mean `false` here: ", /* can_preempt */ true);"?

I assume I should apply this pattern throughout the patch wherever true 
or false is passed explicitly as an argument to p2m_set_allocation(), right?
Thanks.

~ Oleksii