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

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 30/01/2023 04:06, 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>
> Reviewed-by: Michal Orzel <michal.orzel@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?
I would keep it.

> v1 -> v2:
> 1. Add Reviewed-by tag from Michal.
> ---
>   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
NIT: As you are touching the comment, would you mind to fix the typo 
s/one/only/.
>    *    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 f1ccd7efbd..46406a9eb1 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1750,13 +1750,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));
With this change, I think we can also drop the second parameter of 
p2m_teardown(). Preferably with this change in the patch:
Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Preferably, I would like this to happen in this patch.

Cheers,

--
Julien Grall