| Message ID | 20200824181524.1111-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/mm: Introduce CONFIG_M2P and provide common fallback logic | expand |
Hi Andrew, On Mon, 24 Aug 2020, 19:15 Andrew Cooper, <andrew.cooper3@citrix.com> wrote: > Architectures which don't implement an M2P shouldn't be forced to implement > stubs. Implement them in common code. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > I'm pretty sure the mfn_to_gmfn() stub is bogus before and after this > change. > The two uses in common code are getdomaininfo and in memory_exchange(), > which > result in junk. > > I presume ARM toolstacks don't ever try to map info->shared_info_frame, > because I can't see how it would work. > It is broken. I had a series that tried to remove the M2P (see [1]) but there was some disagreement on how to implement it. --- > xen/arch/x86/Kconfig | 1 + > xen/common/Kconfig | 3 +++ > xen/include/asm-arm/mm.h | 5 ----- > xen/include/xen/mm.h | 10 ++++++++++ > 4 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index a636a4bb1e..9bc97a1cf5 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -24,6 +24,7 @@ config X86 > select HAS_SCHED_GRANULARITY > select HAS_UBSAN > select HAS_VPCI if !PV_SHIM_EXCLUSIVE && HVM > + select M2P > select NEEDS_LIBELF > select NUMA > > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index 15e3b79ff5..0bc186d67b 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -63,6 +63,9 @@ config HAS_IOPORTS > config HAS_SCHED_GRANULARITY > bool > > +config M2P > + bool > + > config NEEDS_LIBELF > bool > > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h > index f8ba49b118..f4e1864703 100644 > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -326,11 +326,6 @@ struct page_info *get_page_from_gva(struct vcpu *v, > vaddr_t va, > #define SHARED_M2P_ENTRY (~0UL - 1UL) > #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) > > -/* Xen always owns P2M on ARM */ > -#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } > while (0) > -#define mfn_to_gmfn(_d, mfn) (mfn) > - > - > /* Arch-specific portion of memory_op hypercall. */ > long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg); > > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index 1061765bcd..8f6858f954 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -685,4 +685,14 @@ static inline void put_page_alloc_ref(struct > page_info *page) > } > } > > +/* > + * For architectures which don't maintain their own M2P, provide a stub > + * implementation for common code to use. > + */ > +#ifndef CONFIG_M2P > +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long > pfn) {} > +static inline unsigned long mfn_to_gmfn( > + const struct domain *d, unsigned long mfn) { return mfn; } > +#endif > Please don't add this hack in common code. This is a broken implementation and we are lucky it didn't result to a disaster yet. The correct way to implement it is to remove mfn_to_gmfn from common code. I would be happy to try to revive my series. Cheers, [1] https://patches.linaro.org/cover/165661/ + > #endif /* __XEN_MM_H__ */ > -- > 2.11.0 > >
On 24.08.2020 20:15, Andrew Cooper wrote: > I'm pretty sure the mfn_to_gmfn() stub is bogus before and after this change. > The two uses in common code are getdomaininfo and in memory_exchange(), which > result in junk. It's been a long time back that I think I did suggest to restrict memory_exchange() to non-translated guests. I don't recall what the arguments against this were, but I'm quite sure it wasn't merely "it alters the ABI for such guests". Jan
Hi Jan, On 25/08/2020 08:40, Jan Beulich wrote: > On 24.08.2020 20:15, Andrew Cooper wrote: >> I'm pretty sure the mfn_to_gmfn() stub is bogus before and after this change. >> The two uses in common code are getdomaininfo and in memory_exchange(), which >> result in junk. > > It's been a long time back that I think I did suggest to restrict > memory_exchange() to non-translated guests. I don't recall what > the arguments against this were, but I'm quite sure it wasn't > merely "it alters the ABI for such guests". This was just a low priority for me. But I revived the series (see [1]). Cheers, [1] <20200921180214.4842-1-julien@xen.org>
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index a636a4bb1e..9bc97a1cf5 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -24,6 +24,7 @@ config X86 select HAS_SCHED_GRANULARITY select HAS_UBSAN select HAS_VPCI if !PV_SHIM_EXCLUSIVE && HVM + select M2P select NEEDS_LIBELF select NUMA diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 15e3b79ff5..0bc186d67b 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -63,6 +63,9 @@ config HAS_IOPORTS config HAS_SCHED_GRANULARITY bool +config M2P + bool + config NEEDS_LIBELF bool diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index f8ba49b118..f4e1864703 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -326,11 +326,6 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, #define SHARED_M2P_ENTRY (~0UL - 1UL) #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) -/* Xen always owns P2M on ARM */ -#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0) -#define mfn_to_gmfn(_d, mfn) (mfn) - - /* Arch-specific portion of memory_op hypercall. */ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg); diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 1061765bcd..8f6858f954 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -685,4 +685,14 @@ static inline void put_page_alloc_ref(struct page_info *page) } } +/* + * For architectures which don't maintain their own M2P, provide a stub + * implementation for common code to use. + */ +#ifndef CONFIG_M2P +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {} +static inline unsigned long mfn_to_gmfn( + const struct domain *d, unsigned long mfn) { return mfn; } +#endif + #endif /* __XEN_MM_H__ */
Architectures which don't implement an M2P shouldn't be forced to implement stubs. Implement them in common code. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> I'm pretty sure the mfn_to_gmfn() stub is bogus before and after this change. The two uses in common code are getdomaininfo and in memory_exchange(), which result in junk. I presume ARM toolstacks don't ever try to map info->shared_info_frame, because I can't see how it would work. --- xen/arch/x86/Kconfig | 1 + xen/common/Kconfig | 3 +++ xen/include/asm-arm/mm.h | 5 ----- xen/include/xen/mm.h | 10 ++++++++++ 4 files changed, 14 insertions(+), 5 deletions(-)