[PATCH v4 0/4] xen/arm: Properly disable M2P on Arm

[PATCH v4 0/4] xen/arm: Properly disable M2P on Arm

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

Arm never supported a M2P yet there are some helpers implemented to deal with
the common code. However, the implementation of mfn_to_gmfn is completely
bogus.

This series aims to properly disable the M2P on Arm. See patch #2 for the
rationale regarding the lack of M2P on Arm.

I have dropped the typesafe patches from this series to focus on just the M2P.

Cheers,

Julien Grall (4):
  xen: XENMEM_exchange should only be used/compiled for arch supporting
    PV guest
  xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call
  xen: Remove mfn_to_gmfn macro
  xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

 xen/arch/x86/Kconfig         |  1 +
 xen/common/Kconfig           |  3 +++
 xen/common/domctl.c          |  9 +++++++--
 xen/common/memory.c          |  7 +++++++
 xen/include/asm-arm/domain.h |  5 +++++
 xen/include/asm-arm/mm.h     | 13 -------------
 xen/include/asm-x86/mm.h     |  5 -----
 xen/include/public/domctl.h  |  6 ++++++
 xen/include/xen/domain.h     | 12 ++++++++++++
 xen/include/xen/mm.h         | 13 +++++++++++++
 10 files changed, 54 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH v4 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

XENMEM_exchange can only be used by PV guest but the check is well
hidden in steal_page(). This is because paging_model_external() will
return false only for PV domain.

To make clearer this is PV only, add a check at the beginning of the
implementation. Take the opportunity to compile out the code if
CONFIG_PV is not set.

This change will also help a follow-up patch where the gmfn_mfn() will
be completely removed on arch not supporting the M2P.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

Jan suggested to #ifdef anything after the check to is_pv_domain().
However, it means to have two block of #ifdef as we can't mix
declaration and code.

I am actually thinking to move the implementation outside of mm.c in
possibly arch/x86 or a pv specific directory under common. Any opinion?

    Changes in v4:
        - Patch added
---
 xen/common/memory.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 714077c1e597..9300104943b0 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -504,6 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
 
 static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
 {
+#ifdef CONFIG_PV
     struct xen_memory_exchange exch;
     PAGE_LIST_HEAD(in_chunk_list);
     PAGE_LIST_HEAD(out_chunk_list);
@@ -516,6 +517,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
     struct domain *d;
     struct page_info *page;
 
+    if ( !is_pv_domain(d) )
+        return -EOPNOTSUPP;
+
     if ( copy_from_guest(&exch, arg, 1) )
         return -EFAULT;
 
@@ -797,6 +801,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
     if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
         rc = -EFAULT;
     return rc;
+#else /* !CONFIG_PV */
+    return -EOPNOTSUPP;
+#endif
 }
 
 int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
-- 
2.17.1

[PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

From: Julien Grall <julien.grall@arm.com>

While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
bogus as we directly return the MFN passed in parameter.

Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
are only 2 callers in the common code:
    - memory_exchange: Arm cannot not use it because steal_page is not
    implemented. Note this is already protected by !CONFIG_PV.
    - getdomaininfo: Toolstack may map the shared page. It looks like
    this is mostly used for mapping the P2M of PV guest. Therefore the
    issue might be minor.

Implementing the M2P on Arm is not planned. The M2P would require significant
amount of VA address (very tough on 32-bit) that can hardly be justified with
the current use of mfn_to_gmfn.
    - memory_exchange: This does not work and I haven't seen any
    request for it so far.
    - getdomaininfo: The structure on Arm does not seem to contain a lot
    of useful information on Arm. It is unclear whether we want to
    allow the toolstack mapping it on Arm.

This patch introduces a config option HAS_M2P to tell whether an
architecture implements the M2P.
    - memory_exchange: This is PV only (not supported on Arm) but take
    the opportunity to gate with HAS_M2P as well in case someone decide
    to implement PV without M2P.
    - getdomaininfo: A new helper is introduced to wrap the call to
    mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will
    fail on mapping if used.

Signed-off-by Julien Grall <julien.grall@arm.com>

---
    Changes in v4:
        - The IOMMU code doesn't use the M2P anymore, so this can be
        dropped.
        - Reword the commit message
        - Fix rebase conflict

    Changes in v3:
        - Move the BUG_ON() in domain_shared_info_gfn()
        - Use a fixed value when the field shared_info_frame is not
        supported.
        - Add an ASSERT_UNREACHABLE in iommu_hwdom_init + move printk
        within the #ifdef.

    Changes in v2:
        - Add a warning in public headers
        - Constify local variable in domain_shared_info_gfn
        - Invert the naming (_d / d) in domain_shared_info_gfn
        - Use -EOPNOTSUPP rather than -ENOSYS
        - Rework how the memory_exchange hypercall is removed from Arm
---
 xen/arch/x86/Kconfig         |  1 +
 xen/common/Kconfig           |  3 +++
 xen/common/domctl.c          |  9 +++++++--
 xen/common/memory.c          |  4 ++--
 xen/include/asm-arm/domain.h |  5 +++++
 xen/include/public/domctl.h  |  6 ++++++
 xen/include/xen/domain.h     | 12 ++++++++++++
 7 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index a636a4bb1e69..ad92e756cccc 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -16,6 +16,7 @@ config X86
 	select HAS_IOPORTS
 	select HAS_KEXEC
 	select MEM_ACCESS_ALWAYS_ON
+	select HAS_M2P
 	select HAS_MEM_PAGING
 	select HAS_NS16550
 	select HAS_PASSTHROUGH
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 15e3b79ff57f..b6b4db92d700 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -63,6 +63,9 @@ config HAS_IOPORTS
 config HAS_SCHED_GRANULARITY
 	bool
 
+config HAS_M2P
+	bool
+
 config NEEDS_LIBELF
 	bool
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 5ac6e9c5cafa..2bf3e6659018 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -68,6 +68,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
     u64 cpu_time = 0;
     int flags = XEN_DOMINF_blocked;
     struct vcpu_runstate_info runstate;
+    gfn_t shared_info_frame;
 
     info->domain = d->domain_id;
     info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
@@ -111,8 +112,12 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
     info->outstanding_pages = d->outstanding_pages;
     info->shr_pages         = atomic_read(&d->shr_pages);
     info->paged_pages       = atomic_read(&d->paged_pages);
-    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
-    BUG_ON(SHARED_M2P(info->shared_info_frame));
+
+    shared_info_frame = domain_shared_info_gfn(d);
+    if ( gfn_eq(shared_info_frame, INVALID_GFN) )
+        info->shared_info_frame = XEN_INVALID_SHARED_INFO_FRAME;
+    else
+        info->shared_info_frame = gfn_x(shared_info_frame);
 
     info->cpupool = cpupool_get_id(d);
 
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 9300104943b0..c698e6872596 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -504,7 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
 
 static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
 {
-#ifdef CONFIG_PV
+#if defined(CONFIG_PV) && defined(CONFIG_M2P)
     struct xen_memory_exchange exch;
     PAGE_LIST_HEAD(in_chunk_list);
     PAGE_LIST_HEAD(out_chunk_list);
@@ -801,7 +801,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
     if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
         rc = -EFAULT;
     return rc;
-#else /* !CONFIG_PV */
+#else /* !(CONFIG_PV && CONFIG_M2P) */
     return -EOPNOTSUPP;
 #endif
 }
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 6819a3bf382f..90161dd079a1 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -262,6 +262,11 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
 
 #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag)
 
+static inline gfn_t domain_shared_info_gfn(struct domain *d)
+{
+    return INVALID_GFN;
+}
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 5c5e55ebcb76..7564df5e8374 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -136,6 +136,12 @@ struct xen_domctl_getdomaininfo {
     uint64_aligned_t outstanding_pages;
     uint64_aligned_t shr_pages;
     uint64_aligned_t paged_pages;
+#define XEN_INVALID_SHARED_INFO_FRAME (~(uint64_t)0)
+    /*
+     * GFN of shared_info struct. Some architectures (e.g Arm) may not
+     * provide a mappable address in the field. In that case, the field
+     * will be set to XEN_INVALID_SHARED_INFO_FRAME.
+     */
     uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */
     uint64_aligned_t cpu_time;
     uint32_t nr_online_vcpus;    /* Number of VCPUs currently online. */
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index cde0d9c7fe63..7281eb7b36c7 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma);
 static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); }
 #endif
 
