Re: [PATCH v2 16/17] xen/riscv: implement mfn_valid() and page reference, ownership handling helpers

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/2/25 12:28 PM, Jan Beulich wrote:

> On 02.07.2025 12:09, Jan Beulich wrote:
> > On 10.06.2025 15:05, Oleksii Kurochko wrote:
> > > @@ -613,3 +612,91 @@ void __iomem *ioremap(paddr_t pa, size_t len)
> > >  {
> > >      return ioremap_attr(pa, len, PAGE_HYPERVISOR_NOCACHE);
> > >  }
> > > +
> > > +int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
> > > +{
> > > +    ASSERT_UNREACHABLE();
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static struct domain *page_get_owner_and_nr_reference(struct page_info *page,
> > > +                                                      unsigned long nr)
> > > +{
> > > +    unsigned long x, y = page->count_info;
> > > +    struct domain *owner;
> > > +
> > > +    /* Restrict nr to avoid "double" overflow */
> > > +    if ( nr >= PGC_count_mask )
> > > +    {
> > > +        ASSERT_UNREACHABLE();
> > > +        return NULL;
> > > +    }
> > >         
> > I question the validity of this, already in the Arm original: I can't spot
> > how the caller guarantees to stay below that limit. Without such an
> > (attempted) guarantee, ASSERT_UNREACHABLE() is wrong to use. All I can see
> > is process_shm_node() incrementing shmem_extra[].nr_shm_borrowers, without
> > any limit check.
> >
> > > +    do {
> > > +        x = y;
> > > +        /*
> > > +         * Count ==  0: Page is not allocated, so we cannot take a reference.
> > > +         * Count == -1: Reference count would wrap, which is invalid.
> > > +         */
> > >         
> > May I once again ask that you look carefully at comments (as much as at code)
> > you copy. Clearly this comment wasn't properly updated when the bumping by 1
> > was changed to bumping by nr.
> >
> > > +        if ( unlikely(((x + nr) & PGC_count_mask) <= nr) )
> > > +            return NULL;
> > > +    }
> > > +    while ( (y = cmpxchg(&page->count_info, x, x + nr)) != x );
> > > +
> > > +    owner = page_get_owner(page);
> > > +    ASSERT(owner);
> > > +
> > > +    return owner;
> > > +}
> > >         
> >       
> There also looks to be a dead code concern here (towards the "nr" parameters
> here and elsewhere, when STATIC_SHM=n). Just that apparently we decided to
> leave out Misra rule 2.2 entirely.
>     
I think that I didn't get what is an issue when STATIC_SHM=n, functions is still
going to be called through page_get_owner_and_reference(), at least, in page_alloc.c .
~ Oleksii