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
