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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.10.2025 22:20, Grygorii Strashko wrote:
> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -155,4 +155,19 @@ config DEBUG_INFO
>         "make install-xen" for installing xen.efi, stripping needs to be
>         done outside the Xen build environment).
>  
> +config HAS_VMTRACE
> +    bool
> +
> +config VMTRACE
> +    bool "HW VM tracing support"
> +    depends on HAS_VMTRACE
> +    default y
> +    help
> +      Enables HW VM tracing support which allows to configure HW processor
> +      features (vmtrace_op) to enable capturing information about software
> +      execution using dedicated hardware facilities with minimal interference
> +      to the software being traced. The trace date can be retrieved using 
> buffer

Nit: s/date/data/

> +      shared between Xen and domain
> +      (see XENMEM_acquire_resource(XENMEM_resource_vmtrace_buf)).
> +

I was actually meaning to ask that "VMX only" should somehow be mentioned here,
but then I noticed this is an arch-independent location. I'm not quite sure we
want it like this (just yet).

> @@ -2940,11 +2948,13 @@ static struct hvm_function_table 
> __initdata_cf_clobber vmx_function_table = {
>      .altp2m_vcpu_emulate_ve = vmx_vcpu_emulate_ve,
>      .altp2m_vcpu_emulate_vmfunc = vmx_vcpu_emulate_vmfunc,
>  #endif
> +#ifdef CONFIG_VMTRACE
>      .vmtrace_control = vmtrace_control,
>      .vmtrace_output_position = vmtrace_output_position,
>      .vmtrace_set_option = vmtrace_set_option,
>      .vmtrace_get_option = vmtrace_get_option,
>      .vmtrace_reset = vmtrace_reset,
> +#endif
>  
>      .get_reg = vmx_get_reg,
>      .set_reg = vmx_set_reg,

Blank line ahead of the new #ifdef?

> @@ -738,6 +740,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 )
> @@ -780,6 +783,12 @@ static inline int hvm_vmtrace_reset(struct vcpu *v)
>  
>      return -EOPNOTSUPP;
>  }
> +#else
> +static inline int hvm_vmtrace_reset(struct vcpu *v)
> +{
> +    return 0;
> +}
> +#endif

#ifdef inside the function body please, to reduce redundancy and to reduce the
risk of overlooking multiple places which need editing (when e.g. function
parameters change).

> --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> @@ -155,7 +155,9 @@ struct vmx_vcpu {
>      bool                 ept_spurious_misconfig;
>  
>      /* Processor Trace configured and enabled for the vcpu. */
> +#ifdef CONFIG_VMTRACE
>      bool                 ipt_active;
> +#endif

Imo such an #ifdef would better enclose the comment as well.

> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -253,7 +253,9 @@ void vm_event_fill_regs(vm_event_request_t *req)
>      req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
>      req->data.regs.x86.dr6 = ctxt.dr6;
>  
> +#ifdef CONFIG_VMTRACE
>      if ( hvm_vmtrace_output_position(curr, &req->data.regs.x86.vmtrace_pos) 
> != 1 )
> +#endif
>          req->data.regs.x86.vmtrace_pos = ~0;
>  #endif
>  }

Use IS_ENABLED() together with a function declaration (but no definition) in the
VMTRACE=n case?

Jan