+#ifdef CONFIG_HAS_M2P
+#define domain_shared_info_gfn(d) ({                            \
+    const struct domain *d_ = (d);                              \
+    gfn_t gfn_;                                                 \
+                                                                \
+    gfn_ = mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));\
+    BUG_ON(SHARED_M2P(gfn_x(gfn_)));                            \
+                                                                \
+    gfn_;                                                       \
+})
+#endif
+
 #endif /* __XEN_DOMAIN_H__ */
-- 
2.17.1

[PATCH v4 3/4] xen: Remove mfn_to_gmfn macro

[Julien Grall]

From: Julien Grall <julien.grall@arm.com>

On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are
no more call to mfn_to_gmfn, so the helper can be dropped.

At the same time rework a comment in Arm code that does not make sense.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    Changes in v4:
        - Remove acks as the patch is old

    Changes in v2:
        - Add Jan's and Stefano's acked-by
---
 xen/include/asm-arm/mm.h | 4 +---
 xen/include/asm-x86/mm.h | 5 -----
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index f8ba49b1188f..29489a3e1076 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -326,10 +326,8 @@ 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 */
+/* We don't have a M2P 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/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index deeba75a1cbb..dfa71ce15ac3 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -508,11 +508,6 @@ extern struct rangeset *mmio_ro_ranges;
 
 #define get_gpfn_from_mfn(mfn)      (machine_to_phys_mapping[(mfn)])
 
-#define mfn_to_gmfn(_d, mfn)                            \
-    ( (paging_mode_translate(_d))                       \
-      ? get_gpfn_from_mfn(mfn)                          \
-      : (mfn) )
-
 #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20))
 #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20))
 
-- 
2.17.1

[PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

[Julien Grall]

From: Julien Grall <julien.grall@arm.com>

At the moment, Arm is providing a dummy implementation for the M2P
helpers used in common code. However, they are quite isolated and could
be used by other architecture in the future. So move all the helpers in
xen/mm.h.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    Changes in v4:
        - The tags were dropped as the previous version was sent a long
        time ago.

    Changes in v3:
        - Add Stefano's reviewed-by
        - Add George's acked-by

    Changes in v2:
        - Patch added
---
 xen/include/asm-arm/mm.h | 11 -----------
 xen/include/xen/mm.h     | 13 +++++++++++++
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 29489a3e1076..5929201d0299 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -318,17 +318,6 @@ static inline void *page_to_virt(const struct page_info *pg)
 struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
                                     unsigned long flags);
 
-/*
- * Arm does not have an M2P, but common code expects a handful of
- * M2P-related defines and functions. Provide dummy versions of these.
- */
-#define INVALID_M2P_ENTRY        (~0UL)
-#define SHARED_M2P_ENTRY         (~0UL - 1UL)
-#define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
-
-/* We don't have a M2P on Arm */
-#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
-
 /* 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 4536a62940a1..15bb0aa30d22 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct page_info *page)
     }
 }
 
+/*
+ * Dummy implementation of M2P-related helpers for common code when
+ * the architecture doesn't have an M2P.
+ */
+#ifndef CONFIG_HAS_M2P
+
+#define INVALID_M2P_ENTRY        (~0UL)
+#define SHARED_M2P(_e)           false
+
+static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {}
+
+#endif
+
 #endif /* __XEN_MM_H__ */
-- 
2.17.1

Re: [PATCH v4 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

[Andrew Cooper]

On 21/09/2020 19:02, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> XENMEM_exchange can only be used by PV guest but the check is well
> hidden in steal_page(). This is because paging_model_external() will
> return false only for PV domain.
>
> To make clearer this is PV only, add a check at the beginning of the
> implementation. Take the opportunity to compile out the code if
> CONFIG_PV is not set.
>
> This change will also help a follow-up patch where the gmfn_mfn() will
> be completely removed on arch not supporting the M2P.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
>
> ---
>
> Jan suggested to #ifdef anything after the check to is_pv_domain().
> However, it means to have two block of #ifdef as we can't mix
> declaration and code.
>
> I am actually thinking to move the implementation outside of mm.c in
> possibly arch/x86 or a pv specific directory under common. Any opinion?

arch/x86/pv/mm.c, with the case XENMEM_exchange: moving into
arch_memory_op().

However, making this happen is incredibly tangled, and we're years
overdue a fix here.

Lets go with this for now, and tidying up can come later.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, however...

>
>     Changes in v4:
>         - Patch added
> ---
>  xen/common/memory.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 714077c1e597..9300104943b0 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -504,6 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
>  
>  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>  {
> +#ifdef CONFIG_PV
>      struct xen_memory_exchange exch;
>      PAGE_LIST_HEAD(in_chunk_list);
>      PAGE_LIST_HEAD(out_chunk_list);
> @@ -516,6 +517,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>      struct domain *d;
>      struct page_info *page;
>  
> +    if ( !is_pv_domain(d) )
> +        return -EOPNOTSUPP;
> +
>      if ( copy_from_guest(&exch, arg, 1) )
>          return -EFAULT;
>  
> @@ -797,6 +801,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)

... there are now a load of #ifdef CONFIG_X86 between these two hunks
which can be dropped.

~Andrew

>      if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
>          rc = -EFAULT;
>      return rc;
> +#else /* !CONFIG_PV */
> +    return -EOPNOTSUPP;
> +#endif
>  }
>  
>  int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,

Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Andrew Cooper]

