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

[Julien Grall <julien@xxxxxxx>]

On 14/10/2022 12:19, Henry Wang wrote:
> Hi Julien,
Hi Henry,

> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > >        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.
How about adding the check in p2m_teardown()? So it will be easier to 
remember that the check can be dropped if we move the zeroing outside of 
the function.
> > > +        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.
No, I forgot a word in my sentence. I was meant to say that the return 
of p2m_teardown() can be ignored in our situation because it only return 
0 or -ERESTART. The latter cannnot happen when the preemption is not 
enabled.
But I would like to add some code (either ASSERT() or BUG_ON()) to 
confirm that p2m_teardown() will always return 0.
> 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?
See above.

> > 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()...
For checking the mapping, we can do:

if ( !removing_mapping && (p2m_is_foreign(t) || (p2m_is_ram(t) && 
is_xenheap_mfn(mfn) )
    return -EINVAL;

We also need a way to check whether we are called from 
arch_domain_create(). I think we would need a field in the domain 
structure to indicate whether it is still initializating.
This is a bit ugly though. Any other suggestions?

> > 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?
To me the TODO would make more sense on top of p2m_set_entry() because 
this is where the issue should be fixed. This is also where most of the 
reader will likely look if they want to understand how p2m_set_entry() 
can be used.
We could also have a comment in p2m_final_teardown() stating that the 
relinquish function is not called because the P2M should not contain any 
mapping that requires specific operation when removed. This could point 
to the comment in p2m_set_entry().
Cheers,

--
Julien Grall