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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi Jan,

On 13.11.25 10:36, Jan Beulich wrote:
> 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.
Yep. assignment required ifdef wrapping.

"#ifndef vmtrace_available" will not work out of the box as there are

"if (vmtrace_available)" in code. So, can't just "not define"/undef 
"vmtrace_available".

> > --- 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. 
/* Function declaration(s) here are used without definition(s) to make compiler
   happy when VMTRACE=n while all call sites expected to be protected by ifdefs 
or
   IS_ENABLED() guards, so compiler DCE will eliminate unused code and overall
   build succeeds */

a little bit long :( ?


> 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?
You are talking about HVM=n stubs here, right?
I'll check, most probably they all(most) can be dropped.

> >   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.)
It might be a time to make it void() - what do you think?

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

--
Best regards,
-grygorii