Re: [PATCH 3/3] xen/arm: Clean-up in p2m_init() and p2m_final_teardown()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 20/01/2023 10:23, Michal Orzel wrote:
> Hi Henry,
>
> On 16/01/2023 02:58, Henry Wang wrote:
> > With the change in previous patch, the initial 16 pages in the P2M
> > pool is not necessary anymore. Drop them for code simplification.
> >
> > Also the call to p2m_teardown() from arch_domain_destroy() is not
> > necessary anymore since the movement of the P2M allocation out of
> > arch_domain_create(). Drop the code and the above in-code comment
> > mentioning it.
> >
> > Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> > ---
> > I am not entirely sure if I should also drop the "TODO" on top of
> > the p2m_set_entry(). Because although we are sure there is no
> > p2m pages populated in domain_create() stage now, but we are not
> > sure if anyone will add more in the future...Any comments?
> > ---
> >   xen/arch/arm/include/asm/p2m.h |  4 ----
> >   xen/arch/arm/p2m.c             | 20 +-------------------
> >   2 files changed, 1 insertion(+), 23 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> > index bf5183e53a..cf06d3cc21 100644
> > --- a/xen/arch/arm/include/asm/p2m.h
> > +++ b/xen/arch/arm/include/asm/p2m.h
> > @@ -200,10 +200,6 @@ int p2m_init(struct domain *d);
> >    *  - p2m_final_teardown() will be called when domain struct is been
> >    *    freed. This *cannot* be preempted and therefore one small
> >    *    resources should be freed here.
> > - *  Note that p2m_final_teardown() will also call p2m_teardown(), to properly
> > - *  free the P2M when failures happen in the domain creation with P2M pages
> > - *  already in use. In this case p2m_teardown() is called non-preemptively and
> > - *  p2m_teardown() will always return 0.
> >    */
> >   int p2m_teardown(struct domain *d, bool allow_preemption);
> >   void p2m_final_teardown(struct domain *d);
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 7de7d822e9..d41a316d18 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1744,13 +1744,9 @@ void p2m_final_teardown(struct domain *d)
> >       /*
> >        * 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. For the latter case, also see
> > -     * comment on top of the p2m_set_entry() for more info.
> > +     * where relinquish_p2m_mapping() has been called.
> >        */
> >
> > -    BUG_ON(p2m_teardown(d, false));
> Because you remove this,
> >       ASSERT(page_list_empty(&p2m->pages));
> you no longer need this assert, right?
I think the ASSERT() is still useful as it at least show that the pages 
should have been freed before the call to p2m_final_teardown().
Cheers,

--
Julien Grall