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

[PATCH v5 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.

Cheers,

Julien Grall (4):
  xen: XENMEM_exchange should only be used/compiled for arch supporting
    PV guest
  xen: arm: Stop returning a bogus GFN for the shared info
  xen: arm: Remove mfn_to_gfn() macro
  xen/mm: Provide dummy M2P-related helpers when the M2P is not
    supported

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

-- 
2.17.1

[PATCH v5 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.

In a follow-up patch, mfn_to_gfn() will be completely removed for
arch not supporting M2P as it is a call for trouble to use it.
Take the opportunity to compile out the code if CONFIG_PV is not set.

Ideally, we would want to to move the hypercall implementation in
arch/x86/pv/mm.c. But this is incredibly tangled.

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

---

Ideally we would want to move the hypercall implementation in
arch/x86/pv/mm.c. But this is a bit messy. So for now just #ifdef it.

Changes in v5:
    - Removed the #ifdef CONFIG_X86 as they are not necessary anymore
    - Used paging_mode_translate() rather than is_pv_domain()
    - Reword the commit message to explain why the #ifdef rather than
      implementing mfn_to_gfn() using a BUG_ON() or moving the code
      to arch/x86/pv.

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

diff --git a/xen/common/memory.c b/xen/common/memory.c
index e07bd9a5ea4b..9bc78aae35db 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -522,6 +522,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);
@@ -609,6 +610,13 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
         goto fail_early;
     }
 
+    if ( paging_mode_translate(d) )
+    {
+        rc = -EOPNOTSUPP;
+        rcu_unlock_domain(d);
+        goto fail_early;
+    }
+
     rc = xsm_memory_exchange(XSM_TARGET, d);
     if ( rc )
     {
@@ -648,7 +656,6 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
 
             for ( k = 0; k < (1UL << exch.in.extent_order); k++ )
             {
-#ifdef CONFIG_X86
                 p2m_type_t p2mt;
 
                 /* Shared pages cannot be exchanged */
@@ -659,14 +666,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
                     rc = -ENOMEM;
                     goto fail; 
                 }
-#else /* !CONFIG_X86 */
-                mfn = gfn_to_mfn(d, _gfn(gmfn + k));
-#endif
                 if ( unlikely(!mfn_valid(mfn)) )
                 {
-#ifdef CONFIG_X86
                     put_gfn(d, gmfn + k);
-#endif
                     rc = -EINVAL;
                     goto fail;
                 }
@@ -676,16 +678,12 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
                 rc = steal_page(d, page, MEMF_no_refcount);
                 if ( unlikely(rc) )
                 {
-#ifdef CONFIG_X86
                     put_gfn(d, gmfn + k);
-#endif
                     goto fail;
                 }
 
                 page_list_add(page, &in_chunk_list);
-#ifdef CONFIG_X86
                 put_gfn(d, gmfn + k);
-#endif
             }
         }
 
@@ -768,8 +766,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
             guest_physmap_add_page(d, _gfn(gpfn), mfn,
                                    exch.out.extent_order);
 
-            if ( !paging_mode_translate(d) &&
-                 __copy_mfn_to_guest_offset(exch.out.extent_start,
+            if ( __copy_mfn_to_guest_offset(exch.out.extent_start,
                                             (i << out_chunk_order) + j,
                                             mfn) )
                 rc = -EFAULT;
@@ -815,6 +812,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 v5 2/4] xen: arm: Stop returning a bogus GFN for the shared info

[Julien Grall]

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

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

The last use of mfn_to_gfn() on Arm is in getdomaininfo(). It looks
like this is mostly used for mapping the P2M Of PV guest. Furthermore,
the structure on Arm doesn't seem to contain a lot of useful
information. Therefore it is unclear whether we want to allow the
toolstack to map it.

As there is a high change that RISC-V will not implement an M2P,
provide a new wrapper that will by default return an invalid GFN and
move the code to find the GFN in an x86 specific helper.

If in the future we want to map the shared info, then we should
consider to do it using the acquire hypercall.

Note that as INVALID_GFN is unsigned long, we can't return directly
because the value would differ between 64-bit and 32-bit. Instead,
a fixed value needs to be introduced.

While the fixed value could be shared with other field storing a
GFN, we unfortunately use a mix of type (unsigned long, uint64_t)
for exposing it externally. So to avoid any misuse, it is better to
define a fixed value for just the shared_info_gfn field.

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

---
    I am not comfortable with introduce a generic invalid GFN fixed
    value because we don't use a common type. I also didn't get any
    feedback on whether it would be acceptable to focus on one type.

    So the fixed value has not been changed. I think this is acceptable
    because this a DOMCTL and therefore not stable. If someone still
    disagree, then please provide concrete steps how to solve
    the mixing of type.

    Changes in v5:
        - This was originally "xen: Introduce HAS_M2P config and use to
        protect mfn_to_gmfn call".
        - Rebase to the latest Xen.
        - Rename the helper to arch_shared_info_gfn()
        - The default stub now return INVALID_GFN rather than using the
        M2P

    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/domain.c        |  9 +++++++++
 xen/common/Kconfig           |  3 +++
 xen/common/domctl.c          | 10 +++++++---
 xen/include/asm-x86/domain.h |  3 +++
 xen/include/public/domctl.h  |  6 ++++++
 xen/include/xen/domain.h     |  7 +++++++
 6 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ef1812dc1402..b3f216cd6f51 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2553,6 +2553,15 @@ void domain_pause_for_debugger(void)
 #endif
 }
 
