[PATCH for-4.19? v5 00/10] x86: Make MAX_ALTP2M configurable

[PATCH for-4.19? v5 00/10] x86: Make MAX_ALTP2M configurable

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

This series introduces the ability to configure the maximum number of altp2m
tables during domain creation. Previously, the limits were hardcoded to a
maximum of 10. This change allows for greater flexibility in environments that
require more or fewer altp2m views.

This enhancement is particularly relevant for users leveraging Xen's features
for virtual machine introspection.

Changes since v4:
- Rebased on top of staging (applying Roger's changes).
- Fix mixed tabs/spaces in xenctrl_stubs.c.
- Add missing OCaml bindings for altp2m_opts.
- Substitute altp2m_opts into an unnamed structure. (This is a preparation for
  the next patch that will introduce the `nr` field.)
- altp2m.opts is then shortened to uint16_t and a new field altp2m.nr is added -
  also uint16_t. This value is then verified by libxl to not exceed the maximum
  uint16_t value.

  This puts a hard limit to number of altp2m to 65535, which is enough, at least
  for the time being. Also, altp2m.opts currently uses only 2 bits. Therefore
  I believe this change is justified.
- Introduction of accessor functions for altp2m arrays and refactoring the code
  to use them.
- Added a check to arm/arch_sanitise_domain_config() to disallow creating
  domains with altp2m.nr != 0.
- Added dummy hvm_altp2m_supported() to avoid build errors when CONFIG_HVM is
  disabled.
- Finally, expose altp2m_count to OCaml bindings (and verify both altp2m_opts
  and altp2m_count fit uint16_t).
- I also removed Christian Lindig from the Acked-by, since I think this change
  is significant enough to require a re-review.

Changes since v3:
- Rebased on top of staging (some functions were moved to altp2m.c).
- Re-added the array_index_nospec() where it was removed.

Changes since v2:
- Changed max_altp2m to nr_altp2m.
- Moved arch-dependent check from xen/common/domain.c to xen/arch/x86/domain.c.
- Replaced min(d->nr_altp2m, MAX_EPTP) occurences for just d->nr_altp2m.
- Replaced array_index_nospec(altp2m_idx, ...) for just altp2m_idx.
- Shortened long lines.
- Removed unnecessary comments in altp2m_vcpu_initialise/destroy.
- Moved nr_altp2m field after max_ fields in xen_domctl_createdomain.
- Removed the commit that adjusted the initial allocation of pages from 256
  to 1024. This means that after these patches, technically, the nr_altp2m will
  be capped to (256 - 1 - vcpus - MAX_NESTEDP2M) instead of MAX_EPTP (512).
  Future work will be needed to fix this.

Petr Beneš (10):
  tools/ocaml: Fix mixed tabs/spaces
  tools/ocaml: Add missing ocaml bindings for altp2m_opts
  xen: Refactor altp2m options into a structured format
  tools/xl: Add altp2m_count parameter
  docs/man: Add altp2m_count parameter to the xl.cfg manual
  x86/altp2m: Introduce accessor functions for safer array indexing
  xen: Make the maximum number of altp2m views configurable for x86
  tools/libxl: Activate the altp2m_count feature
  xen/x86: Disallow creating domains with altp2m enabled and altp2m.nr
    == 0
  tools/ocaml: Add altp2m_count parameter

 docs/man/xl.cfg.5.pod.in             |  14 ++++
 tools/golang/xenlight/helpers.gen.go |   2 +
 tools/golang/xenlight/types.gen.go   |   1 +
 tools/include/libxl.h                |   8 ++
 tools/libs/light/libxl_create.c      |  19 ++++-
 tools/libs/light/libxl_types.idl     |   1 +
 tools/ocaml/libs/xc/xenctrl.ml       |   2 +
 tools/ocaml/libs/xc/xenctrl.mli      |   2 +
 tools/ocaml/libs/xc/xenctrl_stubs.c  |  40 +++++++---
 tools/xl/xl_parse.c                  |   9 +++
 xen/arch/arm/domain.c                |   2 +-
 xen/arch/x86/domain.c                |  45 ++++++++---
 xen/arch/x86/hvm/hvm.c               |  10 ++-
 xen/arch/x86/hvm/vmx/vmx.c           |   6 +-
 xen/arch/x86/include/asm/altp2m.h    |  32 ++++++++
 xen/arch/x86/include/asm/domain.h    |   9 ++-
 xen/arch/x86/include/asm/hvm/hvm.h   |   5 ++
 xen/arch/x86/include/asm/p2m.h       |  11 ++-
 xen/arch/x86/mm/altp2m.c             | 110 ++++++++++++++-------------
 xen/arch/x86/mm/hap/hap.c            |  16 ++--
 xen/arch/x86/mm/mem_access.c         |  25 +++---
 xen/arch/x86/mm/mem_sharing.c        |   4 +-
 xen/arch/x86/mm/p2m-ept.c            |  18 ++---
 xen/arch/x86/mm/p2m.c                |  24 +++---
 xen/common/domain.c                  |   1 +
 xen/include/public/domctl.h          |   7 +-
 xen/include/xen/sched.h              |   2 +
 27 files changed, 290 insertions(+), 135 deletions(-)

--
2.34.1

[PATCH for-4.19? v5 01/10] tools/ocaml: Fix mixed tabs/spaces

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

No functional change.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 tools/ocaml/libs/xc/xenctrl_stubs.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index c6da9bb091..e86c455802 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -138,12 +138,12 @@ static void domain_handle_of_uuid_string(xen_domain_handle_t h,
  * integers in the Ocaml ABI for more idiomatic handling.
  */
 static value c_bitmap_to_ocaml_list
-             /* ! */
-             /*
+	     /* ! */
+	     /*
 	      * All calls to this function must be in a form suitable
 	      * for xenctrl_abi_check.  The parsing there is ad-hoc.
 	      */
-             (unsigned int bitmap)
+	         (unsigned int bitmap)
 {
 	CAMLparam0();
 	CAMLlocal2(list, tmp);
@@ -180,8 +180,8 @@ static value c_bitmap_to_ocaml_list
 }

 static unsigned int ocaml_list_to_c_bitmap(value l)
-             /* ! */
-             /*
+	     /* ! */
+	     /*
 	      * All calls to this function must be in a form suitable
 	      * for xenctrl_abi_check.  The parsing there is ad-hoc.
 	      */
@@ -259,7 +259,7 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 		/* Quick & dirty check for ABI changes. */
 		BUILD_BUG_ON(sizeof(cfg) != 68);

-        /* Mnemonics for the named fields inside xen_x86_arch_domainconfig */
+		/* Mnemonics for the named fields inside xen_x86_arch_domainconfig */
 #define VAL_EMUL_FLAGS          Field(arch_domconfig, 0)
 #define VAL_MISC_FLAGS          Field(arch_domconfig, 1)

@@ -351,7 +351,7 @@ static value dom_op(value xch_val, value domid,
 	caml_enter_blocking_section();
 	result = fn(xch, c_domid);
 	caml_leave_blocking_section();
-        if (result)
+	if (result)
 		failwith_xc(xch);
 	CAMLreturn(Val_unit);
 }
@@ -383,7 +383,7 @@ CAMLprim value stub_xc_domain_resume_fast(value xch_val, value domid)
 	caml_enter_blocking_section();
 	result = xc_domain_resume(xch, c_domid, 1);
 	caml_leave_blocking_section();
-        if (result)
+	if (result)
 		failwith_xc(xch);
 	CAMLreturn(Val_unit);
 }
@@ -426,7 +426,7 @@ static value alloc_domaininfo(xc_domaininfo_t * info)
 	Store_field(result, 13, Val_int(info->max_vcpu_id));
 	Store_field(result, 14, caml_copy_int32(info->ssidref));

-        tmp = caml_alloc_small(16, 0);
+	tmp = caml_alloc_small(16, 0);
 	for (i = 0; i < 16; i++) {
 		Field(tmp, i) = Val_int(info->handle[i]);
 	}
--
2.34.1

[PATCH for-4.19? v5 02/10] tools/ocaml: Add missing ocaml bindings for altp2m_opts

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

Fixes: 0291089f6ea8 ("xen: enable altp2m at create domain domctl")

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 tools/ocaml/libs/xc/xenctrl.ml      | 1 +
 tools/ocaml/libs/xc/xenctrl.mli     | 1 +
 tools/ocaml/libs/xc/xenctrl_stubs.c | 9 ++++++---
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 55923857ec..2690f9a923 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -85,6 +85,7 @@ type domctl_create_config =
     max_grant_frames: int;
     max_maptrack_frames: int;
     max_grant_version: int;
+    altp2m_opts: int32;
     vmtrace_buf_kb: int32;
     cpupool_id: int32;
     arch: arch_domainconfig;
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 9b4b45db3a..febbe1f6ae 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -77,6 +77,7 @@ type domctl_create_config = {
   max_grant_frames: int;
   max_maptrack_frames: int;
   max_grant_version: int;
+  altp2m_opts: int32;
   vmtrace_buf_kb: int32;
   cpupool_id: int32;
   arch: arch_domainconfig;
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index e86c455802..a529080129 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -210,9 +210,10 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 #define VAL_MAX_GRANT_FRAMES    Field(config, 6)
 #define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
 #define VAL_MAX_GRANT_VERSION   Field(config, 8)
-#define VAL_VMTRACE_BUF_KB      Field(config, 9)
-#define VAL_CPUPOOL_ID          Field(config, 10)
-#define VAL_ARCH                Field(config, 11)
+#define VAL_ALTP2M_OPTS         Field(config, 9)
+#define VAL_VMTRACE_BUF_KB      Field(config, 10)
+#define VAL_CPUPOOL_ID          Field(config, 11)
+#define VAL_ARCH                Field(config, 12)

 	uint32_t domid = Int_val(wanted_domid);
 	uint64_t vmtrace_size = Int32_val(VAL_VMTRACE_BUF_KB);
@@ -230,6 +231,7 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 		.max_maptrack_frames = Int_val(VAL_MAX_MAPTRACK_FRAMES),
 		.grant_opts =
 		    XEN_DOMCTL_GRANT_version(Int_val(VAL_MAX_GRANT_VERSION)),
+		.altp2m_opts = Int32_val(VAL_ALTP2M_OPTS),
 		.vmtrace_size = vmtrace_size,
 		.cpupool_id = Int32_val(VAL_CPUPOOL_ID),
 	};
@@ -288,6 +290,7 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 #undef VAL_ARCH
 #undef VAL_CPUPOOL_ID
 #undef VAL_VMTRACE_BUF_KB
+#undef VAL_ALTP2M_OPTS
 #undef VAL_MAX_GRANT_VERSION
 #undef VAL_MAX_MAPTRACK_FRAMES
 #undef VAL_MAX_GRANT_FRAMES
--
2.34.1

[PATCH for-4.19? v5 03/10] xen: Refactor altp2m options into a structured format

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

Encapsulate the altp2m options within a struct. This change is preparatory
and sets the groundwork for introducing additional parameter in subsequent
commit.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 tools/libs/light/libxl_create.c     | 6 +++---
 tools/ocaml/libs/xc/xenctrl_stubs.c | 4 +++-
 xen/arch/arm/domain.c               | 2 +-
 xen/arch/x86/domain.c               | 4 ++--
 xen/arch/x86/hvm/hvm.c              | 2 +-
 xen/include/public/domctl.h         | 4 +++-
 6 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index edeadd57ef..569e3d21ed 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -680,17 +680,17 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
         LOG(DETAIL, "altp2m: %s", libxl_altp2m_mode_to_string(b_info->altp2m));
         switch(b_info->altp2m) {
         case LIBXL_ALTP2M_MODE_MIXED:
-            create.altp2m_opts |=
+            create.altp2m.opts |=
                 XEN_DOMCTL_ALTP2M_mode(XEN_DOMCTL_ALTP2M_mixed);
             break;

         case LIBXL_ALTP2M_MODE_EXTERNAL:
-            create.altp2m_opts |=
+            create.altp2m.opts |=
                 XEN_DOMCTL_ALTP2M_mode(XEN_DOMCTL_ALTP2M_external);
             break;

         case LIBXL_ALTP2M_MODE_LIMITED:
-            create.altp2m_opts |=
+            create.altp2m.opts |=
                 XEN_DOMCTL_ALTP2M_mode(XEN_DOMCTL_ALTP2M_limited);
             break;

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index a529080129..e6c977521f 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -231,7 +231,9 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 		.max_maptrack_frames = Int_val(VAL_MAX_MAPTRACK_FRAMES),
 		.grant_opts =
 		    XEN_DOMCTL_GRANT_version(Int_val(VAL_MAX_GRANT_VERSION)),
-		.altp2m_opts = Int32_val(VAL_ALTP2M_OPTS),
+		.altp2m = {
+			.opts = Int32_val(VAL_ALTP2M_OPTS),
+		},
 		.vmtrace_size = vmtrace_size,
 		.cpupool_id = Int32_val(VAL_CPUPOOL_ID),
 	};
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 8bde2f730d..5234b627d0 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -688,7 +688,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }

