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

[Michal Orzel <michal.orzel@xxxxxxx>]

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?

Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal