HAP does a few things beyond what's common, which are left there at
least for now. Common operations, however, are moved to
paging_final_teardown(), allowing shadow_final_teardown() to go away.

While moving (and hence generalizing) the respective SHADOW_PRINTK()
drop the logging of total_pages from the 2nd instance - the value is
necessarily zero after {hap,shadow}_set_allocation().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The remaining parts of hap_final_teardown() could be moved as well, at
the price of a CONFIG_HVM conditional. I wasn't sure whether that was
deemed reasonable.

--- a/xen/arch/x86/include/asm/shadow.h
+++ b/xen/arch/x86/include/asm/shadow.h
@@ -78,9 +78,6 @@ int shadow_domctl(struct domain *d,
 void shadow_vcpu_teardown(struct vcpu *v);
 void shadow_teardown(struct domain *d, bool *preempted);
 
-/* Call once all of the references to the domain have gone away */
-void shadow_final_teardown(struct domain *d);
-
 void sh_remove_shadows(struct domain *d, mfn_t gmfn, int fast, int all);
 
 /* Adjust shadows ready for a guest page to change its type. */
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -268,8 +268,8 @@ static void hap_free(struct domain *d, m
 
     /*
      * For dying domains, actually free the memory here. This way less work is
-     * left to hap_final_teardown(), which cannot easily have preemption checks
-     * added.
+     * left to paging_final_teardown(), which cannot easily have preemption
+     * checks added.
      */
     if ( unlikely(d->is_dying) )
     {
@@ -552,18 +552,6 @@ void hap_final_teardown(struct domain *d
     for (i = 0; i < MAX_NESTEDP2M; i++) {
         p2m_teardown(d->arch.nested_p2m[i], true, NULL);
     }
-
-    if ( d->arch.paging.total_pages != 0 )
-        hap_teardown(d, NULL);
-
-    p2m_teardown(p2m_get_hostp2m(d), true, NULL);
-    /* Free any memory that the p2m teardown released */
-    paging_lock(d);
-    hap_set_allocation(d, 0, NULL);
-    ASSERT(d->arch.paging.p2m_pages == 0);
-    ASSERT(d->arch.paging.free_pages == 0);
-    ASSERT(d->arch.paging.total_pages == 0);
-    paging_unlock(d);
 }
 
 void hap_vcpu_teardown(struct vcpu *v)
--- 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",
+                  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.
+     */
+    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. */
+    paging_lock(d);
+
+    if ( hap )
+        hap_set_allocation(d, 0, NULL);
     else
-        shadow_final_teardown(d);
+        shadow_set_allocation(d, 0, NULL);
+
+    PAGING_PRINTK("%pd final teardown done.  Pages free = %u, p2m = %u\n",
+                  d, d->arch.paging.free_pages, d->arch.paging.p2m_pages);
+    ASSERT(!d->arch.paging.p2m_pages);
+    ASSERT(!d->arch.paging.free_pages);
+    ASSERT(!d->arch.paging.total_pages);
+
+    paging_unlock(d);
 
     p2m_final_teardown(d);
 }
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1232,7 +1232,7 @@ void shadow_free(struct domain *d, mfn_t
 
         /*
          * For dying domains, actually free the memory here. This way less
-         * work is left to shadow_final_teardown(), which cannot easily have
+         * work is left to paging_final_teardown(), which cannot easily have
          * preemption checks added.
          */
         if ( unlikely(dying) )
@@ -2940,35 +2940,6 @@ out:
     }
 }
 
-void shadow_final_teardown(struct domain *d)
-/* Called by arch_domain_destroy(), when it's safe to pull down the p2m map. */
-{
-    SHADOW_PRINTK("dom %u final teardown starts."
-                   "  Shadow pages total = %u, free = %u, p2m=%u\n",
-                   d->domain_id, d->arch.paging.total_pages,
-                   d->arch.paging.free_pages, d->arch.paging.p2m_pages);
-
-    /* Double-check that the domain didn't have any shadow memory.
-     * It is possible for a domain that never got domain_kill()ed
-     * to get here with its shadow allocation intact. */
-    if ( d->arch.paging.total_pages != 0 )
-        shadow_teardown(d, NULL);
-
-    /* It is now safe to pull down the p2m map. */
-    p2m_teardown(p2m_get_hostp2m(d), true, NULL);
-    /* Free any shadow memory that the p2m teardown released */
-    paging_lock(d);
-    shadow_set_allocation(d, 0, NULL);
-    SHADOW_PRINTK("dom %u final teardown done."
-                   "  Shadow pages total = %u, free = %u, p2m=%u\n",
-                   d->domain_id, d->arch.paging.total_pages,
-                   d->arch.paging.free_pages, d->arch.paging.p2m_pages);
-    ASSERT(d->arch.paging.p2m_pages == 0);
-    ASSERT(d->arch.paging.free_pages == 0);
-    ASSERT(d->arch.paging.total_pages == 0);
-    paging_unlock(d);
-}
-
 static int shadow_one_bit_enable(struct domain *d, u32 mode)
 /* Turn on a single shadow mode feature */
 {

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.

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

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

~Andrew

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

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

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