-    if ( config->altp2m_opts )
+    if ( config->altp2m.opts )
     {
         dprintk(XENLOG_INFO, "Altp2m not supported\n");
         return -EINVAL;
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 536542841e..bb5ba8fc1e 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -637,7 +637,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
     bool hap = config->flags & XEN_DOMCTL_CDF_hap;
     bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
     unsigned int max_vcpus;
-    unsigned int altp2m_mode = MASK_EXTR(config->altp2m_opts,
+    unsigned int altp2m_mode = MASK_EXTR(config->altp2m.opts,
                                          XEN_DOMCTL_ALTP2M_mode_mask);

     if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
@@ -717,7 +717,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }

-    if ( config->altp2m_opts & ~XEN_DOMCTL_ALTP2M_mode_mask )
+    if ( config->altp2m.opts & ~XEN_DOMCTL_ALTP2M_mode_mask )
     {
         dprintk(XENLOG_INFO, "Invalid altp2m options selected: %#x\n",
                 config->flags);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8334ab1711..a66ebaaceb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -659,7 +659,7 @@ int hvm_domain_initialise(struct domain *d,
     d->arch.hvm.params[HVM_PARAM_TRIPLE_FAULT_REASON] = SHUTDOWN_reboot;

     /* Set altp2m based on domctl flags. */
-    switch ( MASK_EXTR(config->altp2m_opts, XEN_DOMCTL_ALTP2M_mode_mask) )
+    switch ( MASK_EXTR(config->altp2m.opts, XEN_DOMCTL_ALTP2M_mode_mask) )
     {
     case XEN_DOMCTL_ALTP2M_mixed:
         d->arch.hvm.params[HVM_PARAM_ALTP2M] = XEN_ALTP2M_mixed;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 2a49fe46ce..dea399aa8e 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -86,6 +86,7 @@ struct xen_domctl_createdomain {

     uint32_t grant_opts;

+    struct {
 /*
  * Enable altp2m mixed mode.
  *
@@ -102,7 +103,8 @@ struct xen_domctl_createdomain {
 /* Altp2m mode signaling uses bits [0, 1]. */
 #define XEN_DOMCTL_ALTP2M_mode_mask  (0x3U)
 #define XEN_DOMCTL_ALTP2M_mode(m)    ((m) & XEN_DOMCTL_ALTP2M_mode_mask)
-    uint32_t altp2m_opts;
+        uint32_t opts;
+    } altp2m;

     /* Per-vCPU buffer size in bytes.  0 to disable. */
     uint32_t vmtrace_size;
--
2.34.1

[PATCH for-4.19? v5 04/10] tools/xl: Add altp2m_count parameter

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

Introduce a new altp2m_count parameter to control the maximum number of altp2m
views a domain can use. By default, if altp2m_count is unspecified and altp2m
is enabled, the value is set to 10, reflecting the legacy behavior.

This change is preparatory; it establishes the groundwork for the feature but
does not activate it.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 tools/golang/xenlight/helpers.gen.go | 2 ++
 tools/golang/xenlight/types.gen.go   | 1 +
 tools/include/libxl.h                | 8 ++++++++
 tools/libs/light/libxl_create.c      | 9 +++++++++
 tools/libs/light/libxl_types.idl     | 1 +
 tools/xl/xl_parse.c                  | 9 +++++++++
 6 files changed, 30 insertions(+)

diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index fe5110474d..0449c55f31 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1159,6 +1159,7 @@ if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil {
 return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
 }
 x.Altp2M = Altp2MMode(xc.altp2m)
+x.Altp2MCount = uint32(xc.altp2m_count)
 x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
 if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
 return fmt.Errorf("converting field Vpmu: %v", err)
@@ -1676,6 +1677,7 @@ if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
 return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
 }
 xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
+xc.altp2m_count = C.uint32_t(x.Altp2MCount)
 xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
 if err := x.Vpmu.toC(&xc.vpmu); err != nil {
 return fmt.Errorf("converting field Vpmu: %v", err)
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index c9e45b306f..54607758d3 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -603,6 +603,7 @@ ArchX86 struct {
 MsrRelaxed Defbool
 }
 Altp2M Altp2MMode
+Altp2MCount uint32
 VmtraceBufKb int
 Vpmu Defbool
 }
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index f5c7167742..bfa06caad2 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1250,6 +1250,14 @@ typedef struct libxl__ctx libxl_ctx;
  */
 #define LIBXL_HAVE_ALTP2M 1

+/*
+ * LIBXL_HAVE_ALTP2M_COUNT
+ * If this is defined, then libxl supports setting the maximum number of
+ * alternate p2m tables.
+ */
+#define LIBXL_HAVE_ALTP2M_COUNT 1
+#define LIBXL_ALTP2M_COUNT_DEFAULT (~(uint32_t)0)
+
 /*
  * LIBXL_HAVE_REMUS
  * If this is defined, then libxl supports remus.
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 569e3d21ed..11d2f282f5 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -482,6 +482,15 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         return -ERROR_INVAL;
     }

+    if (b_info->altp2m_count == LIBXL_ALTP2M_COUNT_DEFAULT) {
+        if ((libxl_defbool_val(b_info->u.hvm.altp2m) ||
+            b_info->altp2m != LIBXL_ALTP2M_MODE_DISABLED))
+            /* Reflect the default legacy count */
+            b_info->altp2m_count = 10;
+        else
+            b_info->altp2m_count = 0;
+    }
+
     /* Assume that providing a bootloader user implies enabling restrict. */
     libxl_defbool_setdefault(&b_info->bootloader_restrict,
                              !!b_info->bootloader_user);
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 4e65e6fda5..2963c5e250 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -729,6 +729,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     # Alternate p2m is not bound to any architecture or guest type, as it is
     # supported by x86 HVM and ARM support is planned.
     ("altp2m", libxl_altp2m_mode),
+    ("altp2m_count", uint32, {'init_val': 'LIBXL_ALTP2M_COUNT_DEFAULT'}),

     # Size of preallocated vmtrace trace buffers (in KBYTES).
     # Use zero value to disable this feature.
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index e3a4800f6e..a82b8fe6e4 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2063,6 +2063,15 @@ void parse_config_data(const char *config_source,
         }
     }

+    if (!xlu_cfg_get_long(config, "altp2m_count", &l, 1)) {
+        if (l != (uint16_t)l) {
+            fprintf(stderr, "ERROR: invalid value %ld for \"altp2m_count\"\n", l);
+            exit (1);
+        }
+
+        b_info->altp2m_count = l;
+    }
+
     if (!xlu_cfg_get_long(config, "vmtrace_buf_kb", &l, 1) && l) {
         b_info->vmtrace_buf_kb = l;
     }
--
2.34.1

[PATCH for-4.19? v5 05/10] docs/man: Add altp2m_count parameter to the xl.cfg manual

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

Update manual pages to include detailed information about the altp2m_count
configuration parameter.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 docs/man/xl.cfg.5.pod.in | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index ac3f88fd57..ff03b43884 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2039,6 +2039,20 @@ a single guest HVM domain. B<This option is deprecated, use the option
 B<Note>: While the option "altp2mhvm" is deprecated, legacy applications for
 x86 systems will continue to work using it.

+=item B<altp2m_count=NUMBER>
+
+Specifies the maximum number of alternate-p2m views available to the guest.
+This setting is crucial in domain introspection scenarios that require
+multiple physical-to-machine (p2m) memory mappings to be established
+simultaneously.
+
+Enabling multiple p2m views may increase memory usage. It is advisable to
+review and adjust the B<shadow_memory> setting as necessary to accommodate
+the additional memory requirements.
+
+B<Note>: This option is ignored if B<altp2m> is disabled. The default value
+is 10.
+
 =item B<nestedhvm=BOOLEAN>

 Enable or disables guest access to hardware virtualisation features,
--
2.34.1

[PATCH for-4.19? v5 06/10] x86/altp2m: Introduce accessor functions for safer array indexing

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

This patch introduces a set of accessor functions for altp2m array operations,
and refactoring direct array accesses with them.

This approach aims on improving the code clarity and also future-proofing the
codebase against potential speculative execution attacks.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 xen/arch/x86/hvm/vmx/vmx.c        |  4 +--
 xen/arch/x86/include/asm/altp2m.h | 32 +++++++++++++++++
 xen/arch/x86/include/asm/p2m.h    |  7 ++--
 xen/arch/x86/mm/altp2m.c          | 60 +++++++++++++------------------
 xen/arch/x86/mm/hap/hap.c         |  8 ++---
 xen/arch/x86/mm/mem_access.c      | 17 ++++-----
 xen/arch/x86/mm/mem_sharing.c     |  2 +-
 xen/arch/x86/mm/p2m-ept.c         | 14 ++++----
 xen/arch/x86/mm/p2m.c             | 16 ++++-----
 9 files changed, 91 insertions(+), 69 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f16faa6a61..a420d452b3 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4887,10 +4887,10 @@ bool asmlinkage vmx_vmenter_helper(const struct cpu_user_regs *regs)

             for ( i = 0; i < MAX_ALTP2M; ++i )
             {
-                if ( currd->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
+                if ( altp2m_get_eptp(currd, i) == mfn_x(INVALID_MFN) )
                     continue;

-                ept = &currd->arch.altp2m_p2m[i]->ept;
+                ept = &altp2m_get_p2m(currd, i)->ept;
                 if ( cpumask_test_cpu(cpu, ept->invalidate) )
                 {
                     cpumask_clear_cpu(cpu, ept->invalidate);
diff --git a/xen/arch/x86/include/asm/altp2m.h b/xen/arch/x86/include/asm/altp2m.h
index e5e59cbd68..2f064c61a2 100644
--- a/xen/arch/x86/include/asm/altp2m.h
+++ b/xen/arch/x86/include/asm/altp2m.h
@@ -19,6 +19,38 @@ static inline bool altp2m_active(const struct domain *d)
     return d->arch.altp2m_active;
 }

+static inline struct p2m_domain *altp2m_get_p2m(const struct domain* d,
+                                                unsigned int idx)
+{
+    return d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
+}
+
+static inline uint64_t altp2m_get_eptp(const struct domain* d,
+                                       unsigned int idx)
+{
+    return d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)];
+}
+
+static inline void altp2m_set_eptp(const struct domain* d,
+                                   unsigned int idx,
+                                   uint64_t eptp)
+{
+    d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] = eptp;
+}
+
+static inline uint64_t altp2m_get_visible_eptp(const struct domain* d,
+                                               unsigned int idx)
+{
+    return d->arch.altp2m_visible_eptp[array_index_nospec(idx, MAX_EPTP)];
+}
+
+static inline void altp2m_set_visible_eptp(const struct domain* d,
+                                           unsigned int idx,
+                                           uint64_t eptp)
+{
+    d->arch.altp2m_visible_eptp[array_index_nospec(idx, MAX_EPTP)] = eptp;
+}
+
 /* Alternate p2m VCPU */
 void altp2m_vcpu_initialise(struct vcpu *v);
 void altp2m_vcpu_destroy(struct vcpu *v);
diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h
index c1478ffc36..e6f7764f9f 100644
--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -18,6 +18,7 @@
 #include <xen/mem_access.h>
 #include <asm/mem_sharing.h>
 #include <asm/page.h>    /* for pagetable_t */
+#include <asm/altp2m.h>

 /* Debugging and auditing of the P2M code? */
 #if !defined(NDEBUG) && defined(CONFIG_HVM)
@@ -888,13 +889,14 @@ static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)

     BUG_ON(index >= MAX_ALTP2M);