+gfn_t arch_shared_info_gfn(const struct domain *d)
+{
+    gfn_t gfn = mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));
+
+    BUG_ON(SHARED_M2P(gfn_x(gfn)));
+
+    return gfn;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 0ddd18e11af3..cd5b89f2f02e 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -64,6 +64,9 @@ config MEM_ACCESS
 	  Framework to configure memory access types for guests and receive
 	  related events in userspace.
 
+config HAS_M2P
+	bool
+
 config NEEDS_LIBELF
 	bool
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 12d6144d2896..834a2183fda1 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,9 +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 =
-        gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
-    BUG_ON(SHARED_M2P(info->shared_info_frame));
+
+    shared_info_frame = arch_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/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 92d54de0b9a1..912a545c93f6 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -756,6 +756,9 @@ static inline void pv_inject_sw_interrupt(unsigned int vector)
                       : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \
                                               : PV64_VM_ASSIST_MASK)
 
+gfn_t arch_shared_info_gfn(const struct domain *d);
+#define arch_shared_info_gfn arch_shared_info_gfn
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 4dbf107785c3..02a2bfa969ec 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -141,6 +141,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 1708c369642e..d8044b7936ae 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -133,4 +133,11 @@ static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); }
 
 extern bool vmtrace_available;
 
+#ifndef arch_shared_info_gfn
+static inline gfn_t arch_shared_info_gfn(const struct domain *d)
+{
+    return INVALID_GFN;
+}
+#endif
+
 #endif /* __XEN_DOMAIN_H__ */
-- 
2.17.1

[PATCH v5 3/4] xen: arm: Remove mfn_to_gfn() macro

[Julien Grall]

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

The current implementation of mfn_to_gfn() is completely bogus and
there are no plan to implement an M2P on Arm. As there are no more
users, drop the helper.

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 v5:
        - Rebase to the latest Xen
        - The patch is now arm only because mfn_to_gmfn() has
        been dropped on x86 and the arm helper was renamed to
        mfn_to_gfn().

    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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index ded74d29da0c..07c24654a0b6 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -326,9 +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_gfn(d, mfn) ((void)(d), _gfn(mfn_x(mfn)))
 
 /* Arch-specific portion of memory_op hypercall. */
 long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
-- 
2.17.1

[PATCH v5 4/4] xen/mm: Provide dummy M2P-related helpers when the M2P is not supported

