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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.11.2025 14:50, Grygorii Strashko wrote:
> 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).
> 
> ?

To rephrase the question: Are we expecting anything other than VMX to support
VMTRACE any time soon?

>>> @@ -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;
>   }

No, the request was for just the single function that you add a 2nd instance of.

Jan