-    return v->domain->arch.altp2m_p2m[index];
+    return altp2m_get_p2m(v->domain, index);
 }

 /* set current alternate p2m table */
 static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx)
 {
     struct p2m_domain *orig;
+    struct p2m_domain *ap2m;

     BUG_ON(idx >= MAX_ALTP2M);

@@ -906,7 +908,8 @@ static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx)
     atomic_dec(&orig->active_vcpus);

     vcpu_altp2m(v).p2midx = idx;
-    atomic_inc(&v->domain->arch.altp2m_p2m[idx]->active_vcpus);
+    ap2m = altp2m_get_p2m(v->domain, idx);
+    atomic_inc(&ap2m->active_vcpus);

     return true;
 }
diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 6fe1e9ed6b..7fb1738376 100644
--- a/xen/arch/x86/mm/altp2m.c
+++ b/xen/arch/x86/mm/altp2m.c
@@ -205,7 +205,7 @@ bool p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx)

     altp2m_list_lock(d);

-    if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
+    if ( altp2m_get_eptp(d, idx) != mfn_x(INVALID_MFN) )
     {
         if ( p2m_set_altp2m(v, idx) )
             altp2m_vcpu_update_p2m(v);
@@ -307,7 +307,7 @@ static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
     struct p2m_domain *p2m;

     ASSERT(idx < MAX_ALTP2M);
-    p2m = array_access_nospec(d->arch.altp2m_p2m, idx);
+    p2m = altp2m_get_p2m(d, idx);

     p2m_lock(p2m);

@@ -335,8 +335,8 @@ void p2m_flush_altp2m(struct domain *d)
     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
         p2m_reset_altp2m(d, i, ALTP2M_DEACTIVATE);
-        d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
-        d->arch.altp2m_visible_eptp[i] = mfn_x(INVALID_MFN);
+        altp2m_set_eptp(d, i, mfn_x(INVALID_MFN));
+        altp2m_set_visible_eptp(d, i, mfn_x(INVALID_MFN));
     }

     altp2m_list_unlock(d);
@@ -350,7 +350,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx,

     ASSERT(idx < MAX_ALTP2M);

-    p2m = array_access_nospec(d->arch.altp2m_p2m, idx);
+    p2m = altp2m_get_p2m(d, idx);
     hostp2m = p2m_get_hostp2m(d);

     p2m_lock(p2m);
@@ -393,8 +393,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)

     altp2m_list_lock(d);

-    if ( d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
-         mfn_x(INVALID_MFN) )
+    if ( altp2m_get_eptp(d, idx) == mfn_x(INVALID_MFN) )
         rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);

     altp2m_list_unlock(d);
@@ -417,7 +416,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,

     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+        if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) )
             continue;

         rc = p2m_activate_altp2m(d, i, a);
@@ -447,18 +446,15 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
     rc = -EBUSY;
     altp2m_list_lock(d);

-    if ( d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] !=
-         mfn_x(INVALID_MFN) )
+    if ( altp2m_get_eptp(d, idx) != mfn_x(INVALID_MFN) )
     {
-        p2m = array_access_nospec(d->arch.altp2m_p2m, idx);
+        p2m = altp2m_get_p2m(d, idx);

         if ( !_atomic_read(p2m->active_vcpus) )
         {
             p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
-            d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
-                mfn_x(INVALID_MFN);
-            d->arch.altp2m_visible_eptp[array_index_nospec(idx, MAX_EPTP)] =
-                mfn_x(INVALID_MFN);
+            altp2m_set_eptp(d, idx, mfn_x(INVALID_MFN));
+            altp2m_set_visible_eptp(d, idx, mfn_x(INVALID_MFN));
             rc = 0;
         }
     }
@@ -485,7 +481,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
     rc = -EINVAL;
     altp2m_list_lock(d);

-    if ( d->arch.altp2m_visible_eptp[idx] != mfn_x(INVALID_MFN) )
+    if ( altp2m_get_visible_eptp(d, idx) != mfn_x(INVALID_MFN) )
     {
         for_each_vcpu( d, v )
             if ( p2m_set_altp2m(v, idx) )
@@ -510,13 +506,12 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     mfn_t mfn;
     int rc = -EINVAL;

-    if ( idx >=  min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
-         d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
-         mfn_x(INVALID_MFN) )
+    if ( idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
+         altp2m_get_eptp(d, idx) == mfn_x(INVALID_MFN) )
         return rc;

     hp2m = p2m_get_hostp2m(d);
-    ap2m = array_access_nospec(d->arch.altp2m_p2m, idx);
+    ap2m = altp2m_get_p2m(d, idx);

     p2m_lock(hp2m);
     p2m_lock(ap2m);
@@ -577,10 +572,10 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
         p2m_type_t t;
         p2m_access_t a;

-        if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
+        if ( altp2m_get_eptp(d, i) == mfn_x(INVALID_MFN) )
             continue;

-        p2m = d->arch.altp2m_p2m[i];
+        p2m = altp2m_get_p2m(d, i);

         /* Check for a dropped page that may impact this altp2m */
         if ( mfn_eq(mfn, INVALID_MFN) &&
@@ -598,7 +593,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
                 for ( i = 0; i < MAX_ALTP2M; i++ )
                 {
                     if ( i == last_reset_idx ||
-                         d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
+                         altp2m_get_eptp(d, i) == mfn_x(INVALID_MFN) )
                         continue;

                     p2m_reset_altp2m(d, i, ALTP2M_RESET);
@@ -660,11 +655,10 @@ int p2m_set_suppress_ve_multi(struct domain *d,
     if ( sve->view > 0 )
     {
         if ( sve->view >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
-             d->arch.altp2m_eptp[array_index_nospec(sve->view, MAX_EPTP)] ==
-             mfn_x(INVALID_MFN) )
+             altp2m_get_eptp(d, sve->view) == mfn_x(INVALID_MFN) )
             return -EINVAL;

-        p2m = ap2m = array_access_nospec(d->arch.altp2m_p2m, sve->view);
+        p2m = ap2m = altp2m_get_p2m(d, sve->view);
     }

     p2m_lock(host_p2m);
@@ -728,11 +722,10 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
     if ( altp2m_idx > 0 )
     {
         if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
-             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
-             mfn_x(INVALID_MFN) )
+             altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) )
             return -EINVAL;

-        p2m = ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
+        p2m = ap2m = altp2m_get_p2m(d, altp2m_idx);
     }
     else
         p2m = host_p2m;
@@ -766,15 +759,12 @@ int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
      * min(MAX_ALTP2M, MAX_EPTP).
      */
     if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
-         d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
-         mfn_x(INVALID_MFN) )
+         altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) )
         rc = -EINVAL;
     else if ( visible )
-        d->arch.altp2m_visible_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] =
-            d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)];
+        altp2m_set_visible_eptp(d, altp2m_idx, altp2m_get_eptp(d, altp2m_idx));
     else
-        d->arch.altp2m_visible_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] =
-            mfn_x(INVALID_MFN);
+        altp2m_set_visible_eptp(d, altp2m_idx, mfn_x(INVALID_MFN));

     altp2m_list_unlock(d);

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index d2011fde24..8fc8348152 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -511,13 +511,13 @@ int hap_enable(struct domain *d, u32 mode)

         for ( i = 0; i < MAX_EPTP; i++ )
         {
-            d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
-            d->arch.altp2m_visible_eptp[i] = mfn_x(INVALID_MFN);
+            altp2m_set_eptp(d, i, mfn_x(INVALID_MFN));
+            altp2m_set_visible_eptp(d, i, mfn_x(INVALID_MFN));
         }

         for ( i = 0; i < MAX_ALTP2M; i++ )
         {
-            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
+            rv = p2m_alloc_table(altp2m_get_p2m(d, i));
             if ( rv != 0 )
                goto out;
         }
@@ -592,7 +592,7 @@ void hap_teardown(struct domain *d, bool *preempted)

         for ( i = 0; i < MAX_ALTP2M; i++ )
         {
-            p2m_teardown(d->arch.altp2m_p2m[i], false, preempted);
+            p2m_teardown(altp2m_get_p2m(d, i), false, preempted);
             if ( preempted && *preempted )
                 return;
         }
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 60a0cce68a..43f3a8c2aa 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -348,11 +348,10 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
     if ( altp2m_idx )
     {
         if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
-             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
-             mfn_x(INVALID_MFN) )
+             altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) )
             return -EINVAL;

-        ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
+        ap2m = altp2m_get_p2m(d, altp2m_idx);
     }

     if ( !xenmem_access_to_p2m_access(p2m, access, &a) )
@@ -404,11 +403,10 @@ long p2m_set_mem_access_multi(struct domain *d,
     if ( altp2m_idx )
     {
         if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
-             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
-             mfn_x(INVALID_MFN) )
+             altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) )
             return -EINVAL;

-        ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
+        ap2m = altp2m_get_p2m(d, altp2m_idx);
     }

     p2m_lock(p2m);
@@ -467,11 +465,10 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
     else if ( altp2m_idx ) /* altp2m view 0 is treated as the hostp2m */
     {
         if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
-             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
-             mfn_x(INVALID_MFN) )
+             altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) )
             return -EINVAL;

-        p2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
+        p2m = altp2m_get_p2m(d, altp2m_idx);
     }

     return _p2m_get_mem_access(p2m, gfn, access);
