[PATCH for-4.17 0/4] XSA-409 fixes

[PATCH for-4.17 0/4] XSA-409 fixes

[Andrew Cooper]

For 4.17, and backport to all stable releases.

Patch 2 doesn't technically need backporting, but it's safe and I'm
dis-enclined to forgo testing in older releases.

Andrew Cooper (4):
  xen: Introduce non-broken hypercalls for the p2m pool size
  tools/tests: Unit test for p2m pool size
  xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls
  xen/arm: Correct the p2m pool size calculations

 tools/include/xenctrl.h              |   3 +
 tools/libs/ctrl/xc_domain.c          |  29 ++++++
 tools/libs/light/libxl_arm.c         |  14 +--
 tools/libs/light/libxl_dom.c         |  19 ++++
 tools/libs/light/libxl_internal.h    |   3 +
 tools/libs/light/libxl_x86.c         |  15 +--
 tools/tests/Makefile                 |   1 +
 tools/tests/p2m-pool/.gitignore      |   1 +
 tools/tests/p2m-pool/Makefile        |  42 ++++++++
 tools/tests/p2m-pool/test-p2m-pool.c | 181 +++++++++++++++++++++++++++++++++++
 xen/arch/arm/domctl.c                |  53 ----------
 xen/arch/arm/include/asm/p2m.h       |   1 -
 xen/arch/arm/p2m.c                   |  31 ++++--
 xen/arch/x86/include/asm/hap.h       |   1 +
 xen/arch/x86/include/asm/shadow.h    |   4 +
 xen/arch/x86/mm/hap/hap.c            |  10 ++
 xen/arch/x86/mm/paging.c             |  39 ++++++++
 xen/arch/x86/mm/shadow/common.c      |  10 ++
 xen/common/domctl.c                  |  14 +++
 xen/include/public/domctl.h          |  26 ++++-
 xen/include/xen/domain.h             |   3 +
 21 files changed, 411 insertions(+), 89 deletions(-)
 create mode 100644 tools/tests/p2m-pool/.gitignore
 create mode 100644 tools/tests/p2m-pool/Makefile
 create mode 100644 tools/tests/p2m-pool/test-p2m-pool.c

-- 
2.11.0

[PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Andrew Cooper]

The existing XEN_DOMCTL_SHADOW_OP_{GET,SET}_ALLOCATION have problems:

 * All set_allocation() flavours have an overflow-before-widen bug when
   calculating "sc->mb << (20 - PAGE_SHIFT)".
 * All flavours have a granularity of of 1M.  This was tolerable when the size
   of the pool could only be set at the same granularity, but is broken now
   that ARM has a 16-page stopgap allocation in use.
 * All get_allocation() flavours round up, and in particular turn 0 into 1,
   meaning the get op returns junk before a successful set op.
 * The x86 flavours reject the hypercalls before the VM has vCPUs allocated,
   despite the pool size being a domain property.
 * Even the hypercall names are long-obsolete.

Implement an interface that doesn't suck, which can be first used to unit test
the behaviour, and subsequently correct a broken implementation.  The old
interface will be retired in due course.

This is part of XSA-409 / CVE-2022-33747.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Xen Security Team <security@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Henry Wang <Henry.Wang@arm.com>
CC: Anthony PERARD <anthony.perard@citrix.com>

Name subject to improvement.  ABI not.  This is the first of many tools ABI
changes required to cleanly separate the logical operation from Xen's choice
of pagetable size.

Future TODOs:
 * x86 shadow still rounds up.  This is buggy as it's a simultaneous equation
   with tot_pages which varies over time with ballooning.
 * x86 PV is weird.  There are no toolstack interact with the shadow pool
   size, but the "shadow" pool it does come into existence when logdirty (or
   pv-l1tf) when first enabled.
 * The shadow+hap logic is in desperate need of deduping.
---
 tools/include/xenctrl.h           |  3 +++
 tools/libs/ctrl/xc_domain.c       | 29 +++++++++++++++++++++++++++++
 xen/arch/arm/p2m.c                | 27 +++++++++++++++++++++++++++
 xen/arch/x86/include/asm/hap.h    |  1 +
 xen/arch/x86/include/asm/shadow.h |  4 ++++
 xen/arch/x86/mm/hap/hap.c         | 10 ++++++++++
 xen/arch/x86/mm/paging.c          | 39 +++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/shadow/common.c   | 10 ++++++++++
 xen/common/domctl.c               | 14 ++++++++++++++
 xen/include/public/domctl.h       | 26 +++++++++++++++++++++++++-
 xen/include/xen/domain.h          |  3 +++
 11 files changed, 165 insertions(+), 1 deletion(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 0c8b4c3aa7a5..f503f03a3927 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -893,6 +893,9 @@ long long xc_logdirty_control(xc_interface *xch,
                               unsigned int mode,
                               xc_shadow_op_stats_t *stats);
 
+int xc_get_p2m_mempool_size(xc_interface *xch, uint32_t domid, uint64_t *size);
+int xc_set_p2m_mempool_size(xc_interface *xch, uint32_t domid, uint64_t size);
+
 int xc_sched_credit_domain_set(xc_interface *xch,
                                uint32_t domid,
                                struct xen_domctl_sched_credit *sdom);
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index 14c0420c35be..9ac09cfab036 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -706,6 +706,35 @@ long long xc_logdirty_control(xc_interface *xch,
     return (rc == 0) ? domctl.u.shadow_op.pages : rc;
 }
 
+int xc_get_p2m_mempool_size(xc_interface *xch, uint32_t domid, uint64_t *size)
+{
+    int rc;
+    struct xen_domctl domctl = {
+        .cmd         = XEN_DOMCTL_get_p2m_mempool_size,
+        .domain      = domid,
+    };
+
+    rc = do_domctl(xch, &domctl);
+    if ( rc )
+        return rc;
+
+    *size = domctl.u.p2m_mempool.size;
+    return 0;
+}
+
+int xc_set_p2m_mempool_size(xc_interface *xch, uint32_t domid, uint64_t size)
+{
+    struct xen_domctl domctl = {
+        .cmd         = XEN_DOMCTL_set_p2m_mempool_size,
+        .domain      = domid,
+        .u.p2m_mempool = {
+            .size = size,
+        },
+    };
+
+    return do_domctl(xch, &domctl);
+}
+
 int xc_domain_setmaxmem(xc_interface *xch,
                         uint32_t domid,
                         uint64_t max_memkb)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 94d3b60b1387..4607cde6f0b8 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -100,6 +100,14 @@ unsigned int p2m_get_allocation(struct domain *d)
     return ROUNDUP(nr_pages, 1 << (20 - PAGE_SHIFT)) >> (20 - PAGE_SHIFT);
 }
 
+/* Return the size of the pool, in bytes. */
+int arch_get_p2m_mempool_size(struct domain *d, uint64_t *size)
+{
+    *size = ACCESS_ONCE(d->arch.paging.p2m_total_pages) << PAGE_SHIFT;
+
+    return 0;
+}
+
 /*
  * Set the pool of pages to the required number of pages.
  * Returns 0 for success, non-zero for failure.
@@ -157,6 +165,25 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
     return 0;
 }
 
+int arch_set_p2m_mempool_size(struct domain *d, uint64_t size)
+{
+    unsigned long pages = size >> PAGE_SHIFT;
+    bool preempted = false;
+    int rc;
+
+    if ( (size & ~PAGE_MASK) ||          /* Non page-sized request? */
+         pages != (size >> PAGE_SHIFT) ) /* 32-bit overflow? */
+        return -EINVAL;
+
+    spin_lock(&d->arch.paging.lock);
+    rc = p2m_set_allocation(d, pages, &preempted);
+    spin_unlock(&d->arch.paging.lock);
+
+    ASSERT(preempted == (rc == -ERESTART));
+
+    return rc;
+}
+
 int p2m_teardown_allocation(struct domain *d)
 {
     int ret = 0;
diff --git a/xen/arch/x86/include/asm/hap.h b/xen/arch/x86/include/asm/hap.h
index 90dece29deca..14d2f212dab9 100644
--- a/xen/arch/x86/include/asm/hap.h
+++ b/xen/arch/x86/include/asm/hap.h
@@ -47,6 +47,7 @@ int   hap_track_dirty_vram(struct domain *d,
 extern const struct paging_mode *hap_paging_get_mode(struct vcpu *);
 int hap_set_allocation(struct domain *d, unsigned int pages, bool *preempted);
 unsigned int hap_get_allocation(struct domain *d);
+int hap_get_allocation_bytes(struct domain *d, uint64_t *size);
 
 #endif /* XEN_HAP_H */
 
diff --git a/xen/arch/x86/include/asm/shadow.h b/xen/arch/x86/include/asm/shadow.h
index 1365fe480518..dad876d29499 100644
--- a/xen/arch/x86/include/asm/shadow.h
+++ b/xen/arch/x86/include/asm/shadow.h
@@ -97,6 +97,8 @@ void shadow_blow_tables_per_domain(struct domain *d);
 int shadow_set_allocation(struct domain *d, unsigned int pages,
                           bool *preempted);
 
+int shadow_get_allocation_bytes(struct domain *d, uint64_t *size);
+
 #else /* !CONFIG_SHADOW_PAGING */
 
 #define shadow_vcpu_teardown(v) ASSERT(is_pv_vcpu(v))
@@ -108,6 +110,8 @@ int shadow_set_allocation(struct domain *d, unsigned int pages,
     ({ ASSERT_UNREACHABLE(); -EOPNOTSUPP; })
 #define shadow_set_allocation(d, pages, preempted) \
     ({ ASSERT_UNREACHABLE(); -EOPNOTSUPP; })
+#define shadow_get_allocation_bytes(d, size) \
+    ({ ASSERT_UNREACHABLE(); -EOPNOTSUPP; })
 
 static inline void sh_remove_shadows(struct domain *d, mfn_t gmfn,
                                      int fast, int all) {}
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index f809ea9aa6ae..50c3d6e63fa5 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -345,6 +345,16 @@ unsigned int hap_get_allocation(struct domain *d)
             + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0));
 }
 
+int hap_get_allocation_bytes(struct domain *d, uint64_t *size)
+{
+    unsigned long pages = (d->arch.paging.hap.total_pages +
+                           d->arch.paging.hap.p2m_pages);
+
+    *size = pages << PAGE_SHIFT;
+
+    return 0;
+}
+
 /* Set the pool of pages to the required number of pages.
  * Returns 0 for success, non-zero for failure. */
 int hap_set_allocation(struct domain *d, unsigned int pages, bool *preempted)
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 3a355eee9ca3..b3f7c46e1dfd 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -977,6 +977,45 @@ int __init paging_set_allocation(struct domain *d, unsigned int pages,
 }
 #endif
 
+int arch_get_p2m_mempool_size(struct domain *d, uint64_t *size)
+{
+    int rc;
+
+    if ( is_pv_domain(d) )
+        return -EOPNOTSUPP;
+
+    if ( hap_enabled(d) )
+        rc = hap_get_allocation_bytes(d, size);
+    else
+        rc = shadow_get_allocation_bytes(d, size);
+
+    return rc;
+}
+
+int arch_set_p2m_mempool_size(struct domain *d, uint64_t size)
+{
+    unsigned long pages = size >> PAGE_SHIFT;
+    bool preempted = false;
+    int rc;
+
+    if ( is_pv_domain(d) )
+        return -EOPNOTSUPP;
+
+    if ( size & ~PAGE_MASK )             /* Non page-sized request? */
+        return -EINVAL;
+
+    ASSERT(paging_mode_enabled(d));
+
+    paging_lock(d);
+    if ( hap_enabled(d) )
+        rc = hap_set_allocation(d, pages, &preempted);
+    else
+        rc = shadow_set_allocation(d, pages, &preempted);
+    paging_unlock(d);
+
+    return preempted ? -ERESTART : rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index badfd53c6b23..d190601c4424 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1427,6 +1427,16 @@ static unsigned int shadow_get_allocation(struct domain *d)
             + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0));
 }
 
+int shadow_get_allocation_bytes(struct domain *d, uint64_t *size)
+{
+    unsigned long pages = (d->arch.paging.shadow.total_pages +
+                           d->arch.paging.shadow.p2m_pages);
+
+    *size = pages << PAGE_SHIFT;
+
+    return 0;
+}
+
 /**************************************************************************/
 /* Hash table for storing the guest->shadow mappings.
  * The table itself is an array of pointers to shadows; the shadows are then
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 69fb9abd346f..8f318b830185 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -874,6 +874,20 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         ret = iommu_do_domctl(op, d, u_domctl);
         break;
 
+    case XEN_DOMCTL_get_p2m_mempool_size:
+        ret = arch_get_p2m_mempool_size(d, &op->u.p2m_mempool.size);
+        if ( !ret )
+            copyback = 1;
+        break;
+
+    case XEN_DOMCTL_set_p2m_mempool_size:
+        ret = arch_set_p2m_mempool_size(d, op->u.p2m_mempool.size);
+
+        if ( ret == -ERESTART )
+            ret = hypercall_create_continuation(
+                __HYPERVISOR_domctl, "h", u_domctl);
+        break;
+
     default:
         ret = arch_do_domctl(op, d, u_domctl);
         break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index b2ae839c3632..7da09d5925c8 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -214,7 +214,10 @@ struct xen_domctl_getpageframeinfo3 {
  /* Return the bitmap but do not modify internal copy. */
 #define XEN_DOMCTL_SHADOW_OP_PEEK        12
 
-/* Memory allocation accessors. */
+/*
+ * Memory allocation accessors.  These APIs are broken and will be removed.
+ * Use XEN_DOMCTL_{get,set}_p2m_mempool_size instead.
+ */
 #define XEN_DOMCTL_SHADOW_OP_GET_ALLOCATION   30
 #define XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION   31
 
@@ -946,6 +949,24 @@ struct xen_domctl_cacheflush {
     xen_pfn_t start_pfn, nr_pfns;
 };
 
