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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

Thanks for reply and sharing your opinions!

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Subject: Re: [PATCH v2] xen/arm: p2m: Populate pages for GICv2 mapping in
> arch_domain_create()
> 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.

Yes, I will turn above if check to a

if ( page_list_empty(&p2m->pages) )
    return 0;

in the beginning of the p2m_teardown(), and do the clean & invalidate
follow-up after the release.

> 
> >
> >>
> >>> +        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.

I added the doc asked in your previous email. Also, I will use a

ASSERT(p2m_teardown(d, false) == 0);

in p2m_final_teardown() here.

> 
> > 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.

Thanks.

> 
> >
> >>
> >> 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;

Thanks for this, I guess without your hint it will take ages for me to
think of this.... 

> 
> 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?

My first thought is checking the implementation of domain_create()
and arch_domain_create() (as both will call arch_domain_destroy()
when fail) to see if there are some fields in struct domain or
struct arch_domain that are set/changed in this stage so probably we
can reuse. Otherwise I think adding a new field sounds a good idea.

> 
> >
> >>
> >> 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.

Good idea, thanks for the suggestion!

> 
> 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().

Yes, my current wording for this would be:
+    /*
+     * No need to call relinquish_p2m_mapping() here because
+     * p2m_final_teardown() is called either after 
domain_relinquish_resources()
+     * where relinquish_p2m_mapping() has been called, or from failure path of
+     * domain_create()/arch_domain_create() where mappings that require
+     * p2m_put_l3_page() should never be created.
+     */

I will add the words pointing to p2m_set_entry().

Kind regards,
Henry


> 
> Cheers,
> 
> --
> Julien Grall