On 21/09/2020 19:02, Julien Grall wrote:
> From: Julien Grall <julien.grall@arm.com>
>
> While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
> bogus as we directly return the MFN passed in parameter.
>
> Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
> are only 2 callers in the common code:
>     - memory_exchange: Arm cannot not use it because steal_page is not
>     implemented. Note this is already protected by !CONFIG_PV.
>     - getdomaininfo: Toolstack may map the shared page. It looks like
>     this is mostly used for mapping the P2M of PV guest. Therefore the
>     issue might be minor.
>
> Implementing the M2P on Arm is not planned. The M2P would require significant
> amount of VA address (very tough on 32-bit) that can hardly be justified with
> the current use of mfn_to_gmfn.

If anyone cares, access to the shared info page should be wired up
through acquire_resource, not through this foreign mapping bodge,
because ...

>     - memory_exchange: This does not work and I haven't seen any
>     request for it so far.
>     - getdomaininfo: The structure on Arm does not seem to contain a lot
>     of useful information on Arm. It is unclear whether we want to
>     allow the toolstack mapping it on Arm.
>
> This patch introduces a config option HAS_M2P to tell whether an
> architecture implements the M2P.
>     - memory_exchange: This is PV only (not supported on Arm) but take
>     the opportunity to gate with HAS_M2P as well in case someone decide
>     to implement PV without M2P.
>     - getdomaininfo: A new helper is introduced to wrap the call to
>     mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will
>     fail on mapping if used.
>
> Signed-off-by Julien Grall <julien.grall@arm.com>

... possibly the single best reason for returning -1 on ARM is that this
is already the behaviour for x86 HVM guests, until the guest has mapped
the shared info frame for the first time.

(XEN) *** d0v6 getdomaininfo(d1) d->shared_info ffff83007cffe000,
shared_info_frame 0x7cffe
(XEN) *** d0v6 getdomaininfo(d2) d->shared_info ffff83007a401000,
shared_info_frame 0xffffffffffffffff

(d1 is PV, d2 is HVM.  This is the result of creating domains at the
python shell, then querying them for their info, without any further
construction or execution).

Furthermore, both PV and HVM guests can invalidate the M2P mappings for
their shared_info page, which in the HVM case would cause -1 to be
returned again.

> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 5ac6e9c5cafa..2bf3e6659018 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -68,6 +68,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>      u64 cpu_time = 0;
>      int flags = XEN_DOMINF_blocked;
>      struct vcpu_runstate_info runstate;
> +    gfn_t shared_info_frame;
>  
>      info->domain = d->domain_id;
>      info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
> @@ -111,8 +112,12 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>      info->outstanding_pages = d->outstanding_pages;
>      info->shr_pages         = atomic_read(&d->shr_pages);
>      info->paged_pages       = atomic_read(&d->paged_pages);
> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> -    BUG_ON(SHARED_M2P(info->shared_info_frame));
> +
> +    shared_info_frame = domain_shared_info_gfn(d);
> +    if ( gfn_eq(shared_info_frame, INVALID_GFN) )
> +        info->shared_info_frame = XEN_INVALID_SHARED_INFO_FRAME;
> +    else
> +        info->shared_info_frame = gfn_x(shared_info_frame);

This can be simplified to just  info->shared_info_frame =
gfn_x(arch_shared_info_gfn(d)) based on the suggestion at the bottom.

>  
>      info->cpupool = cpupool_get_id(d);
>  
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 9300104943b0..c698e6872596 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -504,7 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
>  
>  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>  {
> -#ifdef CONFIG_PV
> +#if defined(CONFIG_PV) && defined(CONFIG_M2P)

There is no such thing as PV && !M2P.  The M2P table is part of the PV
ABI with guests.

These two hunks should be dropped.

>      struct xen_memory_exchange exch;
>      PAGE_LIST_HEAD(in_chunk_list);
>      PAGE_LIST_HEAD(out_chunk_list);
> @@ -801,7 +801,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>      if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
>          rc = -EFAULT;
>      return rc;
> -#else /* !CONFIG_PV */
> +#else /* !(CONFIG_PV && CONFIG_M2P) */
>      return -EOPNOTSUPP;
>  #endif
>  }
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 6819a3bf382f..90161dd079a1 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -262,6 +262,11 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
>  
>  #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag)
>  
> +static inline gfn_t domain_shared_info_gfn(struct domain *d)
> +{
> +    return INVALID_GFN;
> +}

We don't want every every architecture to provide a stub.  Instead, ...
(bottom reply)

> +
>  #endif /* __ASM_DOMAIN_H__ */
>  
>  /*
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 5c5e55ebcb76..7564df5e8374 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -136,6 +136,12 @@ struct xen_domctl_getdomaininfo {
>      uint64_aligned_t outstanding_pages;
>      uint64_aligned_t shr_pages;
>      uint64_aligned_t paged_pages;
> +#define XEN_INVALID_SHARED_INFO_FRAME (~(uint64_t)0)

We've already got INVALID_GFN as a constant used in the interface.  Lets
not proliferate more.

> +    /*
> +     * GFN of shared_info struct. Some architectures (e.g Arm) may not
> +     * provide a mappable address in the field. In that case, the field
> +     * will be set to XEN_INVALID_SHARED_INFO_FRAME.
> +     */
>      uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */

Delete this comment, especially as it is using obsolete naming terminology.

>      uint64_aligned_t cpu_time;
>      uint32_t nr_online_vcpus;    /* Number of VCPUs currently online. */
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index cde0d9c7fe63..7281eb7b36c7 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma);
>  static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); }
>  #endif
>  
> +#ifdef CONFIG_HAS_M2P
> +#define domain_shared_info_gfn(d) ({                            \
> +    const struct domain *d_ = (d);                              \
> +    gfn_t gfn_;                                                 \
> +                                                                \
> +    gfn_ = mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));\
> +    BUG_ON(SHARED_M2P(gfn_x(gfn_)));                            \
> +                                                                \
> +    gfn_;                                                       \
> +})