+/*
+ * XEN_DOMCTL_get_p2m_mempool_size / XEN_DOMCTL_set_p2m_mempool_size.
+ *
+ * Get or set the P2M memory pool size.  The size is in bytes.
+ *
+ * The P2M memory pool is a dedicated pool of memory for managing the guest
+ * physical -> host physical mappings, usually containing pagetables.
+ * Implementation details cause there to be a minimum granularity, usually the
+ * size of pagetables used by Xen.  Users of this interface are required to
+ * identify the granularity by other means.
+ *
+ * The set operation can fail midway through the request (e.g. Xen running out
+ * of memory, no free memory to reclaim from the pool, etc.).
+ */
+struct xen_domctl_p2m_mempool {
+    uint64_aligned_t size; /* IN/OUT.  Size in bytes. */
+};
+
 #if defined(__i386__) || defined(__x86_64__)
 struct xen_domctl_vcpu_msr {
     uint32_t         index;
@@ -1274,6 +1295,8 @@ struct xen_domctl {
 #define XEN_DOMCTL_get_cpu_policy                82
 #define XEN_DOMCTL_set_cpu_policy                83
 #define XEN_DOMCTL_vmtrace_op                    84
+#define XEN_DOMCTL_get_p2m_mempool_size          85
+#define XEN_DOMCTL_set_p2m_mempool_size          86
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1335,6 +1358,7 @@ struct xen_domctl {
         struct xen_domctl_psr_alloc         psr_alloc;
         struct xen_domctl_vuart_op          vuart_op;
         struct xen_domctl_vmtrace_op        vmtrace_op;
+        struct xen_domctl_p2m_mempool       p2m_mempool;
         uint8_t                             pad[128];
     } u;
 };
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 2c8116afba27..01aaf4dedbe8 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -98,6 +98,9 @@ void arch_get_info_guest(struct vcpu *, vcpu_guest_context_u);
 int arch_initialise_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
 int default_initialise_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
 
+int arch_get_p2m_mempool_size(struct domain *d, uint64_t *size /* bytes */);
+int arch_set_p2m_mempool_size(struct domain *d, uint64_t size /* bytes */);
+
 int domain_relinquish_resources(struct domain *d);
 
 void dump_pageframe_info(struct domain *d);
-- 
2.11.0

[PATCH 2/4] tools/tests: Unit test for p2m pool size

[Andrew Cooper]

Exercise some basic functionality of the new xc_{get,set}_p2m_mempool_size()
hypercalls.

This passes on x86, but fails currently on ARM.  ARM will be fixed up in
future patches.

This is part of XSA-409 / CVE-2022-33747.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Xen Security Team <security@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Henry Wang <Henry.Wang@arm.com>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/tests/Makefile                 |   1 +
 tools/tests/p2m-pool/.gitignore      |   1 +
 tools/tests/p2m-pool/Makefile        |  42 ++++++++
 tools/tests/p2m-pool/test-p2m-pool.c | 181 +++++++++++++++++++++++++++++++++++
 4 files changed, 225 insertions(+)
 create mode 100644 tools/tests/p2m-pool/.gitignore
 create mode 100644 tools/tests/p2m-pool/Makefile
 create mode 100644 tools/tests/p2m-pool/test-p2m-pool.c

diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index d99146d56a64..7ce8b7b881db 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -11,6 +11,7 @@ endif
 SUBDIRS-y += xenstore
 SUBDIRS-y += depriv
 SUBDIRS-y += vpci
+SUBDIRS-y += p2m-pool
 
 .PHONY: all clean install distclean uninstall
 all clean distclean install uninstall: %: subdirs-%
diff --git a/tools/tests/p2m-pool/.gitignore b/tools/tests/p2m-pool/.gitignore
new file mode 100644
index 000000000000..cce6d97b1cc8
--- /dev/null
+++ b/tools/tests/p2m-pool/.gitignore
@@ -0,0 +1 @@
+test-p2m-pool
diff --git a/tools/tests/p2m-pool/Makefile b/tools/tests/p2m-pool/Makefile
new file mode 100644
index 000000000000..24f348f20582
--- /dev/null
+++ b/tools/tests/p2m-pool/Makefile
@@ -0,0 +1,42 @@
+XEN_ROOT = $(CURDIR)/../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+TARGET := test-p2m-pool
+
+.PHONY: all
+all: $(TARGET)
+
+.PHONY: clean
+clean:
+	$(RM) -- *.o $(TARGET) $(DEPS_RM)
+
+.PHONY: distclean
+distclean: clean
+	$(RM) -- *~
+
+.PHONY: install
+install: all
+	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
+	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
+
+.PHONY: uninstall
+uninstall:
+	$(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET)
+
+CFLAGS += $(CFLAGS_xeninclude)
+CFLAGS += $(CFLAGS_libxenctrl)
+CFLAGS += $(CFLAGS_libxenforeginmemory)
+CFLAGS += $(CFLAGS_libxengnttab)
+CFLAGS += $(APPEND_CFLAGS)
+
+LDFLAGS += $(LDLIBS_libxenctrl)
+LDFLAGS += $(LDLIBS_libxenforeignmemory)
+LDFLAGS += $(LDLIBS_libxengnttab)
+LDFLAGS += $(APPEND_LDFLAGS)
+
+%.o: Makefile
+
+$(TARGET): test-p2m-pool.o
+	$(CC) -o $@ $< $(LDFLAGS)
+
+-include $(DEPS_INCLUDE)
diff --git a/tools/tests/p2m-pool/test-p2m-pool.c b/tools/tests/p2m-pool/test-p2m-pool.c
new file mode 100644
index 000000000000..1ffb19eeb420
--- /dev/null
+++ b/tools/tests/p2m-pool/test-p2m-pool.c
@@ -0,0 +1,181 @@
+#include <err.h>
+#include <errno.h>
+#include <inttypes.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/mman.h>
+
+#include <xenctrl.h>
+#include <xenforeignmemory.h>
+#include <xengnttab.h>
+#include <xen-tools/libs.h>
+
+static unsigned int nr_failures;
+#define fail(fmt, ...)                          \
+({                                              \
+    nr_failures++;                              \
+    (void)printf(fmt, ##__VA_ARGS__);           \
+})
+
+static xc_interface *xch;
+static uint32_t domid;
+
+static struct xen_domctl_createdomain create = {
+    .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
+    .max_vcpus = 1,
+    .max_grant_frames = 1,
+    .grant_opts = XEN_DOMCTL_GRANT_version(1),
+
+    .arch = {
+#if defined(__x86_64__) || defined(__i386__)
+        .emulation_flags = XEN_X86_EMU_LAPIC,
+#endif
+    },
+};
+
+static uint64_t default_p2m_size_bytes =
+#if defined(__x86_64__) || defined(__i386__)
+    256 << 12; /* Only x86 HAP for now.  x86 Shadow broken. */
+#elif defined (__arm__) || defined(__aarch64__)
+    16 << 12;
+#endif
+
+static void run_tests(void)
+{
+    xen_pfn_t physmap[] = { 0 };
+    uint64_t size_bytes, old_size_bytes;
+    int rc;
+
+    printf("Test default p2m mempool size\n");
+
+    rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes);
+    if ( rc )
+        return fail("  Fail: get p2m mempool size: %d - %s\n",
+                    errno, strerror(errno));
+
+    printf("P2M pool size %"PRIu64" bytes (%"PRIu64"kB, %"PRIu64"MB)\n",
+           size_bytes, size_bytes >> 10, size_bytes >> 20);
+
+
+    /*
+     * Check that the domain has the expected default allocation size.  This
+     * will fail if the logic in Xen is altered without an equivelent
+     * adjustment here.
+     */
+    if ( size_bytes != default_p2m_size_bytes )
+        return fail("  Fail: size %"PRIu64" != expected size %"PRIu64"\n",
+                    size_bytes, default_p2m_size_bytes);
+
+
+    printf("Test that allocate doesn't alter pool size\n");
+
+    /*
+     * Populate the domain with some RAM.  This will cause more of the p2m
+     * pool to be used.
+     */
+    old_size_bytes = size_bytes;
+
+    rc = xc_domain_setmaxmem(xch, domid, -1);
+    if ( rc )
+        return fail("  Fail: setmaxmem: : %d - %s\n",
+                    errno, strerror(errno));
+
+    rc = xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, physmap);
+    if ( rc )
+        return fail("  Fail: populate physmap: %d - %s\n",
+                    errno, strerror(errno));
+
+    /*
+     * Re-get the p2m size.  Should not have changed as a consequence of
+     * populate physmap.
+     */
+    rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes);
+    if ( rc )
+        return fail("  Fail: get p2m mempool size: %d - %s\n",
+                    errno, strerror(errno));
+
+    if ( old_size_bytes != size_bytes )
+        return fail("  Fail: p2m mempool size changed %"PRIu64" => %"PRIu64"\n",
+                    old_size_bytes, size_bytes);
+
+
+
+    printf("Test bad set size\n");
+
+    /*
+     * Check that setting a non-page size results in failure.
+     */
+    rc = xc_set_p2m_mempool_size(xch, domid, size_bytes + 1);
+    if ( rc != -1 || errno != EINVAL )
+        return fail("  Fail: Bad set size: expected -1/EINVAL, got %d/%d - %s\n",
+                    rc, errno, strerror(errno));
+
+
+    printf("Test very large set size\n");
+
+    /*
+     * Check that setting a large P2M size succeeds.  This is expecting to
+     * trigger continuations.
+     */
+    rc = xc_set_p2m_mempool_size(xch, domid, 64 << 20);
+    if ( rc )
+        return fail("  Fail: Set size 64MB: %d - %s\n",
+                    errno, strerror(errno));
+
+
+    /*
+     * Check that the reported size matches what set consumed.
+     */
+    rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes);
+    if ( rc )
+        return fail("  Fail: get p2m mempool size: %d - %s\n",
+                    errno, strerror(errno));
+
+    if ( size_bytes != 64 << 20 )
+        return fail("  Fail: expected mempool size %u, got %"PRIu64"\n",
+                    64 << 20, size_bytes);
+}
+
+int main(int argc, char **argv)
+{
+    int rc;
+
+    printf("P2M Shadow memory pool tests\n");
+
+    xch = xc_interface_open(NULL, NULL, 0);
+
+    if ( !xch )
+        err(1, "xc_interface_open");
+
+    rc = xc_domain_create(xch, &domid, &create);
+    if ( rc )
+    {
+        if ( errno == EINVAL || errno == EOPNOTSUPP )
+            printf("  Skip: %d - %s\n", errno, strerror(errno));
+        else
+            fail("  Domain create failure: %d - %s\n",
+                 errno, strerror(errno));
+        goto out;
+    }
+
+    printf("  Created d%u\n", domid);
+
+    run_tests();
+
+    rc = xc_domain_destroy(xch, domid);
+    if ( rc )
+        fail("  Failed to destroy domain: %d - %s\n",
+             errno, strerror(errno));
+ out:
+    return !!nr_failures;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.11.0

[PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Andrew Cooper]

This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.

First of all, with ARM borrowing x86's implementation, the logic to set the
pool size should have been common, not duplicated.  Introduce
libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
the ARM and x86 paths.  It is left as an exercise to the reader to judge how
libxl/xl can reasonably function without the ability to query the pool size...

Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
replaced with a working and unit tested interface.

This is part of XSA-409 / CVE-2022-33747.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Xen Security Team <security@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Henry Wang <Henry.Wang@arm.com>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libs/light/libxl_arm.c      | 14 +----------
 tools/libs/light/libxl_dom.c      | 19 ++++++++++++++
 tools/libs/light/libxl_internal.h |  3 +++
 tools/libs/light/libxl_x86.c      | 15 ++---------
 xen/arch/arm/domctl.c             | 53 ---------------------------------------
 xen/arch/arm/include/asm/p2m.h    |  1 -
 xen/arch/arm/p2m.c                |  8 ------
 7 files changed, 25 insertions(+), 88 deletions(-)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 2a5e93c28403..2f5615263543 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -209,19 +209,7 @@ int libxl__arch_domain_create(libxl__gc *gc,
                               libxl__domain_build_state *state,
                               uint32_t domid)
 {
-    libxl_ctx *ctx = libxl__gc_owner(gc);
-    unsigned int shadow_mb = DIV_ROUNDUP(d_config->b_info.shadow_memkb, 1024);
-
-    int r = xc_shadow_control(ctx->xch, domid,
-                              XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
-                              &shadow_mb, 0);
-    if (r) {
-        LOGED(ERROR, domid,
-              "Failed to set %u MiB shadow allocation", shadow_mb);
-        return ERROR_FAIL;
-    }
-
-    return 0;
+    return libxl__domain_set_p2m_pool_size(gc, d_config, domid);
 }
 
 int libxl__arch_extra_memory(libxl__gc *gc,
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 2abaab439c4f..f93b221f1c1f 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -1448,6 +1448,25 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
     return rc;
 }
 
+int libxl__domain_set_p2m_pool_size(
+    libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    uint64_t shadow_mem;
+
+    shadow_mem = d_config->b_info.shadow_memkb;
+    shadow_mem <<= 10;
+
+    int r = xc_get_p2m_mempool_size(ctx->xch, domid, &shadow_mem);
+    if (r) {
+        LOGED(ERROR, domid,
+              "Failed to set p2m pool size to %"PRIu64"kB", shadow_mem);
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index cb9e8b3b8b5a..f31164bc6c0d 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -4864,6 +4864,9 @@ int libxl__is_domid_recent(libxl__gc *gc, uint32_t domid, bool *recent);
 /* os-specific implementation of setresuid() */
 int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid);
 
+_hidden int libxl__domain_set_p2m_pool_size(
+    libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid);
+
 #endif
 
 /*
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 7c5ee74443e5..99aba51d05df 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -538,20 +538,9 @@ int libxl__arch_domain_create(libxl__gc *gc,
         xc_domain_set_time_offset(ctx->xch, domid, rtc_timeoffset);
 
     if (d_config->b_info.type != LIBXL_DOMAIN_TYPE_PV) {
-        unsigned int shadow_mb = DIV_ROUNDUP(d_config->b_info.shadow_memkb,
-                                             1024);
-        int r = xc_shadow_control(ctx->xch, domid,
-                                  XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
-                                  &shadow_mb, 0);
-
-        if (r) {
-            LOGED(ERROR, domid,
-                  "Failed to set %u MiB %s allocation",
-                  shadow_mb,
-                  libxl_defbool_val(d_config->c_info.hap) ? "HAP" : "shadow");
-            ret = ERROR_FAIL;
+        ret = libxl__domain_set_p2m_pool_size(gc, d_config, domid);
+        if (ret)
             goto out;
-        }
     }
 
     if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index c8fdeb124084..1baf25c3d98b 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -47,64 +47,11 @@ static int handle_vuart_init(struct domain *d,
     return rc;
 }
 
-static long p2m_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
-                       XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
-{
-    long rc;
-    bool preempted = false;
-
-    if ( unlikely(d == current->domain) )
-    {
-        printk(XENLOG_ERR "Tried to do a p2m domctl op on itself.\n");
-        return -EINVAL;
-    }
-
-    if ( unlikely(d->is_dying) )
-    {
-        printk(XENLOG_ERR "Tried to do a p2m domctl op on dying domain %u\n",
-               d->domain_id);
-        return -EINVAL;
-    }
-
-    switch ( sc->op )
-    {
-    case XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION:
-    {
-        /* Allow and handle preemption */
-        spin_lock(&d->arch.paging.lock);
-        rc = p2m_set_allocation(d, sc->mb << (20 - PAGE_SHIFT), &preempted);
-        spin_unlock(&d->arch.paging.lock);
-
-        if ( preempted )
-            /* Not finished. Set up to re-run the call. */
-            rc = hypercall_create_continuation(__HYPERVISOR_domctl, "h",
-                                               u_domctl);
-        else
-            /* Finished. Return the new allocation. */
-            sc->mb = p2m_get_allocation(d);
-
-        return rc;
-    }
-    case XEN_DOMCTL_SHADOW_OP_GET_ALLOCATION:
-    {
-        sc->mb = p2m_get_allocation(d);
-        return 0;
-    }
-    default:
-    {
-        printk(XENLOG_ERR "Bad p2m domctl op %u\n", sc->op);
-        return -EINVAL;
-    }
-    }
-}
-
 long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
                     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
     switch ( domctl->cmd )
     {
-    case XEN_DOMCTL_shadow_op:
-        return p2m_domctl(d, &domctl->u.shadow_op, u_domctl);
     case XEN_DOMCTL_cacheflush:
     {
         gfn_t s = _gfn(domctl->u.cacheflush.start_pfn);
diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
index c8f14d13c2c5..91df922e1c9f 100644
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -222,7 +222,6 @@ void p2m_restore_state(struct vcpu *n);
 /* Print debugging/statistial info about a domain's p2m */
 void p2m_dump_info(struct domain *d);
 
-unsigned int p2m_get_allocation(struct domain *d);
 int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted);
 int p2m_teardown_allocation(struct domain *d);
 
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 4607cde6f0b8..92b678cf0d09 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -92,14 +92,6 @@ static void p2m_free_page(struct domain *d, struct page_info *pg)
     spin_unlock(&d->arch.paging.lock);
 }
 