@@ -488,7 +485,7 @@ void arch_p2m_set_access_required(struct domain *d, bool access_required)
         unsigned int i;
         for ( i = 0; i < MAX_ALTP2M; i++ )
         {
-            struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
+            struct p2m_domain *p2m = altp2m_get_p2m(d, i);

             if ( p2m )
                 p2m->access_required = access_required;
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index da28266ef0..21ac361111 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -914,7 +914,7 @@ static int nominate_page(struct domain *d, gfn_t gfn,

         for ( i = 0; i < MAX_ALTP2M; i++ )
         {
-            ap2m = d->arch.altp2m_p2m[i];
+            ap2m = altp2m_get_p2m(d, i);
             if ( !ap2m )
                 continue;

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index f83610cb8c..ed4252822e 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1297,10 +1297,10 @@ static void ept_set_ad_sync(struct domain *d, bool value)
         {
             struct p2m_domain *p2m;

-            if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
+            if ( altp2m_get_eptp(d, i) == mfn_x(INVALID_MFN) )
                 continue;

-            p2m = d->arch.altp2m_p2m[i];
+            p2m = altp2m_get_p2m(d, i);

             p2m_lock(p2m);
             p2m->ept.ad = value;
@@ -1500,15 +1500,15 @@ void setup_ept_dump(void)

 void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
 {
-    struct p2m_domain *p2m = array_access_nospec(d->arch.altp2m_p2m, i);
+    struct p2m_domain *p2m = altp2m_get_p2m(d, i);
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
     struct ept_data *ept;

     p2m->ept.ad = hostp2m->ept.ad;
     ept = &p2m->ept;
     ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
-    d->arch.altp2m_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
-    d->arch.altp2m_visible_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
+    altp2m_set_eptp(d, i, ept->eptp);
+    altp2m_set_visible_eptp(d, i, ept->eptp);
 }

 unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
@@ -1521,10 +1521,10 @@ unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)

     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
+        if ( altp2m_get_eptp(d, i) == mfn_x(INVALID_MFN) )
             continue;

-        p2m = d->arch.altp2m_p2m[i];
+        p2m = altp2m_get_p2m(d, i);
         ept = &p2m->ept;

         if ( eptp == ept->eptp )
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index e7e327d6a6..30a44441ba 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -107,9 +107,9 @@ void p2m_change_entry_type_global(struct domain *d,

         for ( i = 0; i < MAX_ALTP2M; i++ )
         {
-            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) )
             {
-                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+                struct p2m_domain *altp2m = altp2m_get_p2m(d, i);

                 p2m_lock(altp2m);
                 change_entry_type_global(altp2m, ot, nt);
@@ -142,9 +142,9 @@ void p2m_memory_type_changed(struct domain *d)

         for ( i = 0; i < MAX_ALTP2M; i++ )
         {
-            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) )
             {
-                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+                struct p2m_domain *altp2m = altp2m_get_p2m(d, i);

                 p2m_lock(altp2m);
                 _memory_type_changed(altp2m);
@@ -915,9 +915,9 @@ void p2m_change_type_range(struct domain *d,

         for ( i = 0; i < MAX_ALTP2M; i++ )
         {
-            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) )
             {
-                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+                struct p2m_domain *altp2m = altp2m_get_p2m(d, i);

                 p2m_lock(altp2m);
                 change_type_range(altp2m, start, end, ot, nt);
@@ -988,9 +988,9 @@ int p2m_finish_type_change(struct domain *d,

         for ( i = 0; i < MAX_ALTP2M; i++ )
         {
-            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) )
             {
-                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+                struct p2m_domain *altp2m = altp2m_get_p2m(d, i);

                 p2m_lock(altp2m);
                 rc = finish_type_change(altp2m, first_gfn, max_nr);
--
2.34.1

[PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

x86: Make the maximum number of altp2m views configurable

This commit introduces the ability to configure the maximum number of altp2m
views for the domain during its creation. Previously, the limits were hardcoded
to a maximum of 10. This change allows for greater flexibility in environments
that require more or fewer altp2m views.

The maximum configurable limit for nr_altp2m on x86 is now set to
MAX_NR_ALTP2M (which currently holds the MAX_EPTP value - 512). This cap is
linked to the architectural limit of the EPTP-switching VMFUNC, which supports
up to 512 entries. Despite there being no inherent need for limiting nr_altp2m
in scenarios not utilizing VMFUNC, decoupling these components would necessitate
substantial code changes.

xen_domctl_createdomain::altp2m is extended for a new field `nr`, that will
configure this limit for a domain. Additionally, previous altp2m.opts value
has been reduced from uint32_t to uint16_t so that both of these fields occupy
as little space as possible.

altp2m_get_p2m() function is modified to respect the new nr_altp2m value.
Accessor functions that operate on EPT arrays are unmodified, since these
arrays always have fixed size of MAX_EPTP.

A dummy hvm_altp2m_supported() function is introduced for non-HVM builds, so
that the compilation won't fail for them.

Additional sanitization is introduced in the x86/arch_sanitise_domain_config
to fix the altp2m.nr value to 10 if it is previously set to 0. This behavior
is only temporary and immediately removed in the upcoming commit (which will
disallow creating a domain with enabled altp2m with zero nr_altp2m).

The reason for this temporary workaround is to retain the legacy behavior
until the feature is fully activated in libxl.

Also, arm/arch_sanitise_domain_config is extended to not allow requesting
non-zero altp2ms.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 xen/arch/arm/domain.c              |  2 +-
 xen/arch/x86/domain.c              | 40 ++++++++++++++++++----
 xen/arch/x86/hvm/hvm.c             |  8 ++++-
 xen/arch/x86/hvm/vmx/vmx.c         |  2 +-
 xen/arch/x86/include/asm/altp2m.h  |  2 +-
 xen/arch/x86/include/asm/domain.h  |  9 ++---
 xen/arch/x86/include/asm/hvm/hvm.h |  5 +++
 xen/arch/x86/include/asm/p2m.h     |  4 +--
 xen/arch/x86/mm/altp2m.c           | 54 ++++++++++++++++++++----------
 xen/arch/x86/mm/hap/hap.c          |  8 ++---
 xen/arch/x86/mm/mem_access.c       |  8 ++---
 xen/arch/x86/mm/mem_sharing.c      |  2 +-
 xen/arch/x86/mm/p2m-ept.c          |  4 +--
 xen/arch/x86/mm/p2m.c              |  8 ++---
 xen/common/domain.c                |  1 +
 xen/include/public/domctl.h        |  5 ++-
 xen/include/xen/sched.h            |  2 ++
 17 files changed, 113 insertions(+), 51 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 5234b627d0..e5785d2d96 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -688,7 +688,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }

-    if ( config->altp2m.opts )
+    if ( config->altp2m.opts || config->altp2m.nr )
     {
         dprintk(XENLOG_INFO, "Altp2m not supported\n");
         return -EINVAL;
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index bb5ba8fc1e..011cffb07e 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -724,16 +724,42 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }

-    if ( altp2m_mode && nested_virt )
+    if ( altp2m_mode )
     {
-        dprintk(XENLOG_INFO,
-                "Nested virt and altp2m are not supported together\n");
-        return -EINVAL;
-    }
+        if ( nested_virt )
+        {
+            dprintk(XENLOG_INFO,
+                    "Nested virt and altp2m are not supported together\n");
+            return -EINVAL;
+        }
+
+        if ( !hap )
+        {
+            dprintk(XENLOG_INFO, "altp2m is only supported with HAP\n");
+            return -EINVAL;
+        }
+
+        if ( !hvm_altp2m_supported() )
+        {
+            dprintk(XENLOG_INFO, "altp2m is not supported\n");
+            return -EINVAL;
+        }
+
+        if ( !config->altp2m.nr )
+        {
+            /* Fix the value to the legacy default */
+            config->altp2m.nr = 10;
+        }

-    if ( altp2m_mode && !hap )
+        if ( config->altp2m.nr > MAX_NR_ALTP2M )
+        {
+            dprintk(XENLOG_INFO, "altp2m.nr must be <= %lu\n", MAX_NR_ALTP2M);
+            return -EINVAL;
+        }
+    }
+    else if ( config->altp2m.nr )
     {
-        dprintk(XENLOG_INFO, "altp2m is only supported with HAP\n");
+        dprintk(XENLOG_INFO, "altp2m.nr must be zero when altp2m is off\n");
         return -EINVAL;
     }

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a66ebaaceb..3d0357a0f8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4657,6 +4657,12 @@ static int do_altp2m_op(
         goto out;
     }

+    if ( d->nr_altp2m == 0 )
+    {
+        rc = -EINVAL;
+        goto out;
+    }
+
     if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d, mode, a.cmd)) )
         goto out;

@@ -5245,7 +5251,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx)
     if ( !hvm_is_singlestep_supported() )
         return;

-    if ( p2midx >= MAX_ALTP2M )
+    if ( p2midx >= v->domain->nr_altp2m )
         return;

     v->arch.hvm.single_step = true;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index a420d452b3..9292a2c8d8 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4885,7 +4885,7 @@ bool asmlinkage vmx_vmenter_helper(const struct cpu_user_regs *regs)
         {
             unsigned int i;

-            for ( i = 0; i < MAX_ALTP2M; ++i )
+            for ( i = 0; i < currd->nr_altp2m; ++i )
             {
                 if ( altp2m_get_eptp(currd, i) == mfn_x(INVALID_MFN) )
                     continue;
diff --git a/xen/arch/x86/include/asm/altp2m.h b/xen/arch/x86/include/asm/altp2m.h
index 2f064c61a2..a4cc3d3ffc 100644
--- a/xen/arch/x86/include/asm/altp2m.h
+++ b/xen/arch/x86/include/asm/altp2m.h
@@ -22,7 +22,7 @@ static inline bool altp2m_active(const struct domain *d)
 static inline struct p2m_domain *altp2m_get_p2m(const struct domain* d,
                                                 unsigned int idx)
 {
-    return d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
+    return d->arch.altp2m_p2m[array_index_nospec(idx, d->nr_altp2m)];
 }

 static inline uint64_t altp2m_get_eptp(const struct domain* d,
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index f5daeb182b..855e844bed 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -258,11 +258,12 @@ struct paging_vcpu {
     struct shadow_vcpu shadow;
 };

-#define MAX_NESTEDP2M 10
+#define MAX_EPTP        (PAGE_SIZE / sizeof(uint64_t))
+#define MAX_NR_ALTP2M   MAX_EPTP
+#define MAX_NESTEDP2M   10

-#define MAX_ALTP2M      10 /* arbitrary */
 #define INVALID_ALTP2M  0xffff
-#define MAX_EPTP        (PAGE_SIZE / sizeof(uint64_t))
+
 struct p2m_domain;
 struct time_scale {
     int shift;
@@ -353,7 +354,7 @@ struct arch_domain

     /* altp2m: allow multiple copies of host p2m */
     bool altp2m_active;
-    struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
+    struct p2m_domain **altp2m_p2m;
     mm_lock_t altp2m_list_lock;
     uint64_t *altp2m_eptp;
     uint64_t *altp2m_visible_eptp;
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index 1c01e22c8e..277648dd18 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -828,6 +828,11 @@ static inline bool hvm_hap_supported(void)
     return false;
 }

+static inline bool hvm_altp2m_supported(void)
+{
+    return false;
+}
+
 static inline bool hvm_nested_virt_supported(void)
 {
     return false;
diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h
index e6f7764f9f..a1094fc7b3 100644
--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -887,7 +887,7 @@ static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)
     if ( index == INVALID_ALTP2M )
         return NULL;

-    BUG_ON(index >= MAX_ALTP2M);
+    BUG_ON(index >= v->domain->nr_altp2m);

     return altp2m_get_p2m(v->domain, index);
 }
@@ -898,7 +898,7 @@ static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx)
     struct p2m_domain *orig;
     struct p2m_domain *ap2m;

-    BUG_ON(idx >= MAX_ALTP2M);
+    BUG_ON(idx >= v->domain->nr_altp2m);

     if ( idx == vcpu_altp2m(v).p2midx )
         return false;
diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 7fb1738376..d47277e5e5 100644
--- a/xen/arch/x86/mm/altp2m.c
+++ b/xen/arch/x86/mm/altp2m.c
@@ -15,6 +15,11 @@
 void
 altp2m_vcpu_initialise(struct vcpu *v)
 {
+    struct domain *d = v->domain;
+
+    if ( d->nr_altp2m == 0 )
+        return;
+
     if ( v != current )
         vcpu_pause(v);

@@ -30,8 +35,12 @@ altp2m_vcpu_initialise(struct vcpu *v)
 void
 altp2m_vcpu_destroy(struct vcpu *v)
 {
+    struct domain *d = v->domain;
     struct p2m_domain *p2m;

+    if ( d->nr_altp2m == 0 )
+        return;
+
     if ( v != current )
         vcpu_pause(v);

@@ -122,7 +131,12 @@ int p2m_init_altp2m(struct domain *d)
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);

     mm_lock_init(&d->arch.altp2m_list_lock);
-    for ( i = 0; i < MAX_ALTP2M; i++ )
+    d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m);
+
+    if ( !d->arch.altp2m_p2m )
+        return -ENOMEM;
+
+    for ( i = 0; i < d->nr_altp2m; i++ )
     {
         d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
         if ( p2m == NULL )
@@ -143,7 +157,10 @@ void p2m_teardown_altp2m(struct domain *d)
     unsigned int i;
     struct p2m_domain *p2m;

-    for ( i = 0; i < MAX_ALTP2M; i++ )
+    if ( !d->arch.altp2m_p2m )
+        return;
+
+    for ( i = 0; i < d->nr_altp2m; i++ )
     {
         if ( !d->arch.altp2m_p2m[i] )
             continue;
@@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d)
         d->arch.altp2m_p2m[i] = NULL;
         p2m_free_one(p2m);
     }
+
+    XFREE(d->arch.altp2m_p2m);
 }

 int altp2m_get_effective_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn,
@@ -200,7 +219,7 @@ bool p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx)
     struct domain *d = v->domain;
     bool rc = false;

-    if ( idx >= MAX_ALTP2M )
+    if ( idx >= d->nr_altp2m )
         return rc;

     altp2m_list_lock(d);
