Re: [PATCH 1/2] arm/p2m: Rework p2m_init()

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Andrew,

> On 17 Oct 2022, at 20:12, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> 
> p2m_init() is mostly trivial initialisation, but has two failable operations

Maybe say “two operations that can fail” as the failable does seem fully right 
:-)
But I am not a native speaker so I will let that to you.

> which are on either side of the backpointer trigger for teardown to take
> actions.
> 
> p2m_free_vmid() is idempotent with a failed p2m_alloc_vmid(), so rearrange
> p2m_init() to perform all trivial setup, then set the backpointer, then
> perform all failable setup.
> 
> This will simplify a future bugfix which needs to add a third failabile
> operation.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Cheers
Bertrand

> ---
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Henry Wang <Henry.Wang@xxxxxxx>
> ---
> xen/arch/arm/p2m.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index f17500ddf3a3..6826f6315080 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1754,7 +1754,7 @@ void p2m_final_teardown(struct domain *d)
> int p2m_init(struct domain *d)
> {
>     struct p2m_domain *p2m = p2m_get_hostp2m(d);
> -    int rc = 0;
> +    int rc;
>     unsigned int cpu;
> 
>     rwlock_init(&p2m->lock);
> @@ -1763,11 +1763,6 @@ int p2m_init(struct domain *d)
>     INIT_PAGE_LIST_HEAD(&d->arch.paging.p2m_freelist);
> 
>     p2m->vmid = INVALID_VMID;
> -
> -    rc = p2m_alloc_vmid(d);
> -    if ( rc != 0 )
> -        return rc;
> -
>     p2m->max_mapped_gfn = _gfn(0);
>     p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
> 
> @@ -1783,8 +1778,6 @@ int p2m_init(struct domain *d)
>     p2m->clean_pte = is_iommu_enabled(d) &&
>         !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
> 
> -    rc = p2m_alloc_table(d);
> -
>     /*
>      * Make sure that the type chosen to is able to store the an vCPU ID
>      * between 0 and the maximum of virtual CPUS supported as long as
> @@ -1797,13 +1790,20 @@ int p2m_init(struct domain *d)
>        p2m->last_vcpu_ran[cpu] = INVALID_VCPU_ID;
> 
>     /*
> -     * Besides getting a domain when we only have the p2m in hand,
> -     * the back pointer to domain is also used in p2m_teardown()
> -     * as an end-of-initialization indicator.
> +     * "Trivial" initialisation is now complete.  Set the backpointer so
> +     * p2m_teardown() and friends know to do something.
>      */
>     p2m->domain = d;
> 
> -    return rc;
> +    rc = p2m_alloc_vmid(d);
> +    if ( rc )
> +        return rc;
> +
> +    rc = p2m_alloc_table(d);
> +    if ( rc )
> +        return rc;
> +
> +    return 0;
> }
> 
> /*
> -- 
> 2.11.0
>