-/* Return the size of the pool, rounded up to the nearest MB */
-unsigned int p2m_get_allocation(struct domain *d)
-{
-    unsigned long nr_pages = ACCESS_ONCE(d->arch.paging.p2m_total_pages);
-
-    return ROUNDUP(nr_pages, 1 << (20 - PAGE_SHIFT)) >> (20 - PAGE_SHIFT);
-}
-
 /* Return the size of the pool, in bytes. */
 int arch_get_p2m_mempool_size(struct domain *d, uint64_t *size)
 {
-- 
2.11.0

[PATCH 4/4] xen/arm: Correct the p2m pool size calculations

[Andrew Cooper]

Allocating or freeing p2m pages doesn't alter the size of the mempool; only
the split between free and used pages.

Right now, the hypercalls operate on the free subset of the pool, meaning that
XEN_DOMCTL_get_p2m_mempool_size varies with time as the guest shuffles its
physmap, and XEN_DOMCTL_set_p2m_mempool_size ignores the used subset of the
pool and lets the guest grow unbounded.

This fixes test-p2m-pool on ARM so that the behaviour matches x86.

This is part of XSA-409 / CVE-2022-33747.

Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Xen Security Team <security@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Henry Wang <Henry.Wang@arm.com>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 xen/arch/arm/p2m.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 92b678cf0d09..dd9696c48312 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -72,7 +72,6 @@ static struct page_info *p2m_alloc_page(struct domain *d)
             spin_unlock(&d->arch.paging.lock);
             return NULL;
         }
-        d->arch.paging.p2m_total_pages--;
     }
     spin_unlock(&d->arch.paging.lock);
 
@@ -85,10 +84,7 @@ static void p2m_free_page(struct domain *d, struct page_info *pg)
     if ( is_hardware_domain(d) )
         free_domheap_page(pg);
     else
-    {
-        d->arch.paging.p2m_total_pages++;
         page_list_add_tail(pg, &d->arch.paging.p2m_freelist);
-    }
     spin_unlock(&d->arch.paging.lock);
 }
 
-- 
2.11.0

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Jason Andryuk]

On Wed, Oct 26, 2022 at 6:21 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
>
> First of all, with ARM borrowing x86's implementation, the logic to set the
> pool size should have been common, not duplicated.  Introduce
> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
> libxl/xl can reasonably function without the ability to query the pool size...
>
> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
> replaced with a working and unit tested interface.
>
> This is part of XSA-409 / CVE-2022-33747.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---

> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 2abaab439c4f..f93b221f1c1f 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -1448,6 +1448,25 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
>      return rc;
>  }
>
> +int libxl__domain_set_p2m_pool_size(
> +    libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    uint64_t shadow_mem;
> +
> +    shadow_mem = d_config->b_info.shadow_memkb;
> +    shadow_mem <<= 10;
> +
> +    int r = xc_get_p2m_mempool_size(ctx->xch, domid, &shadow_mem);

Should this be xc_*set*_p2m_mempool_size?

Regards,
Jason

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Andrew Cooper]

On 26/10/2022 14:22, Jason Andryuk wrote:
> On Wed, Oct 26, 2022 at 6:21 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
>> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
>>
>> First of all, with ARM borrowing x86's implementation, the logic to set the
>> pool size should have been common, not duplicated.  Introduce
>> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
>> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
>> libxl/xl can reasonably function without the ability to query the pool size...
>>
>> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
>> replaced with a working and unit tested interface.
>>
>> This is part of XSA-409 / CVE-2022-33747.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
>> index 2abaab439c4f..f93b221f1c1f 100644
>> --- a/tools/libs/light/libxl_dom.c
>> +++ b/tools/libs/light/libxl_dom.c
>> @@ -1448,6 +1448,25 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
>>      return rc;
>>  }
>>
>> +int libxl__domain_set_p2m_pool_size(
>> +    libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid)
>> +{
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>> +    uint64_t shadow_mem;
>> +
>> +    shadow_mem = d_config->b_info.shadow_memkb;
>> +    shadow_mem <<= 10;
>> +
>> +    int r = xc_get_p2m_mempool_size(ctx->xch, domid, &shadow_mem);
> Should this be xc_*set*_p2m_mempool_size?

Hmm, yes it should be.

And the reason this doesn't break any tests is because all examples in
CI match the default that Xen that sets.

~Andrew

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Jan Beulich]

On 26.10.2022 12:20, Andrew Cooper wrote:
> The existing XEN_DOMCTL_SHADOW_OP_{GET,SET}_ALLOCATION have problems:
> 
>  * All set_allocation() flavours have an overflow-before-widen bug when
>    calculating "sc->mb << (20 - PAGE_SHIFT)".
>  * All flavours have a granularity of of 1M.  This was tolerable when the size
>    of the pool could only be set at the same granularity, but is broken now
>    that ARM has a 16-page stopgap allocation in use.
>  * All get_allocation() flavours round up, and in particular turn 0 into 1,
>    meaning the get op returns junk before a successful set op.
>  * The x86 flavours reject the hypercalls before the VM has vCPUs allocated,
>    despite the pool size being a domain property.

I guess this is merely a remnant and could easily be dropped there.

>  * Even the hypercall names are long-obsolete.
> 
> Implement an interface that doesn't suck, which can be first used to unit test
> the behaviour, and subsequently correct a broken implementation.  The old
> interface will be retired in due course.
> 
> This is part of XSA-409 / CVE-2022-33747.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Xen Security Team <security@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Henry Wang <Henry.Wang@arm.com>
> CC: Anthony PERARD <anthony.perard@citrix.com>
> 
> Name subject to improvement.

paging_{get,set}_mempool_size() for the arch helpers (in particular
fitting better with them living in paging.c as well its multi-purpose use
on x86) and XEN_DOMCTL_{get,set}_paging_mempool_size? Perhaps even the
"mem" could be dropped?

>  ABI not.

With the comment in the public header saying "Users of this interface are
required to identify the granularity by other means" I wonder why the
interface needs to be byte-granular. If the caller needs to know page size
by whatever means, it can as well pass in a page count.

> Future TODOs:
>  * x86 shadow still rounds up.  This is buggy as it's a simultaneous equation
>    with tot_pages which varies over time with ballooning.
>  * x86 PV is weird.  There are no toolstack interact with the shadow pool
>    size, but the "shadow" pool it does come into existence when logdirty (or
>    pv-l1tf) when first enabled.
>  * The shadow+hap logic is in desperate need of deduping.

I have a tiny step towards this queued as post-XSA-410 work, folding HAP's
and shadow's freelist, total_pages, free_pages, and p2m_pages. Here this
would mean {hap,shadow}_get_allocation_bytes() could be done away with,
having the logic exclusively in paging.c.

> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -100,6 +100,14 @@ unsigned int p2m_get_allocation(struct domain *d)
>      return ROUNDUP(nr_pages, 1 << (20 - PAGE_SHIFT)) >> (20 - PAGE_SHIFT);
>  }
>  
> +/* Return the size of the pool, in bytes. */
> +int arch_get_p2m_mempool_size(struct domain *d, uint64_t *size)
> +{
> +    *size = ACCESS_ONCE(d->arch.paging.p2m_total_pages) << PAGE_SHIFT;

This may overflow for Arm32.

> @@ -157,6 +165,25 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
>      return 0;
>  }
>  
> +int arch_set_p2m_mempool_size(struct domain *d, uint64_t size)
> +{
> +    unsigned long pages = size >> PAGE_SHIFT;
> +    bool preempted = false;
> +    int rc;
> +
> +    if ( (size & ~PAGE_MASK) ||          /* Non page-sized request? */
> +         pages != (size >> PAGE_SHIFT) ) /* 32-bit overflow? */
> +        return -EINVAL;

Simply "(pages << PAGE_SHIFT) != size"? And then move the check into
common code?

> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -345,6 +345,16 @@ unsigned int hap_get_allocation(struct domain *d)
>              + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0));
>  }
>  
> +int hap_get_allocation_bytes(struct domain *d, uint64_t *size)
> +{
> +    unsigned long pages = (d->arch.paging.hap.total_pages +
> +                           d->arch.paging.hap.p2m_pages);

Unlike for Arm no ACCESS_ONCE() here? Also the addition can in
principle overflow, because being done only in 32 bits.

> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -977,6 +977,45 @@ int __init paging_set_allocation(struct domain *d, unsigned int pages,
>  }
>  #endif
>  
> +int arch_get_p2m_mempool_size(struct domain *d, uint64_t *size)
> +{
> +    int rc;
> +
> +    if ( is_pv_domain(d) )
> +        return -EOPNOTSUPP;
> +
> +    if ( hap_enabled(d) )
> +        rc = hap_get_allocation_bytes(d, size);
> +    else
> +        rc = shadow_get_allocation_bytes(d, size);
> +
> +    return rc;
> +}
> +
> +int arch_set_p2m_mempool_size(struct domain *d, uint64_t size)
> +{
> +    unsigned long pages = size >> PAGE_SHIFT;
> +    bool preempted = false;
> +    int rc;
> +
> +    if ( is_pv_domain(d) )
> +        return -EOPNOTSUPP;

Why? You do say "PV is weird" in a post-commit-message remark, but why
do you want to retain this weirdness? Even if today the tool stack
doesn't set the size when enabling log-dirty mode, I'd view this as a
bug which could be addressed purely in the tool stack if this check
wasn't there.

> +    if ( size & ~PAGE_MASK )             /* Non page-sized request? */
> +        return -EINVAL;
> +
> +    ASSERT(paging_mode_enabled(d));

Not only with the PV aspect in mind - why? It looks reasonable to me
to set the pool size before enabling any paging mode.

> +    paging_lock(d);
> +    if ( hap_enabled(d) )
> +        rc = hap_set_allocation(d, pages, &preempted);
> +    else
> +        rc = shadow_set_allocation(d, pages, &preempted);

Potential truncation from the "unsigned long" -> "unsigned int"
conversions.

Jan

Re: [PATCH 2/4] tools/tests: Unit test for p2m pool size

[Jan Beulich]

On 26.10.2022 12:20, Andrew Cooper wrote:
> --- /dev/null
> +++ b/tools/tests/p2m-pool/Makefile
> @@ -0,0 +1,42 @@
> +XEN_ROOT = $(CURDIR)/../../..
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +TARGET := test-p2m-pool
> +
> +.PHONY: all
> +all: $(TARGET)
> +
> +.PHONY: clean
> +clean:
> +	$(RM) -- *.o $(TARGET) $(DEPS_RM)
> +
> +.PHONY: distclean
> +distclean: clean
> +	$(RM) -- *~
> +
> +.PHONY: install
> +install: all
> +	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
> +	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
> +
> +.PHONY: uninstall
> +uninstall:
> +	$(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET)
> +
> +CFLAGS += $(CFLAGS_xeninclude)
> +CFLAGS += $(CFLAGS_libxenctrl)
> +CFLAGS += $(CFLAGS_libxenforeginmemory)

Typo here or typo also elsewhere: CFLAGS_libxenforeignmemory? I
have to admit that I can't really spot where these variables are
populated. The place in Rules.mk that I could spot uses

 CFLAGS_libxen$(1) = $$(CFLAGS_xeninclude)

i.e. the expansion doesn't really depend on the library.

Apart from this looks okay to me, maybe apart from ...

> --- /dev/null
> +++ b/tools/tests/p2m-pool/test-p2m-pool.c
> @@ -0,0 +1,181 @@
> +#include <err.h>
> +#include <errno.h>
> +#include <inttypes.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +
> +#include <xenctrl.h>
> +#include <xenforeignmemory.h>
> +#include <xengnttab.h>
> +#include <xen-tools/libs.h>
> +
> +static unsigned int nr_failures;
> +#define fail(fmt, ...)                          \
> +({                                              \
> +    nr_failures++;                              \
> +    (void)printf(fmt, ##__VA_ARGS__);           \
> +})
> +
> +static xc_interface *xch;
> +static uint32_t domid;
> +
> +static struct xen_domctl_createdomain create = {
> +    .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> +    .max_vcpus = 1,
> +    .max_grant_frames = 1,
> +    .grant_opts = XEN_DOMCTL_GRANT_version(1),
> +
> +    .arch = {
> +#if defined(__x86_64__) || defined(__i386__)
> +        .emulation_flags = XEN_X86_EMU_LAPIC,
> +#endif
> +    },
> +};
> +
> +static uint64_t default_p2m_size_bytes =
> +#if defined(__x86_64__) || defined(__i386__)
> +    256 << 12; /* Only x86 HAP for now.  x86 Shadow broken. */

... this shadow related comment (the commit message could at least
say what's wrong there, to give a hint at what would need fixing to
remove this restriction) and ...

> +#elif defined (__arm__) || defined(__aarch64__)
> +    16 << 12;
> +#endif
> +
> +static void run_tests(void)
> +{
> +    xen_pfn_t physmap[] = { 0 };
> +    uint64_t size_bytes, old_size_bytes;
> +    int rc;
> +
> +    printf("Test default p2m mempool size\n");
> +
> +    rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes);
> +    if ( rc )
> +        return fail("  Fail: get p2m mempool size: %d - %s\n",
> +                    errno, strerror(errno));
> +
> +    printf("P2M pool size %"PRIu64" bytes (%"PRIu64"kB, %"PRIu64"MB)\n",
> +           size_bytes, size_bytes >> 10, size_bytes >> 20);
> +
> +
> +    /*
> +     * Check that the domain has the expected default allocation size.  This
> +     * will fail if the logic in Xen is altered without an equivelent
> +     * adjustment here.
> +     */
> +    if ( size_bytes != default_p2m_size_bytes )
> +        return fail("  Fail: size %"PRIu64" != expected size %"PRIu64"\n",
> +                    size_bytes, default_p2m_size_bytes);
> +
> +
> +    printf("Test that allocate doesn't alter pool size\n");
> +
> +    /*
> +     * Populate the domain with some RAM.  This will cause more of the p2m
> +     * pool to be used.
> +     */
> +    old_size_bytes = size_bytes;
> +
> +    rc = xc_domain_setmaxmem(xch, domid, -1);
> +    if ( rc )
> +        return fail("  Fail: setmaxmem: : %d - %s\n",
> +                    errno, strerror(errno));
> +
> +    rc = xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, physmap);
> +    if ( rc )
> +        return fail("  Fail: populate physmap: %d - %s\n",
> +                    errno, strerror(errno));
> +
> +    /*
> +     * Re-get the p2m size.  Should not have changed as a consequence of
> +     * populate physmap.
> +     */
> +    rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes);
> +    if ( rc )
> +        return fail("  Fail: get p2m mempool size: %d - %s\n",
> +                    errno, strerror(errno));
> +
> +    if ( old_size_bytes != size_bytes )
> +        return fail("  Fail: p2m mempool size changed %"PRIu64" => %"PRIu64"\n",
> +                    old_size_bytes, size_bytes);
> +
> +
> +
> +    printf("Test bad set size\n");
> +
> +    /*
> +     * Check that setting a non-page size results in failure.
> +     */
> +    rc = xc_set_p2m_mempool_size(xch, domid, size_bytes + 1);
> +    if ( rc != -1 || errno != EINVAL )
> +        return fail("  Fail: Bad set size: expected -1/EINVAL, got %d/%d - %s\n",
> +                    rc, errno, strerror(errno));
> +
> +
> +    printf("Test very large set size\n");
> +
> +    /*
> +     * Check that setting a large P2M size succeeds.  This is expecting to
> +     * trigger continuations.
> +     */
> +    rc = xc_set_p2m_mempool_size(xch, domid, 64 << 20);
> +    if ( rc )
> +        return fail("  Fail: Set size 64MB: %d - %s\n",
> +                    errno, strerror(errno));
> +
> +
> +    /*
> +     * Check that the reported size matches what set consumed.
> +     */
> +    rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes);
> +    if ( rc )
> +        return fail("  Fail: get p2m mempool size: %d - %s\n",
> +                    errno, strerror(errno));
> +
> +    if ( size_bytes != 64 << 20 )
> +        return fail("  Fail: expected mempool size %u, got %"PRIu64"\n",
> +                    64 << 20, size_bytes);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    int rc;
> +
> +    printf("P2M Shadow memory pool tests\n");

