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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.07.2025 10:31, Petr Beneš wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1873,6 +1873,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>                                P2M_ALLOC | (npfec.write_access ? P2M_UNSHARE 
> : 0),
>                                &page_order);
>  
> +#ifdef CONFIG_ALTP2M
>      if ( altp2m_active(currd) )
>      {
>          p2m = p2m_get_altp2m(curr);
> @@ -1891,6 +1892,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>          }
>      }
>      else
> +#endif
>          p2m = hostp2m;
>  
>      /* Check access permissions first, then handle faults */

Why is this needed? I can't spot any access to a struct field here that's
unavailable when ALTP2M=n.

> @@ -2863,10 +2867,12 @@ static struct hvm_function_table 
> __initdata_cf_clobber vmx_function_table = {
>      .update_vlapic_mode = vmx_vlapic_msr_changed,
>      .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
>      .enable_msr_interception = vmx_enable_msr_interception,
> +#ifdef CONFIG_ALTP2M
>      .altp2m_vcpu_update_p2m = vmx_vcpu_update_eptp,
>      .altp2m_vcpu_update_vmfunc_ve = vmx_vcpu_update_vmfunc_ve,
>      .altp2m_vcpu_emulate_ve = vmx_vcpu_emulate_ve,
>      .altp2m_vcpu_emulate_vmfunc = vmx_vcpu_emulate_vmfunc,
> +#endif

With this, the hook pointers in struct hvm_function_table should also become
conditional (and then of course also the wrapper functions there). Overall I
wonder whether at least part of this conditionalizing wouldn't better be a
separate, prereq patch.

> @@ -4225,6 +4231,7 @@ void asmlinkage vmx_vmexit_handler(struct cpu_user_regs 
> *regs)
>      /* Now enable interrupts so it's safe to take locks. */
>      local_irq_enable();
>  
> +#ifdef CONFIG_ALTP2M
>      /*
>       * If the guest has the ability to switch EPTP without an exit,
>       * figure out whether it has done so and update the altp2m data.
> @@ -4256,6 +4263,7 @@ void asmlinkage vmx_vmexit_handler(struct cpu_user_regs 
> *regs)
>  
>          p2m_set_altp2m(v, idx);
>      }
> +#endif /* CONFIG_ALTP2M */
>  
>      if ( unlikely(currd->arch.monitor.vmexit_enabled) )
>      {

Same question as at the top - is this really needed?

> @@ -962,16 +964,24 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t 
> gfn,
>  /* Set a specific p2m view visibility */
>  int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx,
>                                     uint8_t visible);
> -#else /* !CONFIG_HVM */
> -struct p2m_domain *p2m_get_altp2m(struct vcpu *v);
> -#endif /* CONFIG_HVM */
>  
> -#ifdef CONFIG_ALTP2M
>  /* Check to see if vcpu should be switched to a different p2m. */
>  void p2m_altp2m_check(struct vcpu *v, uint16_t idx);
> -#else
> +#else /* !CONFIG_ALTP2M */
> +static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)
> +{
> +    return NULL;
> +}
> +
> +static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx)
> +{
> +    return false;
> +}

While the "set" stub would likely be okay in this form, I don't really like
the plain NULL return of the "get" one. Looking at use sites, can't we get
away with just a declaration (but no definition) of both? As leveraged in
many other places, compiler DCE would get rid of all of the call sites at
long a altp2m_active() yields compile-time-constant "false".

> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -9,12 +9,16 @@
>  #include <asm/altp2m.h>
>  #include <public/hvm/hvm_op.h>
>  #include <xen/event.h>
> +#include <xen/xvmalloc.h>
>  #include "mm-locks.h"
>  #include "p2m.h"
>  
>  void
>  altp2m_vcpu_initialise(struct vcpu *v)
>  {
> +    if ( v->domain->nr_altp2m == 0 )
> +        return;

Just as a remark, without insisting on a change: We generally prefer the
shorter ! form, much like you do e.g. ...

> @@ -122,7 +129,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 = xvzalloc_array(struct p2m_domain *, d->nr_altp2m);
> +
> +    if ( !d->arch.altp2m_p2m )

... here.

> @@ -143,7 +155,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;

This isn't really needed, is it? The ...

> +    for ( i = 0; i < d->nr_altp2m; i++ )

... loop bound ought to be zero in that case, so nothing will be done
here, and ...

>      {
>          if ( !d->arch.altp2m_p2m[i] )
>              continue;
> @@ -151,6 +166,8 @@ void p2m_teardown_altp2m(struct domain *d)
>          d->arch.altp2m_p2m[i] = NULL;
>          p2m_free_one(p2m);
>      }
> +
> +    XVFREE(d->arch.altp2m_p2m);
>  }

... this simply ends up being a no-op.

> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -79,8 +79,11 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
>      struct domain *d = v->domain;
>      struct p2m_domain *p2m = NULL;
>  
> +#ifdef CONFIG_ALTP2M
>      if ( altp2m_active(d) )
>          p2m = p2m_get_altp2m(v);
> +#endif
> +
>      if ( !p2m )
>          p2m = p2m_get_hostp2m(d);
>  
> @@ -143,8 +146,11 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>      vm_event_request_t *req;
>      int rc;
>  
> +#ifdef CONFIG_ALTP2M
>      if ( altp2m_active(d) )
>          p2m = p2m_get_altp2m(v);
> +#endif
> +
>      if ( !p2m )
>          p2m = p2m_get_hostp2m(d);

These two changes again look as if they weren't really needed.

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -771,8 +771,10 @@ bool ept_handle_misconfig(uint64_t gpa)
>      bool spurious;
>      int rc;
>  
> +#ifdef CONFIG_ALTP2M
>      if ( altp2m_active(curr->domain) )
>          p2m = p2m_get_altp2m(curr);
> +#endif
>  
>      p2m_lock(p2m);
>  
> @@ -994,12 +996,14 @@ out:
>      if ( is_epte_present(&old_entry) )
>          ept_free_entry(p2m, &old_entry, target);
>  
> +#ifdef CONFIG_ALTP2M
>      if ( hvm_altp2m_supported() && entry_written && p2m_is_hostp2m(p2m) )
>      {
>          ret = p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, 
> p2ma);
>          if ( !rc )
>              rc = ret;
>      }
> +#endif
>  
>      return rc;
>  }

Same here.

> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -519,12 +519,14 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
>      struct p2m_domain *p2m = p2m_get_hostp2m(current->domain);
>      int rc;
>  
> +#ifdef CONFIG_ALTP2M
>      /*
>       * Should altp2m ever be enabled for NPT / shadow use, this code
>       * should be updated to make use of the active altp2m, like
>       * ept_handle_misconfig().
>       */
>      ASSERT(!altp2m_active(current->domain));
> +#endif

And again here.

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -811,6 +811,9 @@ struct domain *domain_create(domid_t domid,
>      if ( config )
>      {
>          d->options = config->flags;
> +#ifdef CONFIG_ALTP2M
> +        d->nr_altp2m = config->altp2m.nr;
> +#endif

Add #else asserting that config->altp2m.nr is zero?

Jan