... this wants to be

#ifndef arch_shared_info_gfn
static inline gfn_t arch_shared_info_gfn(const struct domain *d) {
return INVALID_GFN; }
#endif

with

gfn_t arch_shared_info_gfn(const struct domain *d);
#define arch_shared_info_gfn arch_shared_info_gfn

in asm-x86/domain.h

and the actual implementation in arch/x86/domain.c

~Andrew

Re: [PATCH v4 3/4] xen: Remove mfn_to_gmfn macro

[Andrew Cooper]

On 21/09/2020 19:02, Julien Grall wrote:
> From: Julien Grall <julien.grall@arm.com>
>
> On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are
> no more call to mfn_to_gmfn, so the helper can be dropped.

The previous patch dropped the mfn_to_gmfn() call from the domain shared
info path, but without a hunk adjusting the innards of
memory_exchange(), this is going to break the x86 build.

> At the same time rework a comment in Arm code that does not make sense.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>

To save a round trip, Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com> with the appropriate hunk to memory_exchange().

Alternatively, it might make sense to fold the adjustment into patch 1
which is perhaps more obvious, given the insertion of an is_pv_domain()
check.

~Andrew

Re: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

[Andrew Cooper]

On 21/09/2020 19:02, Julien Grall wrote:
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 4536a62940a1..15bb0aa30d22 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct page_info *page)
>      }
>  }
>  
> +/*
> + * Dummy implementation of M2P-related helpers for common code when
> + * the architecture doesn't have an M2P.
> + */
> +#ifndef CONFIG_HAS_M2P
> +
> +#define INVALID_M2P_ENTRY        (~0UL)
> +#define SHARED_M2P(_e)           false

((void)_e, false)

Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +
> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {}
> +
> +#endif
> +
>  #endif /* __XEN_MM_H__ */

Re: [PATCH v4 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

[Jan Beulich]

On 21.09.2020 20:02, Julien Grall wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -504,6 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
>  
>  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>  {
> +#ifdef CONFIG_PV
>      struct xen_memory_exchange exch;
>      PAGE_LIST_HEAD(in_chunk_list);
>      PAGE_LIST_HEAD(out_chunk_list);
> @@ -516,6 +517,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>      struct domain *d;
>      struct page_info *page;
>  
> +    if ( !is_pv_domain(d) )
> +        return -EOPNOTSUPP;

I think "paging_mode_translate(d)" would be more correct, at which
point the use later in the function ought to be dropped (as that's
precisely one of the things making this function not really
sensible to use on translated guests).

I also wonder whether the #ifdef wouldn't better be left out. Yes,
that'll mean retaining a stub mfn_to_gmfn(), but it could expand
to simply BUG() then, for example.

Jan

Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Jan Beulich]

On 21.09.2020 20:02, Julien Grall wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -63,6 +63,9 @@ config HAS_IOPORTS
>  config HAS_SCHED_GRANULARITY
>  	bool
>  
> +config HAS_M2P
> +	bool

Following Andrew's comments I think the need for this addition
disappears, but in case I'm missing something I'd like to ask for
this to be added higher up - see the patch I've just sent to
re-sort the various HAS_* here.

Jan

Re: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

[Jan Beulich]

On 21.09.2020 20:02, Julien Grall wrote:
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct page_info *page)
>      }
>  }
>  
> +/*
> + * Dummy implementation of M2P-related helpers for common code when
> + * the architecture doesn't have an M2P.
> + */
> +#ifndef CONFIG_HAS_M2P
> +
> +#define INVALID_M2P_ENTRY        (~0UL)
> +#define SHARED_M2P(_e)           false
> +
> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {}

While I think this would better BUG() or at least ASSERT_UNREACHABLE(),
I realize its use in page_alloc.c prevents this. However, if this was a
macro, I think the need for having INVALID_P2M_ENTRY would vanish, as
long as the stub macro didn't evaluate its 2nd argument.

I'm feeling somewhat uneasy with the SHARED_M2P() definition: This
would seem to better be tied to CONFIG_MEM_SHARING rather than M2P
existence.

Jan