[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 the helpers
necessary for compilation in xen/mm.h and gate them with a new config
!HAS_M2P. The other M2P related helpers are removed.

Take the opportunity to encode that CONFIG_MEM_SHARING requires
the M2P. It is done in the header rather than the Kconfig because
the option is not defined in the common Kconfig.

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

---
    INVALID_M2P_ENTRY needs to be defined because it is a parameter
    of set_gpfn_from_mfn(). The alternative would be to not
    evaluate the parameter 'pfn' (e.g. leave the helper as a macro)
    however this is a bit risky.

    Changes in v5:
        - Check that MEM_SHARING is not enabled without the M2P.

    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/arch/x86/Kconfig     |  1 +
 xen/include/asm-arm/mm.h | 11 -----------
 xen/include/xen/mm.h     | 17 +++++++++++++++++
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 9b164db64187..a083d8194680 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -16,6 +16,7 @@ config X86
 	select HAS_FAST_MULTIPLY
 	select HAS_IOPORTS
 	select HAS_KEXEC
+	select HAS_M2P
 	select HAS_NS16550
 	select HAS_PASSTHROUGH
 	select HAS_PCI
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 07c24654a0b6..beff43786bda 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 667f9dac83a4..b98a1a7f423f 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -612,4 +612,21 @@ 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
+
+#ifdef CONFIG_MEM_SHARING
+# error "Memory sharing depends on the M2P"
+#endif
+
+#define INVALID_M2P_ENTRY        (~0UL)
+#define SHARED_M2P(_e)           ((void)(_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 v5 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

[Jan Beulich]

On 03.07.2021 19:11, Julien Grall wrote:
> Changes in v5:
>     - Removed the #ifdef CONFIG_X86 as they are not necessary anymore
>     - Used paging_mode_translate() rather than is_pv_domain()

Is there a particular reason you use this in favor of steal_page()'s
paging_mode_external()?

> @@ -815,6 +812,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;

I'm afraid that for correctness of the interface you need to keep
this part even in the !PV case.

Seeing the 2nd use of steal_page() I wonder if it was too much to
ask you to do a similar transformation for gnttab_transfer(), even
if it's not directly related to the immediate purpose of this
series. If you're not going to do so, I guess I'd put it on my list
(perhaps together with moving x86'es steal_page() and dropping
Arm's stub).

Jan

Re: [PATCH v5 2/4] xen: arm: Stop returning a bogus GFN for the shared info

[Jan Beulich]

On 03.07.2021 19:11, Julien Grall wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> While Arm never had a M2P, the implementation of mfn_to_gfn() is pretty
> bogus as we directly return the MFN passed in parameter.
> 
> The last use of mfn_to_gfn() on Arm is in getdomaininfo(). It looks
> like this is mostly used for mapping the P2M Of PV guest. Furthermore,
> the structure on Arm doesn't seem to contain a lot of useful
> information. Therefore it is unclear whether we want to allow the
> toolstack to map it.
> 
> As there is a high change that RISC-V will not implement an M2P,
> provide a new wrapper that will by default return an invalid GFN and
> move the code to find the GFN in an x86 specific helper.
> 
> If in the future we want to map the shared info, then we should
> consider to do it using the acquire hypercall.
> 
> Note that as INVALID_GFN is unsigned long, we can't return directly
> because the value would differ between 64-bit and 32-bit. Instead,
> a fixed value needs to be introduced.
> 
> While the fixed value could be shared with other field storing a
> GFN, we unfortunately use a mix of type (unsigned long, uint64_t)
> for exposing it externally. So to avoid any misuse, it is better to
> define a fixed value for just the shared_info_gfn field.
> 
> Signed-off-by Julien Grall <julien.grall@arm.com>
> 
> ---
>     I am not comfortable with introduce a generic invalid GFN fixed
>     value because we don't use a common type. I also didn't get any
>     feedback on whether it would be acceptable to focus on one type.

It's been quite a while since the prior discussion, so "I am not
comfortable" may refer to something you do here but that you'd
prefer not to do, or something that you elected not to do because
you dislike it. Unfortunately "because we don't use a common type"
is insufficient context to tell, and hence I can't very well
support your view or object to it.

>     So the fixed value has not been changed. I think this is acceptable
>     because this a DOMCTL and therefore not stable. If someone still
>     disagree, then please provide concrete steps how to solve
>     the mixing of type.

Mixing of which types? You have explicit translation between
internal and external representation in getdomaininfo() - this
looks okay to me, fwiw.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2553,6 +2553,15 @@ void domain_pause_for_debugger(void)
>  #endif
>  }
>  
> +gfn_t arch_shared_info_gfn(const struct domain *d)
> +{
> +    gfn_t gfn = mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)));

In order to be able to easily and immediately spot these once
virt_to_mfn() finally becomes a global type-safe wrapper around
__virt_to_mfn(), please retain the prior use of the not-
underscore-prefixed variant.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -64,6 +64,9 @@ config MEM_ACCESS
>  	  Framework to configure memory access types for guests and receive
>  	  related events in userspace.
>  
> +config HAS_M2P
> +	bool
> +
>  config NEEDS_LIBELF
>  	bool
>  

Stale change?

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -141,6 +141,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 */

