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