Re: [PATCH v4 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

[Julien Grall]

Hi Andrew,

On 21/09/2020 20:46, Andrew Cooper wrote:
> On 21/09/2020 19:02, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> XENMEM_exchange can only be used by PV guest but the check is well
>> hidden in steal_page(). This is because paging_model_external() will
>> return false only for PV domain.
>>
>> To make clearer this is PV only, add a check at the beginning of the
>> implementation. Take the opportunity to compile out the code if
>> CONFIG_PV is not set.
>>
>> This change will also help a follow-up patch where the gmfn_mfn() will
>> be completely removed on arch not supporting the M2P.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ---
>>
>> Jan suggested to #ifdef anything after the check to is_pv_domain().
>> However, it means to have two block of #ifdef as we can't mix
>> declaration and code.
>>
>> I am actually thinking to move the implementation outside of mm.c in
>> possibly arch/x86 or a pv specific directory under common. Any opinion?
> 
> arch/x86/pv/mm.c, with the case XENMEM_exchange: moving into
> arch_memory_op().
> 
> However, making this happen is incredibly tangled, and we're years
> overdue a fix here.
> 
> Lets go with this for now, and tidying up can come later.
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, however...

Thanks!

> 
>>
>>      Changes in v4:
>>          - Patch added
>> ---
>>   xen/common/memory.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index 714077c1e597..9300104943b0 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -504,6 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
>>   
>>   static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>   {
>> +#ifdef CONFIG_PV
>>       struct xen_memory_exchange exch;
>>       PAGE_LIST_HEAD(in_chunk_list);
>>       PAGE_LIST_HEAD(out_chunk_list);
>> @@ -516,6 +517,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>       struct domain *d;
>>       struct page_info *page;
>>   
>> +    if ( !is_pv_domain(d) )
>> +        return -EOPNOTSUPP;
>> +
>>       if ( copy_from_guest(&exch, arg, 1) )
>>           return -EFAULT;
>>   
>> @@ -797,6 +801,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
> 
> ... there are now a load of #ifdef CONFIG_X86 between these two hunks
> which can be dropped.

I didn't drop them because I wasn't sure whether we wanted to cater 
future arch.

Anyway, I am happy to do the cleanup :).

Cheers,

-- 
Julien Grall

Re: [PATCH v4 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

[Julien Grall]

Hi Jan,

On 22/09/2020 08:35, Jan Beulich wrote:
> On 21.09.2020 20:02, Julien Grall wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -504,6 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
>>   
>>   static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>   {
>> +#ifdef CONFIG_PV
>>       struct xen_memory_exchange exch;
>>       PAGE_LIST_HEAD(in_chunk_list);
>>       PAGE_LIST_HEAD(out_chunk_list);
>> @@ -516,6 +517,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>       struct domain *d;
>>       struct page_info *page;
>>   
>> +    if ( !is_pv_domain(d) )
>> +        return -EOPNOTSUPP;
> 
> I think "paging_mode_translate(d)" would be more correct, at which
> point the use later in the function ought to be dropped (as that's
> precisely one of the things making this function not really
> sensible to use on translated guests).

I have used paging_mode_translate(d) and remove the later use.

> 
> I also wonder whether the #ifdef wouldn't better be left out. Yes,
> that'll mean retaining a stub mfn_to_gmfn(), but it could expand
> to simply BUG() then, for example.

Keeping mfn_to_gmfn() will not discourage developper to add more use of 
it. The risk is we don't notice it when reviewing  and this would lead 
to hit a BUG_ON().

Given this will be the only place where mfn_to_gmfn() is used, I think 
it is best to stub the code. Bear in mind that long term, this function 
should leave outside of common/mm.c (see Andrew's e-mail).

Cheers,

-- 
Julien Grall

Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi Andrew,

On 21/09/2020 21:29, Andrew Cooper wrote:
> On 21/09/2020 19:02, Julien Grall wrote:
>> From: Julien Grall <julien.grall@arm.com>
>>
>> While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
>> bogus as we directly return the MFN passed in parameter.
>>
>> Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
>> are only 2 callers in the common code:
>>      - memory_exchange: Arm cannot not use it because steal_page is not
>>      implemented. Note this is already protected by !CONFIG_PV.
>>      - getdomaininfo: Toolstack may map the shared page. It looks like
>>      this is mostly used for mapping the P2M of PV guest. Therefore the
>>      issue might be minor.
>>
>> Implementing the M2P on Arm is not planned. The M2P would require significant
>> amount of VA address (very tough on 32-bit) that can hardly be justified with
>> the current use of mfn_to_gmfn.
> 
> If anyone cares, access to the shared info page should be wired up
> through acquire_resource, not through this foreign mapping bodge,
> because ...
> 
>>      - memory_exchange: This does not work and I haven't seen any
>>      request for it so far.
>>      - getdomaininfo: The structure on Arm does not seem to contain a lot
>>      of useful information on Arm. It is unclear whether we want to
>>      allow the toolstack mapping it on Arm.
>>
>> This patch introduces a config option HAS_M2P to tell whether an
>> architecture implements the M2P.
>>      - memory_exchange: This is PV only (not supported on Arm) but take
>>      the opportunity to gate with HAS_M2P as well in case someone decide
>>      to implement PV without M2P.
>>      - getdomaininfo: A new helper is introduced to wrap the call to
>>      mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will
>>      fail on mapping if used.
>>
>> Signed-off-by Julien Grall <julien.grall@arm.com>
> 
> ... possibly the single best reason for returning -1 on ARM is that this
> is already the behaviour for x86 HVM guests, until the guest has mapped
> the shared info frame for the first time.
> 
> (XEN) *** d0v6 getdomaininfo(d1) d->shared_info ffff83007cffe000,
> shared_info_frame 0x7cffe
> (XEN) *** d0v6 getdomaininfo(d2) d->shared_info ffff83007a401000,
> shared_info_frame 0xffffffffffffffff
> 
> (d1 is PV, d2 is HVM.  This is the result of creating domains at the
> python shell, then querying them for their info, without any further
> construction or execution).
> 
> Furthermore, both PV and HVM guests can invalidate the M2P mappings for
> their shared_info page, which in the HVM case would cause -1 to be
> returned again.
> 
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index 5ac6e9c5cafa..2bf3e6659018 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -68,6 +68,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>>       u64 cpu_time = 0;
>>       int flags = XEN_DOMINF_blocked;
>>       struct vcpu_runstate_info runstate;
>> +    gfn_t shared_info_frame;
>>   
>>       info->domain = d->domain_id;
>>       info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
>> @@ -111,8 +112,12 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>>       info->outstanding_pages = d->outstanding_pages;
>>       info->shr_pages         = atomic_read(&d->shr_pages);
>>       info->paged_pages       = atomic_read(&d->paged_pages);
>> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
>> -    BUG_ON(SHARED_M2P(info->shared_info_frame));
>> +
>> +    shared_info_frame = domain_shared_info_gfn(d);
>> +    if ( gfn_eq(shared_info_frame, INVALID_GFN) )
>> +        info->shared_info_frame = XEN_INVALID_SHARED_INFO_FRAME;
>> +    else
>> +        info->shared_info_frame = gfn_x(shared_info_frame);
> 
> This can be simplified to just  info->shared_info_frame =
> gfn_x(arch_shared_info_gfn(d)) based on the suggestion at the bottom.
> 
>>   
>>       info->cpupool = cpupool_get_id(d);
>>   
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index 9300104943b0..c698e6872596 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -504,7 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
>>   
>>   static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>   {
>> -#ifdef CONFIG_PV
>> +#if defined(CONFIG_PV) && defined(CONFIG_M2P)
> 
> There is no such thing as PV && !M2P.  The M2P table is part of the PV
> ABI with guests.
> 
> These two hunks should be dropped.

I will drop them.

> 
>>       struct xen_memory_exchange exch;
>>       PAGE_LIST_HEAD(in_chunk_list);
>>       PAGE_LIST_HEAD(out_chunk_list);
>> @@ -801,7 +801,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>       if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
>>           rc = -EFAULT;
>>       return rc;
>> -#else /* !CONFIG_PV */
>> +#else /* !(CONFIG_PV && CONFIG_M2P) */
>>       return -EOPNOTSUPP;
>>   #endif
>>   }
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 6819a3bf382f..90161dd079a1 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -262,6 +262,11 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
>>   
>>   #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag)
>>   
>> +static inline gfn_t domain_shared_info_gfn(struct domain *d)
>> +{
>> +    return INVALID_GFN;
>> +}
> 
> We don't want every every architecture to provide a stub.  Instead, ...
> (bottom reply)
> 
>> +
>>   #endif /* __ASM_DOMAIN_H__ */
>>   
>>   /*
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index 5c5e55ebcb76..7564df5e8374 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -136,6 +136,12 @@ struct xen_domctl_getdomaininfo {
>>       uint64_aligned_t outstanding_pages;
>>       uint64_aligned_t shr_pages;
>>       uint64_aligned_t paged_pages;
>> +#define XEN_INVALID_SHARED_INFO_FRAME (~(uint64_t)0)
> 
> We've already got INVALID_GFN as a constant used in the interface.  Lets
> not proliferate more.

This was my original approach (see [1]) but this was reworked because:
    1) INVALID_GFN is not technically defined in the ABI. So the 
toolstack has to hardcode the value in the check.
    2) The value is different between 32-bit and 64-bit Arm as 
INVALID_GFN is defined as an unsigned long.

So providing a new define is the right way to go.

> 
>> +    /*
>> +     * GFN of shared_info struct. Some architectures (e.g Arm) may not
>> +     * provide a mappable address in the field. In that case, the field
>> +     * will be set to XEN_INVALID_SHARED_INFO_FRAME.
>> +     */
>>       uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */
> 
> Delete this comment, especially as it is using obsolete naming terminology.

Sure.

> 
>>       uint64_aligned_t cpu_time;
>>       uint32_t nr_online_vcpus;    /* Number of VCPUs currently online. */
>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
>> index cde0d9c7fe63..7281eb7b36c7 100644
>> --- a/xen/include/xen/domain.h
>> +++ b/xen/include/xen/domain.h
>> @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma);
>>   static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); }
>>   #endif
>>   
>> +#ifdef CONFIG_HAS_M2P
>> +#define domain_shared_info_gfn(d) ({                            \
>> +    const struct domain *d_ = (d);                              \
>> +    gfn_t gfn_;                                                 \
>> +                                                                \
>> +    gfn_ = mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));\
>> +    BUG_ON(SHARED_M2P(gfn_x(gfn_)));                            \
>> +                                                                \
>> +    gfn_;                                                       \
>> +})
> 
> ... this wants to be
> 
> #ifndef arch_shared_info_gfn
> static inline gfn_t arch_shared_info_gfn(const struct domain *d) {
> return INVALID_GFN; }
> #endif
> 
> with
> 
> gfn_t arch_shared_info_gfn(const struct domain *d);
> #define arch_shared_info_gfn arch_shared_info_gfn
> 
> in asm-x86/domain.h
> 
> and the actual implementation in arch/x86/domain.c

