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