@@ -306,7 +325,7 @@ static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
 {
     struct p2m_domain *p2m;

-    ASSERT(idx < MAX_ALTP2M);
+    ASSERT(idx < d->nr_altp2m);
     p2m = altp2m_get_p2m(d, idx);

     p2m_lock(p2m);
@@ -332,7 +351,7 @@ void p2m_flush_altp2m(struct domain *d)

     altp2m_list_lock(d);

-    for ( i = 0; i < MAX_ALTP2M; i++ )
+    for ( i = 0; i < d->nr_altp2m; i++ )
     {
         p2m_reset_altp2m(d, i, ALTP2M_DEACTIVATE);
         altp2m_set_eptp(d, i, mfn_x(INVALID_MFN));
@@ -348,7 +367,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx,
     struct p2m_domain *hostp2m, *p2m;
     int rc;

-    ASSERT(idx < MAX_ALTP2M);
+    ASSERT(idx < d->nr_altp2m);

     p2m = altp2m_get_p2m(d, idx);
     hostp2m = p2m_get_hostp2m(d);
@@ -388,7 +407,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
     int rc = -EINVAL;
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);

-    if ( idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) )
+    if ( idx >= d->nr_altp2m )
         return rc;

     altp2m_list_lock(d);
@@ -414,7 +433,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,

     altp2m_list_lock(d);

-    for ( i = 0; i < MAX_ALTP2M; i++ )
+    for ( i = 0; i < d->nr_altp2m; i++ )
     {
         if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) )
             continue;
@@ -436,7 +455,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
     struct p2m_domain *p2m;
     int rc = -EBUSY;

-    if ( !idx || idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) )
+    if ( !idx || idx >= d->nr_altp2m )
         return rc;

     rc = domain_pause_except_self(d);
@@ -471,7 +490,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
     struct vcpu *v;
     int rc = -EINVAL;

-    if ( idx >= MAX_ALTP2M )
+    if ( idx >= d->nr_altp2m )
         return rc;

     rc = domain_pause_except_self(d);
@@ -506,8 +525,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     mfn_t mfn;
     int rc = -EINVAL;

-    if ( idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
-         altp2m_get_eptp(d, idx) == mfn_x(INVALID_MFN) )
+    if ( idx >= d->nr_altp2m || altp2m_get_eptp(d, idx) == mfn_x(INVALID_MFN) )
         return rc;

     hp2m = p2m_get_hostp2m(d);
@@ -567,7 +585,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,

     altp2m_list_lock(d);

