RE: [PATCH v2] 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 v2] xen/arm: p2m: Populate pages for GICv2 mapping in
> arch_domain_create()
> 
> Hi Henry,
> 
> >
> > -int p2m_teardown(struct domain *d)
> > +int p2m_teardown(struct domain *d, bool allow_preemption)
> >   {
> I think the part to clean & invalidate the root should not be necessary
> if the domain is not scheduled. Similarly, I think we might only need to
> do once by domain (rather than for every call). So I would consider to
> move the logic outside of the function.
> 
> That's not for 4.17 thought.

Sure, I can prepare the follow up patch after 4.17 as (1) I am also worried
about if this patch would become bigger and bigger (2) I checked you also
want other things in your below comment.

> 
> >       struct p2m_domain *p2m = p2m_get_hostp2m(d);
> >       unsigned long count = 0;
> > @@ -1716,7 +1716,7 @@ int p2m_teardown(struct domain *d)
> >           p2m_free_page(p2m->domain, pg);
> >           count++;
> >           /* Arbitrarily preempt every 512 iterations */
> > -        if ( !(count % 512) && hypercall_preempt_check() )
> > +        if ( allow_preemption && !(count % 512) &&
> hypercall_preempt_check() )
> >           {
> >               rc = -ERESTART;
> >               break;
> > @@ -1736,6 +1736,17 @@ void p2m_final_teardown(struct domain *d)
> >       if ( !p2m->domain )
> >           return;
> >
> > +    if ( !page_list_empty(&p2m->pages) )
> 
> Did you add this check to avoid the clean & invalidate if the list is empty?

Yep. I think we only need the p2m_teardown() if we actually have something
in p2m->pages list.

> 
> > +        p2m_teardown(d, false);
> 
> Today, it should be fine to ignore p2m_teardown(). But I would prefer if
> we add an ASSERT()/BUG_ON() (or else) to make confirm this is the case.

Sorry I do not really understand why we can ignore the p2m_teardown()
probably because of my English. Let's talk a bit more in C if you don't mind :))
Do you mean p2m_teardown() should be called here unconditionally without
the if ( !page_list_empty(&p2m->pages) ) check?

> 
> This also wants to be documented on top of p2m_teardown() as it would be
> easier to know that the function should always return 0 when
> !allow_preemption is not set.

Ok, will do.

> 
> I also noticed that relinquish_p2m_mapping() is not called. This should
> be fine for us because arch_domain_create() should never create a
> mapping that requires p2m_put_l3_page() to be called.
> 
> I think it would be good to check it in __p2m_set_entry(). So we don't
> end up to add such mappings by mistake.

I thought for a while but failed to translate the above requirements
to proper if conditions in __p2m_set_entry()...

> 
> I would have suggested to add a comment only for version and send a
> follow-up patch. But I don't exactly know where to put it.

...how about p2m_final_teardown(), we can use a TODO to explain why
we don't need to call relinquish_p2m_mapping() and a following patch
can fix this?

> 
> > +
> > +    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);
> > +    }
> > +
> >       ASSERT(page_list_empty(&p2m->pages));
> 
> I would move this assert between the two ifs you added.

Sure, will do in v3.

Kind regards,
Henry

> 
> >       ASSERT(page_list_empty(&d->arch.paging.p2m_freelist));
> >
> 
> Cheers,
> 
> --
> Julien Grall