RE: [PATCH v3] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Subject: Re: [PATCH v3] xen/arm: p2m: Populate pages for GICv2 mapping in
> arch_domain_create()
> 
> Hi Henry,
> 
> On 17/10/2022 08:46, Henry Wang wrote:
> >       if ( p2m->root )
> > @@ -1762,6 +1778,20 @@ int p2m_init(struct domain *d)
> >       INIT_PAGE_LIST_HEAD(&p2m->pages);
> >       INIT_PAGE_LIST_HEAD(&d->arch.paging.p2m_freelist);
> >
> > +    /*
> > +     * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2
> area
> > +     * when the domain is created. Considering the worst case for page
> > +     * tables and keep a buffer, populate 16 pages to the P2M pages pool
> here.
> > +     * For GICv3, the above-mentioned P2M mapping is not necessary, but
> since
> > +     * the allocated 16 pages here would not be lost, hence populate these
> > +     * pages unconditionally.
> > +     */
> > +    spin_lock(&d->arch.paging.lock);
> > +    rc = p2m_set_allocation(d, 16, NULL);
> > +    spin_unlock(&d->arch.paging.lock);
> > +    if ( rc != 0 )
> > +        return rc;
> 
> 
> p2m_set_allocation() wants to be called after 'p->domain' is set. So
> p2m_teardown_final() will not return early and leak memory (as Andrew
> pointed out).

Yes this is a really silly mistake that I made, and I am really sorry for that.

> 
> For simplicity I would move the code right at the end of the function.

Definitely.

> But if you want to keep it here then...
> 
> > +
> >       p2m->vmid = INVALID_VMID;
> 
> ... this needs to be done first as well.

I agree moving that part to the end would be a better solution.

> 
> >
> >       rc = p2m_alloc_vmid(d);
> 
> Note that if you move the code at the end. Then you will need to add:
> 
> if ( rc )
>    return rc;
> 
> after the call to p2m_alloc_table();

Yes, thanks very much for the reminder.

> 
> Other than that the code looks good to me.

As Andrew stated in his email that he will draft a patch, TBH I am not
sure if my patch would be superseded by his, but I will try to see if it
is possible for me to send a v4 today.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall