Re: [PATCH v1 01/27] xen/riscv: Implement ARCH_PAGING_MEMPOOL

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 3/11/26 9:18 AM, Jan Beulich wrote:
> On 10.03.2026 18:08, Oleksii Kurochko wrote:
> > The p2m_freelist is used to allocate pages for the P2M, but to initialize
> > this list, domain_p2m_set_allocation() might be called.
> > This function is invoked in construct_domU() within the common Dom0less
> > code, and providing an implementation of domain_p2m_set_allocation() when
> > CONFIG_ARCH_PAGING_MEMPOOL=y is appropriate for RISC-V.
> With this wording it is odd to see ...
>
> > --- a/xen/arch/riscv/Kconfig
> > +++ b/xen/arch/riscv/Kconfig
> > @@ -1,5 +1,6 @@
> >   config RISCV
> >         def_bool y
> > +       select ARCH_PAGING_MEMPOOL
> ... this. You really want to settle on whether it is selected unconditionally
> or not. Also for the code below, where ...
>
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -10,7 +10,7 @@ obj-y += irq.o
> >   obj-y += kernel.o
> >   obj-y += mm.o
> >   obj-y += p2m.o
> > -obj-y += paging.o
> > +obj-$(CONFIG_ARCH_PAGING_MEMPOOL) += paging.o
> >   obj-y += pt.o
> >   obj-$(CONFIG_RISCV_64) += riscv64/
> >   obj-y += sbi.o
> ... this change and any #ifdef-ary further down aren't needed unless the
> select became conditional. (Plus with the change above things likely wouldn't
> even build if ARCH_PAGING_MEMPOOL could be off under certain conditions.)
I missed if ARCH_PAGING_MEMPOOL=n then p2m.c, at least, will fail to 
compile so much more things would be needed to be #ifdef-ed.
Just for simplicity then I will set CONFIG_ARCH_PAGING_MEMPOOL=y 
unconditionally and drop all the #ifdef-s related to this config I've 
added in this patch.
> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -1568,3 +1568,34 @@ struct page_info *get_page_from_gfn(struct domain *d, 
> > unsigned long gfn,
> >   
> >       return page;
> >   }
> > +
> > +#ifdef CONFIG_ARCH_PAGING_MEMPOOL
> > +
> > +int arch_set_paging_mempool_size(struct domain *d, uint64_t size)
> > +{
> > +    unsigned long pages = PFN_DOWN(size);
> > +    bool preempted = false;
> > +    int rc;
> > +
> > +    if ( (size & ~PAGE_MASK) || /* Non page-sized request? */
> > +         pages != PFN_DOWN(size) ) /* 32-bit overflow? */
> > +        return -EINVAL;
> Can't this be had with just
>
>      if ( ((paddr_t)pages << PAGE_SHIFT) != size )
>          return -EINVAL;
>
> (and perhaps utilizing pfn_to_paddr(), even if it's not a PFN we're dealing
> with here)?
It makes sense, I will apply that for RISC-V.

Then for Arm and x86 could be done the same, I can send a separate patch 
for them.
> > +    spin_lock(&d->arch.paging.lock);
> > +    rc = p2m_set_allocation(d, pages, &preempted);
> > +    spin_unlock(&d->arch.paging.lock);
> > +
> > +    ASSERT(preempted == (rc == -ERESTART));
> This actually suggests that (once again) Arm code perhaps shouldn't have been
> copied verbatim: There shouldn't be a need for the "preempted" state to be
> returned back in two distinct ways.
Agree.

The preempted argument of p2m_set_allocation() could be switched to a 
plain 'bool could_preempt', preemption would then be signaled solely via 
return -ERESTART, and the caller would just check rc, so no local bool 
preempted in this function and no ASSERT.
I'm also thinking that the preempted argument could be dropped entirely, 
as it seems to exist only to conditionally enable the 
general_preempt_check() call inside the function. It is skipped only 
during domain_p2m_set_allocation(), which won't be a significant penalty 
if general_preempt_check() is called every time. All other callers pass 
a non-NULL preempted, so general_preempt_check() would always be 
executed regardless.
> > +    return rc;
> > +}
> > +
> > +/* Return the size of the pool, in bytes. */
> > +int arch_get_paging_mempool_size(struct domain *d, uint64_t *size)
> > +{
> > +    *size = (uint64_t)ACCESS_ONCE(d->arch.paging.total_pages) << PAGE_SHIFT;
> As per above, maybe use pfn_to_paddr()?
Yes, it could be used to be in sync with a code above.

Thanks.

~ Oleksii