Re: [PATCH 2/8] x86/paging: fold most HAP and shadow final teardown

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 21/12/2022 1:25 pm, Jan Beulich wrote:
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -842,10 +842,46 @@ int paging_teardown(struct domain *d)
>  /* Call once all of the references to the domain have gone away */
>  void paging_final_teardown(struct domain *d)
>  {
> -    if ( hap_enabled(d) )
> +    bool hap = hap_enabled(d);
> +
> +    PAGING_PRINTK("%pd final teardown starts.  Pages total = %u, free = %u, 
> p2m = %u\n",

PAGING_PRINTK() already includes __func__, so just "%pd start: total %u,
free %u, p2m %u\n" which is shorter.

> +                  d, d->arch.paging.total_pages,
> +                  d->arch.paging.free_pages, d->arch.paging.p2m_pages);
> +
> +    if ( hap )
>          hap_final_teardown(d);
> +
> +    /*
> +     * Double-check that the domain didn't have any paging memory.
> +     * It is possible for a domain that never got domain_kill()ed
> +     * to get here with its paging allocation intact.

I know you're mostly just moving this comment, but it's misleading.

This path is used for the domain_create() error path, and there will be
a nonzero allocation for HVM guests.

I think we do want to rework this eventually - we will simplify things
massively by splitting the things can can only happen for a domain which
has run into relinquish_resources.

At a minimum, I'd suggest dropping the first sentence.  "double check"
implies it's an extraordinary case, which isn't warranted here IMO.

> +     */
> +    if ( d->arch.paging.total_pages )
> +    {
> +        if ( hap )
> +            hap_teardown(d, NULL);
> +        else
> +            shadow_teardown(d, NULL);
> +    }
> +
> +    /* It is now safe to pull down the p2m map. */
> +    p2m_teardown(p2m_get_hostp2m(d), true, NULL);
> +
> +    /* Free any paging memory that the p2m teardown released. */

I don't think this isn't true any more.  410 also made HAP/shadow free
pages fully for a dying domain, so p2m_teardown() at this point won't
add to the free memory pool.

I think the subsequent *_set_allocation() can be dropped, and the
assertions left.

~Andrew