Since you repeat it anyway in the new comment, may I ask that you drop
the old one, thus getting rid of one more instance of "GMFN"?

> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -133,4 +133,11 @@ static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); }
>  
>  extern bool vmtrace_available;
>  
> +#ifndef arch_shared_info_gfn
> +static inline gfn_t arch_shared_info_gfn(const struct domain *d)
> +{
> +    return INVALID_GFN;
> +}
> +#endif

This doesn't need to live in a header, does it? I don't think there's
any remote expectation that a 2nd caller may surface.

Jan

Re: [PATCH v5 4/4] xen/mm: Provide dummy M2P-related helpers when the M2P is not supported

[Jan Beulich]

On 03.07.2021 19:11, Julien Grall wrote:
> 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 the helpers
> necessary for compilation in xen/mm.h and gate them with a new config
> !HAS_M2P. The other M2P related helpers are removed.
> 
> Take the opportunity to encode that CONFIG_MEM_SHARING requires
> the M2P. It is done in the header rather than the Kconfig because
> the option is not defined in the common Kconfig.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

With the other Kconfig hunk moved here (from the earlier patch)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v5 3/4] xen: arm: Remove mfn_to_gfn() macro

[Michal Orzel]

On 03.07.2021 19:11, Julien Grall wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> The current implementation of mfn_to_gfn() is completely bogus and
> there are no plan to implement an M2P on Arm. As there are no more
> users, drop the helper.
> 
s/plan/plans/
> At the same time rework a comment in Arm code that does not make sense.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
> 
> ---
>     Changes in v5:
>         - Rebase to the latest Xen
>         - The patch is now arm only because mfn_to_gmfn() has
>         been dropped on x86 and the arm helper was renamed to
>         mfn_to_gfn().
> 
>     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 | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index ded74d29da0c..07c24654a0b6 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -326,9 +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_gfn(d, mfn) ((void)(d), _gfn(mfn_x(mfn)))
>  
>  /* Arch-specific portion of memory_op hypercall. */
>  long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
> 

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

[Julien Grall]

Hi Jan,

On 05/07/2021 09:41, Jan Beulich wrote:
> On 03.07.2021 19:11, Julien Grall wrote:
>> Changes in v5:
>>      - Removed the #ifdef CONFIG_X86 as they are not necessary anymore
>>      - Used paging_mode_translate() rather than is_pv_domain()
> 
> Is there a particular reason you use this in favor of steal_page()'s
> paging_mode_external()?

This is what you suggested in v4 [1]. I can switch to 
paging_mode_external() if this is what you now prefer.

> 
>> @@ -815,6 +812,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;
> 
> I'm afraid that for correctness of the interface you need to keep
> this part even in the !PV case.

Xen never initializes the field nr_exchanged. Instead, it expects the 
guest to set to 0. So I am not quite to sure why we would need to keep 
this line.

> Seeing the 2nd use of steal_page() I wonder if it was too much to
> ask you to do a similar transformation for gnttab_transfer(), even
> if it's not directly related to the immediate purpose of this
> series. If you're not going to do so, I guess I'd put it on my list
> (perhaps together with moving x86'es steal_page() and dropping
> Arm's stub).

I will have a look to #ifdef the code in gnttab_transfer() and drop 
steal_page() on Arm.

Cheers,

[1] <834db49d-dda5-784c-1135-8427086a04eb@suse.com>

-- 
Julien Grall

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

[Jan Beulich]

On 07.07.2021 15:39, Julien Grall wrote:
> On 05/07/2021 09:41, Jan Beulich wrote:
>> On 03.07.2021 19:11, Julien Grall wrote:
>>> Changes in v5:
>>>      - Removed the #ifdef CONFIG_X86 as they are not necessary anymore
>>>      - Used paging_mode_translate() rather than is_pv_domain()
>>
>> Is there a particular reason you use this in favor of steal_page()'s
>> paging_mode_external()?
> 
> This is what you suggested in v4 [1]. I can switch to 
> paging_mode_external() if this is what you now prefer.

Well, I did say this would be better than is_pv_*(). I probably didn't
pay enough attention to you already pointing out paging_mode_external()
in the description; I'm sorry. On x86 both are in sync anyway, and I
have to admit I don't see clearly enough which of the two would be the
right one to use here, partly because I'm afraid I also don't see why
steal_page() has such a restriction in the first place (which you now
build upon).

>>> @@ -815,6 +812,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;
>>
>> I'm afraid that for correctness of the interface you need to keep
>> this part even in the !PV case.
> 
> Xen never initializes the field nr_exchanged. Instead, it expects the 
> guest to set to 0. So I am not quite to sure why we would need to keep 
> this line.

Hmm, the public header is wrong then, as it documents the field as
[OUT] only _despite_ the shouting warning in point 5 of the comment.
I guess I never really understood why this sub-op differs from
others in where the continuation indicator lives.

Never mind then, indeed no code adjustment needed:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Julien Grall]