... the question of why "Shadow" here.

Jan

Re: [PATCH 2/4] tools/tests: Unit test for p2m pool size

[Andrew Cooper]

On 26/10/2022 15:24, Jan Beulich wrote:
> On 26.10.2022 12:20, Andrew Cooper wrote:
>> --- /dev/null
>> +++ b/tools/tests/p2m-pool/Makefile
>> @@ -0,0 +1,42 @@
>> +XEN_ROOT = $(CURDIR)/../../..
>> +include $(XEN_ROOT)/tools/Rules.mk
>> +
>> +TARGET := test-p2m-pool
>> +
>> +.PHONY: all
>> +all: $(TARGET)
>> +
>> +.PHONY: clean
>> +clean:
>> +	$(RM) -- *.o $(TARGET) $(DEPS_RM)
>> +
>> +.PHONY: distclean
>> +distclean: clean
>> +	$(RM) -- *~
>> +
>> +.PHONY: install
>> +install: all
>> +	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
>> +	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
>> +
>> +.PHONY: uninstall
>> +uninstall:
>> +	$(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET)
>> +
>> +CFLAGS += $(CFLAGS_xeninclude)
>> +CFLAGS += $(CFLAGS_libxenctrl)
>> +CFLAGS += $(CFLAGS_libxenforeginmemory)
> Typo here or typo also elsewhere: CFLAGS_libxenforeignmemory? I
> have to admit that I can't really spot where these variables are
> populated. The place in Rules.mk that I could spot uses
>
>  CFLAGS_libxen$(1) = $$(CFLAGS_xeninclude)
>
> i.e. the expansion doesn't really depend on the library.

Hmm.  It was copy&paste from test-resource and I forgot to trim
foreignmemory, but I guess I'll need to go and fix the typo everywhere.

>
> Apart from this looks okay to me, maybe apart from ...
>
>> --- /dev/null
>> +++ b/tools/tests/p2m-pool/test-p2m-pool.c
>> @@ -0,0 +1,181 @@
>> +#include <err.h>
>> +#include <errno.h>
>> +#include <inttypes.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <sys/mman.h>
>> +
>> +#include <xenctrl.h>
>> +#include <xenforeignmemory.h>
>> +#include <xengnttab.h>
>> +#include <xen-tools/libs.h>
>> +
>> +static unsigned int nr_failures;
>> +#define fail(fmt, ...)                          \
>> +({                                              \
>> +    nr_failures++;                              \
>> +    (void)printf(fmt, ##__VA_ARGS__);           \
>> +})
>> +
>> +static xc_interface *xch;
>> +static uint32_t domid;
>> +
>> +static struct xen_domctl_createdomain create = {
>> +    .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>> +    .max_vcpus = 1,
>> +    .max_grant_frames = 1,
>> +    .grant_opts = XEN_DOMCTL_GRANT_version(1),
>> +
>> +    .arch = {
>> +#if defined(__x86_64__) || defined(__i386__)
>> +        .emulation_flags = XEN_X86_EMU_LAPIC,
>> +#endif
>> +    },
>> +};
>> +
>> +static uint64_t default_p2m_size_bytes =
>> +#if defined(__x86_64__) || defined(__i386__)
>> +    256 << 12; /* Only x86 HAP for now.  x86 Shadow broken. */
> ... this shadow related comment (the commit message could at least
> say what's wrong there, to give a hint at what would need fixing to
> remove this restriction) and ...

That was in reference to the PV issue.  Will follow it up on patch 1
where there's more context.

>
>> +#elif defined (__arm__) || defined(__aarch64__)
>> +    16 << 12;
>> +#endif
>> +
>> +static void run_tests(void)
>> +{
>> +    xen_pfn_t physmap[] = { 0 };
>> +    uint64_t size_bytes, old_size_bytes;
>> +    int rc;
>> +
>> +    printf("Test default p2m mempool size\n");
>> +
>> +    rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes);
>> +    if ( rc )
>> +        return fail("  Fail: get p2m mempool size: %d - %s\n",
>> +                    errno, strerror(errno));
>> +
>> +    printf("P2M pool size %"PRIu64" bytes (%"PRIu64"kB, %"PRIu64"MB)\n",
>> +           size_bytes, size_bytes >> 10, size_bytes >> 20);
>> +
>> +
>> +    /*
>> +     * Check that the domain has the expected default allocation size.  This
>> +     * will fail if the logic in Xen is altered without an equivelent
>> +     * adjustment here.
>> +     */
>> +    if ( size_bytes != default_p2m_size_bytes )
>> +        return fail("  Fail: size %"PRIu64" != expected size %"PRIu64"\n",
>> +                    size_bytes, default_p2m_size_bytes);
>> +
>> +
>> +    printf("Test that allocate doesn't alter pool size\n");
>> +
>> +    /*
>> +     * Populate the domain with some RAM.  This will cause more of the p2m
>> +     * pool to be used.
>> +     */
>> +    old_size_bytes = size_bytes;
>> +
>> +    rc = xc_domain_setmaxmem(xch, domid, -1);
>> +    if ( rc )
>> +        return fail("  Fail: setmaxmem: : %d - %s\n",
>> +                    errno, strerror(errno));
>> +
>> +    rc = xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, physmap);
>> +    if ( rc )
>> +        return fail("  Fail: populate physmap: %d - %s\n",
>> +                    errno, strerror(errno));
>> +
>> +    /*
>> +     * Re-get the p2m size.  Should not have changed as a consequence of
>> +     * populate physmap.
>> +     */
>> +    rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes);
>> +    if ( rc )
>> +        return fail("  Fail: get p2m mempool size: %d - %s\n",
>> +                    errno, strerror(errno));
>> +
>> +    if ( old_size_bytes != size_bytes )
>> +        return fail("  Fail: p2m mempool size changed %"PRIu64" => %"PRIu64"\n",
>> +                    old_size_bytes, size_bytes);
>> +
>> +
>> +
>> +    printf("Test bad set size\n");
>> +
>> +    /*
>> +     * Check that setting a non-page size results in failure.
>> +     */
>> +    rc = xc_set_p2m_mempool_size(xch, domid, size_bytes + 1);
>> +    if ( rc != -1 || errno != EINVAL )
>> +        return fail("  Fail: Bad set size: expected -1/EINVAL, got %d/%d - %s\n",
>> +                    rc, errno, strerror(errno));
>> +
>> +
>> +    printf("Test very large set size\n");
>> +
>> +    /*
>> +     * Check that setting a large P2M size succeeds.  This is expecting to
>> +     * trigger continuations.
>> +     */
>> +    rc = xc_set_p2m_mempool_size(xch, domid, 64 << 20);
>> +    if ( rc )
>> +        return fail("  Fail: Set size 64MB: %d - %s\n",
>> +                    errno, strerror(errno));
>> +
>> +
>> +    /*
>> +     * Check that the reported size matches what set consumed.
>> +     */
>> +    rc = xc_get_p2m_mempool_size(xch, domid, &size_bytes);
>> +    if ( rc )
>> +        return fail("  Fail: get p2m mempool size: %d - %s\n",
>> +                    errno, strerror(errno));
>> +
>> +    if ( size_bytes != 64 << 20 )
>> +        return fail("  Fail: expected mempool size %u, got %"PRIu64"\n",
>> +                    64 << 20, size_bytes);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    int rc;
>> +
>> +    printf("P2M Shadow memory pool tests\n");
> ... the question of why "Shadow" here.

Hmm, stale, but even the name of this test (p2m-pool) is subject to a
resolution of the naming question on patch 1.

~Andrew

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Andrew Cooper]

On 26/10/2022 14:42, Jan Beulich wrote:
> On 26.10.2022 12:20, Andrew Cooper wrote:
>> The existing XEN_DOMCTL_SHADOW_OP_{GET,SET}_ALLOCATION have problems:
>>
>>  * All set_allocation() flavours have an overflow-before-widen bug when
>>    calculating "sc->mb << (20 - PAGE_SHIFT)".
>>  * All flavours have a granularity of of 1M.  This was tolerable when the size
>>    of the pool could only be set at the same granularity, but is broken now
>>    that ARM has a 16-page stopgap allocation in use.
>>  * All get_allocation() flavours round up, and in particular turn 0 into 1,
>>    meaning the get op returns junk before a successful set op.
>>  * The x86 flavours reject the hypercalls before the VM has vCPUs allocated,
>>    despite the pool size being a domain property.
> I guess this is merely a remnant and could easily be dropped there.

It's intermixed the other shadow operations.  It wasn't trivially-safe
enough to do here, and needs coming back to in future work.

>
>>  * Even the hypercall names are long-obsolete.
>>
>> Implement an interface that doesn't suck, which can be first used to unit test
>> the behaviour, and subsequently correct a broken implementation.  The old
>> interface will be retired in due course.
>>
>> This is part of XSA-409 / CVE-2022-33747.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Xen Security Team <security@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>> CC: Henry Wang <Henry.Wang@arm.com>
>> CC: Anthony PERARD <anthony.perard@citrix.com>
>>
>> Name subject to improvement.
> paging_{get,set}_mempool_size() for the arch helpers (in particular
> fitting better with them living in paging.c as well its multi-purpose use
> on x86) and XEN_DOMCTL_{get,set}_paging_mempool_size? Perhaps even the
> "mem" could be dropped?

Yeah, this was a placeholder for "what are we actually going to call it
in Xen".

I went with mempool over just simply pool because pool has a very
different meaning slightly higher in the toolstack where you talk about
pools of servers.  Admittedly, that's code outside of xen.git, but the
hypercall names do percolate up into those codebases.

paging isn't a great name.  While it's what we call the infrastructure
in x86, it has nothing to do with paging things out to disk (the thing
everyone associates the name with), nor the xenpaging infrastructure
(Xen's version of what OS paging supposedly means).

>
>>  ABI not.
> With the comment in the public header saying "Users of this interface are
> required to identify the granularity by other means" I wonder why the
> interface needs to be byte-granular. If the caller needs to know page size
> by whatever means, it can as well pass in a page count.

Not all architectures have pagetable levels of uniform size.  Not all
architectures have the mapping granularity equal to the pagetable size. 
x86 has examples of both of these (and in a rogue move, one x86 hardware
vendor is trying to add even more pagetable asymmetry).  Other
architectures substantially more variety.

Even on x86, there are performance advantages from using 8k or 16k
arrangements, which could cause us insist upon >4k requirements here. 
(TBH, not actually for this usecase, but the principle is still valid.)


The reason is because this is a size.  Sizes are in bytes, and that's
how everyone thinks about them.  Its how the value is already specified
in an xl cfg file, and it entirely unambiguous at all levels of the stack.

Every translation of the value in the software stack risks breaking
things, even stuff as simple as debugging.  As proof, count the number
of translation errors I've already identified in this patch alone.

This ABI does not require any changes at all (not even recompiling
userspace) for ARM to decide to use 16k or 64k pagetables in Xen, or for
x86 to decide that 8k or 16k is beneficial enough to actually require.

Attempting to compress this uint64_t into something smaller by any means
will create bugs, or at increased complexity and a high risk of bugs. 
There isn't enough money on earth right now to afford a 128bit processor
with enough ram for this current ABI to need changing.


This is going to be a reoccurring theme through fixing the ABIs.  Its
one of a several areas where there is objectively one right answer, both
in terms of ease of use, and compatibility to future circumstances.



>
>> Future TODOs:
>>  * x86 shadow still rounds up.  This is buggy as it's a simultaneous equation
>>    with tot_pages which varies over time with ballooning.
>>  * x86 PV is weird.  There are no toolstack interact with the shadow pool
>>    size, but the "shadow" pool it does come into existence when logdirty (or
>>    pv-l1tf) when first enabled.
>>  * The shadow+hap logic is in desperate need of deduping.
> I have a tiny step towards this queued as post-XSA-410 work, folding HAP's
> and shadow's freelist, total_pages, free_pages, and p2m_pages. Here this
> would mean {hap,shadow}_get_allocation_bytes() could be done away with,
> having the logic exclusively in paging.c.

Thanks.  I'll drop that task from my todo list.

But really, it need to be fully common, because RISC-V is going to need
it too.  (I'm told development on RISC-V will start back up any time now.)

>
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -100,6 +100,14 @@ unsigned int p2m_get_allocation(struct domain *d)
>>      return ROUNDUP(nr_pages, 1 << (20 - PAGE_SHIFT)) >> (20 - PAGE_SHIFT);
>>  }
>>  
>> +/* Return the size of the pool, in bytes. */
>> +int arch_get_p2m_mempool_size(struct domain *d, uint64_t *size)
>> +{
>> +    *size = ACCESS_ONCE(d->arch.paging.p2m_total_pages) << PAGE_SHIFT;
> This may overflow for Arm32.

So it will.  I'll widen first.

>
>> @@ -157,6 +165,25 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
>>      return 0;
>>  }
>>  
>> +int arch_set_p2m_mempool_size(struct domain *d, uint64_t size)
>> +{
>> +    unsigned long pages = size >> PAGE_SHIFT;
>> +    bool preempted = false;
>> +    int rc;
>> +
>> +    if ( (size & ~PAGE_MASK) ||          /* Non page-sized request? */
>> +         pages != (size >> PAGE_SHIFT) ) /* 32-bit overflow? */
>> +        return -EINVAL;
> Simply "(pages << PAGE_SHIFT) != size"? And then move the check into
> common code?

These checks are deliberately not in common code.  That's just creating
work that someone will need to undo in due course.

>
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -345,6 +345,16 @@ unsigned int hap_get_allocation(struct domain *d)
>>              + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0));
>>  }
>>  
>> +int hap_get_allocation_bytes(struct domain *d, uint64_t *size)
>> +{
>> +    unsigned long pages = (d->arch.paging.hap.total_pages +
>> +                           d->arch.paging.hap.p2m_pages);
> Unlike for Arm no ACCESS_ONCE() here? Also the addition can in
> principle overflow, because being done only in 32 bits.

I'm not actually convinced ARM needs ACCESS_ONCE() to begin with.  I
can't see any legal transformation of that logic which could result in a
torn load.

Both examples were written to match the existing code, because this
needs backporting to all security trees.

I forgot to mention the overflow on x86 in the future todo section. 
This code is rife with them.

>
>> --- a/xen/arch/x86/mm/paging.c
>> +++ b/xen/arch/x86/mm/paging.c
>> @@ -977,6 +977,45 @@ int __init paging_set_allocation(struct domain *d, unsigned int pages,
>>  }
>>  #endif
>>  
>> +int arch_get_p2m_mempool_size(struct domain *d, uint64_t *size)
>> +{
>> +    int rc;
>> +
>> +    if ( is_pv_domain(d) )
>> +        return -EOPNOTSUPP;
>> +
>> +    if ( hap_enabled(d) )
>> +        rc = hap_get_allocation_bytes(d, size);
>> +    else
>> +        rc = shadow_get_allocation_bytes(d, size);
>> +
>> +    return rc;
>> +}
>> +
>> +int arch_set_p2m_mempool_size(struct domain *d, uint64_t size)
>> +{
>> +    unsigned long pages = size >> PAGE_SHIFT;
>> +    bool preempted = false;
>> +    int rc;
>> +
>> +    if ( is_pv_domain(d) )
>> +        return -EOPNOTSUPP;
> Why? You do say "PV is weird" in a post-commit-message remark, but why
> do you want to retain this weirdness? Even if today the tool stack
> doesn't set the size when enabling log-dirty mode, I'd view this as a
> bug which could be addressed purely in the tool stack if this check
> wasn't there.

I want to clean up PV, but again, it wasn't sufficiently trivially-safe
to do right now.

PV is weird because it is neither hap_enabled() (fundamentally), nor
shadow_enabled() when logdirty isn't active.  While the freelist is
suitably constructed, the get/set operations were previously rejected
and cleanup is local to the disable op, not domain shutdown.

I could put in a /* TODO: relax in due course */ if you'd prefer?

>> +    if ( size & ~PAGE_MASK )             /* Non page-sized request? */
>> +        return -EINVAL;
>> +
>> +    ASSERT(paging_mode_enabled(d));
> Not only with the PV aspect in mind - why? It looks reasonable to me
> to set the pool size before enabling any paging mode.

Because this is how all the existing logic is expressed, and this patch
wants backporting.

There is sooo much to clean up...

>
>> +    paging_lock(d);
>> +    if ( hap_enabled(d) )
>> +        rc = hap_set_allocation(d, pages, &preempted);
>> +    else
>> +        rc = shadow_set_allocation(d, pages, &preempted);
> Potential truncation from the "unsigned long" -> "unsigned int"
> conversions.

I'd not even spotted that ARM and x86 were different in this regard.

More short term hacks, it seems.

~Andrew

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Julien Grall]

Hi Andrew,

On 26/10/2022 20:22, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -345,6 +345,16 @@ unsigned int hap_get_allocation(struct domain *d)
>>>               + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0));
>>>   }
>>>   
>>> +int hap_get_allocation_bytes(struct domain *d, uint64_t *size)
>>> +{
>>> +    unsigned long pages = (d->arch.paging.hap.total_pages +
>>> +                           d->arch.paging.hap.p2m_pages);
>> Unlike for Arm no ACCESS_ONCE() here? Also the addition can in
>> principle overflow, because being done only in 32 bits.
> 
> I'm not actually convinced ARM needs ACCESS_ONCE() to begin with.  I
> can't see any legal transformation of that logic which could result in a
> torn load.

AFAIU, ACCESS_ONCE() is not only about torn load but also making sure 
that the compiler will only read the value once.

When LTO is enabled (not yet supported) in Xen, can we guarantee the 
compiler will not try to access total_pages twice (obviously it would be 
caller dependent)?

With that in mind, when LTO is enabled on Linux arm64, the 
implementation of READ_ONCE() is not a simple (volatile *) to prevent 
the compiler to do harmful convertion. Possibly something we will need 
to consider in Xen in the future if we enable LTO. In this context, the 
ACCESS_ONCE() would make sense because we don't know (or should not 
assume) how the caller will use it.

