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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi Jan,

On 06.11.25 14:00, Jan Beulich wrote:
> 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. 
Right, Arch code advertise VMTRACE support with HAS_VMTRACE.
In this particular case:
config INTEL_VMX
...
        select HAS_VMTRACE


> 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).
All hvm_vmtrace_x() functions are inline - do you mean like below for all of 
them?

 static inline int hvm_vmtrace_get_option(
     struct vcpu *v, uint64_t key, uint64_t *value)
 {
+#ifdef CONFIG_VMTRACE
     if ( hvm_funcs.vmtrace_get_option )
         return alternative_call(hvm_funcs.vmtrace_get_option, v, key, value);
+#endif
 
     return -EOPNOTSUPP;
 }


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

--
Best regards,
-grygorii