* [PATCH for-4.19? v4 0/6] x86: Make MAX_ALTP2M configurable
@ 2024-05-18 11:02 Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 1/6] x86/p2m: Add braces for better code clarity Petr Beneš
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Petr Beneš @ 2024-05-18 11:02 UTC (permalink / raw)
To: xen-devel
Cc: Petr Beneš, Jan Beulich, Andrew Cooper, George Dunlap,
Roger Pau Monné, Nick Rosbrook, Anthony PERARD,
Juergen Gross, Julien Grall, Stefano Stabellini, Tamas K Lengyel,
Alexandru Isaila, Petre Pircalabu, Christian Lindig, David Scott
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.
Adjustments include:
- "Prepare" commits with style changes.
- Adding a new `altp2m_count` parameter to xl.
- Adding a new `altp2m_count` parameter to the OCaml bindings.
- Adding a new `altp2m_count` parameter to the xl.cfg manual.
- Adding a new `nr_altp2m` field into the `xen_domctl_createdomain`, which,
after sanity checks, is stored in newly introduced `nr_altp2m` field of
`struct domain` - leaving room for other architectures to implement the
altp2m feature.
- Replacing MAX_ALTP2M macro occurrences with `domain->nr_altp2m`.
This enhancement is particularly relevant for users leveraging Xen's features
for virtual machine introspection.
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.
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.
Petr Beneš (6):
x86/p2m: Add braces for better code clarity
tools/xl: Add altp2m_count parameter
docs/man: Add altp2m_count parameter to the xl.cfg manual
x86: Make the maximum number of altp2m views configurable
tools/libxl: Activate the altp2m_count feature
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 | 10 +++
tools/libs/light/libxl_types.idl | 1 +
tools/ocaml/libs/xc/xenctrl.ml | 1 +
tools/ocaml/libs/xc/xenctrl.mli | 1 +
tools/ocaml/libs/xc/xenctrl_stubs.c | 11 ++--
tools/xl/xl_parse.c | 4 ++
xen/arch/x86/domain.c | 12 ++++
xen/arch/x86/hvm/hvm.c | 8 ++-
xen/arch/x86/hvm/vmx/vmx.c | 2 +-
xen/arch/x86/include/asm/domain.h | 7 +--
xen/arch/x86/include/asm/p2m.h | 6 +-
xen/arch/x86/mm/altp2m.c | 91 +++++++++++++++++-----------
xen/arch/x86/mm/hap/hap.c | 6 +-
xen/arch/x86/mm/mem_access.c | 24 ++++----
xen/arch/x86/mm/mem_sharing.c | 2 +-
xen/arch/x86/mm/p2m-ept.c | 12 ++--
xen/arch/x86/mm/p2m.c | 12 ++--
xen/common/domain.c | 1 +
xen/include/public/domctl.h | 5 +-
xen/include/xen/sched.h | 2 +
24 files changed, 169 insertions(+), 74 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH for-4.19? v4 1/6] x86/p2m: Add braces for better code clarity
2024-05-18 11:02 [PATCH for-4.19? v4 0/6] x86: Make MAX_ALTP2M configurable Petr Beneš
@ 2024-05-18 11:02 ` Petr Beneš
2024-05-20 8:31 ` Roger Pau Monné
2024-05-18 11:02 ` [PATCH for-4.19? v4 2/6] tools/xl: Add altp2m_count parameter Petr Beneš
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Petr Beneš @ 2024-05-18 11:02 UTC (permalink / raw)
To: xen-devel
Cc: Petr Beneš, Jan Beulich, Andrew Cooper, George Dunlap,
Roger Pau Monné, Stefano Stabellini
From: Petr Beneš <w1benny@gmail.com>
No functional change.
Signed-off-by: Petr Beneš <w1benny@gmail.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/x86/mm/p2m.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 4a4620e870..db5d9b6c2a 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -106,6 +106,7 @@ void p2m_change_entry_type_global(struct domain *d,
unsigned int i;
for ( i = 0; i < MAX_ALTP2M; i++ )
+ {
if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
{
struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
@@ -114,6 +115,7 @@ void p2m_change_entry_type_global(struct domain *d,
change_entry_type_global(altp2m, ot, nt);
p2m_unlock(altp2m);
}
+ }
}
p2m_unlock(hostp2m);
@@ -139,6 +141,7 @@ void p2m_memory_type_changed(struct domain *d)
unsigned int i;
for ( i = 0; i < MAX_ALTP2M; i++ )
+ {
if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
{
struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
@@ -147,6 +150,7 @@ void p2m_memory_type_changed(struct domain *d)
_memory_type_changed(altp2m);
p2m_unlock(altp2m);
}
+ }
}
p2m_unlock(hostp2m);
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH for-4.19? v4 2/6] tools/xl: Add altp2m_count parameter
2024-05-18 11:02 [PATCH for-4.19? v4 0/6] x86: Make MAX_ALTP2M configurable Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 1/6] x86/p2m: Add braces for better code clarity Petr Beneš
@ 2024-05-18 11:02 ` Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 3/6] docs/man: Add altp2m_count parameter to the xl.cfg manual Petr Beneš
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Petr Beneš @ 2024-05-18 11:02 UTC (permalink / raw)
To: xen-devel
Cc: Petr Beneš, George Dunlap, Nick Rosbrook, Anthony PERARD,
Juergen Gross
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 | 4 ++++
6 files changed, 25 insertions(+)
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 78bdb08b15..40c247a0d0 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1158,6 +1158,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)
@@ -1674,6 +1675,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 ccfe18019e..a3ae8ef35a 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -602,6 +602,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 62cb07dea6..1f149a8015 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1239,6 +1239,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 41252ec553..236b8c1965 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -483,6 +483,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 79e9c656cc..eb306fedf5 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -728,6 +728,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 c504ab3711..048ab6be0d 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2063,6 +2063,10 @@ void parse_config_data(const char *config_source,
}
}
+ if (!xlu_cfg_get_long(config, "altp2m_count", &l, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH for-4.19? v4 3/6] docs/man: Add altp2m_count parameter to the xl.cfg manual
2024-05-18 11:02 [PATCH for-4.19? v4 0/6] x86: Make MAX_ALTP2M configurable Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 1/6] x86/p2m: Add braces for better code clarity Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 2/6] tools/xl: Add altp2m_count parameter Petr Beneš
@ 2024-05-18 11:02 ` Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable Petr Beneš
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Petr Beneš @ 2024-05-18 11:02 UTC (permalink / raw)
To: xen-devel; +Cc: Petr Beneš, Anthony PERARD
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 8f2b375ce9..5c09610cf4 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable
2024-05-18 11:02 [PATCH for-4.19? v4 0/6] x86: Make MAX_ALTP2M configurable Petr Beneš
` (2 preceding siblings ...)
2024-05-18 11:02 ` [PATCH for-4.19? v4 3/6] docs/man: Add altp2m_count parameter to the xl.cfg manual Petr Beneš
@ 2024-05-18 11:02 ` Petr Beneš
2024-05-20 8:25 ` Roger Pau Monné
2024-05-21 10:59 ` Jan Beulich
2024-05-18 11:02 ` [PATCH for-4.19? v4 5/6] tools/libxl: Activate the altp2m_count feature Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 6/6] tools/ocaml: Add altp2m_count parameter Petr Beneš
5 siblings, 2 replies; 13+ messages in thread
From: Petr Beneš @ 2024-05-18 11:02 UTC (permalink / raw)
To: xen-devel
Cc: Petr Beneš, Jan Beulich, Andrew Cooper, Roger Pau Monné,
George Dunlap, Julien Grall, Stefano Stabellini, Tamas K Lengyel,
Alexandru Isaila, Petre Pircalabu
From: Petr Beneš <w1benny@gmail.com>
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 max_altp2m on x86 is now set to MAX_EPTP
(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 max_altp2m in scenarios not utilizing VMFUNC, decoupling these
components would necessitate substantial code changes.
Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
xen/arch/x86/domain.c | 12 ++++
xen/arch/x86/hvm/hvm.c | 8 ++-
xen/arch/x86/hvm/vmx/vmx.c | 2 +-
xen/arch/x86/include/asm/domain.h | 7 +--
xen/arch/x86/include/asm/p2m.h | 6 +-
xen/arch/x86/mm/altp2m.c | 91 +++++++++++++++++++------------
xen/arch/x86/mm/hap/hap.c | 6 +-
xen/arch/x86/mm/mem_access.c | 24 ++++----
xen/arch/x86/mm/mem_sharing.c | 2 +-
xen/arch/x86/mm/p2m-ept.c | 12 ++--
xen/arch/x86/mm/p2m.c | 8 +--
xen/common/domain.c | 1 +
xen/include/public/domctl.h | 5 +-
xen/include/xen/sched.h | 2 +
14 files changed, 116 insertions(+), 70 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 00a3aaa576..3bd18cb2d0 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -685,6 +685,18 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
+ if ( config->nr_altp2m && !hvm_altp2m_supported() )
+ {
+ dprintk(XENLOG_INFO, "altp2m requested but not available\n");
+ return -EINVAL;
+ }
+
+ if ( config->nr_altp2m > MAX_EPTP )
+ {
+ dprintk(XENLOG_INFO, "nr_altp2m must be <= %lu\n", MAX_EPTP);
+ return -EINVAL;
+ }
+
if ( config->vmtrace_size )
{
unsigned int size = config->vmtrace_size;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 9594e0a5c5..77e4016bdb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4639,6 +4639,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;
@@ -5228,7 +5234,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 5f67a48592..76ee09b701 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4888,7 +4888,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 ( currd->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
continue;
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index f5daeb182b..3935328781 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -258,11 +258,10 @@ struct paging_vcpu {
struct shadow_vcpu shadow;
};
-#define MAX_NESTEDP2M 10
-
-#define MAX_ALTP2M 10 /* arbitrary */
#define INVALID_ALTP2M 0xffff
#define MAX_EPTP (PAGE_SIZE / sizeof(uint64_t))
+#define MAX_NESTEDP2M 10
+
struct p2m_domain;
struct time_scale {
int shift;
@@ -353,7 +352,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/p2m.h b/xen/arch/x86/include/asm/p2m.h
index 111badf89a..e66c081149 100644
--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -881,7 +881,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 v->domain->arch.altp2m_p2m[index];
}
@@ -891,7 +891,7 @@ static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx)
{
struct p2m_domain *orig;
- BUG_ON(idx >= MAX_ALTP2M);
+ BUG_ON(idx >= v->domain->nr_altp2m);
if ( idx == vcpu_altp2m(v).p2midx )
return false;
@@ -943,7 +943,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
p2m_type_t p2mt, p2m_access_t p2ma);
/* Set a specific p2m view visibility */
-int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
+int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
uint8_t visible);
#else /* !CONFIG_HVM */
struct p2m_domain *p2m_get_altp2m(struct vcpu *v);
diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 6fe1e9ed6b..5cb71c8b8e 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,8 +325,8 @@ 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);
+ ASSERT(idx < d->nr_altp2m);
+ p2m = d->arch.altp2m_p2m[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);
d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
@@ -348,9 +367,9 @@ 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 = array_access_nospec(d->arch.altp2m_p2m, idx);
+ p2m = d->arch.altp2m_p2m[idx];
hostp2m = p2m_get_hostp2m(d);
p2m_lock(p2m);
@@ -388,12 +407,12 @@ 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);
- if ( d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
+ if ( d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)] ==
mfn_x(INVALID_MFN) )
rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
@@ -415,7 +434,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 ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
continue;
@@ -437,7 +456,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);
@@ -447,17 +466,17 @@ 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)] !=
+ if ( d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)] !=
mfn_x(INVALID_MFN) )
{
- p2m = array_access_nospec(d->arch.altp2m_p2m, idx);
+ p2m = d->arch.altp2m_p2m[array_index_nospec(idx, d->nr_altp2m)];
if ( !_atomic_read(p2m->active_vcpus) )
{
p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
- d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
+ d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)] =
mfn_x(INVALID_MFN);
- d->arch.altp2m_visible_eptp[array_index_nospec(idx, MAX_EPTP)] =
+ d->arch.altp2m_visible_eptp[array_index_nospec(idx, d->nr_altp2m)] =
mfn_x(INVALID_MFN);
rc = 0;
}
@@ -475,7 +494,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);
@@ -510,13 +529,13 @@ 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)] ==
+ if ( idx >= d->nr_altp2m ||
+ d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)] ==
mfn_x(INVALID_MFN) )
return rc;
hp2m = p2m_get_hostp2m(d);
- ap2m = array_access_nospec(d->arch.altp2m_p2m, idx);
+ ap2m = d->arch.altp2m_p2m[array_index_nospec(idx, d->nr_altp2m)];
p2m_lock(hp2m);
p2m_lock(ap2m);
@@ -572,7 +591,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;
@@ -595,7 +614,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 ||
d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
@@ -659,12 +678,13 @@ 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)] ==
+ if ( sve->view >= d->nr_altp2m ||
+ d->arch.altp2m_eptp[array_index_nospec(sve->view, d->nr_altp2m)] ==
mfn_x(INVALID_MFN) )
return -EINVAL;
- p2m = ap2m = array_access_nospec(d->arch.altp2m_p2m, sve->view);
+ p2m = ap2m =
+ d->arch.altp2m_p2m[array_index_nospec(sve->view, d->nr_altp2m)];
}
p2m_lock(host_p2m);
@@ -727,12 +747,13 @@ 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)] ==
+ if ( altp2m_idx >= d->nr_altp2m ||
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)] ==
mfn_x(INVALID_MFN) )
return -EINVAL;
- p2m = ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
+ p2m = ap2m =
+ d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, d->nr_altp2m)];
}
else
p2m = host_p2m;
@@ -754,7 +775,7 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
return rc;
}
-int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
+int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
uint8_t visible)
{
int rc = 0;
@@ -763,17 +784,17 @@ 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) ||
- d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
+ if ( idx >= d->nr_altp2m ||
+ d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)] ==
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)];
+ d->arch.altp2m_visible_eptp[array_index_nospec(idx, d->nr_altp2m)] =
+ d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)];
else
- d->arch.altp2m_visible_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] =
+ d->arch.altp2m_visible_eptp[array_index_nospec(idx, d->nr_altp2m)] =
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..501fd9848b 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)
d->arch.altp2m_visible_eptp[i] = mfn_x(INVALID_MFN);
}
- for ( i = 0; i < MAX_ALTP2M; i++ )
+ for ( i = 0; i < d->nr_altp2m; i++ )
{
rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
if ( rv != 0 )
@@ -538,7 +538,7 @@ void hap_final_teardown(struct domain *d)
unsigned int i;
if ( hvm_altp2m_supported() )
- for ( i = 0; i < MAX_ALTP2M; i++ )
+ for ( i = 0; i < d->nr_altp2m; i++ )
p2m_teardown(d->arch.altp2m_p2m[i], true, NULL);
/* Destroy nestedp2m's first */
@@ -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(d->arch.altp2m_p2m[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 60a0cce68a..63bb2e10ed 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -347,12 +347,12 @@ 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) ||
- d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
+ if ( altp2m_idx >= d->nr_altp2m ||
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)] ==
mfn_x(INVALID_MFN) )
return -EINVAL;
- ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
+ ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, d->nr_altp2m)];
}
if ( !xenmem_access_to_p2m_access(p2m, access, &a) )
@@ -403,12 +403,12 @@ 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) ||
- d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
- mfn_x(INVALID_MFN) )
+ if ( altp2m_idx >= d->nr_altp2m ||
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)]
+ == mfn_x(INVALID_MFN) )
return -EINVAL;
- ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
+ ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, d->nr_altp2m)];
}
p2m_lock(p2m);
@@ -466,12 +466,12 @@ 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) )
+ if ( altp2m_idx >= d->nr_altp2m ||
+ d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)]
+ == mfn_x(INVALID_MFN) )
return -EINVAL;
- p2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
+ p2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, d->nr_altp2m)];
}
return _p2m_get_mem_access(p2m, gfn, access);
@@ -486,7 +486,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 = d->arch.altp2m_p2m[i];
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index da28266ef0..83bb9dd5df 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 = d->arch.altp2m_p2m[i];
if ( !ap2m )
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index f83610cb8c..42b868ca45 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;
@@ -1500,15 +1500,17 @@ 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 =
+ d->arch.altp2m_p2m[array_index_nospec(i, d->nr_altp2m)];
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;
+ d->arch.altp2m_eptp[array_index_nospec(i, d->nr_altp2m)] = ept->eptp;
+ d->arch.altp2m_visible_eptp[array_index_nospec(i, d->nr_altp2m)] =
+ ept->eptp;
}
unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
@@ -1519,7 +1521,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 ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
continue;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index db5d9b6c2a..549aec8d6b 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 ( d->arch.altp2m_eptp[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 ( d->arch.altp2m_eptp[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 ( d->arch.altp2m_eptp[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 ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
{
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6773f7fb90..a10a70e9d4 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->nr_altp2m;
d->vmtrace_size = config->vmtrace_size;
}
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index a33f9ec32b..60a871f123 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -21,7 +21,7 @@
#include "hvm/save.h"
#include "memory.h"
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017
/*
* NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -86,6 +86,9 @@ struct xen_domctl_createdomain {
uint32_t grant_opts;
+ /* Number of altp2ms to allocate. */
+ uint32_t nr_altp2m;
+
/* Per-vCPU buffer size in bytes. 0 to disable. */
uint32_t vmtrace_size;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 132b841995..18cc0748a1 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -602,6 +602,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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH for-4.19? v4 5/6] tools/libxl: Activate the altp2m_count feature
2024-05-18 11:02 [PATCH for-4.19? v4 0/6] x86: Make MAX_ALTP2M configurable Petr Beneš
` (3 preceding siblings ...)
2024-05-18 11:02 ` [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable Petr Beneš
@ 2024-05-18 11:02 ` Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 6/6] tools/ocaml: Add altp2m_count parameter Petr Beneš
5 siblings, 0 replies; 13+ messages in thread
From: Petr Beneš @ 2024-05-18 11:02 UTC (permalink / raw)
To: xen-devel; +Cc: Petr Beneš, Anthony PERARD, Juergen Gross
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 | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 236b8c1965..f5eb16d0dc 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -657,6 +657,7 @@ 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),
+ .nr_altp2m = b_info->altp2m_count,
.vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SHIFT),
.cpupool_id = info->poolid,
};
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH for-4.19? v4 6/6] tools/ocaml: Add altp2m_count parameter
2024-05-18 11:02 [PATCH for-4.19? v4 0/6] x86: Make MAX_ALTP2M configurable Petr Beneš
` (4 preceding siblings ...)
2024-05-18 11:02 ` [PATCH for-4.19? v4 5/6] tools/libxl: Activate the altp2m_count feature Petr Beneš
@ 2024-05-18 11:02 ` Petr Beneš
5 siblings, 0 replies; 13+ messages in thread
From: Petr Beneš @ 2024-05-18 11:02 UTC (permalink / raw)
To: xen-devel
Cc: Petr Beneš, Christian Lindig, David Scott, Anthony PERARD,
Christian Lindig
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>
Acked-by: Christian Lindig <christian.lindig@cloud.com>
---
tools/ocaml/libs/xc/xenctrl.ml | 1 +
tools/ocaml/libs/xc/xenctrl.mli | 1 +
tools/ocaml/libs/xc/xenctrl_stubs.c | 11 +++++++----
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 55923857ec..dfb3d331c9 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_count: int;
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..ff0e309c56 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_count: int;
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 2b6d3c09df..1f544cd2e4 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_COUNT 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)),
+ .nr_altp2m = Int_val(VAL_ALTP2M_COUNT),
.vmtrace_size = vmtrace_size,
.cpupool_id = Int32_val(VAL_CPUPOOL_ID),
};
@@ -257,7 +259,7 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
#if defined(__i386__) || defined(__x86_64__)
/* Quick & dirty check for ABI changes. */
- BUILD_BUG_ON(sizeof(cfg) != 64);
+ BUILD_BUG_ON(sizeof(cfg) != 68);
/* Mnemonics for the named fields inside xen_x86_arch_domainconfig */
#define VAL_EMUL_FLAGS Field(arch_domconfig, 0)
@@ -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_COUNT
#undef VAL_MAX_GRANT_VERSION
#undef VAL_MAX_MAPTRACK_FRAMES
#undef VAL_MAX_GRANT_FRAMES
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable
2024-05-18 11:02 ` [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable Petr Beneš
@ 2024-05-20 8:25 ` Roger Pau Monné
2024-05-21 10:59 ` Jan Beulich
1 sibling, 0 replies; 13+ messages in thread
From: Roger Pau Monné @ 2024-05-20 8:25 UTC (permalink / raw)
To: Petr Beneš
Cc: xen-devel, Jan Beulich, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Tamas K Lengyel,
Alexandru Isaila, Petre Pircalabu
On Sat, May 18, 2024 at 11:02:15AM +0000, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
>
> 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 max_altp2m on x86 is now set to MAX_EPTP
> (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 max_altp2m in scenarios not utilizing VMFUNC, decoupling these
> components would necessitate substantial code changes.
>
> Signed-off-by: Petr Beneš <w1benny@gmail.com>
> ---
> xen/arch/x86/domain.c | 12 ++++
> xen/arch/x86/hvm/hvm.c | 8 ++-
> xen/arch/x86/hvm/vmx/vmx.c | 2 +-
> xen/arch/x86/include/asm/domain.h | 7 +--
> xen/arch/x86/include/asm/p2m.h | 6 +-
> xen/arch/x86/mm/altp2m.c | 91 +++++++++++++++++++------------
> xen/arch/x86/mm/hap/hap.c | 6 +-
> xen/arch/x86/mm/mem_access.c | 24 ++++----
> xen/arch/x86/mm/mem_sharing.c | 2 +-
> xen/arch/x86/mm/p2m-ept.c | 12 ++--
> xen/arch/x86/mm/p2m.c | 8 +--
> xen/common/domain.c | 1 +
> xen/include/public/domctl.h | 5 +-
> xen/include/xen/sched.h | 2 +
> 14 files changed, 116 insertions(+), 70 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 00a3aaa576..3bd18cb2d0 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -685,6 +685,18 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> return -EINVAL;
> }
You also need to adjust arch_sanitise_domain_config() in ARM to return
an error if nr_altp2m is set, as there's no support for altp2m on ARM
yet.
>
> + if ( config->nr_altp2m && !hvm_altp2m_supported() )
> + {
> + dprintk(XENLOG_INFO, "altp2m requested but not available\n");
> + return -EINVAL;
> + }
> +
> + if ( config->nr_altp2m > MAX_EPTP )
> + {
> + dprintk(XENLOG_INFO, "nr_altp2m must be <= %lu\n", MAX_EPTP);
> + return -EINVAL;
> + }
> +
> if ( config->vmtrace_size )
> {
> unsigned int size = config->vmtrace_size;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 9594e0a5c5..77e4016bdb 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4639,6 +4639,12 @@ static int do_altp2m_op(
> goto out;
> }
>
> + if ( d->nr_altp2m == 0 )
I would merge this with the previous check, which also returns
-EINVAL.
> + {
> + rc = -EINVAL;
> + goto out;
> + }
> +
> if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d, mode, a.cmd)) )
> goto out;
>
> @@ -5228,7 +5234,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 5f67a48592..76ee09b701 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4888,7 +4888,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 ( currd->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
> continue;
> diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
> index f5daeb182b..3935328781 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -258,11 +258,10 @@ struct paging_vcpu {
> struct shadow_vcpu shadow;
> };
>
> -#define MAX_NESTEDP2M 10
> -
> -#define MAX_ALTP2M 10 /* arbitrary */
> #define INVALID_ALTP2M 0xffff
> #define MAX_EPTP (PAGE_SIZE / sizeof(uint64_t))
> +#define MAX_NESTEDP2M 10
> +
> struct p2m_domain;
> struct time_scale {
> int shift;
> @@ -353,7 +352,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/p2m.h b/xen/arch/x86/include/asm/p2m.h
> index 111badf89a..e66c081149 100644
> --- a/xen/arch/x86/include/asm/p2m.h
> +++ b/xen/arch/x86/include/asm/p2m.h
> @@ -881,7 +881,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 v->domain->arch.altp2m_p2m[index];
> }
> @@ -891,7 +891,7 @@ static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx)
> {
> struct p2m_domain *orig;
>
> - BUG_ON(idx >= MAX_ALTP2M);
> + BUG_ON(idx >= v->domain->nr_altp2m);
>
> if ( idx == vcpu_altp2m(v).p2midx )
> return false;
> @@ -943,7 +943,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
> p2m_type_t p2mt, p2m_access_t p2ma);
>
> /* Set a specific p2m view visibility */
> -int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
> +int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
> uint8_t visible);
> #else /* !CONFIG_HVM */
> struct p2m_domain *p2m_get_altp2m(struct vcpu *v);
> diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
> index 6fe1e9ed6b..5cb71c8b8e 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,8 +325,8 @@ 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);
> + ASSERT(idx < d->nr_altp2m);
> + p2m = d->arch.altp2m_p2m[idx];
If the array_access_nospec() is not required removing it should be a
separate commit. Also there's no mention of why removing the nospec
is safe in the commit message.
>
> 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);
> d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> @@ -348,9 +367,9 @@ 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 = array_access_nospec(d->arch.altp2m_p2m, idx);
> + p2m = d->arch.altp2m_p2m[idx];
Same here.
> hostp2m = p2m_get_hostp2m(d);
>
> p2m_lock(p2m);
> @@ -388,12 +407,12 @@ 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);
>
> - if ( d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
> + if ( d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)] ==
> mfn_x(INVALID_MFN) )
> rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
>
> @@ -415,7 +434,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 ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> continue;
> @@ -437,7 +456,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);
> @@ -447,17 +466,17 @@ 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)] !=
> + if ( d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)] !=
> mfn_x(INVALID_MFN) )
> {
> - p2m = array_access_nospec(d->arch.altp2m_p2m, idx);
> + p2m = d->arch.altp2m_p2m[array_index_nospec(idx, d->nr_altp2m)];
>
> if ( !_atomic_read(p2m->active_vcpus) )
> {
> p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
> - d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
> + d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)] =
> mfn_x(INVALID_MFN);
> - d->arch.altp2m_visible_eptp[array_index_nospec(idx, MAX_EPTP)] =
> + d->arch.altp2m_visible_eptp[array_index_nospec(idx, d->nr_altp2m)] =
> mfn_x(INVALID_MFN);
> rc = 0;
> }
> @@ -475,7 +494,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);
> @@ -510,13 +529,13 @@ 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)] ==
> + if ( idx >= d->nr_altp2m ||
> + d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)] ==
> mfn_x(INVALID_MFN) )
> return rc;
>
> hp2m = p2m_get_hostp2m(d);
> - ap2m = array_access_nospec(d->arch.altp2m_p2m, idx);
> + ap2m = d->arch.altp2m_p2m[array_index_nospec(idx, d->nr_altp2m)];
>
> p2m_lock(hp2m);
> p2m_lock(ap2m);
> @@ -572,7 +591,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;
> @@ -595,7 +614,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 ||
> d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
> @@ -659,12 +678,13 @@ 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)] ==
> + if ( sve->view >= d->nr_altp2m ||
> + d->arch.altp2m_eptp[array_index_nospec(sve->view, d->nr_altp2m)] ==
> mfn_x(INVALID_MFN) )
> return -EINVAL;
>
> - p2m = ap2m = array_access_nospec(d->arch.altp2m_p2m, sve->view);
> + p2m = ap2m =
> + d->arch.altp2m_p2m[array_index_nospec(sve->view, d->nr_altp2m)];
This expression is so common that it might be helpful to introduce a
helper: altp2m_get_p2m() or similar that encapsulates it (in a
separate patch).
> }
>
> p2m_lock(host_p2m);
> @@ -727,12 +747,13 @@ 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)] ==
> + if ( altp2m_idx >= d->nr_altp2m ||
> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)] ==
> mfn_x(INVALID_MFN) )
> return -EINVAL;
>
> - p2m = ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
> + p2m = ap2m =
> + d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, d->nr_altp2m)];
> }
> else
> p2m = host_p2m;
> @@ -754,7 +775,7 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
> return rc;
> }
>
> -int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
> +int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
> uint8_t visible)
> {
> int rc = 0;
> @@ -763,17 +784,17 @@ 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) ||
> - d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> + if ( idx >= d->nr_altp2m ||
> + d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)] ==
> 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)];
> + d->arch.altp2m_visible_eptp[array_index_nospec(idx, d->nr_altp2m)] =
> + d->arch.altp2m_eptp[array_index_nospec(idx, d->nr_altp2m)];
> else
> - d->arch.altp2m_visible_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] =
> + d->arch.altp2m_visible_eptp[array_index_nospec(idx, d->nr_altp2m)] =
> 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..501fd9848b 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)
> d->arch.altp2m_visible_eptp[i] = mfn_x(INVALID_MFN);
> }
>
> - for ( i = 0; i < MAX_ALTP2M; i++ )
> + for ( i = 0; i < d->nr_altp2m; i++ )
> {
> rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> if ( rv != 0 )
> @@ -538,7 +538,7 @@ void hap_final_teardown(struct domain *d)
> unsigned int i;
>
> if ( hvm_altp2m_supported() )
> - for ( i = 0; i < MAX_ALTP2M; i++ )
> + for ( i = 0; i < d->nr_altp2m; i++ )
> p2m_teardown(d->arch.altp2m_p2m[i], true, NULL);
>
> /* Destroy nestedp2m's first */
> @@ -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(d->arch.altp2m_p2m[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 60a0cce68a..63bb2e10ed 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -347,12 +347,12 @@ 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) ||
> - d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> + if ( altp2m_idx >= d->nr_altp2m ||
> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)] ==
> mfn_x(INVALID_MFN) )
> return -EINVAL;
>
> - ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
> + ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, d->nr_altp2m)];
> }
>
> if ( !xenmem_access_to_p2m_access(p2m, access, &a) )
> @@ -403,12 +403,12 @@ 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) ||
> - d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> - mfn_x(INVALID_MFN) )
> + if ( altp2m_idx >= d->nr_altp2m ||
> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)]
> + == mfn_x(INVALID_MFN) )
> return -EINVAL;
>
> - ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
> + ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, d->nr_altp2m)];
> }
>
> p2m_lock(p2m);
> @@ -466,12 +466,12 @@ 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) )
> + if ( altp2m_idx >= d->nr_altp2m ||
> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)]
> + == mfn_x(INVALID_MFN) )
> return -EINVAL;
>
> - p2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
> + p2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, d->nr_altp2m)];
> }
>
> return _p2m_get_mem_access(p2m, gfn, access);
> @@ -486,7 +486,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 = d->arch.altp2m_p2m[i];
>
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index da28266ef0..83bb9dd5df 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 = d->arch.altp2m_p2m[i];
> if ( !ap2m )
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index f83610cb8c..42b868ca45 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;
>
> @@ -1500,15 +1500,17 @@ 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 =
> + d->arch.altp2m_p2m[array_index_nospec(i, d->nr_altp2m)];
> 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;
> + d->arch.altp2m_eptp[array_index_nospec(i, d->nr_altp2m)] = ept->eptp;
> + d->arch.altp2m_visible_eptp[array_index_nospec(i, d->nr_altp2m)] =
> + ept->eptp;
> }
>
> unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
> @@ -1519,7 +1521,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 ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
> continue;
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index db5d9b6c2a..549aec8d6b 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 ( d->arch.altp2m_eptp[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 ( d->arch.altp2m_eptp[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 ( d->arch.altp2m_eptp[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 ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> {
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 6773f7fb90..a10a70e9d4 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->nr_altp2m;
> d->vmtrace_size = config->vmtrace_size;
> }
>
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index a33f9ec32b..60a871f123 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -21,7 +21,7 @@
> #include "hvm/save.h"
> #include "memory.h"
>
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017
>
> /*
> * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> @@ -86,6 +86,9 @@ struct xen_domctl_createdomain {
>
> uint32_t grant_opts;
>
> + /* Number of altp2ms to allocate. */
> + uint32_t nr_altp2m;
I've got a colliding patch that introduces altp2m_opts, and uses the
bottom 2 bits to signal the altp2m mode. On EPT this is limited to
512 altp2m, so using 16bits (from the high part of altp2m_opts) should
be enough, hopefully also for other architectures.
> +
> /* Per-vCPU buffer size in bytes. 0 to disable. */
> uint32_t vmtrace_size;
>
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 132b841995..18cc0748a1 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -602,6 +602,8 @@ struct domain
> unsigned int guest_request_sync : 1;
> } monitor;
>
> + unsigned int nr_altp2m; /* Number of altp2m tables */
Shouldn't this new field be placed in arch_domain with the rest of the
altp2m fields? I know that number of altp2m is an option that's
suitable to be shared between all architectures that have altp2m
implementations, but it's IMO better if it's grouped together with the
rest of the altp2m fields for consistency.
Thanks, Roger.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-4.19? v4 1/6] x86/p2m: Add braces for better code clarity
2024-05-18 11:02 ` [PATCH for-4.19? v4 1/6] x86/p2m: Add braces for better code clarity Petr Beneš
@ 2024-05-20 8:31 ` Roger Pau Monné
0 siblings, 0 replies; 13+ messages in thread
From: Roger Pau Monné @ 2024-05-20 8:31 UTC (permalink / raw)
To: Petr Beneš
Cc: xen-devel, Jan Beulich, Andrew Cooper, George Dunlap,
Stefano Stabellini
On Sat, May 18, 2024 at 11:02:12AM +0000, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
>
> No functional change.
>
> Signed-off-by: Petr Beneš <w1benny@gmail.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
TBH I'm fine without the braces, but if lack of them can cause
confusion:
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
CODING_STYLE states that braces can be omitted for blocks with single
statements, I guess we should clarify whether multi-line statements
are accepted, as the example contains a single-line statement.
Thanks, Roger.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable
2024-05-18 11:02 ` [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable Petr Beneš
2024-05-20 8:25 ` Roger Pau Monné
@ 2024-05-21 10:59 ` Jan Beulich
2024-05-26 23:55 ` Petr Beneš
1 sibling, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2024-05-21 10:59 UTC (permalink / raw)
To: Petr Beneš
Cc: Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall,
Stefano Stabellini, Tamas K Lengyel, Alexandru Isaila,
Petre Pircalabu, xen-devel
On 18.05.2024 13:02, Petr Beneš wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -685,6 +685,18 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> + if ( config->nr_altp2m && !hvm_altp2m_supported() )
> + {
> + dprintk(XENLOG_INFO, "altp2m requested but not available\n");
> + return -EINVAL;
> + }
> +
> + if ( config->nr_altp2m > MAX_EPTP )
The compared entities don't really fit together. I think we want a new
MAX_NR_ALTP2M, which - for the time being - could simply be
#define MAX_NR_ALTP2M MAX_EPTP
in the header. That would then be a suitable replacement for the
min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) that you're adjusting
elsewhere. Which however raises the question whether in EPT-specific
code the min() wouldn't better survive, as min(d->nr_altp2m, MAX_EPTP).
> @@ -5228,7 +5234,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;
You don't introduce a new local variable here. I'd like to ask that you also
don't ...
> @@ -403,12 +403,12 @@ 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) ||
> - d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> - mfn_x(INVALID_MFN) )
> + if ( altp2m_idx >= d->nr_altp2m ||
> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)]
> + == mfn_x(INVALID_MFN) )
Please don't break previously correct style: Binary operators (here: == )
belong onto the end of the earlier line. That'll render the line too long
again, but you want to deal with that e.g. thus:
d->arch.altp2m_eptp[array_index_nospec(altp2m_idx,
d->nr_altp2m)] ==
mfn_x(INVALID_MFN) )
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable
2024-05-21 10:59 ` Jan Beulich
@ 2024-05-26 23:55 ` Petr Beneš
2024-05-27 6:19 ` Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: Petr Beneš @ 2024-05-26 23:55 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall,
Stefano Stabellini, Tamas K Lengyel, Alexandru Isaila,
Petre Pircalabu, xen-devel
On Tue, May 21, 2024 at 12:59 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> The compared entities don't really fit together. I think we want a new
> MAX_NR_ALTP2M, which - for the time being - could simply be
>
> #define MAX_NR_ALTP2M MAX_EPTP
>
> in the header. That would then be a suitable replacement for the
> min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) that you're adjusting
> elsewhere. Which however raises the question whether in EPT-specific
> code the min() wouldn't better survive, as min(d->nr_altp2m, MAX_EPTP).
>
As you mentioned in a previous email, I've removed all the min(...,
MAX_EPTP) invocations from the code, since nr_altp2m is validated to
be no greater than that value. The only remaining places where this
value occurs are:
- In my newly introduced condition in arch_sanitise_domain_config:
if ( config->nr_altp2m > MAX_EPTP )
{
dprintk(XENLOG_INFO, "nr_altp2m must be <= %lu\n", MAX_NR_ALTP2M);
return -EINVAL;
}
- In hap_enable():
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);
}
Note that altp2m_eptp/altp2m_visible_eptp is never accessed beyond
nr_altp2m. From what you're saying, it sounds to me like I should only
replace the first mentioned occurrence with MAX_NR_ALTP2M. Correct me
if I'm wrong.
> > @@ -5228,7 +5234,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;
>
> You don't introduce a new local variable here. I'd like to ask that you also
> don't ...
>
> > @@ -403,12 +403,12 @@ 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) ||
> > - d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> > - mfn_x(INVALID_MFN) )
> > + if ( altp2m_idx >= d->nr_altp2m ||
> > + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)]
> > + == mfn_x(INVALID_MFN) )
>
> Please don't break previously correct style: Binary operators (here: == )
> belong onto the end of the earlier line. That'll render the line too long
> again, but you want to deal with that e.g. thus:
>
> d->arch.altp2m_eptp[array_index_nospec(altp2m_idx,
> d->nr_altp2m)] ==
> mfn_x(INVALID_MFN) )
>
Roger suggested introducing the altp2m_get_p2m() function, which I
like. I think introducing altp2m_get_eptp/visible_eptp and
altp2m_set_eptp/visible_eptp would also elegantly solve the issue of
overly long lines. My question is: if I go this route, should I
strictly replace with these functions only accesses that use
array_index_nospec()? Or should I replace all array accesses? For
example:
for ( i = 0; i < d->nr_altp2m; i++ )
{
struct p2m_domain *p2m;
if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
continue;
p2m = d->arch.altp2m_p2m[i];
p2m_lock(p2m);
p2m->ept.ad = value;
p2m_unlock(p2m);
}
... should I be consistent and also replace these accesses with
altp2m_get_eptp/altp2m_get_p2m (which will internally use
array_index_nospec), or should I leave them as they are?
P.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable
2024-05-26 23:55 ` Petr Beneš
@ 2024-05-27 6:19 ` Jan Beulich
2024-05-29 19:20 ` Petr Beneš
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2024-05-27 6:19 UTC (permalink / raw)
To: Petr Beneš
Cc: Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall,
Stefano Stabellini, Tamas K Lengyel, Alexandru Isaila,
Petre Pircalabu, xen-devel
On 27.05.2024 01:55, Petr Beneš wrote:
> On Tue, May 21, 2024 at 12:59 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> The compared entities don't really fit together. I think we want a new
>> MAX_NR_ALTP2M, which - for the time being - could simply be
Note that you've stripped too much context - "the compared entities" is
left without any meaning here, yet that's relevant to my earlier reply.
>> #define MAX_NR_ALTP2M MAX_EPTP
>>
>> in the header. That would then be a suitable replacement for the
>> min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) that you're adjusting
>> elsewhere. Which however raises the question whether in EPT-specific
>> code the min() wouldn't better survive, as min(d->nr_altp2m, MAX_EPTP).
>>
>
> As you mentioned in a previous email, I've removed all the min(...,
> MAX_EPTP) invocations from the code, since nr_altp2m is validated to
> be no greater than that value. The only remaining places where this
> value occurs are:
>
> - In my newly introduced condition in arch_sanitise_domain_config:
>
> if ( config->nr_altp2m > MAX_EPTP )
> {
> dprintk(XENLOG_INFO, "nr_altp2m must be <= %lu\n", MAX_NR_ALTP2M);
> return -EINVAL;
> }
This is suspicious: You compare against one value but log another. This
isn't EPT-specific, so shouldn't use MAX_EPTP.
> - In hap_enable():
>
> 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);
> }
>
> Note that altp2m_eptp/altp2m_visible_eptp is never accessed beyond
> nr_altp2m. From what you're saying, it sounds to me like I should only
> replace the first mentioned occurrence with MAX_NR_ALTP2M. Correct me
> if I'm wrong.
Yes. I suspect though that there may be further places that want adjusting.
>>> @@ -403,12 +403,12 @@ 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) ||
>>> - d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
>>> - mfn_x(INVALID_MFN) )
>>> + if ( altp2m_idx >= d->nr_altp2m ||
>>> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, d->nr_altp2m)]
>>> + == mfn_x(INVALID_MFN) )
>>
>> Please don't break previously correct style: Binary operators (here: == )
>> belong onto the end of the earlier line. That'll render the line too long
>> again, but you want to deal with that e.g. thus:
>>
>> d->arch.altp2m_eptp[array_index_nospec(altp2m_idx,
>> d->nr_altp2m)] ==
>> mfn_x(INVALID_MFN) )
>>
>
> Roger suggested introducing the altp2m_get_p2m() function, which I
> like. I think introducing altp2m_get_eptp/visible_eptp and
> altp2m_set_eptp/visible_eptp would also elegantly solve the issue of
> overly long lines. My question is: if I go this route, should I
> strictly replace with these functions only accesses that use
> array_index_nospec()? Or should I replace all array accesses? For
> example:
>
> for ( i = 0; i < d->nr_altp2m; i++ )
> {
> struct p2m_domain *p2m;
>
> if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
> continue;
>
> p2m = d->arch.altp2m_p2m[i];
>
> p2m_lock(p2m);
> p2m->ept.ad = value;
> p2m_unlock(p2m);
> }
>
> ... should I be consistent and also replace these accesses with
> altp2m_get_eptp/altp2m_get_p2m (which will internally use
> array_index_nospec), or should I leave them as they are?
Perhaps leave them as they are, unless you can technically justify the
adjustment.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable
2024-05-27 6:19 ` Jan Beulich
@ 2024-05-29 19:20 ` Petr Beneš
0 siblings, 0 replies; 13+ messages in thread
From: Petr Beneš @ 2024-05-29 19:20 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall,
Stefano Stabellini, Tamas K Lengyel, Alexandru Isaila,
Petre Pircalabu, xen-devel
On Mon, May 27, 2024 at 8:19 AM Jan Beulich <jbeulich@suse.com> wrote:
>
>
> This is suspicious: You compare against one value but log another. This
> isn't EPT-specific, so shouldn't use MAX_EPTP.
Sorry, I copy-pasted a snippet and didn't edit it correctly. Of
course, it should have been:
if ( config->nr_altp2m > MAX_NR_ALTP2M )
{
dprintk(XENLOG_INFO, "nr_altp2m must be <= %lu\n", MAX_NR_ALTP2M);
return -EINVAL;
}
> > ... should I be consistent and also replace these accesses with
> > altp2m_get_eptp/altp2m_get_p2m (which will internally use
> > array_index_nospec), or should I leave them as they are?
>
> Perhaps leave them as they are, unless you can technically justify the
> adjustment.
If we can avoid speculative execution, why just not do it? The
performance overhead array_index_nospec is negligible compared to the
whole VMEXIT handling. It will also serve as future-proofing, since
nobody will be confused whether they should directly access the array,
but instead use the accessor function.
Currently, the idea seems to be that array_index_nospec() is used when
the index is user-controlled, and not used when the index is under
xen's control (i.e. in loops). But I found at least 2 instances where
the index _is_ user controlled and the nospec access is not used -
further proving my previous point.
That being said, if there are no protests, I would replace all array
index accesses with the newly introduced accessor functions, which
will unconditionally use array_index_nospec().
P.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-05-29 19:20 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-18 11:02 [PATCH for-4.19? v4 0/6] x86: Make MAX_ALTP2M configurable Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 1/6] x86/p2m: Add braces for better code clarity Petr Beneš
2024-05-20 8:31 ` Roger Pau Monné
2024-05-18 11:02 ` [PATCH for-4.19? v4 2/6] tools/xl: Add altp2m_count parameter Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 3/6] docs/man: Add altp2m_count parameter to the xl.cfg manual Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable Petr Beneš
2024-05-20 8:25 ` Roger Pau Monné
2024-05-21 10:59 ` Jan Beulich
2024-05-26 23:55 ` Petr Beneš
2024-05-27 6:19 ` Jan Beulich
2024-05-29 19:20 ` Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 5/6] tools/libxl: Activate the altp2m_count feature Petr Beneš
2024-05-18 11:02 ` [PATCH for-4.19? v4 6/6] tools/ocaml: Add altp2m_count parameter Petr Beneš
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.