Re: [PATCH v2 4/7] x86: Make the maximum number of altp2m views configurable
On 30.04.2024 18:00, Petr Beneš wrote:
> On Tue, Apr 30, 2024 at 4:27 PM Jan Beulich wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -685,6 +685,12 @@ int arch_sanitise_domain_config(struct
>>> xen_domctl_createdomain *config)
>>> return -EINVAL;
>>> }
>>>
>>> +if ( config->max_altp2m > MAX_EPTP )
>>> +{
>>> +dprintk(XENLOG_INFO, "max_altp2m must be <= %u\n", MAX_EPTP);
>>> +return -EINVAL;
>>> +}
>>
>> ... using MAX_EPTP here feels like a layering violation to me. Imo there want
>> to be separate constants, tied together with a suitably placed
>> BUILD_BUG_ON().
>>
>> Furthermore comparisons like this (there are further ones elsewhere) suggest
>> there is a (continued) naming issue: A max_ or MAX_ prefix ought to name a
>> "maximum valid value", not "number of permitted values". This is not a
>> request to alter MAX_EPTP, but one to make sure the new struct fields really
>> have matching names and purposes.
>
> Do you have any proposals? I was considering nr_altp2m. Another
> question is what it should be named in xl.cfg - also nr_altp2m? I was
> too hesitant to name it like that, since there aren't any nr_* fields
> currently.
Internally nr_ or num_ are going to be fine. For xl whether either of
those would be, or maybe altp2ms= (along the lines of e.g. vcpus=), or
altp2m_count, or yet something else I simply don't know. That'll be
the maintainers there to help with.
Jan
Re: [PATCH v2 4/7] x86: Make the maximum number of altp2m views configurable
On Tue, Apr 30, 2024 at 4:27 PM Jan Beulich wrote:
>
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -685,6 +685,12 @@ int arch_sanitise_domain_config(struct
> > xen_domctl_createdomain *config)
> > return -EINVAL;
> > }
> >
> > +if ( config->max_altp2m > MAX_EPTP )
> > +{
> > +dprintk(XENLOG_INFO, "max_altp2m must be <= %u\n", MAX_EPTP);
> > +return -EINVAL;
> > +}
>
> ... using MAX_EPTP here feels like a layering violation to me. Imo there want
> to be separate constants, tied together with a suitably placed BUILD_BUG_ON().
>
> Furthermore comparisons like this (there are further ones elsewhere) suggest
> there is a (continued) naming issue: A max_ or MAX_ prefix ought to name a
> "maximum valid value", not "number of permitted values". This is not a
> request to alter MAX_EPTP, but one to make sure the new struct fields really
> have matching names and purposes.
Do you have any proposals? I was considering nr_altp2m. Another
question is what it should be named in xl.cfg - also nr_altp2m? I was
too hesitant to name it like that, since there aren't any nr_* fields
currently.
> > --- 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 INVALID_ALTP2M 0x
> > +#define MAX_EPTP((unsigned int)(PAGE_SIZE / sizeof(uint64_t)))
>
> Aiui you add this cast because of the various min() uses. However, besides
> wanting to avoid such casts (or in fact any, whenever possible), I don't
> see why you need to retain all those min(): You bound d->max_altp2m against
> MAX_EPTP during domain creation anyway.
Fair. I considered removing the min()s altogether. I left them in
place because I was too paranoid.
>
> > @@ -28,8 +34,13 @@ altp2m_vcpu_initialise(struct vcpu *v)
> > void
> > altp2m_vcpu_destroy(struct vcpu *v)
> > {
> > +struct domain *d = v->domain;
> > struct p2m_domain *p2m;
> >
> > +/* Skip destruction if no altp2m was used. */
> > +if ( d->max_altp2m == 0 )
> > +return;
>
> ... this one doesn't look quite right: Even if altp2m-s were initialized,
> none may have been used (yet).
Fair. Bad choice of words.
>
> > --- 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 max_altp2m; /* Maximum number of altp2m tables */
> > +
> > unsigned int vmtrace_size; /* Buffer size in bytes, or 0 to disable. */
>
> ... this suggest you're confident other architectures will also want
> to support altp2m. Yet nothing like that is said in the description.
> In the absence of that common code should not require touching (much).
Depends on the definition of "want". altp2m patches for arm/aarch64
were sent to Xen some 6 years ago but were unfortunately rejected. I
would very much welcome them.
I have added the field to domain instead of arch_domain simply because
it is not an arch-bound feature - similarly to vmtrace below, which
also doesn't have an equivalent implementation on other archs than x86
(yet).
As far as other comments/nits go - understood.
P.
Re: [PATCH v2 4/7] x86: Make the maximum number of altp2m views configurable
On 28.04.2024 18:52, Petr Beneš wrote:
> From: Petr Beneš
>
> This commit 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.
>
> 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.
While I don't mind this connection, ...
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -685,6 +685,12 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> +if ( config->max_altp2m > MAX_EPTP )
> +{
> +dprintk(XENLOG_INFO, "max_altp2m must be <= %u\n", MAX_EPTP);
> +return -EINVAL;
> +}
... using MAX_EPTP here feels like a layering violation to me. Imo there want
to be separate constants, tied together with a suitably placed BUILD_BUG_ON().
Furthermore comparisons like this (there are further ones elsewhere) suggest
there is a (continued) naming issue: A max_ or MAX_ prefix ought to name a
"maximum valid value", not "number of permitted values". This is not a
request to alter MAX_EPTP, but one to make sure the new struct fields really
have matching names and purposes.
> --- 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 INVALID_ALTP2M 0x
> +#define MAX_EPTP((unsigned int)(PAGE_SIZE / sizeof(uint64_t)))
Aiui you add this cast because of the various min() uses. However, besides
wanting to avoid such casts (or in fact any, whenever possible), I don't
see why you need to retain all those min(): You bound d->max_altp2m against
MAX_EPTP during domain creation anyway.
> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -13,6 +13,12 @@
> void
> altp2m_vcpu_initialise(struct vcpu *v)
> {
> +struct domain *d = v->domain;
> +
> +/* Skip initialisation if no altp2m will be used. */
> +if ( d->max_altp2m == 0 )
> +return;
While I'm fine with this comment, ...
> @@ -28,8 +34,13 @@ altp2m_vcpu_initialise(struct vcpu *v)
> void
> altp2m_vcpu_destroy(struct vcpu *v)
> {
> +struct domain *d = v->domain;
> struct p2m_domain *p2m;
>
> +/* Skip destruction if no altp2m was used. */
> +if ( d->max_altp2m == 0 )
> +return;
... this one doesn't look quite right: Even if altp2m-s were initialized,
none may have been used (yet).
> @@ -120,7 +131,13 @@ int p2m_init_altp2m(struct domain *d)
> struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>
> mm_lock_init(>arch.altp2m_list_lock);
> -for ( i = 0; i < MAX_ALTP2M; i++ )
> +
> +if ( (d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *,
> d->max_altp2m)) == NULL )
> +{
> +return -ENOMEM;
> +}
Nit: Overlong line and no need for the braces.
> +for ( i = 0; i < d->max_altp2m; i++ )
> {
> d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
This loop, btw, also demonstrates that "maximum" isn't true here. The
domain gets all of them allocated in one go.
> @@ -141,7 +158,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->max_altp2m; i++ )
> {
> if ( !d->arch.altp2m_p2m[i] )
> continue;
> @@ -149,6 +169,9 @@ void p2m_teardown_altp2m(struct domain *d)
> d->arch.altp2m_p2m[i] = NULL;
> p2m_free_one(p2m);
> }
> +
> +xfree(d->arch.altp2m_p2m);
> +d->arch.altp2m_p2m = NULL;
> }
Please don't (wrongly) open-code XFREE().
> @@ -2090,13 +2090,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) ||
> +if ( idx >= min(d->max_altp2m, MAX_EPTP) ||
Nit: Please take the opportunity and remove the excess blank.
> d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
This use of MAX_EPTP also needs replacing, to avoid speculatively overrunning
the allocated array. At least one more instance elsewhere.
> @@ -2575,12 +2575,12 @@ 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 >= min(d->max_altp2m, MAX_EPTP) ||
> d->arch.altp2m_eptp[array_index_nospec(sve->view, MAX_EPTP)] ==
>
[PATCH v2 4/7] x86: Make the maximum number of altp2m views configurable
From: Petr Beneš
This commit 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.
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š
---
Changed since v1:
* Added xen_domctl_createdomain::max_altp2m field to
xen/include/public/domctl.h
* Bumped the XEN_DOMCTL_INTERFACE_VERSION
* More elaborate commit message
xen/arch/x86/domain.c | 6
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| 4 +--
xen/arch/x86/mm/altp2m.c | 27 +++--
xen/arch/x86/mm/hap/hap.c | 6 ++--
xen/arch/x86/mm/mem_access.c | 14 -
xen/arch/x86/mm/mem_sharing.c | 2 +-
xen/arch/x86/mm/p2m-ept.c | 6 ++--
xen/arch/x86/mm/p2m.c | 50 +++
xen/common/domain.c | 7 +
xen/include/public/domctl.h | 3 +-
xen/include/xen/sched.h | 2 ++
14 files changed, 94 insertions(+), 50 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 20e83cf38b..6b458f330c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -685,6 +685,12 @@ int arch_sanitise_domain_config(struct
xen_domctl_createdomain *config)
return -EINVAL;
}
+if ( config->max_altp2m > MAX_EPTP )
+{
+dprintk(XENLOG_INFO, "max_altp2m must be <= %u\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 0ce45b177c..9b70fe7cfc 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4633,6 +4633,12 @@ static int do_altp2m_op(
goto out;
}
+if ( d->max_altp2m == 0 )
+{
+rc = -EINVAL;
+goto out;
+}
+
if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d, mode, a.cmd)) )
goto out;
@@ -5222,7 +5228,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->max_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..8f57f3a7f5 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->max_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..5bb0bcae81 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 INVALID_ALTP2M 0x
+#define MAX_EPTP((unsigned int)(PAGE_SIZE / sizeof(uint64_t)))
#define MAX_NESTEDP2M 10
-#define MAX_ALTP2M 10 /* arbitrary */
-#define INVALID_ALTP2M 0x
-#define MAX_EPTP(PAGE_SIZE / sizeof(uint64_t))
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..2086bcb633 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->max_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->max_altp2m);
if ( idx == vcpu_altp2m(v).p2midx )
return false;
diff --git a/xen/arch/x86/mm/altp2m.c
