On 21.12.2022 18:16, Andrew Cooper wrote:
> 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.
Hmm, yes, can do.
>> + 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.
Instead of dropping I think I would prefer to make it start "Make sure
...".
>> + */
>> + 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.
I agree, but if anything this will want to be a separate patch then.
Jan
