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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.07.2025 21:54, Petr Beneš wrote:
> From: Petr Beneš <w1benny@xxxxxxxxx>
> 
> This commit introduces the ability to configure the maximum number of altp2m
> views for the domain during its creation. Previously, the limits were 
> hardcoded
> to a maximum of 10. This change allows for greater flexibility in environments
> that require more or fewer altp2m views.
> 
> The maximum configurable limit for nr_altp2m on x86 is now set to
> MAX_NR_ALTP2M (which currently holds the MAX_EPTP value - 512). This cap is
> linked to the architectural limit of the EPTP-switching VMFUNC, which supports
> up to 512 entries. Despite there being no inherent need for limiting nr_altp2m
> in scenarios not utilizing VMFUNC, decoupling these components would 
> necessitate
> substantial code changes.
> 
> xen_domctl_createdomain::altp2m is extended for a new field `nr`, that will
> configure this limit for a domain. Additionally, previous altp2m.opts value
> has been reduced from uint32_t to uint16_t so that both of these fields occupy
> as little space as possible.
> 
> Accesses to the altp2m_p2m array are modified to respect the new nr_altp2m
> value. Accesses to the altp2m_(visible_)eptp arrays are unmodified, since
> these arrays always have fixed size of MAX_EPTP.
> 
> A dummy hvm_altp2m_supported() function is introduced for non-HVM builds, so
> that the compilation won't fail for them.

I can't spot such an introduction; instead I can spot such a stub already in
the tree.

> --- a/xen/arch/x86/include/asm/altp2m.h
> +++ b/xen/arch/x86/include/asm/altp2m.h
> @@ -13,12 +13,32 @@
>  #include <xen/sched.h>         /* for struct vcpu, struct domain */
>  #include <asm/hvm/vcpu.h>      /* for vcpu_altp2m */
>  
> +static inline bool altp2m_is_eptp_valid(const struct domain *d,
> +                                        unsigned int idx)
> +{
> +    ASSERT(d->nr_altp2m <= MAX_EPTP);
> +
> +    /*
> +     * EPTP index is correlated with altp2m index and should not exceed
> +     * d->nr_altp2m.
> +     */

If the two were merely correlated, that wouldn't be enough for ...

> +    return idx < d->nr_altp2m &&
> +        d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] !=

... this to be safe. The two match, don't they? And to better make the
connection here, maybe also add BUILD_BUG_ON(MAX_EPTP < MAX_NR_ALTP2M)?
Perhaps along with making the ASSERT() above use MAX_NR_ALTP2M.

> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -258,11 +258,12 @@ struct paging_vcpu {
>      struct shadow_vcpu shadow;
>  };
>  
> -#define MAX_NESTEDP2M 10
> +#define MAX_EPTP        (PAGE_SIZE / sizeof(uint64_t))
> +#define MAX_NR_ALTP2M   MAX_EPTP
> +#define MAX_NESTEDP2M   10
>  
> -#define MAX_ALTP2M      10 /* arbitrary */
>  #define INVALID_ALTP2M  0xffff
> -#define MAX_EPTP        (PAGE_SIZE / sizeof(uint64_t))

You touch many of the uses of MAX_EPTP anyway. I wonder if this wasn't a good
opportunity to correct its name (to e.g. NR_EPTP or NUM_EPTP). But maybe that
would better be a follow-on change ...

> @@ -122,7 +128,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);

Note that new code is now expected to use the xv*alloc*() family of functions.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -619,6 +619,8 @@ struct domain
>          unsigned int guest_request_sync          : 1;
>      } monitor;
>  
> +    unsigned int nr_altp2m;    /* Number of altp2m tables. */

We may have had that discussion earlier on, but it has been too long ago now:
Why is this not an x86-only field (i.e. in struct arch_domain)? Or one
dependent upon CONFIG_ALTP2M?

Jan