| Message ID | 67b9378f-cf4a-f210-aa2d-85af51c51ab0@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: address aspects noticed during XSA-410 work | expand |
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 <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. > --- > 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()? Also hap_final_teardown() already contains a call to hap_teardown() if total_pages != 0, so this is just redundant in the HAP case? Maybe we want to pull that hap_teardown() out of hap_final_teardown() and re-order the logic so hap_teardown() is called before hap_final_teardown()? Thanks, Roger.
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 <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. >> --- >> 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. > and re-order the logic so hap_teardown() is called before > hap_final_teardown()? I'm not convinced re-ordering would be correct; even if it was I wouldn't want to change order of operations here. Instead I want to limit the changes to just the folding of duplicate (with shadow) code. Jan
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 <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. > >> --- > >> 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é <roge.rpau@citrix.com> Thanks, Roger.
On 16.03.2023 14:28, Roger Pau Monné wrote: > On Thu, Mar 16, 2023 at 01:57:45PM +0100, Jan Beulich wrote: >> On 16.03.2023 13:24, Roger Pau Monné wrote: >>> 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é <roge.rpau@citrix.com> Thanks. I'll take the liberty and swap '.' with 'r' ... Jan
--- 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); + } + + /* 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 done: 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 @@ -1194,7 +1194,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) ) @@ -2898,35 +2898,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 */ {
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 <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. --- v2: Shorten PAGING_PRINTK() messages. Adjust comments while being moved.