Regardless that, I think using ACCESS_ONCE() help to document how the 
variable should be used. This will reduce the risk that someone decides 
to add a new use total_pages like below:

val = d->arch.paging.total_pages;

if ( val == 0 )
   return ...

/* use val */

AFAIU, a compiler would be allow to read total_pages twice here. Which 
is not what we would want. I am ready to bet this will be missed.

So consistency here is IMO much better. An alternative would be to 
document why we think the compiler would not be naughty.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Jan Beulich]

On 26.10.2022 23:24, Julien Grall wrote:
> On 26/10/2022 20:22, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/mm/hap/hap.c
>>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>>> @@ -345,6 +345,16 @@ unsigned int hap_get_allocation(struct domain *d)
>>>>               + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0));
>>>>   }
>>>>   
>>>> +int hap_get_allocation_bytes(struct domain *d, uint64_t *size)
>>>> +{
>>>> +    unsigned long pages = (d->arch.paging.hap.total_pages +
>>>> +                           d->arch.paging.hap.p2m_pages);
>>> Unlike for Arm no ACCESS_ONCE() here? Also the addition can in
>>> principle overflow, because being done only in 32 bits.
>>
>> I'm not actually convinced ARM needs ACCESS_ONCE() to begin with.  I
>> can't see any legal transformation of that logic which could result in a
>> torn load.
> 
> AFAIU, ACCESS_ONCE() is not only about torn load but also making sure 
> that the compiler will only read the value once.
> 
> When LTO is enabled (not yet supported) in Xen, can we guarantee the 
> compiler will not try to access total_pages twice (obviously it would be 
> caller dependent)?

Aren't all accesses (supposed to be) under paging lock? At which point
there's no issue with multiple (or torn) accesses?

Jan

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Jan Beulich]

On 26.10.2022 21:22, Andrew Cooper wrote:
> On 26/10/2022 14:42, Jan Beulich wrote:
>> On 26.10.2022 12:20, Andrew Cooper wrote:
>>> The existing XEN_DOMCTL_SHADOW_OP_{GET,SET}_ALLOCATION have problems:
>>>
>>>  * All set_allocation() flavours have an overflow-before-widen bug when
>>>    calculating "sc->mb << (20 - PAGE_SHIFT)".
>>>  * All flavours have a granularity of of 1M.  This was tolerable when the size
>>>    of the pool could only be set at the same granularity, but is broken now
>>>    that ARM has a 16-page stopgap allocation in use.
>>>  * All get_allocation() flavours round up, and in particular turn 0 into 1,
>>>    meaning the get op returns junk before a successful set op.
>>>  * The x86 flavours reject the hypercalls before the VM has vCPUs allocated,
>>>    despite the pool size being a domain property.
>> I guess this is merely a remnant and could easily be dropped there.
> 
> It's intermixed the other shadow operations.  It wasn't trivially-safe
> enough to do here, and needs coming back to in future work.

Right, and I should have said that this is merely a remark, not a request
for any change here.

>>> Name subject to improvement.
>> paging_{get,set}_mempool_size() for the arch helpers (in particular
>> fitting better with them living in paging.c as well its multi-purpose use
>> on x86) and XEN_DOMCTL_{get,set}_paging_mempool_size? Perhaps even the
>> "mem" could be dropped?
> 
> Yeah, this was a placeholder for "what are we actually going to call it
> in Xen".
> 
> I went with mempool over just simply pool because pool has a very
> different meaning slightly higher in the toolstack where you talk about
> pools of servers.  Admittedly, that's code outside of xen.git, but the
> hypercall names do percolate up into those codebases.
> 
> paging isn't a great name.  While it's what we call the infrastructure
> in x86, it has nothing to do with paging things out to disk (the thing
> everyone associates the name with), nor the xenpaging infrastructure
> (Xen's version of what OS paging supposedly means).

Okay, "paging" can be somewhat misleading. But "p2m" also doesn't fit
the use(s) on x86. Yet we'd like to use a name clearly better than the
previous (and yet more wrong/misleading) "shadow". I have to admit that
I can't think of any other sensible name, and among the ones discussed
I still think "paging" is the one coming closest despite the
generally different meaning of the word elsewhere.

>>>  ABI not.
>> With the comment in the public header saying "Users of this interface are
>> required to identify the granularity by other means" I wonder why the
>> interface needs to be byte-granular. If the caller needs to know page size
>> by whatever means, it can as well pass in a page count.
> 
> Not all architectures have pagetable levels of uniform size.  Not all
> architectures have the mapping granularity equal to the pagetable size. 
> x86 has examples of both of these (and in a rogue move, one x86 hardware
> vendor is trying to add even more pagetable asymmetry).  Other
> architectures substantially more variety.
> 
> Even on x86, there are performance advantages from using 8k or 16k
> arrangements, which could cause us insist upon >4k requirements here. 
> (TBH, not actually for this usecase, but the principle is still valid.)

Perhaps, but that doesn't change the picture: The tool stack still needs
to know how many of the low bits in the request need to be clear (unless
you would accept to go back to rounding an unaligned input value). And
once it knows this value, it can still convert to a count of that-unit-
sized blocks of memory.

> The reason is because this is a size.  Sizes are in bytes, and that's
> how everyone thinks about them.  Its how the value is already specified
> in an xl cfg file, and it entirely unambiguous at all levels of the stack.
> 
> Every translation of the value in the software stack risks breaking
> things, even stuff as simple as debugging.  As proof, count the number
> of translation errors I've already identified in this patch alone.
> 
> This ABI does not require any changes at all (not even recompiling
> userspace) for ARM to decide to use 16k or 64k pagetables in Xen, or for
> x86 to decide that 8k or 16k is beneficial enough to actually require.
> 
> Attempting to compress this uint64_t into something smaller by any means
> will create bugs, or at increased complexity and a high risk of bugs. 
> There isn't enough money on earth right now to afford a 128bit processor
> with enough ram for this current ABI to need changing.

I didn't suggest to use a type other than uint64_t. I'm merely puzzled
by your insistence on byte granularity while at the same time requiring
inputs to be suitable multiples of a base granularity, obtaining of
which is not even specified alongside this new interface.

> This is going to be a reoccurring theme through fixing the ABIs.  Its
> one of a several areas where there is objectively one right answer, both
> in terms of ease of use, and compatibility to future circumstances.

Well, I wouldn't say using whatever base granularity as a unit is
"objectively" less right.

>>> @@ -157,6 +165,25 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
>>>      return 0;
>>>  }
>>>  
>>> +int arch_set_p2m_mempool_size(struct domain *d, uint64_t size)
>>> +{
>>> +    unsigned long pages = size >> PAGE_SHIFT;
>>> +    bool preempted = false;
>>> +    int rc;
>>> +
>>> +    if ( (size & ~PAGE_MASK) ||          /* Non page-sized request? */
>>> +         pages != (size >> PAGE_SHIFT) ) /* 32-bit overflow? */
>>> +        return -EINVAL;
>> Simply "(pages << PAGE_SHIFT) != size"? And then move the check into
>> common code?
> 
> These checks are deliberately not in common code.  That's just creating
> work that someone will need to undo in due course.

Would you mind clarifying why you think so? If the base unit isn't PAGE_SIZE
then all it takes is to introduce a suitable #define and/or global
specifying the intended per-arch value. Even if you expected this to become
a domain-dependent property, the corresponding value could still be a field
in (common) struct domain.

>>> --- a/xen/arch/x86/mm/paging.c
>>> +++ b/xen/arch/x86/mm/paging.c
>>> @@ -977,6 +977,45 @@ int __init paging_set_allocation(struct domain *d, unsigned int pages,
>>>  }
>>>  #endif
>>>  
>>> +int arch_get_p2m_mempool_size(struct domain *d, uint64_t *size)
>>> +{
>>> +    int rc;
>>> +
>>> +    if ( is_pv_domain(d) )
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    if ( hap_enabled(d) )
>>> +        rc = hap_get_allocation_bytes(d, size);
>>> +    else
>>> +        rc = shadow_get_allocation_bytes(d, size);
>>> +
>>> +    return rc;
>>> +}
>>> +
>>> +int arch_set_p2m_mempool_size(struct domain *d, uint64_t size)
>>> +{
>>> +    unsigned long pages = size >> PAGE_SHIFT;
>>> +    bool preempted = false;
>>> +    int rc;
>>> +
>>> +    if ( is_pv_domain(d) )
>>> +        return -EOPNOTSUPP;
>> Why? You do say "PV is weird" in a post-commit-message remark, but why
>> do you want to retain this weirdness? Even if today the tool stack
>> doesn't set the size when enabling log-dirty mode, I'd view this as a
>> bug which could be addressed purely in the tool stack if this check
>> wasn't there.
> 
> I want to clean up PV, but again, it wasn't sufficiently trivially-safe
> to do right now.
> 
> PV is weird because it is neither hap_enabled() (fundamentally), nor
> shadow_enabled() when logdirty isn't active.  While the freelist is
> suitably constructed, the get/set operations were previously rejected
> and cleanup is local to the disable op, not domain shutdown.
> 
> I could put in a /* TODO: relax in due course */ if you'd prefer?

Yes please - that would clarify this isn't a hard requirement.

>>> +    if ( size & ~PAGE_MASK )             /* Non page-sized request? */
>>> +        return -EINVAL;
>>> +
>>> +    ASSERT(paging_mode_enabled(d));
>> Not only with the PV aspect in mind - why? It looks reasonable to me
>> to set the pool size before enabling any paging mode.
> 
> Because this is how all the existing logic is expressed, and this patch
> wants backporting.

What do you mean by "is expressed"? I can't seem to be able to find a
similar check on the existing code paths. But given that yesterday I
almost overlooked the d->vcpu check in paging_domctl(), I can easily
accept that I might be overlooking something somewhere.

Jan

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Jan Beulich]

On 26.10.2022 12:20, Andrew Cooper wrote:
> +int arch_set_p2m_mempool_size(struct domain *d, uint64_t size)
> +{
> +    unsigned long pages = size >> PAGE_SHIFT;
> +    bool preempted = false;
> +    int rc;
> +
> +    if ( is_pv_domain(d) )
> +        return -EOPNOTSUPP;
> +
> +    if ( size & ~PAGE_MASK )             /* Non page-sized request? */
> +        return -EINVAL;
> +
> +    ASSERT(paging_mode_enabled(d));
> +
> +    paging_lock(d);
> +    if ( hap_enabled(d) )
> +        rc = hap_set_allocation(d, pages, &preempted);
> +    else
> +        rc = shadow_set_allocation(d, pages, &preempted);
> +    paging_unlock(d);
> +
> +    return preempted ? -ERESTART : rc;
> +}

There's a further difference between HAP and shadow which may want/need
reflecting here: shadow's handling of XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION
rejects 0 as an input when shadow mode is still enabled. On one hand
that's reasonable from an abstract pov, while otoh it may be viewed as
questionable when at the same time setting to a very small value (which
will then be upped to the minimum acceptable one) is permitted. At the
very least this guards against emptying of the pool where active shadows
would be allocated from (which isn't a problem on HAP as there apart
from the allocations through hap_alloc_p2m_page() the only thing coming
from the pool are the monitor tables of each vCPU, which set-allocation
wouldn't attempt to free).

Jan

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Julien Grall]

Hi Jan,

On 27/10/2022 07:56, Jan Beulich wrote:
> On 26.10.2022 23:24, Julien Grall wrote:
>> On 26/10/2022 20:22, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/mm/hap/hap.c
>>>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>>>> @@ -345,6 +345,16 @@ unsigned int hap_get_allocation(struct domain *d)
>>>>>                + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0));
>>>>>    }
>>>>>    
>>>>> +int hap_get_allocation_bytes(struct domain *d, uint64_t *size)
>>>>> +{
>>>>> +    unsigned long pages = (d->arch.paging.hap.total_pages +
>>>>> +                           d->arch.paging.hap.p2m_pages);
>>>> Unlike for Arm no ACCESS_ONCE() here? Also the addition can in
>>>> principle overflow, because being done only in 32 bits.
>>>
>>> I'm not actually convinced ARM needs ACCESS_ONCE() to begin with.  I
>>> can't see any legal transformation of that logic which could result in a
>>> torn load.
>>
>> AFAIU, ACCESS_ONCE() is not only about torn load but also making sure
>> that the compiler will only read the value once.
>>
>> When LTO is enabled (not yet supported) in Xen, can we guarantee the
>> compiler will not try to access total_pages twice (obviously it would be
>> caller dependent)?
> 
> Aren't all accesses (supposed to be) under paging lock? At which point
> there's no issue with multiple (or torn) accesses?

Not in the current code base for Arm. I haven't checked whether this is 
the case with the new version.

If it is suitably locked, then I think we should remove all the 
ACCESS_ONCE() and add an ASSERT(spin_is_locked(...)) to make clear this 
should be called with the lock held.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]

