| Message ID | 5CD586E3020000780022D9D1@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: free_page_type() is PV-only | expand |
Hi Jan, Thank you for sending the patch! I will rebase my M2P series on top of this. Cheers, On 10/05/2019 15:12, Jan Beulich wrote: > While it already has a CONFIG_PV wrapped around its entire body, it is > still uselessly invoking mfn_to_gmfn(), which is about to be replaced. > Avoid morphing this code into even more suspicious shape and remove the > effectively dead code - translated mode has been made impossible for PV > quite some time ago. > > Adjust and extend the assertions at the same time: The original > ASSERT(!shadow_mode_refcounts(owner)) really means > ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)), > which isn't what we want here. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -2632,7 +2632,6 @@ int free_page_type(struct page_info *pag > { > #ifdef CONFIG_PV > struct domain *owner = page_get_owner(page); > - unsigned long gmfn; > int rc; > > if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) ) > @@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag > /* A page table is dirtied when its type count becomes zero. */ > paging_mark_dirty(owner, page_to_mfn(page)); > > - ASSERT(!shadow_mode_refcounts(owner)); > + ASSERT(shadow_mode_enabled(owner)); > + ASSERT(!paging_mode_refcounts(owner)); > + ASSERT(!paging_mode_translate(owner)); > > - gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page))); > - if ( VALID_M2P(gmfn) ) > - shadow_remove_all_shadows(owner, _mfn(gmfn)); > + shadow_remove_all_shadows(owner, page_to_mfn(page)); > } > > if ( !(type & PGT_partial) ) > > > >
On 10/05/2019 15:12, Jan Beulich wrote: > While it already has a CONFIG_PV wrapped around its entire body, it is > still uselessly invoking mfn_to_gmfn(), which is about to be replaced. > Avoid morphing this code into even more suspicious shape and remove the > effectively dead code - translated mode has been made impossible for PV > quite some time ago. > > Adjust and extend the assertions at the same time: The original > ASSERT(!shadow_mode_refcounts(owner)) really means > ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)), > which isn't what we want here. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
On Fri, May 10, 2019 at 08:12:51AM -0600, Jan Beulich wrote: > While it already has a CONFIG_PV wrapped around its entire body, it is > still uselessly invoking mfn_to_gmfn(), which is about to be replaced. > Avoid morphing this code into even more suspicious shape and remove the > effectively dead code - translated mode has been made impossible for PV > quite some time ago. > > Adjust and extend the assertions at the same time: The original > ASSERT(!shadow_mode_refcounts(owner)) really means > ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)), > which isn't what we want here. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
On 5/10/19 3:12 PM, Jan Beulich wrote: > While it already has a CONFIG_PV wrapped around its entire body, it is > still uselessly invoking mfn_to_gmfn(), which is about to be replaced. > Avoid morphing this code into even more suspicious shape and remove the > effectively dead code - translated mode has been made impossible for PV > quite some time ago. > > Adjust and extend the assertions at the same time: The original > ASSERT(!shadow_mode_refcounts(owner)) really means > ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)), > which isn't what we want here. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -2632,7 +2632,6 @@ int free_page_type(struct page_info *pag > { > #ifdef CONFIG_PV > struct domain *owner = page_get_owner(page); > - unsigned long gmfn; > int rc; > > if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) ) > @@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag > /* A page table is dirtied when its type count becomes zero. */ > paging_mark_dirty(owner, page_to_mfn(page)); > > - ASSERT(!shadow_mode_refcounts(owner)); > + ASSERT(shadow_mode_enabled(owner)); > + ASSERT(!paging_mode_refcounts(owner)); > + ASSERT(!paging_mode_translate(owner)); In the context of my patch to CODING_STYLE about the use of ASSERTs, thinking about ASSERT vs BUG_ON vs something else here. I guess in this case: 1. PV guests can't be in translate mode 2. If that ever changed, we'd probably trip over the ASSERT() while debugging So I guess ASSERT() is probably fine. Reviewed-by: George Dunlap <george.dunlap@citrix.com> So does that mean, though, that SHARED_M2P_ENTRY & friends are entirely vestigal now, and can be removed? -George
>>> On 13.05.19 at 12:33, <george.dunlap@citrix.com> wrote: > On 5/10/19 3:12 PM, Jan Beulich wrote: >> @@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag >> /* A page table is dirtied when its type count becomes zero. */ >> paging_mark_dirty(owner, page_to_mfn(page)); >> >> - ASSERT(!shadow_mode_refcounts(owner)); >> + ASSERT(shadow_mode_enabled(owner)); >> + ASSERT(!paging_mode_refcounts(owner)); >> + ASSERT(!paging_mode_translate(owner)); > > In the context of my patch to CODING_STYLE about the use of ASSERTs, > thinking about ASSERT vs BUG_ON vs something else here. I guess in this > case: > > 1. PV guests can't be in translate mode > 2. If that ever changed, we'd probably trip over the ASSERT() while > debugging > > So I guess ASSERT() is probably fine. Right, in other (more likely to be [wrongly] exposed to actual execution) cases I'd probably not have used plain ASSERT() here. > Reviewed-by: George Dunlap <george.dunlap@citrix.com> Thanks. > So does that mean, though, that SHARED_M2P_ENTRY & friends are entirely > vestigal now, and can be removed? No, it's pointless to use here only because there's no M2P translation done here in the first place, due to the code being PV only. In code paths reachable for HVM these ought to still be necessary. Jan
--- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2632,7 +2632,6 @@ int free_page_type(struct page_info *pag { #ifdef CONFIG_PV struct domain *owner = page_get_owner(page); - unsigned long gmfn; int rc; if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) ) @@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag /* A page table is dirtied when its type count becomes zero. */ paging_mark_dirty(owner, page_to_mfn(page)); - ASSERT(!shadow_mode_refcounts(owner)); + ASSERT(shadow_mode_enabled(owner)); + ASSERT(!paging_mode_refcounts(owner)); + ASSERT(!paging_mode_translate(owner)); - gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page))); - if ( VALID_M2P(gmfn) ) - shadow_remove_all_shadows(owner, _mfn(gmfn)); + shadow_remove_all_shadows(owner, page_to_mfn(page)); } if ( !(type & PGT_partial) )
While it already has a CONFIG_PV wrapped around its entire body, it is still uselessly invoking mfn_to_gmfn(), which is about to be replaced. Avoid morphing this code into even more suspicious shape and remove the effectively dead code - translated mode has been made impossible for PV quite some time ago. Adjust and extend the assertions at the same time: The original ASSERT(!shadow_mode_refcounts(owner)) really means ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)), which isn't what we want here. Signed-off-by: Jan Beulich <jbeulich@suse.com>