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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2025 15:22, Grygorii Strashko wrote:
> --- a/xen/arch/x86/hvm/Kconfig
> +++ b/xen/arch/x86/hvm/Kconfig
> @@ -35,6 +35,18 @@ config INTEL_VMX
>         If your system includes a processor with Intel VT-x support, say Y.
>         If in doubt, say Y.
>  
> +config VMTRACE
> +    bool "HW VM tracing support"
> +    depends on INTEL_VMX
> +    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 data can be retrieved using 
> buffer
> +      shared between Xen and domain
> +      (see XENMEM_acquire_resource(XENMEM_resource_vmtrace_buf)).

Please check adjacent options above or ...

>  config HVM_FEP
>       bool "HVM Forced Emulation Prefix support (UNSUPPORTED)" if UNSUPPORTED
>       default DEBUG

... below for how proper indentation would look like here.

> --- 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

This imo wants to move ahead of the comment. (Feels a little like I may have
said so already, but it may well have been in the context of a different
recent patch.)

> @@ -772,13 +775,24 @@ static inline int hvm_vmtrace_get_option(
>  
>      return -EOPNOTSUPP;
>  }
> +#else
> +/*
> + * Function declaration(s) here are used without definition(s) to make 
> compiler
> + * happy when VMTRACE=n, compiler DCE will eliminate unused code.
> + */
> +int hvm_vmtrace_output_position(struct vcpu *v, uint64_t *pos);
> +#endif
>  
>  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 -EOPNOTSUPP;
> +#endif

No #else please and the #endif moved up.

> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -253,7 +253,8 @@ 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;
>  
> -    if ( hvm_vmtrace_output_position(curr, &req->data.regs.x86.vmtrace_pos) 
> != 1 )
> +    if ( IS_ENABLED(CONFIG_VMTRACE) &&
> +         hvm_vmtrace_output_position(curr, &req->data.regs.x86.vmtrace_pos) 
> != 1 )

Would be nice if the too-long-line issue here was also address, when the line
needs touching anyway.

Jan