Re: [PATCH v10 5/9] x86/altp2m: Wrap altp2m-specific code in #ifdef CONFIG_ALTP2M

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.07.2025 22:15, Petr Beneš wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2686,8 +2686,8 @@ static int cf_check hvmemul_tlb_op(
>      return rc;
>  }
>  
> -static int cf_check hvmemul_vmfunc(
> -    struct x86_emulate_ctxt *ctxt)
> +#ifdef CONFIG_ALTP2M
> +static int cf_check hvmemul_vmfunc(struct x86_emulate_ctxt *ctxt)

Please don't needlessly alter formatting, and ...

>  {
>      int rc;
>  
> @@ -2699,6 +2699,12 @@ static int cf_check hvmemul_vmfunc(
>  
>      return rc;
>  }
> +#else
> +static int cf_check hvmemul_vmfunc(struct x86_emulate_ctxt *ctxt)
> +{
> +    return X86EMUL_UNHANDLEABLE;
> +}
> +#endif

... please don't duplicate function headers when such #ifdef-s can easily
be put inside the function body. Much like you do ...

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4608,6 +4608,7 @@ static int hvmop_get_param(
>  static int do_altp2m_op(
>      XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> +#ifdef CONFIG_ALTP2M
>      struct xen_hvm_altp2m_op a;
>      struct domain *d = NULL;
>      int rc = 0;
> @@ -4944,6 +4945,9 @@ static int do_altp2m_op(
>      rcu_unlock_domain(d);
>  
>      return rc;
> +#else /* !CONFIG_ALTP2M */
> +    return -EOPNOTSUPP;
> +#endif /* CONFIG_ALTP2M */
>  }

... here.

> @@ -5261,6 +5269,7 @@ void hvm_toggle_singlestep(struct vcpu *v)
>  
>  void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx)
>  {
> +#ifdef CONFIG_ALTP2M
>      ASSERT(atomic_read(&v->pause_count));
>  
>      if ( !hvm_is_singlestep_supported() )
> @@ -5272,6 +5281,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t 
> p2midx)
>      v->arch.hvm.single_step = true;
>      v->arch.hvm.fast_single_step.enabled = true;
>      v->arch.hvm.fast_single_step.p2midx = p2midx;
> +#endif
>  }

This function would better be unreachable when ALTP2M=n, imo. Putting an #ifdef
in vm_event_toggle_singlestep() would be the simple solution, but maybe we could
to better.

> @@ -707,6 +709,7 @@ static inline bool hvm_nested_virt_supported(void)
>      return hvm_funcs.caps.nested_virt;
>  }
>  
> +#ifdef CONFIG_ALTP2M
>  /* updates the current hardware p2m */
>  static inline void altp2m_vcpu_update_p2m(struct vcpu *v)
>  {
> @@ -731,6 +734,11 @@ static inline bool altp2m_vcpu_emulate_ve(struct vcpu *v)
>      }
>      return false;
>  }
> +#else /* !CONFIG_ALTP2M */
> +void altp2m_vcpu_update_p2m(struct vcpu *v);
> +void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v);
> +bool altp2m_vcpu_emulate_ve(struct vcpu *v);
> +#endif /* CONFIG_ALTP2M */

Why would the altp2m_vcpu_update_{p2m,vmfunc_ve}() declarations be needed? All
uses are from altp2m.c.

Jan