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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.11.2025 13:53, Grygorii Strashko wrote:
> 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".

I meant this just for the case here, though. Elsewhere you want to stick to
checking CONFIG_VMTRACE.

>>> @@ -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 :( ?

Yes. I'm sure you can shorten this quite a bit.

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

Yes. Are there any others there?

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

Possibly (in a separate patch). I didn't check the call sites, though, i.e.
I can't exclude that the return value ought to be checked there instead.

Jan