-    for ( i = 0; i < MAX_ALTP2M; i++ )
+    for ( i = 0; i < d->nr_altp2m; i++ )
     {
         p2m_type_t t;
         p2m_access_t a;
@@ -590,7 +608,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
             else
             {
                 /* At least 2 altp2m's impacted, so reset everything */
-                for ( i = 0; i < MAX_ALTP2M; i++ )
+                for ( i = 0; i < d->nr_altp2m; i++ )
                 {
                     if ( i == last_reset_idx ||
                          altp2m_get_eptp(d, i) == mfn_x(INVALID_MFN) )
@@ -654,7 +672,7 @@ int p2m_set_suppress_ve_multi(struct domain *d,

     if ( sve->view > 0 )
     {
-        if ( sve->view >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
+        if ( sve->view >= d->nr_altp2m ||
              altp2m_get_eptp(d, sve->view) == mfn_x(INVALID_MFN) )
             return -EINVAL;

@@ -721,7 +739,7 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,

     if ( altp2m_idx > 0 )
     {
-        if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
+        if ( altp2m_idx >= d->nr_altp2m ||
              altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) )
             return -EINVAL;

@@ -756,9 +774,9 @@ int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,

     /*
      * Eptp index is correlated with altp2m index and should not exceed
-     * min(MAX_ALTP2M, MAX_EPTP).
+     * d->nr_altp2m.
      */
-    if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
+    if ( altp2m_idx >= d->nr_altp2m ||
          altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) )
         rc = -EINVAL;
     else if ( visible )
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 8fc8348152..8b23792a0d 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -515,7 +515,7 @@ int hap_enable(struct domain *d, u32 mode)
             altp2m_set_visible_eptp(d, i, mfn_x(INVALID_MFN));
         }

-        for ( i = 0; i < MAX_ALTP2M; i++ )
+        for ( i = 0; i < d->nr_altp2m; i++ )
         {
             rv = p2m_alloc_table(altp2m_get_p2m(d, i));
             if ( rv != 0 )
@@ -538,8 +538,8 @@ void hap_final_teardown(struct domain *d)
     unsigned int i;

     if ( hvm_altp2m_supported() )
-        for ( i = 0; i < MAX_ALTP2M; i++ )
-            p2m_teardown(d->arch.altp2m_p2m[i], true, NULL);
+        for ( i = 0; i < d->nr_altp2m; i++ )
+            p2m_teardown(altp2m_get_p2m(d, i), true, NULL);

     /* Destroy nestedp2m's first */
     for (i = 0; i < MAX_NESTEDP2M; i++) {
@@ -590,7 +590,7 @@ void hap_teardown(struct domain *d, bool *preempted)
         FREE_XENHEAP_PAGE(d->arch.altp2m_eptp);
         FREE_XENHEAP_PAGE(d->arch.altp2m_visible_eptp);

-        for ( i = 0; i < MAX_ALTP2M; i++ )
+        for ( i = 0; i < d->nr_altp2m; i++ )
         {
             p2m_teardown(altp2m_get_p2m(d, i), false, preempted);
             if ( preempted && *preempted )
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 43f3a8c2aa..669a0d0a54 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -347,7 +347,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
     /* altp2m view 0 is treated as the hostp2m */
     if ( altp2m_idx )
     {
-        if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
+        if ( altp2m_idx >= d->nr_altp2m ||
              altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) )
             return -EINVAL;

@@ -402,7 +402,7 @@ long p2m_set_mem_access_multi(struct domain *d,
     /* altp2m view 0 is treated as the hostp2m */
     if ( altp2m_idx )
     {
-        if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
+        if ( altp2m_idx >= d->nr_altp2m ||
              altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) )
             return -EINVAL;

@@ -464,7 +464,7 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
     }
     else if ( altp2m_idx ) /* altp2m view 0 is treated as the hostp2m */
     {
-        if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
+        if ( altp2m_idx >= d->nr_altp2m ||
              altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) )
             return -EINVAL;

@@ -483,7 +483,7 @@ void arch_p2m_set_access_required(struct domain *d, bool access_required)
     if ( altp2m_active(d) )
     {
         unsigned int i;
-        for ( i = 0; i < MAX_ALTP2M; i++ )
+        for ( i = 0; i < d->nr_altp2m; i++ )
         {
             struct p2m_domain *p2m = altp2m_get_p2m(d, i);

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 21ac361111..2139d13009 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -912,7 +912,7 @@ static int nominate_page(struct domain *d, gfn_t gfn,

         altp2m_list_lock(d);

-        for ( i = 0; i < MAX_ALTP2M; i++ )
+        for ( i = 0; i < d->nr_altp2m; i++ )
         {
             ap2m = altp2m_get_p2m(d, i);
             if ( !ap2m )
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index ed4252822e..f90c82f89b 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1293,7 +1293,7 @@ static void ept_set_ad_sync(struct domain *d, bool value)
     {
         unsigned int i;

-        for ( i = 0; i < MAX_ALTP2M; i++ )
+        for ( i = 0; i < d->nr_altp2m; i++ )
         {
             struct p2m_domain *p2m;

@@ -1519,7 +1519,7 @@ unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)

     altp2m_list_lock(d);

-    for ( i = 0; i < MAX_ALTP2M; i++ )
+    for ( i = 0; i < d->nr_altp2m; i++ )
     {
         if ( altp2m_get_eptp(d, i) == mfn_x(INVALID_MFN) )
             continue;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 30a44441ba..380b7ece9c 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -105,7 +105,7 @@ void p2m_change_entry_type_global(struct domain *d,
     {
         unsigned int i;

-        for ( i = 0; i < MAX_ALTP2M; i++ )
+        for ( i = 0; i < d->nr_altp2m; i++ )
         {
             if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) )
             {
@@ -140,7 +140,7 @@ void p2m_memory_type_changed(struct domain *d)
     {
         unsigned int i;

-        for ( i = 0; i < MAX_ALTP2M; i++ )
+        for ( i = 0; i < d->nr_altp2m; i++ )
         {
             if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) )
             {
@@ -913,7 +913,7 @@ void p2m_change_type_range(struct domain *d,
     {
         unsigned int i;

-        for ( i = 0; i < MAX_ALTP2M; i++ )
+        for ( i = 0; i < d->nr_altp2m; i++ )
         {
             if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) )
             {
@@ -986,7 +986,7 @@ int p2m_finish_type_change(struct domain *d,
     {
         unsigned int i;

-        for ( i = 0; i < MAX_ALTP2M; i++ )
+        for ( i = 0; i < d->nr_altp2m; i++ )
         {
             if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) )
             {
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 67cadb7c3f..776442cec0 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -610,6 +610,7 @@ struct domain *domain_create(domid_t domid,
     if ( config )
     {
         d->options = config->flags;
+        d->nr_altp2m = config->altp2m.nr;
         d->vmtrace_size = config->vmtrace_size;
     }

diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index dea399aa8e..056bbc82a2 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -103,7 +103,10 @@ struct xen_domctl_createdomain {
 /* Altp2m mode signaling uses bits [0, 1]. */
 #define XEN_DOMCTL_ALTP2M_mode_mask  (0x3U)
 #define XEN_DOMCTL_ALTP2M_mode(m)    ((m) & XEN_DOMCTL_ALTP2M_mode_mask)
-        uint32_t opts;
+        uint16_t opts;
+
+        /* Number of altp2ms to allocate. */
+        uint16_t nr;
     } altp2m;

     /* Per-vCPU buffer size in bytes.  0 to disable. */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 2dcd1d1a4f..7119f3c44f 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -610,6 +610,8 @@ struct domain
         unsigned int guest_request_sync          : 1;
     } monitor;

+    unsigned int nr_altp2m;    /* Number of altp2m tables */
+
     unsigned int vmtrace_size; /* Buffer size in bytes, or 0 to disable. */

 #ifdef CONFIG_ARGO
--
2.34.1

[PATCH for-4.19? v5 08/10] tools/libxl: Activate the altp2m_count feature

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

This commit activates the previously introduced altp2m_count parameter,
establishing the connection between libxl and Xen.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 tools/libs/light/libxl_create.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 11d2f282f5..5ad552c4ec 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -656,6 +656,10 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
             .max_grant_frames = b_info->max_grant_frames,
             .max_maptrack_frames = b_info->max_maptrack_frames,
             .grant_opts = XEN_DOMCTL_GRANT_version(b_info->max_grant_version),
+            .altp2m = {
+                .opts = 0, /* .opts will be set below */
+                .nr = b_info->altp2m_count,
+            },
             .vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SHIFT),
             .cpupool_id = info->poolid,
         };
--
2.34.1

[PATCH for-4.19? v5 09/10] xen/x86: Disallow creating domains with altp2m enabled and altp2m.nr == 0

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

Since libxl finally sends the altp2m.nr value, we can remove the previously
introduced temporary workaround.

Creating domain with enabled altp2m while setting altp2m.nr == 0 doesn't
make sense and it's probably not what user wants.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 xen/arch/x86/domain.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 011cffb07e..52bfeafe3f 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -747,8 +747,9 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)

         if ( !config->altp2m.nr )
         {
-            /* Fix the value to the legacy default */
-            config->altp2m.nr = 10;
+            dprintk(XENLOG_INFO,
+                    "altp2m must be requested with altp2m.nr > 0\n");
+            return -EINVAL;
         }

         if ( config->altp2m.nr > MAX_NR_ALTP2M )
--
2.34.1

[PATCH for-4.19? v5 10/10] tools/ocaml: Add altp2m_count parameter

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

Allow developers using the OCaml bindings to set the altp2m_count parameter.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 tools/ocaml/libs/xc/xenctrl.ml      |  1 +
 tools/ocaml/libs/xc/xenctrl.mli     |  1 +
 tools/ocaml/libs/xc/xenctrl_stubs.c | 19 +++++++++++++++----
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 2690f9a923..a3e50ac394 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -86,6 +86,7 @@ type domctl_create_config =
     max_maptrack_frames: int;
     max_grant_version: int;
     altp2m_opts: int32;
+    altp2m_count: int32;
     vmtrace_buf_kb: int32;
     cpupool_id: int32;
     arch: arch_domainconfig;
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index febbe1f6ae..b97021d3d2 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -78,6 +78,7 @@ type domctl_create_config = {
   max_maptrack_frames: int;
   max_grant_version: int;
   altp2m_opts: int32;
+  altp2m_count: int32;
   vmtrace_buf_kb: int32;
   cpupool_id: int32;
   arch: arch_domainconfig;
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index e6c977521f..78ae4967e6 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -211,13 +211,22 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 #define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
 #define VAL_MAX_GRANT_VERSION   Field(config, 8)
 #define VAL_ALTP2M_OPTS         Field(config, 9)
-#define VAL_VMTRACE_BUF_KB      Field(config, 10)
-#define VAL_CPUPOOL_ID          Field(config, 11)
-#define VAL_ARCH                Field(config, 12)
+#define VAL_ALTP2M_COUNT        Field(config, 10)
+#define VAL_VMTRACE_BUF_KB      Field(config, 11)
+#define VAL_CPUPOOL_ID          Field(config, 12)
+#define VAL_ARCH                Field(config, 13)

 	uint32_t domid = Int_val(wanted_domid);
+	uint32_t altp2m_opts = Int32_val(VAL_ALTP2M_OPTS);
+	uint32_t altp2m_nr = Int32_val(VAL_ALTP2M_COUNT);
 	uint64_t vmtrace_size = Int32_val(VAL_VMTRACE_BUF_KB);

+	if ( altp2m_opts != (uint16_t)altp2m_opts )
+		caml_invalid_argument("altp2m_opts");
+
+	if ( altp2m_nr != (uint16_t)altp2m_nr )
+		caml_invalid_argument("altp2m_count");
+
 	vmtrace_size = ROUNDUP(vmtrace_size << 10, XC_PAGE_SHIFT);
 	if ( vmtrace_size != (uint32_t)vmtrace_size )
 		caml_invalid_argument("vmtrace_buf_kb");
@@ -232,7 +241,8 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 		.grant_opts =
 		    XEN_DOMCTL_GRANT_version(Int_val(VAL_MAX_GRANT_VERSION)),
 		.altp2m = {
-			.opts = Int32_val(VAL_ALTP2M_OPTS),
+			.opts = altp2m_opts,
+			.nr = altp2m_nr,
 		},
 		.vmtrace_size = vmtrace_size,
 		.cpupool_id = Int32_val(VAL_CPUPOOL_ID),
@@ -292,6 +302,7 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
 #undef VAL_ARCH
 #undef VAL_CPUPOOL_ID
 #undef VAL_VMTRACE_BUF_KB
+#undef VAL_ALTP2M_COUNT
 #undef VAL_ALTP2M_OPTS
 #undef VAL_MAX_GRANT_VERSION
 #undef VAL_MAX_MAPTRACK_FRAMES
--
2.34.1

Re: [PATCH for-4.19? v5 03/10] xen: Refactor altp2m options into a structured format

[Jan Beulich]

On 02.06.2024 22:04, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
> 
> Encapsulate the altp2m options within a struct. This change is preparatory
> and sets the groundwork for introducing additional parameter in subsequent
> commit.
> 
> Signed-off-by: Petr Beneš <w1benny@gmail.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com> # hypervisor

Re: [PATCH for-4.19? v5 06/10] x86/altp2m: Introduce accessor functions for safer array indexing

[Jan Beulich]

On 02.06.2024 22:04, Petr Beneš wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4887,10 +4887,10 @@ bool asmlinkage vmx_vmenter_helper(const struct cpu_user_regs *regs)
> 
>              for ( i = 0; i < MAX_ALTP2M; ++i )
>              {
> -                if ( currd->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
> +                if ( altp2m_get_eptp(currd, i) == mfn_x(INVALID_MFN) )
>                      continue;
> 
> -                ept = &currd->arch.altp2m_p2m[i]->ept;
> +                ept = &altp2m_get_p2m(currd, i)->ept;
>                  if ( cpumask_test_cpu(cpu, ept->invalidate) )
>                  {
>                      cpumask_clear_cpu(cpu, ept->invalidate);

I'm not convinced we want to add the extra overhead in cases like
this one, where we shouldn't need it. I'd like to hear other
maintainers' views.

> --- a/xen/arch/x86/include/asm/altp2m.h
> +++ b/xen/arch/x86/include/asm/altp2m.h
> @@ -19,6 +19,38 @@ static inline bool altp2m_active(const struct domain *d)
>      return d->arch.altp2m_active;
>  }
> 
> +static inline struct p2m_domain *altp2m_get_p2m(const struct domain* d,

Nit: Style (misplaced *).

> +                                                unsigned int idx)
> +{
> +    return d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
> +}
> +
> +static inline uint64_t altp2m_get_eptp(const struct domain* d,

Same here. And more further down.

Further: At this point it's not necessary yet to switch away from
array_access_nospec(). You doing so right away is probably okay, but
then needs justifying in the description.

Jan

Re: [PATCH for-4.19? v5 00/10] x86: Make MAX_ALTP2M configurable

[Christian Lindig]

> On 2 Jun 2024, at 21:04, Petr Beneš <w1benny@gmail.com> wrote:
> 
> tools/ocaml/libs/xc/xenctrl.ml       |   2 +
> tools/ocaml/libs/xc/xenctrl.mli      |   2 +
> tools/ocaml/libs/xc/xenctrl_stubs.c  |  40 +++++++---

Acked-by: Christian Lindig <christian.lindig@cloud.com>

Re: [PATCH for-4.19? v5 03/10] xen: Refactor altp2m options into a structured format

[Julien Grall]

Hi,

On 02/06/2024 21:04, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
> 
> Encapsulate the altp2m options within a struct. This change is preparatory
> and sets the groundwork for introducing additional parameter in subsequent
> commit.
> 
> Signed-off-by: Petr Beneš <w1benny@gmail.com>
> ---
>   tools/libs/light/libxl_create.c     | 6 +++---
>   tools/ocaml/libs/xc/xenctrl_stubs.c | 4 +++-
>   xen/arch/arm/domain.c               | 2 +-

For the small change in Arm:

Acked-by: Julien Grall <jgrall@amazon.com> # arm

>   xen/arch/x86/domain.c               | 4 ++--
>   xen/arch/x86/hvm/hvm.c              | 2 +-
>   xen/include/public/domctl.h         | 4 +++-
>   6 files changed, 13 insertions(+), 9 deletions(-)

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Julien Grall]

Hi Petr,

On 02/06/2024 21:04, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
> 
> x86: Make the maximum number of altp2m views configurable
> 
> This commit introduces the ability to configure the maximum number of altp2m
> views for the domain during its creation. Previously, the limits were hardcoded
> to a maximum of 10. This change allows for greater flexibility in environments
> that require more or fewer altp2m views.
> 
> The maximum configurable limit for nr_altp2m on x86 is now set to
> MAX_NR_ALTP2M (which currently holds the MAX_EPTP value - 512). This cap is
> linked to the architectural limit of the EPTP-switching VMFUNC, which supports
> up to 512 entries. Despite there being no inherent need for limiting nr_altp2m
> in scenarios not utilizing VMFUNC, decoupling these components would necessitate
> substantial code changes.
> 
> xen_domctl_createdomain::altp2m is extended for a new field `nr`, that will
> configure this limit for a domain. Additionally, previous altp2m.opts value
> has been reduced from uint32_t to uint16_t so that both of these fields occupy
> as little space as possible.
> 
> altp2m_get_p2m() function is modified to respect the new nr_altp2m value.
> Accessor functions that operate on EPT arrays are unmodified, since these
> arrays always have fixed size of MAX_EPTP.
> 
> A dummy hvm_altp2m_supported() function is introduced for non-HVM builds, so
> that the compilation won't fail for them.
> 
> Additional sanitization is introduced in the x86/arch_sanitise_domain_config
> to fix the altp2m.nr value to 10 if it is previously set to 0. This behavior
> is only temporary and immediately removed in the upcoming commit (which will
> disallow creating a domain with enabled altp2m with zero nr_altp2m).
> 
> The reason for this temporary workaround is to retain the legacy behavior
> until the feature is fully activated in libxl.
> 
> Also, arm/arch_sanitise_domain_config is extended to not allow requesting
> non-zero altp2ms.
> 
> Signed-off-by: Petr Beneš <w1benny@gmail.com>

For the small change in Arm:

Acked-by: Julien Grall <jgrall@amazon.com> # arm

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Jan Beulich]

On 02.06.2024 22:04, Petr Beneš wrote:
> @@ -5245,7 +5251,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx)
>      if ( !hvm_is_singlestep_supported() )
>          return;
> 
> -    if ( p2midx >= MAX_ALTP2M )
> +    if ( p2midx >= v->domain->nr_altp2m )
>          return;

As (iirc) indicated before, just like you don't add a d local variable
here or ...

> --- a/xen/arch/x86/include/asm/p2m.h
> +++ b/xen/arch/x86/include/asm/p2m.h
> @@ -887,7 +887,7 @@ static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)
>      if ( index == INVALID_ALTP2M )
>          return NULL;
> 
> -    BUG_ON(index >= MAX_ALTP2M);
> +    BUG_ON(index >= v->domain->nr_altp2m);
> 
>      return altp2m_get_p2m(v->domain, index);
>  }
> @@ -898,7 +898,7 @@ static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx)
>      struct p2m_domain *orig;
>      struct p2m_domain *ap2m;
> 
> -    BUG_ON(idx >= MAX_ALTP2M);
> +    BUG_ON(idx >= v->domain->nr_altp2m);
> 
>      if ( idx == vcpu_altp2m(v).p2midx )
>          return false;

... in either of these, I see little reason to have such ...

> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -15,6 +15,11 @@
>  void
>  altp2m_vcpu_initialise(struct vcpu *v)
>  {
> +    struct domain *d = v->domain;
> +
> +    if ( d->nr_altp2m == 0 )
> +        return;
> +
>      if ( v != current )
>          vcpu_pause(v);
> 
> @@ -30,8 +35,12 @@ altp2m_vcpu_initialise(struct vcpu *v)
>  void
>  altp2m_vcpu_destroy(struct vcpu *v)
>  {
> +    struct domain *d = v->domain;
>      struct p2m_domain *p2m;
> 
> +    if ( d->nr_altp2m == 0 )
> +        return;
> +
>      if ( v != current )
>          vcpu_pause(v);

... in both of these.

> @@ -122,7 +131,12 @@ int p2m_init_altp2m(struct domain *d)
>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
> 
>      mm_lock_init(&d->arch.altp2m_list_lock);
> -    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m);
> +
> +    if ( !d->arch.altp2m_p2m )
> +        return -ENOMEM;

This isn't really needed, is it? Both ...

> +    for ( i = 0; i < d->nr_altp2m; i++ )

... this and ...

>      {
>          d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
>          if ( p2m == NULL )
> @@ -143,7 +157,10 @@ void p2m_teardown_altp2m(struct domain *d)
>      unsigned int i;
>      struct p2m_domain *p2m;
> 
> -    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    if ( !d->arch.altp2m_p2m )
> +        return;
> +
> +    for ( i = 0; i < d->nr_altp2m; i++ )
>      {
>          if ( !d->arch.altp2m_p2m[i] )
>              continue;
> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d)
>          d->arch.altp2m_p2m[i] = NULL;
>          p2m_free_one(p2m);
>      }
> +
> +    XFREE(d->arch.altp2m_p2m);
>  }

... this ought to be fine without?

> @@ -538,8 +538,8 @@ void hap_final_teardown(struct domain *d)
>      unsigned int i;
> 
>      if ( hvm_altp2m_supported() )
> -        for ( i = 0; i < MAX_ALTP2M; i++ )
> -            p2m_teardown(d->arch.altp2m_p2m[i], true, NULL);
> +        for ( i = 0; i < d->nr_altp2m; i++ )
> +            p2m_teardown(altp2m_get_p2m(d, i), true, NULL);

Shouldn't the switch to altp2m_get_p2m() be part of the respective
earlier patch?

Jan

Re: [PATCH for-4.19? v5 09/10] xen/x86: Disallow creating domains with altp2m enabled and altp2m.nr == 0

[Jan Beulich]

On 02.06.2024 22:04, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
> 
> Since libxl finally sends the altp2m.nr value, we can remove the previously
> introduced temporary workaround.
> 
> Creating domain with enabled altp2m while setting altp2m.nr == 0 doesn't
> make sense and it's probably not what user wants.

Yet: Do we need separate count and flag anymore? Can't "nr != 0"
take the place of the flag being true?

Jan

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Petr Beneš]

On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> > @@ -122,7 +131,12 @@ int p2m_init_altp2m(struct domain *d)
> >      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
> >
> >      mm_lock_init(&d->arch.altp2m_list_lock);
> > -    for ( i = 0; i < MAX_ALTP2M; i++ )
> > +    d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m);
> > +
> > +    if ( !d->arch.altp2m_p2m )
> > +        return -ENOMEM;
>
> This isn't really needed, is it? Both ...
>
> > +    for ( i = 0; i < d->nr_altp2m; i++ )
>
> ... this and ...
>
> >      {
> >          d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
> >          if ( p2m == NULL )
> > @@ -143,7 +157,10 @@ void p2m_teardown_altp2m(struct domain *d)
> >      unsigned int i;
> >      struct p2m_domain *p2m;
> >
> > -    for ( i = 0; i < MAX_ALTP2M; i++ )
> > +    if ( !d->arch.altp2m_p2m )
> > +        return;
> > +
> > +    for ( i = 0; i < d->nr_altp2m; i++ )
> >      {
> >          if ( !d->arch.altp2m_p2m[i] )
> >              continue;
> > @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d)
> >          d->arch.altp2m_p2m[i] = NULL;
> >          p2m_free_one(p2m);
> >      }
> > +
> > +    XFREE(d->arch.altp2m_p2m);
> >  }
>
> ... this ought to be fine without?

Could you, please, elaborate? I honestly don't know what you mean here
(by "this isn't needed").

P.

Re: [PATCH for-4.19? v5 09/10] xen/x86: Disallow creating domains with altp2m enabled and altp2m.nr == 0

[Petr Beneš]

The flag isn't bool. It can hold one of 3 values (besides 0).

P.

On Thu, Jun 6, 2024 at 9:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.06.2024 22:04, Petr Beneš wrote:
> > From: Petr Beneš <w1benny@gmail.com>
> >
> > Since libxl finally sends the altp2m.nr value, we can remove the previously
> > introduced temporary workaround.
> >
> > Creating domain with enabled altp2m while setting altp2m.nr == 0 doesn't
> > make sense and it's probably not what user wants.
>
> Yet: Do we need separate count and flag anymore? Can't "nr != 0"
> take the place of the flag being true?
>
> Jan

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Jan Beulich]

On 09.06.2024 01:06, Petr Beneš wrote:
> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>> @@ -122,7 +131,12 @@ int p2m_init_altp2m(struct domain *d)
>>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>
>>>      mm_lock_init(&d->arch.altp2m_list_lock);
>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
>>> +    d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m);
>>> +
>>> +    if ( !d->arch.altp2m_p2m )
>>> +        return -ENOMEM;
>>
>> This isn't really needed, is it? Both ...
>>
>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
>>
>> ... this and ...
>>
>>>      {
>>>          d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
>>>          if ( p2m == NULL )
>>> @@ -143,7 +157,10 @@ void p2m_teardown_altp2m(struct domain *d)
>>>      unsigned int i;
>>>      struct p2m_domain *p2m;
>>>
>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
>>> +    if ( !d->arch.altp2m_p2m )
>>> +        return;

I'm sorry, the question was meant to be on this if() instead.

>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
>>>      {
>>>          if ( !d->arch.altp2m_p2m[i] )
>>>              continue;
>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d)
>>>          d->arch.altp2m_p2m[i] = NULL;
>>>          p2m_free_one(p2m);
>>>      }
>>> +
>>> +    XFREE(d->arch.altp2m_p2m);
>>>  }
>>
>> ... this ought to be fine without?
> 
> Could you, please, elaborate? I honestly don't know what you mean here
> (by "this isn't needed").

I hope the above correction is enough?

Jan

Re: [PATCH for-4.19? v5 09/10] xen/x86: Disallow creating domains with altp2m enabled and altp2m.nr == 0

[Jan Beulich]

On 09.06.2024 01:08, Petr Beneš wrote:
> The flag isn't bool. It can hold one of 3 values (besides 0).

Oh, sorry, yes.

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

> On Thu, Jun 6, 2024 at 9:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 02.06.2024 22:04, Petr Beneš wrote:
>>> From: Petr Beneš <w1benny@gmail.com>
>>>
>>> Since libxl finally sends the altp2m.nr value, we can remove the previously
>>> introduced temporary workaround.
>>>
>>> Creating domain with enabled altp2m while setting altp2m.nr == 0 doesn't
>>> make sense and it's probably not what user wants.
>>
>> Yet: Do we need separate count and flag anymore? Can't "nr != 0"
>> take the place of the flag being true?
>>
>> Jan

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Petr Beneš]

On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.06.2024 01:06, Petr Beneš wrote:
> > On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>> @@ -122,7 +131,12 @@ int p2m_init_altp2m(struct domain *d)
> >>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
> >>>
> >>>      mm_lock_init(&d->arch.altp2m_list_lock);
> >>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
> >>> +    d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m);
> >>> +
> >>> +    if ( !d->arch.altp2m_p2m )
> >>> +        return -ENOMEM;
> >>
> >> This isn't really needed, is it? Both ...
> >>
> >>> +    for ( i = 0; i < d->nr_altp2m; i++ )
> >>
> >> ... this and ...
> >>
> >>>      {
> >>>          d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
> >>>          if ( p2m == NULL )
> >>> @@ -143,7 +157,10 @@ void p2m_teardown_altp2m(struct domain *d)
> >>>      unsigned int i;
> >>>      struct p2m_domain *p2m;
> >>>
> >>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
> >>> +    if ( !d->arch.altp2m_p2m )
> >>> +        return;
>
> I'm sorry, the question was meant to be on this if() instead.
>
> >>> +    for ( i = 0; i < d->nr_altp2m; i++ )
> >>>      {
> >>>          if ( !d->arch.altp2m_p2m[i] )
> >>>              continue;
> >>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d)
> >>>          d->arch.altp2m_p2m[i] = NULL;
> >>>          p2m_free_one(p2m);
> >>>      }
> >>> +
> >>> +    XFREE(d->arch.altp2m_p2m);
> >>>  }
> >>
> >> ... this ought to be fine without?
> >
> > Could you, please, elaborate? I honestly don't know what you mean here
> > (by "this isn't needed").
>
> I hope the above correction is enough?

I'm sorry, but not really? I feel like I'm blind but I can't see
anything I could remove without causing (or risking) crash.

P.

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Jan Beulich]

On 10.06.2024 11:10, Petr Beneš wrote:
> On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 09.06.2024 01:06, Petr Beneš wrote:
>>> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>> @@ -122,7 +131,12 @@ int p2m_init_altp2m(struct domain *d)
>>>>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>>>
>>>>>      mm_lock_init(&d->arch.altp2m_list_lock);
>>>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
>>>>> +    d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m);
>>>>> +
>>>>> +    if ( !d->arch.altp2m_p2m )
>>>>> +        return -ENOMEM;
>>>>
>>>> This isn't really needed, is it? Both ...
>>>>
>>>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
>>>>
>>>> ... this and ...
>>>>
>>>>>      {
>>>>>          d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
>>>>>          if ( p2m == NULL )
>>>>> @@ -143,7 +157,10 @@ void p2m_teardown_altp2m(struct domain *d)
>>>>>      unsigned int i;
>>>>>      struct p2m_domain *p2m;
>>>>>
>>>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
>>>>> +    if ( !d->arch.altp2m_p2m )
>>>>> +        return;
>>
>> I'm sorry, the question was meant to be on this if() instead.
>>
>>>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
>>>>>      {
>>>>>          if ( !d->arch.altp2m_p2m[i] )
>>>>>              continue;
>>>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d)
>>>>>          d->arch.altp2m_p2m[i] = NULL;
>>>>>          p2m_free_one(p2m);
>>>>>      }
>>>>> +
>>>>> +    XFREE(d->arch.altp2m_p2m);
>>>>>  }
>>>>
>>>> ... this ought to be fine without?
>>>
>>> Could you, please, elaborate? I honestly don't know what you mean here
>>> (by "this isn't needed").
>>
>> I hope the above correction is enough?
> 
> I'm sorry, but not really? I feel like I'm blind but I can't see
> anything I could remove without causing (or risking) crash.

The loop is going to do nothing when d->nr_altp2m == 0, and the XFREE() is
going to do nothing when d->arch.altp2m_p2m == NULL. Hence what does the
if() guard against? IOW what possible crashes are you seeing that I don't
see?

Jan

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Petr Beneš]

On Mon, Jun 10, 2024 at 12:16 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 10.06.2024 11:10, Petr Beneš wrote:
> > On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 09.06.2024 01:06, Petr Beneš wrote:
> >>> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> @@ -122,7 +131,12 @@ int p2m_init_altp2m(struct domain *d)
> >>>>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
> >>>>>
> >>>>>      mm_lock_init(&d->arch.altp2m_list_lock);
> >>>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
> >>>>> +    d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m);
> >>>>> +
> >>>>> +    if ( !d->arch.altp2m_p2m )
> >>>>> +        return -ENOMEM;
> >>>>
> >>>> This isn't really needed, is it? Both ...
> >>>>
> >>>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
> >>>>
> >>>> ... this and ...
> >>>>
> >>>>>      {
> >>>>>          d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
> >>>>>          if ( p2m == NULL )
> >>>>> @@ -143,7 +157,10 @@ void p2m_teardown_altp2m(struct domain *d)
> >>>>>      unsigned int i;
> >>>>>      struct p2m_domain *p2m;
> >>>>>
> >>>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
> >>>>> +    if ( !d->arch.altp2m_p2m )
> >>>>> +        return;
> >>
> >> I'm sorry, the question was meant to be on this if() instead.
> >>
> >>>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
> >>>>>      {
> >>>>>          if ( !d->arch.altp2m_p2m[i] )
> >>>>>              continue;
> >>>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d)
> >>>>>          d->arch.altp2m_p2m[i] = NULL;
> >>>>>          p2m_free_one(p2m);
> >>>>>      }
> >>>>> +
> >>>>> +    XFREE(d->arch.altp2m_p2m);
> >>>>>  }
> >>>>
> >>>> ... this ought to be fine without?
> >>>
> >>> Could you, please, elaborate? I honestly don't know what you mean here
> >>> (by "this isn't needed").
> >>
> >> I hope the above correction is enough?
> >
> > I'm sorry, but not really? I feel like I'm blind but I can't see
> > anything I could remove without causing (or risking) crash.
>
> The loop is going to do nothing when d->nr_altp2m == 0, and the XFREE() is
> going to do nothing when d->arch.altp2m_p2m == NULL. Hence what does the
> if() guard against? IOW what possible crashes are you seeing that I don't
> see?

I see now. I was seeing ghosts - my thinking was that if
p2m_init_altp2m fails to allocate altp2m_p2m, it will call
p2m_teardown_altp2m - which, without the if(), would start to iterate
through an array that is not allocated. It doesn't happen, it just
returns -ENOMEM.

So to reiterate:

    if ( !d->arch.altp2m_p2m )
        return;

... are we talking that this condition inside p2m_teardown_altp2m
isn't needed? Or is there anything else?

P.

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Jan Beulich]

