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

[Julien Grall <julien@xxxxxxx>]

On 14/10/2022 10:28, Henry Wang wrote:
> Hi Jan,
>
> Thanks for the review.
>
> > -----Original Message-----
> > From: Jan Beulich <jbeulich@xxxxxxxx>
> > Subject: Re: [PATCH v2] xen/arm: p2m: Populate pages for GICv2 mapping in
> > arch_domain_create()
> >
> > On 14.10.2022 10:09, Henry Wang wrote:
> > > --- a/xen/arch/arm/domain.c
> > > +++ b/xen/arch/arm/domain.c
> > > @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d,
> > >           BUG();
> > >       }
> > >
> > > +    /*
> > > +     * 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 )
> > > +        goto fail;
> > Putting this level of knowledge here feels like a layering violation to
> > me. My first suggestion would be to move this call somewhere under
> > p2m_init().
> That is definitely possible. If Julien or other Arm maintainers are not
> against that I am happy to move this to p2m_init() in v3.
I understand both of Jan and your concern. I don't really have a strong 
opinion either way.
You are the author of the patch, so I will let you chose.

> The reason why the above block is placed here is just I thought to use
> d->arch.vgic.version to only populate the 16 pages for GICv2 in the
> beginning, and d->arch.vgic.version is first assigned later after p2m_init(),
> but later we decided to populated the pages unconditionally so actually
> now we can move the part to p2m_init().
>
> > If that's not possible for some reason, I'd like to suggest
> > passing 1 here as the count and then adding a min-acceptable check to
> > p2m_set_allocation() along the lines of x86'es shadow_set_allocation().
> > That way you'd also guarantee the minimum number of pages in case a
> > subsequent tiny allocation request came in via domctl.
I really dislike this. If the user ask for 1 pages and we only allow 16. 
Then this should be rejected (not bumped to 16).
However, the code in p2m_set_allocation() will only look at the free 
pages (like x86 does). So what you suggest would not do what you want.
> > > @@ -1736,6 +1736,17 @@ void p2m_final_teardown(struct domain *d)
> > >       if ( !p2m->domain )
> > >           return;
> > >
> > > +    if ( !page_list_empty(&p2m->pages) )
> > > +        p2m_teardown(d, false);
> > > +
> > > +    if ( d->arch.paging.p2m_total_pages != 0 )
> > > +    {
> > > +        spin_lock(&d->arch.paging.lock);
> > > +        p2m_set_allocation(d, 0, NULL);
> > > +        spin_unlock(&d->arch.paging.lock);
> > > +        ASSERT(d->arch.paging.p2m_total_pages == 0);
> > > +    }
> > Is it intentional to largely open-code p2m_teardown_allocation() here?
> Yes, AFAICT p2m_teardown_allocation() is preemptible and we don't want
> any preemption here.
Like Jan, I would prefer if we can avoid the duplication. The loop 
suggested by Jan should work.
Cheers,

--
Julien Grall