On 07/07/2021 15:06, Jan Beulich wrote:
> On 07.07.2021 15:39, Julien Grall wrote:
>> On 05/07/2021 09:41, Jan Beulich wrote:
>>> On 03.07.2021 19:11, Julien Grall wrote:
>>>> Changes in v5:
>>>>       - Removed the #ifdef CONFIG_X86 as they are not necessary anymore
>>>>       - Used paging_mode_translate() rather than is_pv_domain()
>>>
>>> Is there a particular reason you use this in favor of steal_page()'s
>>> paging_mode_external()?
>>
>> This is what you suggested in v4 [1]. I can switch to
>> paging_mode_external() if this is what you now prefer.
> 
> Well, I did say this would be better than is_pv_*(). I probably didn't
> pay enough attention to you already pointing out paging_mode_external()
> in the description; I'm sorry. On x86 both are in sync anyway, and I
> have to admit I don't see clearly enough which of the two would be the
> right one to use here, partly because I'm afraid I also don't see why
> steal_page() has such a restriction in the first place (which you now
> build upon).

 From a quick git blame, I have found this:

commit fae7d5be8bb8b7a5b7005c4f3b812a47661a721e
Author: Jan Beulich <jbeulich@suse.com>
Date:   Tue Jun 20 14:29:51 2017 +0200

     x86/mm: disallow page stealing from HVM domains

     The operation's success can't be controlled by the guest, as the device
     model may have an active mapping of the page. If we nevertheless
     permitted this operation, we'd have to add further TLB flushing to
     prevent scenarios like

     "Domains A (HVM), B (PV), C (PV); B->target==A
      Steps:
      1. B maps page X from A as writable
      2. B unmaps page X without a TLB flush
      3. A sends page X to C via GNTTABOP_transfer
      4. C maps page X as pagetable (potentially causing a TLB flush in C,
      but not in B)

      At this point, X would be mapped as a pagetable in C while being
      writable through a stale TLB entry in B."

     A similar scenario could be constructed for A using XENMEM_exchange and
     some arbitrary PV domain C then having this page allocated.

     This is XSA-217.

     Reported-by: Jann Horn <jannh@google.com>
     Signed-off-by: Jan Beulich <jbeulich@suse.com>
     Acked-by: George Dunlap <george.dunlap@citrix.com>
     Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> 
>>>> @@ -815,6 +812,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;
>>>
>>> I'm afraid that for correctness of the interface you need to keep
>>> this part even in the !PV case.
>>
>> Xen never initializes the field nr_exchanged. Instead, it expects the
>> guest to set to 0. So I am not quite to sure why we would need to keep
>> this line.
> 
> Hmm, the public header is wrong then, as it documents the field as
> [OUT] only _despite_ the shouting warning in point 5 of the comment.
That's confusing... I will look to update the doc.

> I guess I never really understood why this sub-op differs from
> others in where the continuation indicator lives.

I am guessing the continuation was added after the fact without 
coordination?

> 
> Never mind then, indeed no code adjustment needed:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks!

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

