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
