Re: [PATCH v2 4/7] x86: Make the maximum number of altp2m views configurable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2024 18:52, Petr Beneš wrote:
> From: Petr Beneš <w1benny@xxxxxxxxx>
> 
> 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  0xffff
> +#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(&d->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)] ==
>               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->max_altp2m)];

Nit: Overlong line (more elsewhere).

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -568,6 +568,12 @@ static int sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          }
>      }
> 
> +    if ( config->max_altp2m && !hvm_altp2m_supported() )

This looks like it'll break the build on non-x86.

> --- 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.
> @@ -77,6 +77,7 @@ struct xen_domctl_createdomain {
>       */
>      uint32_t max_vcpus;
>      uint32_t max_evtchn_port;
> +    uint32_t max_altp2m;
>      int32_t max_grant_frames;
>      int32_t max_maptrack_frames;

Both this and ...

> --- 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).

Jan