What's wrong with implement it in xen/domain.h? After all there is 
nothing x86 specific in the implementation...

Cheers,

[1] <20190507151458.29350-10-julien.grall@arm.com>

-- 
Julien Grall

Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi Jan,

On 22/09/2020 08:54, Jan Beulich wrote:
> On 21.09.2020 20:02, Julien Grall wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -63,6 +63,9 @@ config HAS_IOPORTS
>>   config HAS_SCHED_GRANULARITY
>>   	bool
>>   
>> +config HAS_M2P
>> +	bool
> 
> Following Andrew's comments I think the need for this addition
> disappears, but in case I'm missing something I'd like to ask for
> this to be added higher up - see the patch I've just sent to
> re-sort the various HAS_* here.

It is required for patch #4.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 3/4] xen: Remove mfn_to_gmfn macro

[Julien Grall]

On 21/09/2020 21:34, Andrew Cooper wrote:
> On 21/09/2020 19:02, Julien Grall wrote:
>> From: Julien Grall <julien.grall@arm.com>
>>
>> On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are
>> no more call to mfn_to_gmfn, so the helper can be dropped.
> 
> The previous patch dropped the mfn_to_gmfn() call from the domain shared
> info path, but without a hunk adjusting the innards of
> memory_exchange(), this is going to break the x86 build.

Urgh, I thought I build test it the code. I will fix it.

> 
>> At the same time rework a comment in Arm code that does not make sense.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> To save a round trip, Acked-by: Andrew Cooper
> <andrew.cooper3@citrix.com> with the appropriate hunk to memory_exchange().
> 
> Alternatively, it might make sense to fold the adjustment into patch 1
> which is perhaps more obvious, given the insertion of an is_pv_domain()
> check.

I will add in this patch unless someone else also prefer that the change 
is added in patch #1.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

[Julien Grall]

Hi Jan,

On 22/09/2020 09:02, Jan Beulich wrote:
> On 21.09.2020 20:02, Julien Grall wrote:
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct page_info *page)
>>       }
>>   }
>>   
>> +/*
>> + * Dummy implementation of M2P-related helpers for common code when
>> + * the architecture doesn't have an M2P.
>> + */
>> +#ifndef CONFIG_HAS_M2P
>> +
>> +#define INVALID_M2P_ENTRY        (~0UL)
>> +#define SHARED_M2P(_e)           false
>> +
>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {}
> 
> While I think this would better BUG() or at least ASSERT_UNREACHABLE(),
> I realize its use in page_alloc.c prevents this. However, if this was a
> macro, I think the need for having INVALID_P2M_ENTRY would vanish, as
> long as the stub macro didn't evaluate its 2nd argument.
This is not very future proof... The cost of defining INVALID_M2P_ENTRY 
is very minimal compare to the damage that may result from this choice.

> I'm feeling somewhat uneasy with the SHARED_M2P() definition: This
> would seem to better be tied to CONFIG_MEM_SHARING rather than M2P
> existence.

I can see pros and cons in both solution. To me it contains the word 
"M2P" so it makes sense to be protected by HAS_M2P.

If someone else think that it should be protected by CONFIG_MEM_SHARING, 
then I will do the change.

I have added Tamas to give him an opportunity to share his view.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Andrew Cooper]

