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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 22/12/2022 7:51 am, Jan Beulich wrote:
> On 21.12.2022 18:16, Andrew Cooper wrote:
>> On 21/12/2022 1:25 pm, Jan Beulich wrote:
>>> +                  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
> ...".

That's still awkward grammar, and a bit misleading IMO.  How about
rewriting it entirely?

/* Remove remaining paging memory.  This can be nonzero on certain error
paths. */

>
>>> +     */
>>> +    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.

I'd be happy putting that in this patch, but if you don't want to
combine it, then it should be a prerequisite IMO, so we don't have a
"clean up $X" patch which is shuffling buggy code around.

~Andrew