On Thu, Oct 27, 2022 at 8:12 AM Jan Beulich <jbeulich@suse.com> wrote:

> On 26.10.2022 21:22, Andrew Cooper wrote:
> > On 26/10/2022 14:42, Jan Beulich wrote:
>


> > paging isn't a great name.  While it's what we call the infrastructure
> > in x86, it has nothing to do with paging things out to disk (the thing
> > everyone associates the name with), nor the xenpaging infrastructure
> > (Xen's version of what OS paging supposedly means).
>
> Okay, "paging" can be somewhat misleading. But "p2m" also doesn't fit
> the use(s) on x86. Yet we'd like to use a name clearly better than the
> previous (and yet more wrong/misleading) "shadow". I have to admit that
> I can't think of any other sensible name, and among the ones discussed
> I still think "paging" is the one coming closest despite the
> generally different meaning of the word elsewhere.
>

Inside the world of operating systems / hypervisors, "paging" has always
meant "things related to a pagetable"; this includes "paging out to disk".
In fact, the latter already has a perfectly good name -- "swap" (e.g., swap
file, swappiness, hypervisor swap).

Grep for "paging" inside of Xen.  We have the paging lock, paging modes,
nested paging, and so on.  There's absolutely no reason to start thinking
of "paging" as exclusively meaning "hypervisor swap".

[ A bunch of stuff about using bytes as a unit size]

> This is going to be a reoccurring theme through fixing the ABIs.  Its
> > one of a several areas where there is objectively one right answer, both
> > in terms of ease of use, and compatibility to future circumstances.
>
> Well, I wouldn't say using whatever base granularity as a unit is
> "objectively" less right.
>

Personally I don't think bytes or pages either have a particular advantage:

* Using bytes
 - Advantage: Can always use the same number regardless of the underlying
page size
 - Disadvantage: "Trap" where if you forget to check the page size, you
might accidentally pass an invalid input.  Or to put it differently, most
"reasonable-looking" numbers are actually invalid (since most numbers
aren't page-aligned)/
* Using pages
 - Advantage: No need to check page alignment in HV, no accidentally
invalid input
 - Disadvantage: Caller must check page size and do a shift on every call

What would personally tip me one way or the other is consistency with other
hypercalls.  If most of our hypercalls (or even most of our MM hypercalls)
use bytes, then I'd lean towards bytes.  Whereas if most of our hypercalls
use pages, I'd lean towards pages.

 -George

[-- Attachment #2: Type: text/html, Size: 3643 bytes --]

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Jan Beulich]

On 28.10.2022 17:27, George Dunlap wrote:
> On Thu, Oct 27, 2022 at 8:12 AM Jan Beulich <jbeulich@suse.com> wrote:
> 
>> On 26.10.2022 21:22, Andrew Cooper wrote:
>>> On 26/10/2022 14:42, Jan Beulich wrote:
>>
> 
> 
>>> paging isn't a great name.  While it's what we call the infrastructure
>>> in x86, it has nothing to do with paging things out to disk (the thing
>>> everyone associates the name with), nor the xenpaging infrastructure
>>> (Xen's version of what OS paging supposedly means).
>>
>> Okay, "paging" can be somewhat misleading. But "p2m" also doesn't fit
>> the use(s) on x86. Yet we'd like to use a name clearly better than the
>> previous (and yet more wrong/misleading) "shadow". I have to admit that
>> I can't think of any other sensible name, and among the ones discussed
>> I still think "paging" is the one coming closest despite the
>> generally different meaning of the word elsewhere.
>>
> 
> Inside the world of operating systems / hypervisors, "paging" has always
> meant "things related to a pagetable"; this includes "paging out to disk".
> In fact, the latter already has a perfectly good name -- "swap" (e.g., swap
> file, swappiness, hypervisor swap).
> 
> Grep for "paging" inside of Xen.  We have the paging lock, paging modes,
> nested paging, and so on.  There's absolutely no reason to start thinking
> of "paging" as exclusively meaning "hypervisor swap".

Just to clarify: You actually support my thinking that "paging" is an okay
term to use here? I ask because, perhaps merely because of not being a
native speaker, to me content and wording suggest different things: The
former appears to support my response to Andrew, while the latter reads to
me as if you were objecting.

Jan

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[George Dunlap]

[-- Attachment #1.1: Type: text/plain, Size: 2252 bytes --]



> On 31 Oct 2022, at 09:26, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 28.10.2022 17:27, George Dunlap wrote:
>> On Thu, Oct 27, 2022 at 8:12 AM Jan Beulich <jbeulich@suse.com> wrote:
>> 
>>> On 26.10.2022 21:22, Andrew Cooper wrote:
>>>> On 26/10/2022 14:42, Jan Beulich wrote:
>>> 
>> 
>> 
>>>> paging isn't a great name. While it's what we call the infrastructure
>>>> in x86, it has nothing to do with paging things out to disk (the thing
>>>> everyone associates the name with), nor the xenpaging infrastructure
>>>> (Xen's version of what OS paging supposedly means).
>>> 
>>> Okay, "paging" can be somewhat misleading. But "p2m" also doesn't fit
>>> the use(s) on x86. Yet we'd like to use a name clearly better than the
>>> previous (and yet more wrong/misleading) "shadow". I have to admit that
>>> I can't think of any other sensible name, and among the ones discussed
>>> I still think "paging" is the one coming closest despite the
>>> generally different meaning of the word elsewhere.
>>> 
>> 
>> Inside the world of operating systems / hypervisors, "paging" has always
>> meant "things related to a pagetable"; this includes "paging out to disk".
>> In fact, the latter already has a perfectly good name -- "swap" (e.g., swap
>> file, swappiness, hypervisor swap).
>> 
>> Grep for "paging" inside of Xen. We have the paging lock, paging modes,
>> nested paging, and so on. There's absolutely no reason to start thinking
>> of "paging" as exclusively meaning "hypervisor swap".
> 
> Just to clarify: You actually support my thinking that "paging" is an okay
> term to use here? I ask because, perhaps merely because of not being a
> native speaker, to me content and wording suggest different things: The
> former appears to support my response to Andrew, while the latter reads to
> me as if you were objecting.

Sorry, the tone was “objecting” because it was directed mainly at Andrew’s arguments.  I thought about replying only to his mail, but it seemed like since I was clearly “joining the discussion”, it would make more sense to quote you too.  I could probably have made it more clear by leading with something like, “I tend to agree with Jan here. …”

 -George


[-- Attachment #1.2: Type: text/html, Size: 8407 bytes --]

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 4/4] xen/arm: Correct the p2m pool size calculations

[Henry Wang]

Hi Andrew,

> -----Original Message-----
> Subject: [PATCH 4/4] xen/arm: Correct the p2m pool size calculations
> 
> Allocating or freeing p2m pages doesn't alter the size of the mempool; only
> the split between free and used pages.
> 
> Right now, the hypercalls operate on the free subset of the pool, meaning
> that
> XEN_DOMCTL_get_p2m_mempool_size varies with time as the guest shuffles
> its
> physmap, and XEN_DOMCTL_set_p2m_mempool_size ignores the used
> subset of the
> pool and lets the guest grow unbounded.
> 
> This fixes test-p2m-pool on ARM so that the behaviour matches x86.
> 
> This is part of XSA-409 / CVE-2022-33747.
> 
> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M
> pool")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Hi Arm maintainers, may I ask for a reviewed-by/ack from you for the
correctness of the code in the release? Thank you very much!

Kind regards,
Henry

Re: [PATCH 4/4] xen/arm: Correct the p2m pool size calculations

[Julien Grall]

On 11/11/2022 10:11, Henry Wang wrote:
>> -----Original Message-----
>> Subject: [PATCH 4/4] xen/arm: Correct the p2m pool size calculations
>>
>> Allocating or freeing p2m pages doesn't alter the size of the mempool; only
>> the split between free and used pages.
>>
>> Right now, the hypercalls operate on the free subset of the pool, meaning
>> that
>> XEN_DOMCTL_get_p2m_mempool_size varies with time as the guest shuffles
>> its
>> physmap, and XEN_DOMCTL_set_p2m_mempool_size ignores the used
>> subset of the
>> pool and lets the guest grow unbounded.
>>
>> This fixes test-p2m-pool on ARM so that the behaviour matches x86.
>>
>> This is part of XSA-409 / CVE-2022-33747.
>>
>> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M
>> pool")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> 
> Hi Arm maintainers, may I ask for a reviewed-by/ack from you for the
> correctness of the code in the release? Thank you very much!

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

Cheers,

-- 
Julien Grall

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3559 bytes --]

On Fri, 28 Oct 2022, George Dunlap wrote:
> On Thu, Oct 27, 2022 at 8:12 AM Jan Beulich <jbeulich@suse.com> wrote:
>       On 26.10.2022 21:22, Andrew Cooper wrote:
>       > On 26/10/2022 14:42, Jan Beulich wrote:
> 
>  
>       > paging isn't a great name.  While it's what we call the infrastructure
>       > in x86, it has nothing to do with paging things out to disk (the thing
>       > everyone associates the name with), nor the xenpaging infrastructure
>       > (Xen's version of what OS paging supposedly means).
> 
>       Okay, "paging" can be somewhat misleading. But "p2m" also doesn't fit
>       the use(s) on x86. Yet we'd like to use a name clearly better than the
>       previous (and yet more wrong/misleading) "shadow". I have to admit that
>       I can't think of any other sensible name, and among the ones discussed
>       I still think "paging" is the one coming closest despite the
>       generally different meaning of the word elsewhere.
> 
> 
> Inside the world of operating systems / hypervisors, "paging" has always meant "things related to a pagetable"; this includes "paging out
> to disk".  In fact, the latter already has a perfectly good name -- "swap" (e.g., swap file, swappiness, hypervisor swap).
> 
> Grep for "paging" inside of Xen.  We have the paging lock, paging modes, nested paging, and so on.  There's absolutely no reason to start
> thinking of "paging" as exclusively meaning "hypervisor swap".
>  
> [ A bunch of stuff about using bytes as a unit size]
> 
>       > This is going to be a reoccurring theme through fixing the ABIs.  Its
>       > one of a several areas where there is objectively one right answer, both
>       > in terms of ease of use, and compatibility to future circumstances.
> 
>       Well, I wouldn't say using whatever base granularity as a unit is
>       "objectively" less right.
> 
> 
> Personally I don't think bytes or pages either have a particular advantage:
> 
> * Using bytes
>  - Advantage: Can always use the same number regardless of the underlying page size
>  - Disadvantage: "Trap" where if you forget to check the page size, you might accidentally pass an invalid input.  Or to put it
> differently, most "reasonable-looking" numbers are actually invalid (since most numbers aren't page-aligned)/
> * Using pages
>  - Advantage: No need to check page alignment in HV, no accidentally invalid input
>  - Disadvantage: Caller must check page size and do a shift on every call
> 
> What would personally tip me one way or the other is consistency with other hypercalls.  If most of our hypercalls (or even most of our MM
> hypercalls) use bytes, then I'd lean towards bytes.  Whereas if most of our hypercalls use pages, I'd lean towards pages.


Joining the discussion late to try to move things forward.

Let me premise that I don't have a strong feeling either way, but I
think it would be clearer to use "bytes" instead of "pages" as argument.
The reason is that with pages you are never sure of the actual
granularity. Is it 4K? 16K? 64K? Especially considering that hypervisor
pages can be of different size than guest pages. In theory you could
have a situation where Xen uses 4K, Dom0 uses 16K and domU uses 64K, or
any combination of the three. With bytes, at least you know the actual
size.

If we use "bytes" as argument, then it also makes sense not to use the
word "pages" in the hypercall name.

That said, any name would work and both bytes and pages would work, so
I would leave it to the contributor who is doing the work to choose.

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 9477 bytes --]

On Wed, 26 Oct 2022, Andrew Cooper wrote:
> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
> 
> First of all, with ARM borrowing x86's implementation, the logic to set the
> pool size should have been common, not duplicated.  Introduce
> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
> libxl/xl can reasonably function without the ability to query the pool size...
> 
> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
> replaced with a working and unit tested interface.
> 
> This is part of XSA-409 / CVE-2022-33747.

Genuine question: I can see this patch removes the implementation of
XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both
ARM and x86) to the new hypercall.

Why keep the old hypercall (XEN_DOMCTL_shadow_op and
XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)?

Is that because it was only recently implemented? And not actually
present in any past Xen release?

If so, please add a note about this in the commit message. Also, if that
is the case, I think this patch series should go in 4.17. If it is too
late to get it in before the release, then we should backport it to 4.17
as soon as possible. That's because ideally we want to keep the
hypercall interface changes down to a minimum.


> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Xen Security Team <security@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Henry Wang <Henry.Wang@arm.com>
> CC: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libs/light/libxl_arm.c      | 14 +----------
>  tools/libs/light/libxl_dom.c      | 19 ++++++++++++++
>  tools/libs/light/libxl_internal.h |  3 +++
>  tools/libs/light/libxl_x86.c      | 15 ++---------
>  xen/arch/arm/domctl.c             | 53 ---------------------------------------
>  xen/arch/arm/include/asm/p2m.h    |  1 -
>  xen/arch/arm/p2m.c                |  8 ------
>  7 files changed, 25 insertions(+), 88 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 2a5e93c28403..2f5615263543 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -209,19 +209,7 @@ int libxl__arch_domain_create(libxl__gc *gc,
>                                libxl__domain_build_state *state,
>                                uint32_t domid)
>  {
> -    libxl_ctx *ctx = libxl__gc_owner(gc);
> -    unsigned int shadow_mb = DIV_ROUNDUP(d_config->b_info.shadow_memkb, 1024);
> -
> -    int r = xc_shadow_control(ctx->xch, domid,
> -                              XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
> -                              &shadow_mb, 0);
> -    if (r) {
> -        LOGED(ERROR, domid,
> -              "Failed to set %u MiB shadow allocation", shadow_mb);
> -        return ERROR_FAIL;
> -    }
> -
> -    return 0;
> +    return libxl__domain_set_p2m_pool_size(gc, d_config, domid);
>  }
>  
>  int libxl__arch_extra_memory(libxl__gc *gc,
> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 2abaab439c4f..f93b221f1c1f 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -1448,6 +1448,25 @@ int libxl_userdata_unlink(libxl_ctx *ctx, uint32_t domid,
>      return rc;
>  }
>  
> +int libxl__domain_set_p2m_pool_size(
> +    libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    uint64_t shadow_mem;
> +
> +    shadow_mem = d_config->b_info.shadow_memkb;
> +    shadow_mem <<= 10;
> +
> +    int r = xc_get_p2m_mempool_size(ctx->xch, domid, &shadow_mem);
> +    if (r) {
> +        LOGED(ERROR, domid,
> +              "Failed to set p2m pool size to %"PRIu64"kB", shadow_mem);
> +        return ERROR_FAIL;
> +    }
> +
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
> index cb9e8b3b8b5a..f31164bc6c0d 100644
> --- a/tools/libs/light/libxl_internal.h
> +++ b/tools/libs/light/libxl_internal.h
> @@ -4864,6 +4864,9 @@ int libxl__is_domid_recent(libxl__gc *gc, uint32_t domid, bool *recent);
>  /* os-specific implementation of setresuid() */
>  int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid);
>  
> +_hidden int libxl__domain_set_p2m_pool_size(
> +    libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid);
> +
>  #endif
>  
>  /*
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 7c5ee74443e5..99aba51d05df 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -538,20 +538,9 @@ int libxl__arch_domain_create(libxl__gc *gc,
>          xc_domain_set_time_offset(ctx->xch, domid, rtc_timeoffset);
>  
>      if (d_config->b_info.type != LIBXL_DOMAIN_TYPE_PV) {
> -        unsigned int shadow_mb = DIV_ROUNDUP(d_config->b_info.shadow_memkb,
> -                                             1024);
> -        int r = xc_shadow_control(ctx->xch, domid,
> -                                  XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
> -                                  &shadow_mb, 0);
> -
> -        if (r) {
> -            LOGED(ERROR, domid,
> -                  "Failed to set %u MiB %s allocation",
> -                  shadow_mb,
> -                  libxl_defbool_val(d_config->c_info.hap) ? "HAP" : "shadow");
> -            ret = ERROR_FAIL;
> +        ret = libxl__domain_set_p2m_pool_size(gc, d_config, domid);
> +        if (ret)
>              goto out;
> -        }
>      }
>  
>      if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index c8fdeb124084..1baf25c3d98b 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -47,64 +47,11 @@ static int handle_vuart_init(struct domain *d,
>      return rc;
>  }
>  
> -static long p2m_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
> -                       XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> -{
> -    long rc;
> -    bool preempted = false;
> -
> -    if ( unlikely(d == current->domain) )
> -    {
> -        printk(XENLOG_ERR "Tried to do a p2m domctl op on itself.\n");
> -        return -EINVAL;
> -    }
> -
> -    if ( unlikely(d->is_dying) )
> -    {
> -        printk(XENLOG_ERR "Tried to do a p2m domctl op on dying domain %u\n",
> -               d->domain_id);
> -        return -EINVAL;
> -    }
> -
> -    switch ( sc->op )
> -    {
> -    case XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION:
> -    {
> -        /* Allow and handle preemption */
> -        spin_lock(&d->arch.paging.lock);
> -        rc = p2m_set_allocation(d, sc->mb << (20 - PAGE_SHIFT), &preempted);
> -        spin_unlock(&d->arch.paging.lock);
> -
> -        if ( preempted )
> -            /* Not finished. Set up to re-run the call. */
> -            rc = hypercall_create_continuation(__HYPERVISOR_domctl, "h",
> -                                               u_domctl);
> -        else
> -            /* Finished. Return the new allocation. */
> -            sc->mb = p2m_get_allocation(d);
> -
> -        return rc;
> -    }
> -    case XEN_DOMCTL_SHADOW_OP_GET_ALLOCATION:
> -    {
> -        sc->mb = p2m_get_allocation(d);
> -        return 0;
> -    }
> -    default:
> -    {
> -        printk(XENLOG_ERR "Bad p2m domctl op %u\n", sc->op);
> -        return -EINVAL;
> -    }
> -    }
> -}
> -
>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>  {
>      switch ( domctl->cmd )
>      {
> -    case XEN_DOMCTL_shadow_op:
> -        return p2m_domctl(d, &domctl->u.shadow_op, u_domctl);
>      case XEN_DOMCTL_cacheflush:
>      {
>          gfn_t s = _gfn(domctl->u.cacheflush.start_pfn);
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> index c8f14d13c2c5..91df922e1c9f 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -222,7 +222,6 @@ void p2m_restore_state(struct vcpu *n);
>  /* Print debugging/statistial info about a domain's p2m */
>  void p2m_dump_info(struct domain *d);
>  
> -unsigned int p2m_get_allocation(struct domain *d);
>  int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted);
>  int p2m_teardown_allocation(struct domain *d);
>  
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 4607cde6f0b8..92b678cf0d09 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -92,14 +92,6 @@ static void p2m_free_page(struct domain *d, struct page_info *pg)
>      spin_unlock(&d->arch.paging.lock);
>  }
>  
> -/* Return the size of the pool, rounded up to the nearest MB */
> -unsigned int p2m_get_allocation(struct domain *d)
> -{
> -    unsigned long nr_pages = ACCESS_ONCE(d->arch.paging.p2m_total_pages);
> -
> -    return ROUNDUP(nr_pages, 1 << (20 - PAGE_SHIFT)) >> (20 - PAGE_SHIFT);
> -}
> -
>  /* Return the size of the pool, in bytes. */
>  int arch_get_p2m_mempool_size(struct domain *d, uint64_t *size)
>  {
> -- 
> 2.11.0
> 

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Andrew Cooper]

On 16/11/2022 01:37, Stefano Stabellini wrote:
> On Wed, 26 Oct 2022, Andrew Cooper wrote:
>> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
>> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
>>
>> First of all, with ARM borrowing x86's implementation, the logic to set the
>> pool size should have been common, not duplicated.  Introduce
>> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
>> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
>> libxl/xl can reasonably function without the ability to query the pool size...
>>
>> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
>> replaced with a working and unit tested interface.
>>
>> This is part of XSA-409 / CVE-2022-33747.
> Genuine question: I can see this patch removes the implementation of
> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both
> ARM and x86) to the new hypercall.
>
> Why keep the old hypercall (XEN_DOMCTL_shadow_op and
> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)?
>
> Is that because it was only recently implemented? And not actually
> present in any past Xen release?
>
> If so, please add a note about this in the commit message. Also, if that
> is the case, I think this patch series should go in 4.17. If it is too
> late to get it in before the release, then we should backport it to 4.17
> as soon as possible. That's because ideally we want to keep the
> hypercall interface changes down to a minimum.

On ARM, the hypercall has existed for a little over 4 weeks, and isn't
in any released version of Xen (yet).

On x86, the hypercall has existed for more than a decade, and has known
out-of-tree users.  It needs to be deprecated properly, which in this
case means "phased out in the 4.18 cycle once known callers have been
adapted to the new hypercall".

~Andrew

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2134 bytes --]

On Wed, 16 Nov 2022, Andrew Cooper wrote:
> On 16/11/2022 01:37, Stefano Stabellini wrote:
> > On Wed, 26 Oct 2022, Andrew Cooper wrote:
> >> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
> >> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
> >>
> >> First of all, with ARM borrowing x86's implementation, the logic to set the
> >> pool size should have been common, not duplicated.  Introduce
> >> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
> >> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
> >> libxl/xl can reasonably function without the ability to query the pool size...
> >>
> >> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
> >> replaced with a working and unit tested interface.
> >>
> >> This is part of XSA-409 / CVE-2022-33747.
> > Genuine question: I can see this patch removes the implementation of
> > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both
> > ARM and x86) to the new hypercall.
> >
> > Why keep the old hypercall (XEN_DOMCTL_shadow_op and
> > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)?
> >
> > Is that because it was only recently implemented? And not actually
> > present in any past Xen release?
> >
> > If so, please add a note about this in the commit message. Also, if that
> > is the case, I think this patch series should go in 4.17. If it is too
> > late to get it in before the release, then we should backport it to 4.17
> > as soon as possible. That's because ideally we want to keep the
> > hypercall interface changes down to a minimum.
> 
> On ARM, the hypercall has existed for a little over 4 weeks, and isn't
> in any released version of Xen (yet).
> 
> On x86, the hypercall has existed for more than a decade, and has known
> out-of-tree users.  It needs to be deprecated properly, which in this
> case means "phased out in the 4.18 cycle once known callers have been
> adapted to the new hypercall".

