| Message ID | 1d83fd35-6ea5-289c-d8db-029c50957f85@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: M2P maintenance adjustments (step 1) | expand |
On 06/08/2020 10:29, Jan Beulich wrote: > While in most cases code ahead of the invocation of set_gpfn_from_mfn() > deals with shared pages, at least in set_typed_p2m_entry() I can't spot > such handling (it's entirely possible there's code missing there). Let's > try to play safe and add an extra check. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/include/asm-x86/mm.h > +++ b/xen/include/asm-x86/mm.h > @@ -525,9 +525,14 @@ extern const unsigned int *const compat_ > #endif /* CONFIG_PV32 */ > > #define _set_gpfn_from_mfn(mfn, pfn) ({ \ > - struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn))); \ > - unsigned long entry = (d && (d == dom_cow)) ? \ > - SHARED_M2P_ENTRY : (pfn); \ > + unsigned long entry = (pfn); \ > + if ( entry != INVALID_M2P_ENTRY ) \ > + { \ > + const struct domain *d; \ > + d = page_get_owner(mfn_to_page(_mfn(mfn))); \ > + if ( d && (d == dom_cow) ) \ > + entry = SHARED_M2P_ENTRY; \ > + } \ > set_compat_m2p(mfn, (unsigned int)(entry)); \ > machine_to_phys_mapping[mfn] = (entry); \ > }) > Hmm - we already have a lot of callers, and this is already too complicated to be a define. We have x86 which uses M2P, and ARM which doesn't. We have two more architectures on the way which probably won't want M2P, and certainly won't in the beginning. Can we introduce CONFIG_M2P which is selected by x86, rename this infrastructure to set_m2p() or something, provide a no-op fallback in common code, and move this implementation into x86/mm.c ? In particular, silently clobbering pfn to SHARED_M2P_ENTRY is rude behaviour. It would be better to ASSERT() the right one is passed in, which also simplifies release builds. ~Andrew
On 10.08.2020 18:42, Andrew Cooper wrote: > On 06/08/2020 10:29, Jan Beulich wrote: >> While in most cases code ahead of the invocation of set_gpfn_from_mfn() >> deals with shared pages, at least in set_typed_p2m_entry() I can't spot >> such handling (it's entirely possible there's code missing there). Let's >> try to play safe and add an extra check. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/include/asm-x86/mm.h >> +++ b/xen/include/asm-x86/mm.h >> @@ -525,9 +525,14 @@ extern const unsigned int *const compat_ >> #endif /* CONFIG_PV32 */ >> >> #define _set_gpfn_from_mfn(mfn, pfn) ({ \ >> - struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn))); \ >> - unsigned long entry = (d && (d == dom_cow)) ? \ >> - SHARED_M2P_ENTRY : (pfn); \ >> + unsigned long entry = (pfn); \ >> + if ( entry != INVALID_M2P_ENTRY ) \ >> + { \ >> + const struct domain *d; \ >> + d = page_get_owner(mfn_to_page(_mfn(mfn))); \ >> + if ( d && (d == dom_cow) ) \ >> + entry = SHARED_M2P_ENTRY; \ >> + } \ >> set_compat_m2p(mfn, (unsigned int)(entry)); \ >> machine_to_phys_mapping[mfn] = (entry); \ >> }) >> > > Hmm - we already have a lot of callers, and this is already too > complicated to be a define. I did consider moving this into an out-of-line function, yes. > We have x86 which uses M2P, and ARM which doesn't. We have two more > architectures on the way which probably won't want M2P, and certainly > won't in the beginning. > > Can we introduce CONFIG_M2P which is selected by x86, rename this > infrastructure to set_m2p() or something, provide a no-op fallback in > common code, and move this implementation into x86/mm.c ? We can, sure. Question is whether this isn't more scope creep than is acceptable considering the purpose of this change. > In particular, silently clobbering pfn to SHARED_M2P_ENTRY is rude > behaviour. It would be better to ASSERT() the right one is passed in, > which also simplifies release builds. Now this is, irrespective of me agreeing with the point you make, a change I'm not going to make: There's no way I could guarantee I wouldn't break mem-sharing. A change like this can imo only possibly be done by someone actively working on and with mem-sharing. Jan
--- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -525,9 +525,14 @@ extern const unsigned int *const compat_ #endif /* CONFIG_PV32 */ #define _set_gpfn_from_mfn(mfn, pfn) ({ \ - struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn))); \ - unsigned long entry = (d && (d == dom_cow)) ? \ - SHARED_M2P_ENTRY : (pfn); \ + unsigned long entry = (pfn); \ + if ( entry != INVALID_M2P_ENTRY ) \ + { \ + const struct domain *d; \ + d = page_get_owner(mfn_to_page(_mfn(mfn))); \ + if ( d && (d == dom_cow) ) \ + entry = SHARED_M2P_ENTRY; \ + } \ set_compat_m2p(mfn, (unsigned int)(entry)); \ machine_to_phys_mapping[mfn] = (entry); \ })
While in most cases code ahead of the invocation of set_gpfn_from_mfn() deals with shared pages, at least in set_typed_p2m_entry() I can't spot such handling (it's entirely possible there's code missing there). Let's try to play safe and add an extra check. Signed-off-by: Jan Beulich <jbeulich@suse.com>