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
