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

[Jan Beulich <jbeulich@xxxxxxxx>]

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