Understoon. Then I am in favor of getting all 4 patches in 4.17, either
before the release or via backports.

RE: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Henry Wang]

Hi Andrew and Stefano,

Thanks for pushing things forward!

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op;
> use p2m mempool hypercalls
> 
> On Wed, 16 Nov 2022, Andrew Cooper wrote:
> > On 16/11/2022 01:37, Stefano Stabellini wrote:
> > > On Wed, 26 Oct 2022, Andrew Cooper wrote:
> > >> This reverts most of commit
> cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
> > >> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
> > >>
> > >> First of all, with ARM borrowing x86's implementation, the logic to set
> the
> > >> pool size should have been common, not duplicated.  Introduce
> > >> libxl__domain_set_p2m_pool_size() as a shared implementation, and
> use it from
> > >> the ARM and x86 paths.  It is left as an exercise to the reader to judge
> how
> > >> libxl/xl can reasonably function without the ability to query the pool
> size...
> > >>
> > >> Remove ARM's p2m_domctl() infrastructure now the functioanlity has
> been
> > >> replaced with a working and unit tested interface.
> > >>
> > >> This is part of XSA-409 / CVE-2022-33747.
> > > Genuine question: I can see this patch removes the implementation of
> > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches
> libxl (both
> > > ARM and x86) to the new hypercall.
> > >
> > > Why keep the old hypercall (XEN_DOMCTL_shadow_op and
> > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86
> (not on ARM)?
> > >
> > > Is that because it was only recently implemented? And not actually
> > > present in any past Xen release?
> > >
> > > If so, please add a note about this in the commit message. Also, if that
> > > is the case, I think this patch series should go in 4.17. If it is too
> > > late to get it in before the release, then we should backport it to 4.17
> > > as soon as possible. That's because ideally we want to keep the
> > > hypercall interface changes down to a minimum.
> >
> > On ARM, the hypercall has existed for a little over 4 weeks, and isn't
> > in any released version of Xen (yet).
> >
> > On x86, the hypercall has existed for more than a decade, and has known
> > out-of-tree users.  It needs to be deprecated properly, which in this
> > case means "phased out in the 4.18 cycle once known callers have been
> > adapted to the new hypercall".
> 
> Understoon. Then I am in favor of getting all 4 patches in 4.17, either
> before the release or via backports.

Sorry - today it took me a little bit longer to get the office, so hopefully
I still jumped into discussion on time.

About this series, I don't have strong objection to taking all 4 patches, so
if this series can have proper review/agreements by this weekend, feel free
to add my release-ack for the patches.

However, if we cannot sort out all 4 patches, I think at least patch #4 should
go into 4.17 (with a commit message adjustment). The patch #4 already has
proper tags from Arm maintainer and me.

Kind regards,
Henry

Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Jan Beulich]

On 16.11.2022 02:19, Stefano Stabellini wrote:
> On Fri, 28 Oct 2022, George Dunlap wrote:
>> On Thu, Oct 27, 2022 at 8:12 AM Jan Beulich <jbeulich@suse.com> wrote:
>>       On 26.10.2022 21:22, Andrew Cooper wrote:
>>       > On 26/10/2022 14:42, Jan Beulich wrote:
>>
>>  
>>       > paging isn't a great name.  While it's what we call the infrastructure
>>       > in x86, it has nothing to do with paging things out to disk (the thing
>>       > everyone associates the name with), nor the xenpaging infrastructure
>>       > (Xen's version of what OS paging supposedly means).
>>
>>       Okay, "paging" can be somewhat misleading. But "p2m" also doesn't fit
>>       the use(s) on x86. Yet we'd like to use a name clearly better than the
>>       previous (and yet more wrong/misleading) "shadow". I have to admit that
>>       I can't think of any other sensible name, and among the ones discussed
>>       I still think "paging" is the one coming closest despite the
>>       generally different meaning of the word elsewhere.
>>
>>
>> Inside the world of operating systems / hypervisors, "paging" has always meant "things related to a pagetable"; this includes "paging out
>> to disk".  In fact, the latter already has a perfectly good name -- "swap" (e.g., swap file, swappiness, hypervisor swap).
>>
>> Grep for "paging" inside of Xen.  We have the paging lock, paging modes, nested paging, and so on.  There's absolutely no reason to start
>> thinking of "paging" as exclusively meaning "hypervisor swap".
>>  
>> [ A bunch of stuff about using bytes as a unit size]
>>
>>       > This is going to be a reoccurring theme through fixing the ABIs.  Its
>>       > one of a several areas where there is objectively one right answer, both
>>       > in terms of ease of use, and compatibility to future circumstances.
>>
>>       Well, I wouldn't say using whatever base granularity as a unit is
>>       "objectively" less right.
>>
>>
>> Personally I don't think bytes or pages either have a particular advantage:
>>
>> * Using bytes
>>  - Advantage: Can always use the same number regardless of the underlying page size
>>  - Disadvantage: "Trap" where if you forget to check the page size, you might accidentally pass an invalid input.  Or to put it
>> differently, most "reasonable-looking" numbers are actually invalid (since most numbers aren't page-aligned)/
>> * Using pages
>>  - Advantage: No need to check page alignment in HV, no accidentally invalid input
>>  - Disadvantage: Caller must check page size and do a shift on every call
>>
>> What would personally tip me one way or the other is consistency with other hypercalls.  If most of our hypercalls (or even most of our MM
>> hypercalls) use bytes, then I'd lean towards bytes.  Whereas if most of our hypercalls use pages, I'd lean towards pages.
> 
> 
> Joining the discussion late to try to move things forward.
> 
> Let me premise that I don't have a strong feeling either way, but I
> think it would be clearer to use "bytes" instead of "pages" as argument.
> The reason is that with pages you are never sure of the actual
> granularity. Is it 4K? 16K? 64K? Especially considering that hypervisor
> pages can be of different size than guest pages. In theory you could
> have a situation where Xen uses 4K, Dom0 uses 16K and domU uses 64K, or
> any combination of the three. With bytes, at least you know the actual
> size.
> 
> If we use "bytes" as argument, then it also makes sense not to use the
> word "pages" in the hypercall name.
> 
> That said, any name would work and both bytes and pages would work, so
> I would leave it to the contributor who is doing the work to choose.

FAOD: There was no suggestion to use "pages" in the name; it was "paging"
which was suggested.

Jan

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Jan Beulich]

On 16.11.2022 03:00, Stefano Stabellini wrote:
> On Wed, 16 Nov 2022, Andrew Cooper wrote:
>> On 16/11/2022 01:37, Stefano Stabellini wrote:
>>> On Wed, 26 Oct 2022, Andrew Cooper wrote:
>>>> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
>>>> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
>>>>
>>>> First of all, with ARM borrowing x86's implementation, the logic to set the
>>>> pool size should have been common, not duplicated.  Introduce
>>>> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
>>>> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
>>>> libxl/xl can reasonably function without the ability to query the pool size...
>>>>
>>>> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
>>>> replaced with a working and unit tested interface.
>>>>
>>>> This is part of XSA-409 / CVE-2022-33747.
>>> Genuine question: I can see this patch removes the implementation of
>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both
>>> ARM and x86) to the new hypercall.
>>>
>>> Why keep the old hypercall (XEN_DOMCTL_shadow_op and
>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)?
>>>
>>> Is that because it was only recently implemented? And not actually
>>> present in any past Xen release?
>>>
>>> If so, please add a note about this in the commit message. Also, if that
>>> is the case, I think this patch series should go in 4.17. If it is too
>>> late to get it in before the release, then we should backport it to 4.17
>>> as soon as possible. That's because ideally we want to keep the
>>> hypercall interface changes down to a minimum.
>>
>> On ARM, the hypercall has existed for a little over 4 weeks, and isn't
>> in any released version of Xen (yet).
>>
>> On x86, the hypercall has existed for more than a decade, and has known
>> out-of-tree users.  It needs to be deprecated properly, which in this
>> case means "phased out in the 4.18 cycle once known callers have been
>> adapted to the new hypercall".
> 
> Understoon. Then I am in favor of getting all 4 patches in 4.17, either
> before the release or via backports.

