Re: [PATCH] xen/mm: Introduce CONFIG_M2P and provide common fallback logic
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-jul...@xen.org> -- Julien Grall
Re: [PATCH] xen/mm: Introduce CONFIG_M2P and provide common fallback logic
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
[PATCH] xen/mm: Introduce CONFIG_M2P and provide common fallback logic
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
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
CC: Stefano Stabellini
CC: Julien Grall
CC: Volodymyr Babchuk
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(-)
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__ */
--
2.11.0
Re: [PATCH] xen/mm: Introduce CONFIG_M2P and provide common fallback logic
Hi Andrew,
On Mon, 24 Aug 2020, 19:15 Andrew Cooper, 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
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Wei Liu
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Volodymyr Babchuk
>
> 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
>
>
