Handling of unsupported sysctl commands currently diverges: some commands
return -EOPNOTSUPP, while others fall back to generic -ENOSYS.
Unify the behavior so that unsupported commands consistently return the
appropriate error code, allowing the control domain to correctly identify
unsupported operations.
Using IS_ENABLED() macro to identify disabled commands, allowing
dead code to be removed at compile time.
Signed-off-by: Milan Djokic <milan_djokic@epam.com>
---
xen/common/sysctl.c | 34 ++++++++++++++++++++--------------
xen/include/xen/livepatch.h | 2 +-
xen/include/xen/perfc.h | 4 +++-
xen/include/xen/spinlock.h | 6 +++---
xen/include/xen/trace.h | 2 +-
5 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 5207664252..1c4e0b60d8 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -113,17 +113,20 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
}
break;
-#ifdef CONFIG_PERF_COUNTERS
case XEN_SYSCTL_perfc_op:
- ret = perfc_control(&op->u.perfc_op);
+ if ( !IS_ENABLED(CONFIG_PERF_COUNTERS) )
+ ret = -EOPNOTSUPP;
+ else
+ ret = perfc_control(&op->u.perfc_op);
break;
-#endif
-#ifdef CONFIG_DEBUG_LOCK_PROFILE
case XEN_SYSCTL_lockprof_op:
- ret = spinlock_profile_control(&op->u.lockprof_op);
+ if ( !IS_ENABLED(CONFIG_DEBUG_LOCK_PROFILE) )
+ ret = -EOPNOTSUPP;
+ else
+ ret = spinlock_profile_control(&op->u.lockprof_op);
break;
-#endif
+
case XEN_SYSCTL_debug_keys:
{
char c;
@@ -170,19 +173,22 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
op->u.availheap.avail_bytes <<= PAGE_SHIFT;
break;
-#ifdef CONFIG_PM_STATS
case XEN_SYSCTL_get_pmstat:
- ret = do_get_pm_info(&op->u.get_pmstat);
+ if ( !IS_ENABLED(CONFIG_PM_STATS) )
+ ret = -EOPNOTSUPP;
+ else
+ ret = do_get_pm_info(&op->u.get_pmstat);
break;
-#endif
-#ifdef CONFIG_PM_OP
case XEN_SYSCTL_pm_op:
- ret = do_pm_op(&op->u.pm_op);
- if ( ret == -EAGAIN )
- copyback = 1;
+ if ( !IS_ENABLED(CONFIG_PM_OP) )
+ ret = -EOPNOTSUPP;
+ else {
+ ret = do_pm_op(&op->u.pm_op);
+ if ( ret == -EAGAIN )
+ copyback = 1;
+ }
break;
-#endif
case XEN_SYSCTL_page_offline_op:
{
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index d074a5bebe..1601b860d8 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -157,7 +157,7 @@ void revert_payload_tail(struct payload *data);
static inline int livepatch_op(struct xen_sysctl_livepatch_op *op)
{
- return -ENOSYS;
+ return -EOPNOTSUPP;
}
static inline void check_for_livepatch_work(void) {}
diff --git a/xen/include/xen/perfc.h b/xen/include/xen/perfc.h
index bf0eb032f7..a44c2f74a1 100644
--- a/xen/include/xen/perfc.h
+++ b/xen/include/xen/perfc.h
@@ -92,7 +92,6 @@ DECLARE_PER_CPU(perfc_t[NUM_PERFCOUNTERS], perfcounters);
#endif
struct xen_sysctl_perfc_op;
-int perfc_control(struct xen_sysctl_perfc_op *pc);
extern void cf_check perfc_printall(unsigned char key);
extern void cf_check perfc_reset(unsigned char key);
@@ -114,4 +113,7 @@ extern void cf_check perfc_reset(unsigned char key);
#endif /* CONFIG_PERF_COUNTERS */
+struct xen_sysctl_perfc_op;
+extern int perfc_control(struct xen_sysctl_perfc_op *pc);
+
#endif /* __XEN_PERFC_H__ */
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index ca9d8c7ec0..10ee81b7db 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -7,6 +7,7 @@
#include <asm/system.h>
#include <asm/spinlock.h>
+#include <public/sysctl.h>
#define SPINLOCK_CPU_BITS 16
@@ -40,8 +41,6 @@ union lock_debug { };
#ifdef CONFIG_DEBUG_LOCK_PROFILE
-#include <public/sysctl.h>
-
/*
lock profiling on:
@@ -164,7 +163,6 @@ void _lock_profile_deregister_struct(int32_t type,
#define lock_profile_deregister_struct(type, ptr) \
_lock_profile_deregister_struct(type, &((ptr)->profile_head))
-extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
extern void cf_check spinlock_profile_printall(unsigned char key);
extern void cf_check spinlock_profile_reset(unsigned char key);
@@ -360,4 +358,6 @@ static always_inline void nrspin_lock_irq(rspinlock_t *l)
#define nrspin_unlock_irqrestore(l, f) _nrspin_unlock_irqrestore(l, f)
#define nrspin_unlock_irq(l) _nrspin_unlock_irq(l)
+extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
+
#endif /* __SPINLOCK_H__ */
diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h
index 30ebdcc47f..fff3a4f451 100644
--- a/xen/include/xen/trace.h
+++ b/xen/include/xen/trace.h
@@ -48,7 +48,7 @@ void __trace_hypercall(uint32_t event, unsigned long op,
static inline int tb_control(struct xen_sysctl_tbuf_op *tbc)
{
- return -ENOSYS;
+ return -EOPNOTSUPP;
}
static inline int trace_will_trace_event(uint32_t event)
--
2.43.0On 05.12.2025 21:36, Milan Djokic wrote:
> Handling of unsupported sysctl commands currently diverges: some commands
> return -EOPNOTSUPP, while others fall back to generic -ENOSYS.
>
> Unify the behavior so that unsupported commands consistently return the
> appropriate error code, allowing the control domain to correctly identify
> unsupported operations.
>
> Using IS_ENABLED() macro to identify disabled commands, allowing
> dead code to be removed at compile time.
>
> Signed-off-by: Milan Djokic <milan_djokic@epam.com>
Overall quite okay imo, yet there are a number of small issues.
> @@ -170,19 +173,22 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> op->u.availheap.avail_bytes <<= PAGE_SHIFT;
> break;
>
> -#ifdef CONFIG_PM_STATS
> case XEN_SYSCTL_get_pmstat:
> - ret = do_get_pm_info(&op->u.get_pmstat);
> + if ( !IS_ENABLED(CONFIG_PM_STATS) )
> + ret = -EOPNOTSUPP;
> + else
> + ret = do_get_pm_info(&op->u.get_pmstat);
> break;
> -#endif
>
> -#ifdef CONFIG_PM_OP
> case XEN_SYSCTL_pm_op:
> - ret = do_pm_op(&op->u.pm_op);
> - if ( ret == -EAGAIN )
> - copyback = 1;
> + if ( !IS_ENABLED(CONFIG_PM_OP) )
> + ret = -EOPNOTSUPP;
> + else {
Misplaced figure brace.
> --- a/xen/include/xen/perfc.h
> +++ b/xen/include/xen/perfc.h
> @@ -92,7 +92,6 @@ DECLARE_PER_CPU(perfc_t[NUM_PERFCOUNTERS], perfcounters);
> #endif
>
> struct xen_sysctl_perfc_op;
> -int perfc_control(struct xen_sysctl_perfc_op *pc);
>
> extern void cf_check perfc_printall(unsigned char key);
> extern void cf_check perfc_reset(unsigned char key);
> @@ -114,4 +113,7 @@ extern void cf_check perfc_reset(unsigned char key);
>
> #endif /* CONFIG_PERF_COUNTERS */
>
> +struct xen_sysctl_perfc_op;
> +extern int perfc_control(struct xen_sysctl_perfc_op *pc);
Why would you move the function decl, but duplicate the struct forward decl?
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -7,6 +7,7 @@
>
> #include <asm/system.h>
> #include <asm/spinlock.h>
> +#include <public/sysctl.h>
Why would this be needed? It means effectively the whole codebase gains a
dependency on this header even when DEBUG_LOCK_PROFILE=n. Yes, you may
need ...
> @@ -40,8 +41,6 @@ union lock_debug { };
>
> #ifdef CONFIG_DEBUG_LOCK_PROFILE
>
> -#include <public/sysctl.h>
> -
> /*
> lock profiling on:
>
> @@ -164,7 +163,6 @@ void _lock_profile_deregister_struct(int32_t type,
> #define lock_profile_deregister_struct(type, ptr) \
> _lock_profile_deregister_struct(type, &((ptr)->profile_head))
>
> -extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
> extern void cf_check spinlock_profile_printall(unsigned char key);
> extern void cf_check spinlock_profile_reset(unsigned char key);
>
> @@ -360,4 +358,6 @@ static always_inline void nrspin_lock_irq(rspinlock_t *l)
> #define nrspin_unlock_irqrestore(l, f) _nrspin_unlock_irqrestore(l, f)
> #define nrspin_unlock_irq(l) _nrspin_unlock_irq(l)
>
> +extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
> +
> #endif /* __SPINLOCK_H__ */
... a forward decl of struct xen_sysctl_lockprof_op; I don't see what's
wrong with doing just that.
Jan
On 12/8/25 14:32, Jan Beulich wrote:
> On 05.12.2025 21:36, Milan Djokic wrote:
>> Handling of unsupported sysctl commands currently diverges: some commands
>> return -EOPNOTSUPP, while others fall back to generic -ENOSYS.
>>
>> Unify the behavior so that unsupported commands consistently return the
>> appropriate error code, allowing the control domain to correctly identify
>> unsupported operations.
>>
>> Using IS_ENABLED() macro to identify disabled commands, allowing
>> dead code to be removed at compile time.
>>
>> Signed-off-by: Milan Djokic <milan_djokic@epam.com>
>
> Overall quite okay imo, yet there are a number of small issues.
>
>> @@ -170,19 +173,22 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>> op->u.availheap.avail_bytes <<= PAGE_SHIFT;
>> break;
>>
>> -#ifdef CONFIG_PM_STATS
>> case XEN_SYSCTL_get_pmstat:
>> - ret = do_get_pm_info(&op->u.get_pmstat);
>> + if ( !IS_ENABLED(CONFIG_PM_STATS) )
>> + ret = -EOPNOTSUPP;
>> + else
>> + ret = do_get_pm_info(&op->u.get_pmstat);
>> break;
>> -#endif
>>
>> -#ifdef CONFIG_PM_OP
>> case XEN_SYSCTL_pm_op:
>> - ret = do_pm_op(&op->u.pm_op);
>> - if ( ret == -EAGAIN )
>> - copyback = 1;
>> + if ( !IS_ENABLED(CONFIG_PM_OP) )
>> + ret = -EOPNOTSUPP;
>> + else {
>
> Misplaced figure brace.
>
>> --- a/xen/include/xen/perfc.h
>> +++ b/xen/include/xen/perfc.h
>> @@ -92,7 +92,6 @@ DECLARE_PER_CPU(perfc_t[NUM_PERFCOUNTERS], perfcounters);
>> #endif
>>
>> struct xen_sysctl_perfc_op;
>> -int perfc_control(struct xen_sysctl_perfc_op *pc);
>>
>> extern void cf_check perfc_printall(unsigned char key);
>> extern void cf_check perfc_reset(unsigned char key);
>> @@ -114,4 +113,7 @@ extern void cf_check perfc_reset(unsigned char key);
>>
>> #endif /* CONFIG_PERF_COUNTERS */
>>
>> +struct xen_sysctl_perfc_op;
>> +extern int perfc_control(struct xen_sysctl_perfc_op *pc);
>
> Why would you move the function decl, but duplicate the struct forward decl?
>
>> --- a/xen/include/xen/spinlock.h
>> +++ b/xen/include/xen/spinlock.h
>> @@ -7,6 +7,7 @@
>>
>> #include <asm/system.h>
>> #include <asm/spinlock.h>
>> +#include <public/sysctl.h>
>
> Why would this be needed? It means effectively the whole codebase gains a
> dependency on this header even when DEBUG_LOCK_PROFILE=n. Yes, you may
> need ...
>
>> @@ -40,8 +41,6 @@ union lock_debug { };
>>
>> #ifdef CONFIG_DEBUG_LOCK_PROFILE
>>
>> -#include <public/sysctl.h>
>> -
>> /*
>> lock profiling on:
>>
>> @@ -164,7 +163,6 @@ void _lock_profile_deregister_struct(int32_t type,
>> #define lock_profile_deregister_struct(type, ptr) \
>> _lock_profile_deregister_struct(type, &((ptr)->profile_head))
>>
>> -extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
>> extern void cf_check spinlock_profile_printall(unsigned char key);
>> extern void cf_check spinlock_profile_reset(unsigned char key);
>>
>> @@ -360,4 +358,6 @@ static always_inline void nrspin_lock_irq(rspinlock_t *l)
>> #define nrspin_unlock_irqrestore(l, f) _nrspin_unlock_irqrestore(l, f)
>> #define nrspin_unlock_irq(l) _nrspin_unlock_irq(l)
>>
>> +extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
>> +
>> #endif /* __SPINLOCK_H__ */
>
> ... a forward decl of struct xen_sysctl_lockprof_op; I don't see what's
> wrong with doing just that.
>
> Jan
I’ll correct these mistakes. Thanks for the suggestions.
BR,
Milan
© 2016 - 2025 Red Hat, Inc.