Hi Andrew,
On Mon, 24 Aug 2020, 19:15 Andrew Cooper, <andrew.coop...@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.coop...@citrix.com>
> ---
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Roger Pau Monné <roger....@citrix.com>
> CC: Wei Liu <w...@xen.org>
> CC: Stefano Stabellini <sstabell...@kernel.org>
> CC: Julien Grall <jul...@xen.org>
> CC: Volodymyr Babchuk <volodymyr_babc...@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
>
>