Removing something from the domctl interface generally requires bumping
the interface version, so some extra care may need applying if such an
interface change was to be backported to any stable branch.

Jan

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Andrew Cooper]

On 16/11/2022 08:30, Jan Beulich wrote:
> On 16.11.2022 03:00, Stefano Stabellini wrote:
>> On Wed, 16 Nov 2022, Andrew Cooper wrote:
>>> On 16/11/2022 01:37, Stefano Stabellini wrote:
>>>> On Wed, 26 Oct 2022, Andrew Cooper wrote:
>>>>> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
>>>>> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
>>>>>
>>>>> First of all, with ARM borrowing x86's implementation, the logic to set the
>>>>> pool size should have been common, not duplicated.  Introduce
>>>>> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
>>>>> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
>>>>> libxl/xl can reasonably function without the ability to query the pool size...
>>>>>
>>>>> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
>>>>> replaced with a working and unit tested interface.
>>>>>
>>>>> This is part of XSA-409 / CVE-2022-33747.
>>>> Genuine question: I can see this patch removes the implementation of
>>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both
>>>> ARM and x86) to the new hypercall.
>>>>
>>>> Why keep the old hypercall (XEN_DOMCTL_shadow_op and
>>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)?
>>>>
>>>> Is that because it was only recently implemented? And not actually
>>>> present in any past Xen release?
>>>>
>>>> If so, please add a note about this in the commit message. Also, if that
>>>> is the case, I think this patch series should go in 4.17. If it is too
>>>> late to get it in before the release, then we should backport it to 4.17
>>>> as soon as possible. That's because ideally we want to keep the
>>>> hypercall interface changes down to a minimum.
>>> On ARM, the hypercall has existed for a little over 4 weeks, and isn't
>>> in any released version of Xen (yet).
>>>
>>> On x86, the hypercall has existed for more than a decade, and has known
>>> out-of-tree users.  It needs to be deprecated properly, which in this
>>> case means "phased out in the 4.18 cycle once known callers have been
>>> adapted to the new hypercall".
>> Understoon. Then I am in favor of getting all 4 patches in 4.17, either
>> before the release or via backports.
> Removing something from the domctl interface generally requires bumping
> the interface version, so some extra care may need applying if such an
> interface change was to be backported to any stable branch.

To be clear, I have no plans to remove the x86 "older" interface in this
patch series.  It will definitely break out of tree users.

In the 4.18 timeframe, we can see about retiring the older hypercalls,
but as a non-backportable change.

~Andrew

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2999 bytes --]

On Wed, 16 Nov 2022, Andrew Cooper wrote:
> On 16/11/2022 08:30, Jan Beulich wrote:
> > On 16.11.2022 03:00, Stefano Stabellini wrote:
> >> On Wed, 16 Nov 2022, Andrew Cooper wrote:
> >>> On 16/11/2022 01:37, Stefano Stabellini wrote:
> >>>> On Wed, 26 Oct 2022, Andrew Cooper wrote:
> >>>>> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
> >>>>> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
> >>>>>
> >>>>> First of all, with ARM borrowing x86's implementation, the logic to set the
> >>>>> pool size should have been common, not duplicated.  Introduce
> >>>>> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
> >>>>> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
> >>>>> libxl/xl can reasonably function without the ability to query the pool size...
> >>>>>
> >>>>> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
> >>>>> replaced with a working and unit tested interface.
> >>>>>
> >>>>> This is part of XSA-409 / CVE-2022-33747.
> >>>> Genuine question: I can see this patch removes the implementation of
> >>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both
> >>>> ARM and x86) to the new hypercall.
> >>>>
> >>>> Why keep the old hypercall (XEN_DOMCTL_shadow_op and
> >>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)?
> >>>>
> >>>> Is that because it was only recently implemented? And not actually
> >>>> present in any past Xen release?
> >>>>
> >>>> If so, please add a note about this in the commit message. Also, if that
> >>>> is the case, I think this patch series should go in 4.17. If it is too
> >>>> late to get it in before the release, then we should backport it to 4.17
> >>>> as soon as possible. That's because ideally we want to keep the
> >>>> hypercall interface changes down to a minimum.
> >>> On ARM, the hypercall has existed for a little over 4 weeks, and isn't
> >>> in any released version of Xen (yet).
> >>>
> >>> On x86, the hypercall has existed for more than a decade, and has known
> >>> out-of-tree users.  It needs to be deprecated properly, which in this
> >>> case means "phased out in the 4.18 cycle once known callers have been
> >>> adapted to the new hypercall".
> >> Understoon. Then I am in favor of getting all 4 patches in 4.17, either
> >> before the release or via backports.
> > Removing something from the domctl interface generally requires bumping
> > the interface version, so some extra care may need applying if such an
> > interface change was to be backported to any stable branch.
> 
> To be clear, I have no plans to remove the x86 "older" interface in this
> patch series.  It will definitely break out of tree users.
> 
> In the 4.18 timeframe, we can see about retiring the older hypercalls,
> but as a non-backportable change.

For ARM, given that XEN_DOMCTL_shadow_op has not been enabled for long,
maybe we can get away without bumping the interface version?

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Julien Grall]

Hi,

On 16/11/2022 23:44, Stefano Stabellini wrote:
> On Wed, 16 Nov 2022, Andrew Cooper wrote:
>> On 16/11/2022 08:30, Jan Beulich wrote:
>>> On 16.11.2022 03:00, Stefano Stabellini wrote:
>>>> On Wed, 16 Nov 2022, Andrew Cooper wrote:
>>>>> On 16/11/2022 01:37, Stefano Stabellini wrote:
>>>>>> On Wed, 26 Oct 2022, Andrew Cooper wrote:
>>>>>>> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
>>>>>>> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
>>>>>>>
>>>>>>> First of all, with ARM borrowing x86's implementation, the logic to set the
>>>>>>> pool size should have been common, not duplicated.  Introduce
>>>>>>> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
>>>>>>> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
>>>>>>> libxl/xl can reasonably function without the ability to query the pool size...
>>>>>>>
>>>>>>> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
>>>>>>> replaced with a working and unit tested interface.
>>>>>>>
>>>>>>> This is part of XSA-409 / CVE-2022-33747.
>>>>>> Genuine question: I can see this patch removes the implementation of
>>>>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both
>>>>>> ARM and x86) to the new hypercall.
>>>>>>
>>>>>> Why keep the old hypercall (XEN_DOMCTL_shadow_op and
>>>>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)?
>>>>>>
>>>>>> Is that because it was only recently implemented? And not actually
>>>>>> present in any past Xen release?
>>>>>>
>>>>>> If so, please add a note about this in the commit message. Also, if that
>>>>>> is the case, I think this patch series should go in 4.17. If it is too
>>>>>> late to get it in before the release, then we should backport it to 4.17
>>>>>> as soon as possible. That's because ideally we want to keep the
>>>>>> hypercall interface changes down to a minimum.
>>>>> On ARM, the hypercall has existed for a little over 4 weeks, and isn't
>>>>> in any released version of Xen (yet).
>>>>>
>>>>> On x86, the hypercall has existed for more than a decade, and has known
>>>>> out-of-tree users.  It needs to be deprecated properly, which in this
>>>>> case means "phased out in the 4.18 cycle once known callers have been
>>>>> adapted to the new hypercall".
>>>> Understoon. Then I am in favor of getting all 4 patches in 4.17, either
>>>> before the release or via backports.
>>> Removing something from the domctl interface generally requires bumping
>>> the interface version, so some extra care may need applying if such an
>>> interface change was to be backported to any stable branch.
>>
>> To be clear, I have no plans to remove the x86 "older" interface in this
>> patch series.  It will definitely break out of tree users.
>>
>> In the 4.18 timeframe, we can see about retiring the older hypercalls,
>> but as a non-backportable change.
> 
> For ARM, given that XEN_DOMCTL_shadow_op has not been enabled for long,
> maybe we can get away without bumping the interface version?

IMHO how long it was out doesn't matter. Once we do a release, we should 
avoid changing the interface in minor version.

This is because a user may start to rely on it and we don't want to 
break them for minor releases.

Cheers,

-- 
Julien Grall

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3999 bytes --]

On Wed, 16 Nov 2022, Julien Grall wrote:
> On 16/11/2022 23:44, Stefano Stabellini wrote:
> > On Wed, 16 Nov 2022, Andrew Cooper wrote:
> > > On 16/11/2022 08:30, Jan Beulich wrote:
> > > > On 16.11.2022 03:00, Stefano Stabellini wrote:
> > > > > On Wed, 16 Nov 2022, Andrew Cooper wrote:
> > > > > > On 16/11/2022 01:37, Stefano Stabellini wrote:
> > > > > > > On Wed, 26 Oct 2022, Andrew Cooper wrote:
> > > > > > > > This reverts most of commit
> > > > > > > > cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
> > > > > > > > of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
> > > > > > > > 
> > > > > > > > First of all, with ARM borrowing x86's implementation, the logic
> > > > > > > > to set the
> > > > > > > > pool size should have been common, not duplicated.  Introduce
> > > > > > > > libxl__domain_set_p2m_pool_size() as a shared implementation,
> > > > > > > > and use it from
> > > > > > > > the ARM and x86 paths.  It is left as an exercise to the reader
> > > > > > > > to judge how
> > > > > > > > libxl/xl can reasonably function without the ability to query
> > > > > > > > the pool size...
> > > > > > > > 
> > > > > > > > Remove ARM's p2m_domctl() infrastructure now the functioanlity
> > > > > > > > has been
> > > > > > > > replaced with a working and unit tested interface.
> > > > > > > > 
> > > > > > > > This is part of XSA-409 / CVE-2022-33747.
> > > > > > > Genuine question: I can see this patch removes the implementation
> > > > > > > of
> > > > > > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl
> > > > > > > (both
> > > > > > > ARM and x86) to the new hypercall.
> > > > > > > 
> > > > > > > Why keep the old hypercall (XEN_DOMCTL_shadow_op and
> > > > > > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on
> > > > > > > ARM)?
> > > > > > > 
> > > > > > > Is that because it was only recently implemented? And not actually
> > > > > > > present in any past Xen release?
> > > > > > > 
> > > > > > > If so, please add a note about this in the commit message. Also,
> > > > > > > if that
> > > > > > > is the case, I think this patch series should go in 4.17. If it is
> > > > > > > too
> > > > > > > late to get it in before the release, then we should backport it
> > > > > > > to 4.17
> > > > > > > as soon as possible. That's because ideally we want to keep the
> > > > > > > hypercall interface changes down to a minimum.
> > > > > > On ARM, the hypercall has existed for a little over 4 weeks, and
> > > > > > isn't
> > > > > > in any released version of Xen (yet).
> > > > > > 
> > > > > > On x86, the hypercall has existed for more than a decade, and has
> > > > > > known
> > > > > > out-of-tree users.  It needs to be deprecated properly, which in
> > > > > > this
> > > > > > case means "phased out in the 4.18 cycle once known callers have
> > > > > > been
> > > > > > adapted to the new hypercall".
> > > > > Understoon. Then I am in favor of getting all 4 patches in 4.17,
> > > > > either
> > > > > before the release or via backports.
> > > > Removing something from the domctl interface generally requires bumping
> > > > the interface version, so some extra care may need applying if such an
> > > > interface change was to be backported to any stable branch.
> > > 
> > > To be clear, I have no plans to remove the x86 "older" interface in this
> > > patch series.  It will definitely break out of tree users.
> > > 
> > > In the 4.18 timeframe, we can see about retiring the older hypercalls,
> > > but as a non-backportable change.
> > 
> > For ARM, given that XEN_DOMCTL_shadow_op has not been enabled for long,
> > maybe we can get away without bumping the interface version?
> 
> IMHO how long it was out doesn't matter. Once we do a release, we should avoid
> changing the interface in minor version.
> 
> This is because a user may start to rely on it and we don't want to break them
> for minor releases.

We haven't released 4.17 yet, so I take you are referring to a stable
minor release, right?

Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Jan Beulich]

On 17.11.2022 00:41, Andrew Cooper wrote:
> On 16/11/2022 08:30, Jan Beulich wrote:
>> On 16.11.2022 03:00, Stefano Stabellini wrote:
>>> On Wed, 16 Nov 2022, Andrew Cooper wrote:
>>>> On 16/11/2022 01:37, Stefano Stabellini wrote:
>>>>> On Wed, 26 Oct 2022, Andrew Cooper wrote:
>>>>>> This reverts most of commit cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
>>>>>> of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
>>>>>>
>>>>>> First of all, with ARM borrowing x86's implementation, the logic to set the
>>>>>> pool size should have been common, not duplicated.  Introduce
>>>>>> libxl__domain_set_p2m_pool_size() as a shared implementation, and use it from
>>>>>> the ARM and x86 paths.  It is left as an exercise to the reader to judge how
>>>>>> libxl/xl can reasonably function without the ability to query the pool size...
>>>>>>
>>>>>> Remove ARM's p2m_domctl() infrastructure now the functioanlity has been
>>>>>> replaced with a working and unit tested interface.
>>>>>>
>>>>>> This is part of XSA-409 / CVE-2022-33747.
>>>>> Genuine question: I can see this patch removes the implementation of
>>>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl (both
>>>>> ARM and x86) to the new hypercall.
>>>>>
>>>>> Why keep the old hypercall (XEN_DOMCTL_shadow_op and
>>>>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on ARM)?
>>>>>
>>>>> Is that because it was only recently implemented? And not actually
>>>>> present in any past Xen release?
>>>>>
>>>>> If so, please add a note about this in the commit message. Also, if that
>>>>> is the case, I think this patch series should go in 4.17. If it is too
>>>>> late to get it in before the release, then we should backport it to 4.17
>>>>> as soon as possible. That's because ideally we want to keep the
>>>>> hypercall interface changes down to a minimum.
>>>> On ARM, the hypercall has existed for a little over 4 weeks, and isn't
>>>> in any released version of Xen (yet).
>>>>
>>>> On x86, the hypercall has existed for more than a decade, and has known
>>>> out-of-tree users.  It needs to be deprecated properly, which in this
>>>> case means "phased out in the 4.18 cycle once known callers have been
>>>> adapted to the new hypercall".
>>> Understoon. Then I am in favor of getting all 4 patches in 4.17, either
>>> before the release or via backports.
>> Removing something from the domctl interface generally requires bumping
>> the interface version, so some extra care may need applying if such an
>> interface change was to be backported to any stable branch.
> 
> To be clear, I have no plans to remove the x86 "older" interface in this
> patch series.  It will definitely break out of tree users.
> 
> In the 4.18 timeframe, we can see about retiring the older hypercalls,
> but as a non-backportable change.

Sure, but I was referring to the (pretty new) Arm incarnation thereof.

Jan