On Thursday 04 Mar 2021 at 15:30:36 (+0000), Will Deacon wrote:
> On Tue, Mar 02, 2021 at 02:59:42PM +0000, Quentin Perret wrote:
> > +void *hyp_alloc_pages(struct hyp_pool *pool, unsigned int order)
> > +{
> > + unsigned int i = order;
> > + struct hyp_page *p;
> > +
> > + hyp_spin_lock(&pool->lock);
> > +
> > + /* Look for a high-enough-order page */
> > + while (i < pool->max_order && list_empty(&pool->free_area[i]))
> > + i++;
> > + if (i >= pool->max_order) {
> > + hyp_spin_unlock(&pool->lock);
> > + return NULL;
> > + }
> > +
> > + /* Extract it from the tree at the right order */
> > + p = list_first_entry(&pool->free_area[i], struct hyp_page, node);
> > + p = __hyp_extract_page(pool, p, order);
> > +
> > + hyp_spin_unlock(&pool->lock);
> > + hyp_page_ref_inc(p);
>
> I find this a little scary, as we momentarily drop the lock. It think
> it's ok because the reference count on the page must be 0 at this point,
Yep, @p shouldn't be visible to the caller yet so this should be fine.
> but actually then I think it would be clearer to have a
> hyp_page_ref_init() function which could take the lock, check that the
> refcount is indeed 0 and then set it to 1.
Works for me. Maybe I'll use another name for the API to stay consistent
with the kernel gfp code (hyp_page_ref_inc() and friends are inspired
from their kernel counterpart). And I guess I can hyp_panic() if the
refcount is not 0 at this point to match the VM_BUG_ON_PAGE() in
set_page_refcounted().
Thanks!
Quentin
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