On 10.06.2024 12:34, Petr Beneš wrote:
> On Mon, Jun 10, 2024 at 12:16 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 10.06.2024 11:10, Petr Beneš wrote:
>>> On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 09.06.2024 01:06, Petr Beneš wrote:
>>>>> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>> @@ -122,7 +131,12 @@ int p2m_init_altp2m(struct domain *d)
>>>>>>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>>>>>
>>>>>>>      mm_lock_init(&d->arch.altp2m_list_lock);
>>>>>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
>>>>>>> +    d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m);
>>>>>>> +
>>>>>>> +    if ( !d->arch.altp2m_p2m )
>>>>>>> +        return -ENOMEM;
>>>>>>
>>>>>> This isn't really needed, is it? Both ...
>>>>>>
>>>>>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
>>>>>>
>>>>>> ... this and ...
>>>>>>
>>>>>>>      {
>>>>>>>          d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
>>>>>>>          if ( p2m == NULL )
>>>>>>> @@ -143,7 +157,10 @@ void p2m_teardown_altp2m(struct domain *d)
>>>>>>>      unsigned int i;
>>>>>>>      struct p2m_domain *p2m;
>>>>>>>
>>>>>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
>>>>>>> +    if ( !d->arch.altp2m_p2m )
>>>>>>> +        return;
>>>>
>>>> I'm sorry, the question was meant to be on this if() instead.
>>>>
>>>>>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
>>>>>>>      {
>>>>>>>          if ( !d->arch.altp2m_p2m[i] )
>>>>>>>              continue;
>>>>>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d)
>>>>>>>          d->arch.altp2m_p2m[i] = NULL;
>>>>>>>          p2m_free_one(p2m);
>>>>>>>      }
>>>>>>> +
>>>>>>> +    XFREE(d->arch.altp2m_p2m);
>>>>>>>  }
>>>>>>
>>>>>> ... this ought to be fine without?
>>>>>
>>>>> Could you, please, elaborate? I honestly don't know what you mean here
>>>>> (by "this isn't needed").
>>>>
>>>> I hope the above correction is enough?
>>>
>>> I'm sorry, but not really? I feel like I'm blind but I can't see
>>> anything I could remove without causing (or risking) crash.
>>
>> The loop is going to do nothing when d->nr_altp2m == 0, and the XFREE() is
>> going to do nothing when d->arch.altp2m_p2m == NULL. Hence what does the
>> if() guard against? IOW what possible crashes are you seeing that I don't
>> see?
> 
> I see now. I was seeing ghosts - my thinking was that if
> p2m_init_altp2m fails to allocate altp2m_p2m, it will call
> p2m_teardown_altp2m - which, without the if(), would start to iterate
> through an array that is not allocated. It doesn't happen, it just
> returns -ENOMEM.
> 
> So to reiterate:
> 
>     if ( !d->arch.altp2m_p2m )
>         return;
> 
> ... are we talking that this condition inside p2m_teardown_altp2m
> isn't needed?