On 22/09/2020 19:20, Julien Grall wrote:
>>> +
>>>   #endif /* __ASM_DOMAIN_H__ */
>>>     /*
>>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>>> index 5c5e55ebcb76..7564df5e8374 100644
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -136,6 +136,12 @@ struct xen_domctl_getdomaininfo {
>>>       uint64_aligned_t outstanding_pages;
>>>       uint64_aligned_t shr_pages;
>>>       uint64_aligned_t paged_pages;
>>> +#define XEN_INVALID_SHARED_INFO_FRAME (~(uint64_t)0)
>>
>> We've already got INVALID_GFN as a constant used in the interface.  Lets
>> not proliferate more.
>
> This was my original approach (see [1]) but this was reworked because:
>    1) INVALID_GFN is not technically defined in the ABI. So the
> toolstack has to hardcode the value in the check.
>    2) The value is different between 32-bit and 64-bit Arm as
> INVALID_GFN is defined as an unsigned long.
>
> So providing a new define is the right way to go.

There is nothing special about this field.  It should not have a
dedicated constant, when a general one is the appropriate one to use.

libxl already has LIBXL_INVALID_GFN, which is already used.

If this isn't good enough, them the right thing to do is put a proper
INVALID_GFN in the tools interface.

>>>       uint64_aligned_t cpu_time;
>>>       uint32_t nr_online_vcpus;    /* Number of VCPUs currently
>>> online. */
>>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
>>> index cde0d9c7fe63..7281eb7b36c7 100644
>>> --- a/xen/include/xen/domain.h
>>> +++ b/xen/include/xen/domain.h
>>> @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma);
>>>   static inline void vnuma_destroy(struct vnuma_info *vnuma) {
>>> ASSERT(!vnuma); }
>>>   #endif
>>>   +#ifdef CONFIG_HAS_M2P
>>> +#define domain_shared_info_gfn(d) ({                            \
>>> +    const struct domain *d_ = (d);                              \
>>> +    gfn_t gfn_;                                                 \
>>> +                                                                \
>>> +    gfn_ = mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));\
>>> +    BUG_ON(SHARED_M2P(gfn_x(gfn_)));                            \
>>> +                                                                \
>>> +    gfn_;                                                       \
>>> +})
>>
>> ... this wants to be
>>
>> #ifndef arch_shared_info_gfn
>> static inline gfn_t arch_shared_info_gfn(const struct domain *d) {
>> return INVALID_GFN; }
>> #endif
>>
>> with
>>
>> gfn_t arch_shared_info_gfn(const struct domain *d);
>> #define arch_shared_info_gfn arch_shared_info_gfn
>>
>> in asm-x86/domain.h
>>
>> and the actual implementation in arch/x86/domain.c
>
> What's wrong with implement it in xen/domain.h? After all there is
> nothing x86 specific in the implementation...

d->shared_info is allocated in arch specific code, not common code. 
This macro assumes that __virt_to_mfn() is safe to call on the pointer.

For an approaching obsolete part of the API/ABI (particularly given the
new HVM plans), I'd just stuff it in x86 and call it done.  Its easy
enough to re-evaluate if a second appears.

~Andrew

Re: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

[Andrew Cooper]

On 22/09/2020 19:39, Julien Grall wrote:
> Hi Jan,
>
> On 22/09/2020 09:02, Jan Beulich wrote:
>> On 21.09.2020 20:02, Julien Grall wrote:
>>> --- a/xen/include/xen/mm.h
>>> +++ b/xen/include/xen/mm.h
>>> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct
>>> page_info *page)
>>>       }
>>>   }
>>>   +/*
>>> + * Dummy implementation of M2P-related helpers for common code when
>>> + * the architecture doesn't have an M2P.
>>> + */
>>> +#ifndef CONFIG_HAS_M2P
>>> +
>>> +#define INVALID_M2P_ENTRY        (~0UL)
>>> +#define SHARED_M2P(_e)           false
>>> +
>>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned
>>> long pfn) {}
>>
>> While I think this would better BUG() or at least ASSERT_UNREACHABLE(),
>> I realize its use in page_alloc.c prevents this. However, if this was a
>> macro, I think the need for having INVALID_P2M_ENTRY would vanish, as
>> long as the stub macro didn't evaluate its 2nd argument.
> This is not very future proof... The cost of defining
> INVALID_M2P_ENTRY is very minimal compare to the damage that may
> result from this choice.
>
>> I'm feeling somewhat uneasy with the SHARED_M2P() definition: This
>> would seem to better be tied to CONFIG_MEM_SHARING rather than M2P
>> existence.
>
> I can see pros and cons in both solution. To me it contains the word
> "M2P" so it makes sense to be protected by HAS_M2P.
>
> If someone else think that it should be protected by
> CONFIG_MEM_SHARING, then I will do the change.
>
> I have added Tamas to give him an opportunity to share his view.

This is clearly guarded by HAS_M2P first first and foremost.

However, the work to actually let MEM_SHARING be turned off in this
regard is rather larger, and not appropriate to delay this series with.

~Andrew

RE: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

[Lengyel, Tamas]

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: Tuesday, September 22, 2020 3:00 PM
> To: Julien Grall <julien@xen.org>; Jan Beulich <jbeulich@suse.com>
> Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>;
> Stefano Stabellini <sstabellini@kernel.org>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; George Dunlap
> <george.dunlap@citrix.com>; Ian Jackson <iwj@xenproject.org>; Wei Liu
> <wl@xen.org>; Lengyel, Tamas <tamas.lengyel@intel.com>
> Subject: Re: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers
> when !CONFIG_HAVE_M2P
> 
> On 22/09/2020 19:39, Julien Grall wrote:
> > Hi Jan,
> >
> > On 22/09/2020 09:02, Jan Beulich wrote:
> >> On 21.09.2020 20:02, Julien Grall wrote:
> >>> --- a/xen/include/xen/mm.h
> >>> +++ b/xen/include/xen/mm.h
> >>> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct
> >>> page_info *page)
> >>>       }
> >>>   }
> >>>   +/*
> >>> + * Dummy implementation of M2P-related helpers for common code
> when
> >>> + * the architecture doesn't have an M2P.
> >>> + */
> >>> +#ifndef CONFIG_HAS_M2P
> >>> +
> >>> +#define INVALID_M2P_ENTRY        (~0UL) #define SHARED_M2P(_e)
> >>> +false
> >>> +
> >>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned
> >>> long pfn) {}
> >>
> >> While I think this would better BUG() or at least
> >> ASSERT_UNREACHABLE(), I realize its use in page_alloc.c prevents
> >> this. However, if this was a macro, I think the need for having
> >> INVALID_P2M_ENTRY would vanish, as long as the stub macro didn't
> evaluate its 2nd argument.
> > This is not very future proof... The cost of defining
> > INVALID_M2P_ENTRY is very minimal compare to the damage that may
> > result from this choice.
> >
> >> I'm feeling somewhat uneasy with the SHARED_M2P() definition: This
> >> would seem to better be tied to CONFIG_MEM_SHARING rather than M2P
> >> existence.
> >
> > I can see pros and cons in both solution. To me it contains the word
> > "M2P" so it makes sense to be protected by HAS_M2P.
> >
> > If someone else think that it should be protected by
> > CONFIG_MEM_SHARING, then I will do the change.
> >
> > I have added Tamas to give him an opportunity to share his view.
> 
> This is clearly guarded by HAS_M2P first first and foremost.
> 
> However, the work to actually let MEM_SHARING be turned off in this regard is
> rather larger, and not appropriate to delay this series with.

I don't see any issue with making CONFIG_MEM_SHARING also depend on CONFIG_HAS_M2P, so IMHO it would be enough to put both behind that.

Tamas

Re: [PATCH v4 3/4] xen: Remove mfn_to_gmfn macro

