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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.01.2023 18:56, Andrew Cooper wrote:
> 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. */

Fine with me, changed.

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

Well, doing it the other way around (as I already had it before your
reply) also has its advantages. Are you feeling very strong about the
order?

Jan