Re: [XEN][PATCH v2] xen: make VMTRACE support optional

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.11.2025 21:24, Grygorii Strashko wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -307,6 +307,7 @@ static int vmx_init_vmcs_config(bool bsp)
>      rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
>  
>      /* Check whether IPT is supported in VMX operation. */
> +#ifdef CONFIG_VMTRACE
>      if ( bsp )
>          vmtrace_available = cpu_has_proc_trace &&
>                              (_vmx_misc_cap & VMX_MISC_PROC_TRACE);
> @@ -317,6 +318,7 @@ static int vmx_init_vmcs_config(bool bsp)
>                 smp_processor_id());
>          return -EINVAL;
>      }
> +#endif

Initially I was inclined to ask for use of IS_ENABLED() here, but that wouldn't
work since vmtrace_available isn't an lvalue when VMTRACE=n. Hence why generally
I think it is better to check the particular identifier in such cases, rather
than the original CONFIG_* (i.e. "#ifndef vmtrace_available" here). I'm not
going to insist though, as I expect opinions may differ on this matter.

> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -234,12 +234,14 @@ struct hvm_function_table {
>      int (*altp2m_vcpu_emulate_vmfunc)(const struct cpu_user_regs *regs);
>  #endif
>  
> +#ifdef CONFIG_VMTRACE
>      /* vmtrace */
>      int (*vmtrace_control)(struct vcpu *v, bool enable, bool reset);
>      int (*vmtrace_output_position)(struct vcpu *v, uint64_t *pos);
>      int (*vmtrace_set_option)(struct vcpu *v, uint64_t key, uint64_t value);
>      int (*vmtrace_get_option)(struct vcpu *v, uint64_t key, uint64_t *value);
>      int (*vmtrace_reset)(struct vcpu *v);
> +#endif
>  
>      uint64_t (*get_reg)(struct vcpu *v, unsigned int reg);
>      void (*set_reg)(struct vcpu *v, unsigned int reg, uint64_t val);
> @@ -735,6 +737,7 @@ static inline bool altp2m_vcpu_emulate_ve(struct vcpu *v)
>  bool altp2m_vcpu_emulate_ve(struct vcpu *v);
>  #endif /* CONFIG_ALTP2M */
>  
> +#ifdef CONFIG_VMTRACE
>  static inline int hvm_vmtrace_control(struct vcpu *v, bool enable, bool 
> reset)
>  {
>      if ( hvm_funcs.vmtrace_control )
> @@ -769,13 +772,20 @@ static inline int hvm_vmtrace_get_option(
>  
>      return -EOPNOTSUPP;
>  }
> +#else
> +int hvm_vmtrace_output_position(struct vcpu *v, uint64_t *pos);
> +#endif

There not being any definition for this declaration (regardless of 
configuration),
a comment might have been warranted here. Furthermore, can't the stub further 
down
in the file now go away (addressing a Misra concern of it now being unused, as
HVM=n implies VMTRACE=n)? Possibly this applies to a few other stubs there as
well?

>  static inline int hvm_vmtrace_reset(struct vcpu *v)
>  {
> +#ifdef CONFIG_VMTRACE
>      if ( hvm_funcs.vmtrace_reset )
>          return alternative_call(hvm_funcs.vmtrace_reset, v);
>  
>      return -EOPNOTSUPP;
> +#else
> +    return 0;
> +#endif
>  }

This doesn't look right - if absence of a hook results in -EOPNOTSUPP, so should
VMTRACE=n do. (There's no practical effect from this though, as - perhaps 
wrongly -
neither caller checks the return value.)

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1155,8 +1155,10 @@ static unsigned int resource_max_frames(const struct 
> domain *d,
>      case XENMEM_resource_ioreq_server:
>          return ioreq_server_max_frames(d);
>  
> +#ifdef CONFIG_VMTRACE
>      case XENMEM_resource_vmtrace_buf:
>          return d->vmtrace_size >> PAGE_SHIFT;
> +#endif
>  
>      default:
>          return 0;
> @@ -1198,6 +1200,7 @@ static int acquire_ioreq_server(struct domain *d,
>  #endif
>  }
>  
> +#ifdef CONFIG_VMTRACE
>  static int acquire_vmtrace_buf(
>      struct domain *d, unsigned int id, unsigned int frame,
>      unsigned int nr_frames, xen_pfn_t mfn_list[])
> @@ -1220,6 +1223,7 @@ static int acquire_vmtrace_buf(
>  
>      return nr_frames;
>  }
> +#endif
>  
>  /*
>   * Returns -errno on error, or positive in the range [1, nr_frames] on
> @@ -1238,8 +1242,10 @@ static int _acquire_resource(
>      case XENMEM_resource_ioreq_server:
>          return acquire_ioreq_server(d, id, frame, nr_frames, mfn_list);
>  
> +#ifdef CONFIG_VMTRACE
>      case XENMEM_resource_vmtrace_buf:
>          return acquire_vmtrace_buf(d, id, frame, nr_frames, mfn_list);
> +#endif
>  
>      default:
>          ASSERT_UNREACHABLE();

Without the intention to ask for a change right in this patch, this is a little
awkward: resource_max_frames() returning 0 results in acquire_resource() to
return -EINVAL, when with VMTRACE=n for XENMEM_resource_vmtrace_buf it imo
better would be -EOPNOTSUPP.

Jan