I'm not sure about "isn't" vs "shouldn't". The call from p2m_final_teardown()
also needs to remain safe to make. Which may require that upon allocation
failure you zap d->nr_altp2m. Or which alternatively may mean that the if()
actually needs to stay.

> Or is there anything else?

There was also the question of whether to guard the allocation, to avoid a
de-generate xmalloc_array() of zero size. Yet in the interest of avoiding
not strictly necessary conditionals, that may well want to remain as is.

Jan

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Petr Beneš]

On Mon, Jun 10, 2024 at 1:21 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 10.06.2024 12:34, Petr Beneš wrote:
> > On Mon, Jun 10, 2024 at 12:16 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 10.06.2024 11:10, Petr Beneš wrote:
> >>> On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 09.06.2024 01:06, Petr Beneš wrote:
> >>>>> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>> @@ -122,7 +131,12 @@ int p2m_init_altp2m(struct domain *d)
> >>>>>>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
> >>>>>>>
> >>>>>>>      mm_lock_init(&d->arch.altp2m_list_lock);
> >>>>>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
> >>>>>>> +    d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m);
> >>>>>>> +
> >>>>>>> +    if ( !d->arch.altp2m_p2m )
> >>>>>>> +        return -ENOMEM;
> >>>>>>
> >>>>>> This isn't really needed, is it? Both ...
> >>>>>>
> >>>>>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
> >>>>>>
> >>>>>> ... this and ...
> >>>>>>
> >>>>>>>      {
> >>>>>>>          d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
> >>>>>>>          if ( p2m == NULL )
> >>>>>>> @@ -143,7 +157,10 @@ void p2m_teardown_altp2m(struct domain *d)
> >>>>>>>      unsigned int i;
> >>>>>>>      struct p2m_domain *p2m;
> >>>>>>>
> >>>>>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
> >>>>>>> +    if ( !d->arch.altp2m_p2m )
> >>>>>>> +        return;
> >>>>
> >>>> I'm sorry, the question was meant to be on this if() instead.
> >>>>
> >>>>>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
> >>>>>>>      {
> >>>>>>>          if ( !d->arch.altp2m_p2m[i] )
> >>>>>>>              continue;
> >>>>>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d)
> >>>>>>>          d->arch.altp2m_p2m[i] = NULL;
> >>>>>>>          p2m_free_one(p2m);
> >>>>>>>      }
> >>>>>>> +
> >>>>>>> +    XFREE(d->arch.altp2m_p2m);
> >>>>>>>  }
> >>>>>>
> >>>>>> ... this ought to be fine without?
> >>>>>
> >>>>> Could you, please, elaborate? I honestly don't know what you mean here
> >>>>> (by "this isn't needed").
> >>>>
> >>>> I hope the above correction is enough?
> >>>
> >>> I'm sorry, but not really? I feel like I'm blind but I can't see
> >>> anything I could remove without causing (or risking) crash.
> >>
> >> The loop is going to do nothing when d->nr_altp2m == 0, and the XFREE() is
> >> going to do nothing when d->arch.altp2m_p2m == NULL. Hence what does the
> >> if() guard against? IOW what possible crashes are you seeing that I don't
> >> see?
> >
> > I see now. I was seeing ghosts - my thinking was that if
> > p2m_init_altp2m fails to allocate altp2m_p2m, it will call
> > p2m_teardown_altp2m - which, without the if(), would start to iterate
> > through an array that is not allocated. It doesn't happen, it just
> > returns -ENOMEM.
> >
> > So to reiterate:
> >
> >     if ( !d->arch.altp2m_p2m )
> >         return;
> >
> > ... are we talking that this condition inside p2m_teardown_altp2m
> > isn't needed?
>
> I'm not sure about "isn't" vs "shouldn't". The call from p2m_final_teardown()
> also needs to remain safe to make. Which may require that upon allocation
> failure you zap d->nr_altp2m. Or which alternatively may mean that the if()
> actually needs to stay.

True, p2m_final_teardown is called whenever p2m_init (and by extension
p2m_init_altp2m) fails. Which means that condition must stay - or, as
you suggested, reset nr_altp2m to 0.

I would rather leave the code as is. Modifying nr_altp2m would (in my
opinion) feel semantically incorrect, as that value should behave more
or less as const, that is initialized once.

> > Or is there anything else?
>
> There was also the question of whether to guard the allocation, to avoid a
> de-generate xmalloc_array() of zero size. Yet in the interest of avoiding
> not strictly necessary conditionals, that may well want to remain as is.

True, nr_altp2m would mean zero-sized allocation, as p2m_init_altp2m
is called unconditionally (when booted with altp2m=1). Is it a
problem, though?

P.

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Petr Beneš]

> (when booted with altp2m=1)

Sorry, forgot to remove this statement before sending the email, it's
not really true.

I wanted to add that as I wrote in a previous email exchange - altp2m
should be ideally initialized only when it's requested - as opposed to
the current situation, when it's initialized in the domain_create.
However, that is more suited for 4.20.

P.

Re: [PATCH for-4.19? v5 07/10] xen: Make the maximum number of altp2m views configurable for x86

[Jan Beulich]

On 10.06.2024 14:21, Petr Beneš wrote:
> On Mon, Jun 10, 2024 at 1:21 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 10.06.2024 12:34, Petr Beneš wrote:
>>> On Mon, Jun 10, 2024 at 12:16 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 10.06.2024 11:10, Petr Beneš wrote:
>>>>> On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 09.06.2024 01:06, Petr Beneš wrote:
>>>>>>> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>> @@ -122,7 +131,12 @@ int p2m_init_altp2m(struct domain *d)
>>>>>>>>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>>>>>>>
>>>>>>>>>      mm_lock_init(&d->arch.altp2m_list_lock);
>>>>>>>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
>>>>>>>>> +    d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m);
>>>>>>>>> +
>>>>>>>>> +    if ( !d->arch.altp2m_p2m )
>>>>>>>>> +        return -ENOMEM;
>>>>>>>>
>>>>>>>> This isn't really needed, is it? Both ...
>>>>>>>>
>>>>>>>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
>>>>>>>>
>>>>>>>> ... this and ...
>>>>>>>>
>>>>>>>>>      {
>>>>>>>>>          d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
>>>>>>>>>          if ( p2m == NULL )
>>>>>>>>> @@ -143,7 +157,10 @@ void p2m_teardown_altp2m(struct domain *d)
>>>>>>>>>      unsigned int i;
>>>>>>>>>      struct p2m_domain *p2m;
>>>>>>>>>
>>>>>>>>> -    for ( i = 0; i < MAX_ALTP2M; i++ )
>>>>>>>>> +    if ( !d->arch.altp2m_p2m )
>>>>>>>>> +        return;
>>>>>>
>>>>>> I'm sorry, the question was meant to be on this if() instead.
>>>>>>
>>>>>>>>> +    for ( i = 0; i < d->nr_altp2m; i++ )
>>>>>>>>>      {
>>>>>>>>>          if ( !d->arch.altp2m_p2m[i] )
>>>>>>>>>              continue;
>>>>>>>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d)
>>>>>>>>>          d->arch.altp2m_p2m[i] = NULL;
>>>>>>>>>          p2m_free_one(p2m);
>>>>>>>>>      }
>>>>>>>>> +
>>>>>>>>> +    XFREE(d->arch.altp2m_p2m);
>>>>>>>>>  }
>>>>>>>>
>>>>>>>> ... this ought to be fine without?
>>>>>>>
>>>>>>> Could you, please, elaborate? I honestly don't know what you mean here
>>>>>>> (by "this isn't needed").
>>>>>>
>>>>>> I hope the above correction is enough?
>>>>>
>>>>> I'm sorry, but not really? I feel like I'm blind but I can't see
>>>>> anything I could remove without causing (or risking) crash.
>>>>
>>>> The loop is going to do nothing when d->nr_altp2m == 0, and the XFREE() is
>>>> going to do nothing when d->arch.altp2m_p2m == NULL. Hence what does the
>>>> if() guard against? IOW what possible crashes are you seeing that I don't
>>>> see?
>>>
>>> I see now. I was seeing ghosts - my thinking was that if
>>> p2m_init_altp2m fails to allocate altp2m_p2m, it will call
>>> p2m_teardown_altp2m - which, without the if(), would start to iterate
>>> through an array that is not allocated. It doesn't happen, it just
>>> returns -ENOMEM.
>>>
>>> So to reiterate:
>>>
>>>     if ( !d->arch.altp2m_p2m )
>>>         return;
>>>
>>> ... are we talking that this condition inside p2m_teardown_altp2m
>>> isn't needed?
>>
>> I'm not sure about "isn't" vs "shouldn't". The call from p2m_final_teardown()
>> also needs to remain safe to make. Which may require that upon allocation
>> failure you zap d->nr_altp2m. Or which alternatively may mean that the if()
>> actually needs to stay.
> 
> True, p2m_final_teardown is called whenever p2m_init (and by extension
> p2m_init_altp2m) fails. Which means that condition must stay - or, as
> you suggested, reset nr_altp2m to 0.
> 
> I would rather leave the code as is. Modifying nr_altp2m would (in my
> opinion) feel semantically incorrect, as that value should behave more
> or less as const, that is initialized once.
> 
>>> Or is there anything else?
>>
>> There was also the question of whether to guard the allocation, to avoid a
>> de-generate xmalloc_array() of zero size. Yet in the interest of avoiding
>> not strictly necessary conditionals, that may well want to remain as is.
> 
> True, nr_altp2m would mean zero-sized allocation, as p2m_init_altp2m
> is called unconditionally (when booted with altp2m=1). Is it a
> problem, though?

Not a significant one. Initially I thought this would end up being a non-
zero-size allocation, which we might like to avoid. But as it's a zero-
size one, I think that's okay to leave as is.

Jan