[Stefano Stabellini]

On Mon, 21 Sep 2020, Andrew Cooper wrote:
> On 21/09/2020 19:02, Julien Grall wrote:
> > From: Julien Grall <julien.grall@arm.com>
> >
> > On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are
> > no more call to mfn_to_gmfn, so the helper can be dropped.
> 
> The previous patch dropped the mfn_to_gmfn() call from the domain shared
> info path, but without a hunk adjusting the innards of
> memory_exchange(), this is going to break the x86 build.
> 
> > At the same time rework a comment in Arm code that does not make sense.
> >
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> To save a round trip, Acked-by: Andrew Cooper
> <andrew.cooper3@citrix.com> with the appropriate hunk to memory_exchange().

Same from me:
Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi Andrew,

On 22/09/2020 19:56, Andrew Cooper wrote:
> On 22/09/2020 19:20, Julien Grall wrote:
>>>> +
>>>>    #endif /* __ASM_DOMAIN_H__ */
>>>>      /*
>>>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>>>> index 5c5e55ebcb76..7564df5e8374 100644
>>>> --- a/xen/include/public/domctl.h
>>>> +++ b/xen/include/public/domctl.h
>>>> @@ -136,6 +136,12 @@ struct xen_domctl_getdomaininfo {
>>>>        uint64_aligned_t outstanding_pages;
>>>>        uint64_aligned_t shr_pages;
>>>>        uint64_aligned_t paged_pages;
>>>> +#define XEN_INVALID_SHARED_INFO_FRAME (~(uint64_t)0)
>>>
>>> We've already got INVALID_GFN as a constant used in the interface.  Lets
>>> not proliferate more.
>>
>> This was my original approach (see [1]) but this was reworked because:
>>     1) INVALID_GFN is not technically defined in the ABI. So the
>> toolstack has to hardcode the value in the check.
>>     2) The value is different between 32-bit and 64-bit Arm as
>> INVALID_GFN is defined as an unsigned long.
>>
>> So providing a new define is the right way to go.
> 
> There is nothing special about this field.  It should not have a
> dedicated constant, when a general one is the appropriate one to use.
> 
> libxl already has LIBXL_INVALID_GFN, which is already used.

Right, but that's imply it cannot be used by libxc as this would be a 
layer violation.

> 
> If this isn't good enough, them the right thing to do is put a proper
> INVALID_GFN in the tools interface.

That would be nice but I can see some issue on x86 given that we don't 
consistenly define a GFN in the interface as a 64-bit value.

So would you still be happy to consider introducing XEN_INVALID_GFN in 
the interface with some caveats?

> 
>>>>        uint64_aligned_t cpu_time;
>>>>        uint32_t nr_online_vcpus;    /* Number of VCPUs currently
>>>> online. */
>>>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
>>>> index cde0d9c7fe63..7281eb7b36c7 100644
>>>> --- a/xen/include/xen/domain.h
>>>> +++ b/xen/include/xen/domain.h
>>>> @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma);
>>>>    static inline void vnuma_destroy(struct vnuma_info *vnuma) {
>>>> ASSERT(!vnuma); }
>>>>    #endif
>>>>    +#ifdef CONFIG_HAS_M2P
>>>> +#define domain_shared_info_gfn(d) ({                            \
>>>> +    const struct domain *d_ = (d);                              \
>>>> +    gfn_t gfn_;                                                 \
>>>> +                                                                \
>>>> +    gfn_ = mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));\
>>>> +    BUG_ON(SHARED_M2P(gfn_x(gfn_)));                            \
>>>> +                                                                \
>>>> +    gfn_;                                                       \
>>>> +})
>>>
>>> ... this wants to be
>>>
>>> #ifndef arch_shared_info_gfn
>>> static inline gfn_t arch_shared_info_gfn(const struct domain *d) {
>>> return INVALID_GFN; }
>>> #endif
>>>
>>> with
>>>
>>> gfn_t arch_shared_info_gfn(const struct domain *d);
>>> #define arch_shared_info_gfn arch_shared_info_gfn
>>>
>>> in asm-x86/domain.h
>>>
>>> and the actual implementation in arch/x86/domain.c
>>
>> What's wrong with implement it in xen/domain.h? After all there is
>> nothing x86 specific in the implementation...
> 
> d->shared_info is allocated in arch specific code, not common code.
> This macro assumes that __virt_to_mfn() is safe to call on the pointer.

That's a fair point. I will move the code in an x86 specific files.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 2/4] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi,

On 26/09/2020 14:00, Julien Grall wrote:
> Hi Andrew,
> 
> On 22/09/2020 19:56, Andrew Cooper wrote:
>> On 22/09/2020 19:20, Julien Grall wrote:
>>>>> +
>>>>>    #endif /* __ASM_DOMAIN_H__ */
>>>>>      /*
>>>>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>>>>> index 5c5e55ebcb76..7564df5e8374 100644
>>>>> --- a/xen/include/public/domctl.h
>>>>> +++ b/xen/include/public/domctl.h
>>>>> @@ -136,6 +136,12 @@ struct xen_domctl_getdomaininfo {
>>>>>        uint64_aligned_t outstanding_pages;
>>>>>        uint64_aligned_t shr_pages;
>>>>>        uint64_aligned_t paged_pages;
>>>>> +#define XEN_INVALID_SHARED_INFO_FRAME (~(uint64_t)0)
>>>>
>>>> We've already got INVALID_GFN as a constant used in the interface.  
>>>> Lets
>>>> not proliferate more.
>>>
>>> This was my original approach (see [1]) but this was reworked because:
>>>     1) INVALID_GFN is not technically defined in the ABI. So the
>>> toolstack has to hardcode the value in the check.
>>>     2) The value is different between 32-bit and 64-bit Arm as
>>> INVALID_GFN is defined as an unsigned long.
>>>
>>> So providing a new define is the right way to go.
>>
>> There is nothing special about this field.  It should not have a
>> dedicated constant, when a general one is the appropriate one to use.
>>
>> libxl already has LIBXL_INVALID_GFN, which is already used.
> 
> Right, but that's imply it cannot be used by libxc as this would be a 
> layer violation.
> 
>>
>> If this isn't good enough, them the right thing to do is put a proper
>> INVALID_GFN in the tools interface.
> 
> That would be nice but I can see some issue on x86 given that we don't 
> consistenly define a GFN in the interface as a 64-bit value.
> 
> So would you still be happy to consider introducing XEN_INVALID_GFN in 
> the interface with some caveats?

Gentle ping. @Andrew, are you happy with this approach?

Cheers,

-- 
Julien Grall