Re: [PATCH v10 6/9] xen: Make the maximum number of altp2m views configurable for x86

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.07.2025 22:15, 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.
> 
> Additional sanitization is introduced in the x86/arch_sanitise_domain_config
> to fix the altp2m.nr value to 10 if it is previously set to 0. This behavior
> is only temporary and immediately removed in the upcoming commit (which will
> disallow creating a domain with enabled altp2m with zero nr_altp2m).
> 
> The reason for this temporary workaround is to retain the legacy behavior
> until the feature is fully activated in libxl.
> 
> Also, arm/arch_sanitise_domain_config is extended to not allow requesting
> non-zero altp2ms.
> 
> Signed-off-by: Petr Beneš <w1benny@xxxxxxxxx>
> ---
>  xen/arch/arm/domain.c             |  2 +-
>  xen/arch/x86/domain.c             | 40 ++++++++++++++---
>  xen/arch/x86/hvm/hvm.c            |  8 +++-
>  xen/arch/x86/hvm/vmx/vmx.c        |  2 +-
>  xen/arch/x86/include/asm/altp2m.h | 28 ++++++++++--
>  xen/arch/x86/include/asm/domain.h |  9 ++--
>  xen/arch/x86/include/asm/p2m.h    |  4 +-
>  xen/arch/x86/mm/altp2m.c          | 72 ++++++++++++++++---------------
>  xen/arch/x86/mm/hap/hap.c         |  6 +--
>  xen/arch/x86/mm/mem_access.c      | 22 ++++------
>  xen/arch/x86/mm/mem_sharing.c     |  4 +-
>  xen/arch/x86/mm/p2m-ept.c         |  7 +--
>  xen/arch/x86/mm/p2m-pt.c          |  2 +
>  xen/arch/x86/mm/p2m.c             |  8 ++--
>  xen/common/domain.c               |  6 +++
>  xen/include/public/domctl.h       |  5 ++-
>  xen/include/xen/sched.h           |  4 ++
>  17 files changed, 151 insertions(+), 78 deletions(-)

I understand the cover letter has some revlog, but may I please ask to have
such (also) per-patch?

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4675,6 +4675,12 @@ static int do_altp2m_op(
>          goto out;
>      }
>  
> +    if ( d->nr_altp2m == 0 )
> +    {
> +        rc = -EINVAL;
> +        goto out;
> +    }

Besides disliking EINVAL here, I also question this general rejection. For
e.g. HVMOP_altp2m_get_domain_state it may well make sense to still get back
"disabled". Yet I understand this affects a few of the pre-existing checks
as well.

With the error code changed to EOPNOTSUPP (as every other similar path uses,
with the sole exception of the "if ( XEN_ALTP2M_disabled == mode )" one):
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -81,6 +81,7 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
>  
>      if ( altp2m_active(d) )
>          p2m = p2m_get_altp2m(v);
> +
>      if ( !p2m )
>          p2m = p2m_get_hostp2m(d);
>  
> @@ -145,6 +146,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>  
>      if ( altp2m_active(d) )
>          p2m = p2m_get_altp2m(v);
> +
>      if ( !p2m )
>          p2m = p2m_get_hostp2m(d);

I think such stray changes would better be omitted, especially from an already
large patch.

Jan