On 07.07.2021 16:21, Julien Grall wrote:
> On 07/07/2021 15:06, Jan Beulich wrote:
>> On 07.07.2021 15:39, Julien Grall wrote:
>>> On 05/07/2021 09:41, Jan Beulich wrote:
>>>> On 03.07.2021 19:11, Julien Grall wrote:
>>>>> Changes in v5:
>>>>>       - Removed the #ifdef CONFIG_X86 as they are not necessary anymore
>>>>>       - Used paging_mode_translate() rather than is_pv_domain()
>>>>
>>>> Is there a particular reason you use this in favor of steal_page()'s
>>>> paging_mode_external()?
>>>
>>> This is what you suggested in v4 [1]. I can switch to
>>> paging_mode_external() if this is what you now prefer.
>>
>> Well, I did say this would be better than is_pv_*(). I probably didn't
>> pay enough attention to you already pointing out paging_mode_external()
>> in the description; I'm sorry. On x86 both are in sync anyway, and I
>> have to admit I don't see clearly enough which of the two would be the
>> right one to use here, partly because I'm afraid I also don't see why
>> steal_page() has such a restriction in the first place (which you now
>> build upon).
> 
>  From a quick git blame, I have found this:
> 
> commit fae7d5be8bb8b7a5b7005c4f3b812a47661a721e
> Author: Jan Beulich <jbeulich@suse.com>
> Date:   Tue Jun 20 14:29:51 2017 +0200
> 
>      x86/mm: disallow page stealing from HVM domains
> 
>      The operation's success can't be controlled by the guest, as the device
>      model may have an active mapping of the page. If we nevertheless
>      permitted this operation, we'd have to add further TLB flushing to
>      prevent scenarios like
> 
>      "Domains A (HVM), B (PV), C (PV); B->target==A
>       Steps:
>       1. B maps page X from A as writable
>       2. B unmaps page X without a TLB flush
>       3. A sends page X to C via GNTTABOP_transfer
>       4. C maps page X as pagetable (potentially causing a TLB flush in C,
>       but not in B)
> 
>       At this point, X would be mapped as a pagetable in C while being
>       writable through a stale TLB entry in B."
> 
>      A similar scenario could be constructed for A using XENMEM_exchange and
>      some arbitrary PV domain C then having this page allocated.
> 
>      This is XSA-217.

Well, yes, this was to repair the damage which could result. But it doesn't
explain why page stealing couldn't be made work for translated guests as
well.

>>>>> @@ -815,6 +812,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;
>>>>
>>>> I'm afraid that for correctness of the interface you need to keep
>>>> this part even in the !PV case.
>>>
>>> Xen never initializes the field nr_exchanged. Instead, it expects the
>>> guest to set to 0. So I am not quite to sure why we would need to keep
>>> this line.
>>
>> Hmm, the public header is wrong then, as it documents the field as
>> [OUT] only _despite_ the shouting warning in point 5 of the comment.
> That's confusing... I will look to update the doc.
> 
>> I guess I never really understood why this sub-op differs from
>> others in where the continuation indicator lives.
> 
> I am guessing the continuation was added after the fact without 
> coordination?

I don't think so, no. I rather expect someone to have been in a different
mood that day.

Jan

Re: [PATCH v5 3/4] xen: arm: Remove mfn_to_gfn() macro

[Stefano Stabellini]

On Tue, 6 Jul 2021, Michal Orzel wrote:
> On 03.07.2021 19:11, Julien Grall wrote:
> > From: Julien Grall <julien.grall@arm.com>
> > 
> > The current implementation of mfn_to_gfn() is completely bogus and
> > there are no plan to implement an M2P on Arm. As there are no more
> > users, drop the helper.
> > 
> s/plan/plans/
> > At the same time rework a comment in Arm code that does not make sense.
> > 
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Michal Orzel <michal.orzel@arm.com>

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


> > ---
> >     Changes in v5:
> >         - Rebase to the latest Xen
> >         - The patch is now arm only because mfn_to_gmfn() has
> >         been dropped on x86 and the arm helper was renamed to
> >         mfn_to_gfn().
> > 
> >     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 | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > index ded74d29da0c..07c24654a0b6 100644
> > --- a/xen/include/asm-arm/mm.h
> > +++ b/xen/include/asm-arm/mm.h
> > @@ -326,9 +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_gfn(d, mfn) ((void)(d), _gfn(mfn_x(mfn)))
> >  
> >  /* Arch-specific portion of memory_op hypercall. */
> >  long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
> > 
> 

Re: [PATCH v5 4/4] xen/mm: Provide dummy M2P-related helpers when the M2P is not supported

[Stefano Stabellini]

On Mon, 5 Jul 2021, Jan Beulich wrote:
> On 03.07.2021 19:11, Julien Grall wrote:
> > 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 the helpers
> > necessary for compilation in xen/mm.h and gate them with a new config
> > !HAS_M2P. The other M2P related helpers are removed.
> > 
> > Take the opportunity to encode that CONFIG_MEM_SHARING requires
> > the M2P. It is done in the header rather than the Kconfig because
> > the option is not defined in the common Kconfig.
> > 
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> With the other Kconfig hunk moved here (from the earlier patch)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

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