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