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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi Jan,

On 17.11.25 18:52, Jan Beulich wrote:
> 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.
There is a mix in Kconfigs - some places <Tabs> some places <Spaces> :(
Will change to <Tabs>

> > --- 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.
I left it as is for better readability as an exception.
Will below be ok:

     if ( IS_ENABLED(CONFIG_VMTRACE) &&
-         hvm_vmtrace_output_position(curr, &req->data.regs.x86.vmtrace_pos) != 
1 )
+         hvm_vmtrace_output_position(curr,
+                                     &req->data.regs.x86.vmtrace_pos) != 1 )
         req->data.regs.x86.vmtrace_pos = ~0;

--
Best regards,
-grygorii