On Thu, Mar 16, 2023 at 01:57:45PM +0100, Jan Beulich wrote:
> On 16.03.2023 13:24, Roger Pau Monné wrote:
> > On Mon, Jan 09, 2023 at 02:39:19PM +0100, Jan Beulich wrote:
> >> 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() - and shorten the
> >> messages, in part accounting for PAGING_PRINTK() logging __func__
> >> already.
> >>
> >> Signed-off-by: Jan Beulich <[email protected]>
> >> ---
> >> 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.
> >> ---
> >> v2: Shorten PAGING_PRINTK() messages. Adjust comments while being
> >> moved.
> >>
> >> --- 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,45 @@ 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 start: 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);
> >> +
> >> + /*
> >> + * 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);
> >
> > For a logical PoV, shouldn't hap_teardown() be called before
> > hap_final_teardown()?
>
> Yes and no: The meaning of "final" has changed - previously it meant "the
> final parts of tearing down" while now it means "the parts of tearing
> down which must be done during final cleanup". I can't think of a better
> name, so I left "hap_final_teardown" as it was.
>
> > Also hap_final_teardown() already contains a call to hap_teardown() if
> > total_pages != 0, so this is just redundant in the HAP case?
>
> Well, like in shadow_final_teardown() there was such a call prior to this
> change, but there's none left now.
>
> > Maybe we want to pull that hap_teardown() out of hap_final_teardown()
>
> That's what I'm doing here.
Oh, sorry, I've missed that chunk. Then:
Reviewed-by: Roger Pau Monné <[email protected]>
Thanks, Roger.
