Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
hypervisors.

While moving the #ifdef for the corresponding xsm_*() wrappers, also move
those for xsm_page_offline() (where the hook pointer field already is
suitably guarded).

Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -61,8 +61,10 @@ struct xsm_ops {
 #endif
     int (*set_target)(struct domain *d, struct domain *e);
     int (*domctl)(struct domain *d, struct xen_domctl *op);
+#ifdef CONFIG_SYSCTL
     int (*sysctl)(int cmd);
     int (*readconsole)(uint32_t clear);
+#endif
 
     int (*evtchn_unbound)(struct domain *d, struct evtchn *chn, domid_t id2);
     int (*evtchn_interdomain)(struct domain *d1, struct evtchn *chn1,
@@ -248,23 +250,17 @@ static inline int xsm_domctl(xsm_default
     return alternative_call(xsm_ops.domctl, d, op);
 }
 
+#ifdef CONFIG_SYSCTL
 static inline int xsm_sysctl(xsm_default_t def, int cmd)
 {
-#ifdef CONFIG_SYSCTL
     return alternative_call(xsm_ops.sysctl, cmd);
-#else
-    return -EOPNOTSUPP;
-#endif
 }
 
 static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
 {
-#ifdef CONFIG_SYSCTL
     return alternative_call(xsm_ops.readconsole, clear);
-#else
-    return -EOPNOTSUPP;
-#endif
 }
+#endif
 
 static inline int xsm_evtchn_unbound(
     xsm_default_t def, struct domain *d1, struct evtchn *chn, domid_t id2)
@@ -558,14 +554,12 @@ static inline int xsm_resource_setup_mis
     return alternative_call(xsm_ops.resource_setup_misc);
 }
 
+#ifdef CONFIG_SYSCTL
 static inline int xsm_page_offline(xsm_default_t def, uint32_t cmd)
 {
-#ifdef CONFIG_SYSCTL
     return alternative_call(xsm_ops.page_offline, cmd);
-#else
-    return -EOPNOTSUPP;
-#endif
 }
+#endif
 
 static inline int xsm_hypfs_op(xsm_default_t def)
 {

On 6/18/26 1:32 PM, Jan Beulich wrote:
> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
> hypervisors.
> 
> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
> those for xsm_page_offline() (where the hook pointer field already is
> suitably guarded).
> 
> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 

Release-Acked-by: Oleskii Kurochko <oleskii.kurochko@gmail.com>

~ Oleksii

On 18/06/2026 12:32 pm, Jan Beulich wrote:
> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
> hypervisors.
>
> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
> those for xsm_page_offline() (where the hook pointer field already is
> suitably guarded).
>
> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
too, which should be able to spot things like this.

This is a regression vs 4.21, so does need including.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...

>
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -61,8 +61,10 @@ struct xsm_ops {
>  #endif
>      int (*set_target)(struct domain *d, struct domain *e);
>      int (*domctl)(struct domain *d, struct xen_domctl *op);
> +#ifdef CONFIG_SYSCTL
>      int (*sysctl)(int cmd);
>      int (*readconsole)(uint32_t clear);
> +#endif

... this is now the 3rd CONFIG_SYSCTL in xsm_ops.

I know it will grow the diff, but can we see about collecting them into
a single region, and in dummy_ops too?  It will shrink the overall
result, and the order of pointers in this ops structure is uninteresting.

~Andrew

On 18.06.2026 14:13, Andrew Cooper wrote:
> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>> hypervisors.
>>
>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>> those for xsm_page_offline() (where the hook pointer field already is
>> suitably guarded).
>>
>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
> too, which should be able to spot things like this.
> 
> This is a regression vs 4.21, so does need including.

Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.

> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...

Thanks.

>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>  #endif
>>      int (*set_target)(struct domain *d, struct domain *e);
>>      int (*domctl)(struct domain *d, struct xen_domctl *op);
>> +#ifdef CONFIG_SYSCTL
>>      int (*sysctl)(int cmd);
>>      int (*readconsole)(uint32_t clear);
>> +#endif
> 
> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
> 
> I know it will grow the diff, but can we see about collecting them into
> a single region, and in dummy_ops too?  It will shrink the overall
> result, and the order of pointers in this ops structure is uninteresting.

I have a far more consolidating patch in the works, which is how I actually
noticed the issue. I'd prefer to keep things as simple as possible here.

Jan

On 6/18/26 8:23 AM, Jan Beulich wrote:
> On 18.06.2026 14:13, Andrew Cooper wrote:
>> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>>> hypervisors.
>>>
>>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>>> those for xsm_page_offline() (where the hook pointer field already is
>>> suitably guarded).
>>>
>>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
>> too, which should be able to spot things like this.
>>
>> This is a regression vs 4.21, so does need including.
> 
> Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
> 
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...
> 
> Thanks.
> 
>>> --- a/xen/include/xsm/xsm.h
>>> +++ b/xen/include/xsm/xsm.h
>>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>>   #endif
>>>       int (*set_target)(struct domain *d, struct domain *e);
>>>       int (*domctl)(struct domain *d, struct xen_domctl *op);
>>> +#ifdef CONFIG_SYSCTL
>>>       int (*sysctl)(int cmd);
>>>       int (*readconsole)(uint32_t clear);
>>> +#endif
>>
>> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
>>
>> I know it will grow the diff, but can we see about collecting them into
>> a single region, and in dummy_ops too?  It will shrink the overall
>> result, and the order of pointers in this ops structure is uninteresting.
> 
> I have a far more consolidating patch in the works, which is how I actually
> noticed the issue. I'd prefer to keep things as simple as possible here.

Would also be good to clean up flask_ops similarly.

v/r,
dps

On 18.06.2026 14:40, Daniel P. Smith wrote:
> On 6/18/26 8:23 AM, Jan Beulich wrote:
>> On 18.06.2026 14:13, Andrew Cooper wrote:
>>> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>>>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>>>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>>>> hypervisors.
>>>>
>>>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>>>> those for xsm_page_offline() (where the hook pointer field already is
>>>> suitably guarded).
>>>>
>>>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>>>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
>>> too, which should be able to spot things like this.
>>>
>>> This is a regression vs 4.21, so does need including.
>>
>> Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
>>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...
>>
>> Thanks.
>>
>>>> --- a/xen/include/xsm/xsm.h
>>>> +++ b/xen/include/xsm/xsm.h
>>>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>>>   #endif
>>>>       int (*set_target)(struct domain *d, struct domain *e);
>>>>       int (*domctl)(struct domain *d, struct xen_domctl *op);
>>>> +#ifdef CONFIG_SYSCTL
>>>>       int (*sysctl)(int cmd);
>>>>       int (*readconsole)(uint32_t clear);
>>>> +#endif
>>>
>>> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
>>>
>>> I know it will grow the diff, but can we see about collecting them into
>>> a single region, and in dummy_ops too?  It will shrink the overall
>>> result, and the order of pointers in this ops structure is uninteresting.
>>
>> I have a far more consolidating patch in the works, which is how I actually
>> noticed the issue. I'd prefer to keep things as simple as possible here.
> 
> Would also be good to clean up flask_ops similarly.

Hence the "far more consolidating" - that'll happen (implicitly) all in one
go.

Jan

On 6/18/26 8:43 AM, Jan Beulich wrote:
> On 18.06.2026 14:40, Daniel P. Smith wrote:
>> On 6/18/26 8:23 AM, Jan Beulich wrote:
>>> On 18.06.2026 14:13, Andrew Cooper wrote:
>>>> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>>>>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>>>>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>>>>> hypervisors.
>>>>>
>>>>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>>>>> those for xsm_page_offline() (where the hook pointer field already is
>>>>> suitably guarded).
>>>>>
>>>>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>>>>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
>>>> too, which should be able to spot things like this.
>>>>
>>>> This is a regression vs 4.21, so does need including.
>>>
>>> Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
>>>
>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...
>>>
>>> Thanks.
>>>
>>>>> --- a/xen/include/xsm/xsm.h
>>>>> +++ b/xen/include/xsm/xsm.h
>>>>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>>>>    #endif
>>>>>        int (*set_target)(struct domain *d, struct domain *e);
>>>>>        int (*domctl)(struct domain *d, struct xen_domctl *op);
>>>>> +#ifdef CONFIG_SYSCTL
>>>>>        int (*sysctl)(int cmd);
>>>>>        int (*readconsole)(uint32_t clear);
>>>>> +#endif
>>>>
>>>> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
>>>>
>>>> I know it will grow the diff, but can we see about collecting them into
>>>> a single region, and in dummy_ops too?  It will shrink the overall
>>>> result, and the order of pointers in this ops structure is uninteresting.
>>>
>>> I have a far more consolidating patch in the works, which is how I actually
>>> noticed the issue. I'd prefer to keep things as simple as possible here.
>>
>> Would also be good to clean up flask_ops similarly.
> 
> Hence the "far more consolidating" - that'll happen (implicitly) all in one
> go.
> 

Perfect. Just wanted to be sure it was on your radar.

v/r,
dps

On 6/18/26 8:23 AM, Jan Beulich wrote:
> On 18.06.2026 14:13, Andrew Cooper wrote:
>> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>>> hypervisors.
>>>
>>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>>> those for xsm_page_offline() (where the hook pointer field already is
>>> suitably guarded).
>>>
>>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
>> too, which should be able to spot things like this.
>>
>> This is a regression vs 4.21, so does need including.
> 
> Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
> 
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...
> 
> Thanks.
> 
>>> --- a/xen/include/xsm/xsm.h
>>> +++ b/xen/include/xsm/xsm.h
>>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>>   #endif
>>>       int (*set_target)(struct domain *d, struct domain *e);
>>>       int (*domctl)(struct domain *d, struct xen_domctl *op);
>>> +#ifdef CONFIG_SYSCTL
>>>       int (*sysctl)(int cmd);
>>>       int (*readconsole)(uint32_t clear);
>>> +#endif
>>
>> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
>>
>> I know it will grow the diff, but can we see about collecting them into
>> a single region, and in dummy_ops too?  It will shrink the overall
>> result, and the order of pointers in this ops structure is uninteresting.
> 
> I have a far more consolidating patch in the works, which is how I actually
> noticed the issue. I'd prefer to keep things as simple as possible here.

By the way, I was going back through this and notices that they are not 
ifdef out in xsm/dummy.h. Are we relying on them being inlines to ensure 
that they do not result in dead code?

v/r,
dps

On 18.06.2026 14:34, Daniel P. Smith wrote:
> On 6/18/26 8:23 AM, Jan Beulich wrote:
>> On 18.06.2026 14:13, Andrew Cooper wrote:
>>> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>>>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>>>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>>>> hypervisors.
>>>>
>>>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>>>> those for xsm_page_offline() (where the hook pointer field already is
>>>> suitably guarded).
>>>>
>>>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>>>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
>>> too, which should be able to spot things like this.
>>>
>>> This is a regression vs 4.21, so does need including.
>>
>> Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
>>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...
>>
>> Thanks.
>>
>>>> --- a/xen/include/xsm/xsm.h
>>>> +++ b/xen/include/xsm/xsm.h
>>>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>>>   #endif
>>>>       int (*set_target)(struct domain *d, struct domain *e);
>>>>       int (*domctl)(struct domain *d, struct xen_domctl *op);
>>>> +#ifdef CONFIG_SYSCTL
>>>>       int (*sysctl)(int cmd);
>>>>       int (*readconsole)(uint32_t clear);
>>>> +#endif
>>>
>>> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
>>>
>>> I know it will grow the diff, but can we see about collecting them into
>>> a single region, and in dummy_ops too?  It will shrink the overall
>>> result, and the order of pointers in this ops structure is uninteresting.
>>
>> I have a far more consolidating patch in the works, which is how I actually
>> noticed the issue. I'd prefer to keep things as simple as possible here.
> 
> By the way, I was going back through this and notices that they are not 
> ifdef out in xsm/dummy.h. Are we relying on them being inlines to ensure 
> that they do not result in dead code?

I think so. Maybe it's unhelpful that we have

#define XSM_INLINE __maybe_unused

and we may instead better want explicit #ifdef wherever necessary.

Jan

On 6/18/26 8:42 AM, Jan Beulich wrote:
> On 18.06.2026 14:34, Daniel P. Smith wrote:
>> On 6/18/26 8:23 AM, Jan Beulich wrote:
>>> On 18.06.2026 14:13, Andrew Cooper wrote:
>>>> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>>>>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>>>>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>>>>> hypervisors.
>>>>>
>>>>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>>>>> those for xsm_page_offline() (where the hook pointer field already is
>>>>> suitably guarded).
>>>>>
>>>>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>>>>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
>>>> too, which should be able to spot things like this.
>>>>
>>>> This is a regression vs 4.21, so does need including.
>>>
>>> Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
>>>
>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...
>>>
>>> Thanks.
>>>
>>>>> --- a/xen/include/xsm/xsm.h
>>>>> +++ b/xen/include/xsm/xsm.h
>>>>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>>>>    #endif
>>>>>        int (*set_target)(struct domain *d, struct domain *e);
>>>>>        int (*domctl)(struct domain *d, struct xen_domctl *op);
>>>>> +#ifdef CONFIG_SYSCTL
>>>>>        int (*sysctl)(int cmd);
>>>>>        int (*readconsole)(uint32_t clear);
>>>>> +#endif
>>>>
>>>> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
>>>>
>>>> I know it will grow the diff, but can we see about collecting them into
>>>> a single region, and in dummy_ops too?  It will shrink the overall
>>>> result, and the order of pointers in this ops structure is uninteresting.
>>>
>>> I have a far more consolidating patch in the works, which is how I actually
>>> noticed the issue. I'd prefer to keep things as simple as possible here.
>>
>> By the way, I was going back through this and notices that they are not
>> ifdef out in xsm/dummy.h. Are we relying on them being inlines to ensure
>> that they do not result in dead code?
> 
> I think so. Maybe it's unhelpful that we have
> 

To be clear, you want to leave as is for now (or at least pick up in 
another patch)?

> #define XSM_INLINE __maybe_unused
> 
> and we may instead better want explicit #ifdef wherever necessary.
> 

It's been a while since I tore that apart, but I believe there is some 
trickery hidden in XSM_INLINE depending on whether XSM is "on or off". I 
could be wrong as it's been a while since I tried to unwrap all of it.

v/r,
dps

On 18.06.2026 14:48, Daniel P. Smith wrote:
> On 6/18/26 8:42 AM, Jan Beulich wrote:
>> On 18.06.2026 14:34, Daniel P. Smith wrote:
>>> On 6/18/26 8:23 AM, Jan Beulich wrote:
>>>> On 18.06.2026 14:13, Andrew Cooper wrote:
>>>>> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>>>>>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>>>>>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>>>>>> hypervisors.
>>>>>>
>>>>>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>>>>>> those for xsm_page_offline() (where the hook pointer field already is
>>>>>> suitably guarded).
>>>>>>
>>>>>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>>>>>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
>>>>> too, which should be able to spot things like this.
>>>>>
>>>>> This is a regression vs 4.21, so does need including.
>>>>
>>>> Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
>>>>
>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...
>>>>
>>>> Thanks.
>>>>
>>>>>> --- a/xen/include/xsm/xsm.h
>>>>>> +++ b/xen/include/xsm/xsm.h
>>>>>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>>>>>    #endif
>>>>>>        int (*set_target)(struct domain *d, struct domain *e);
>>>>>>        int (*domctl)(struct domain *d, struct xen_domctl *op);
>>>>>> +#ifdef CONFIG_SYSCTL
>>>>>>        int (*sysctl)(int cmd);
>>>>>>        int (*readconsole)(uint32_t clear);
>>>>>> +#endif
>>>>>
>>>>> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
>>>>>
>>>>> I know it will grow the diff, but can we see about collecting them into
>>>>> a single region, and in dummy_ops too?  It will shrink the overall
>>>>> result, and the order of pointers in this ops structure is uninteresting.
>>>>
>>>> I have a far more consolidating patch in the works, which is how I actually
>>>> noticed the issue. I'd prefer to keep things as simple as possible here.
>>>
>>> By the way, I was going back through this and notices that they are not
>>> ifdef out in xsm/dummy.h. Are we relying on them being inlines to ensure
>>> that they do not result in dead code?
>>
>> I think so. Maybe it's unhelpful that we have
> 
> To be clear, you want to leave as is for now (or at least pick up in 
> another patch)?

It's up to you - I can add the #ifdef there, but it doesn't look to strictly
be needed.

>> #define XSM_INLINE __maybe_unused
>>
>> and we may instead better want explicit #ifdef wherever necessary.
> 
> It's been a while since I tore that apart, but I believe there is some 
> trickery hidden in XSM_INLINE depending on whether XSM is "on or off". I 
> could be wrong as it's been a while since I tried to unwrap all of it.

Aiui the "trickery" is that it's

#define XSM_INLINE __maybe_unused

with XSM=y and

#define XSM_INLINE always_inline

with XSM=n.

Jan

On 18.06.2026 14:51, Jan Beulich wrote:
> On 18.06.2026 14:48, Daniel P. Smith wrote:
>> On 6/18/26 8:42 AM, Jan Beulich wrote:
>>> On 18.06.2026 14:34, Daniel P. Smith wrote:
>>>> On 6/18/26 8:23 AM, Jan Beulich wrote:
>>>>> On 18.06.2026 14:13, Andrew Cooper wrote:
>>>>>> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>>>>>>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>>>>>>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>>>>>>> hypervisors.
>>>>>>>
>>>>>>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>>>>>>> those for xsm_page_offline() (where the hook pointer field already is
>>>>>>> suitably guarded).
>>>>>>>
>>>>>>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>>>>>>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>
>>>>>> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
>>>>>> too, which should be able to spot things like this.
>>>>>>
>>>>>> This is a regression vs 4.21, so does need including.
>>>>>
>>>>> Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
>>>>>
>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...
>>>>>
>>>>> Thanks.
>>>>>
>>>>>>> --- a/xen/include/xsm/xsm.h
>>>>>>> +++ b/xen/include/xsm/xsm.h
>>>>>>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>>>>>>    #endif
>>>>>>>        int (*set_target)(struct domain *d, struct domain *e);
>>>>>>>        int (*domctl)(struct domain *d, struct xen_domctl *op);
>>>>>>> +#ifdef CONFIG_SYSCTL
>>>>>>>        int (*sysctl)(int cmd);
>>>>>>>        int (*readconsole)(uint32_t clear);
>>>>>>> +#endif
>>>>>>
>>>>>> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
>>>>>>
>>>>>> I know it will grow the diff, but can we see about collecting them into
>>>>>> a single region, and in dummy_ops too?  It will shrink the overall
>>>>>> result, and the order of pointers in this ops structure is uninteresting.
>>>>>
>>>>> I have a far more consolidating patch in the works, which is how I actually
>>>>> noticed the issue. I'd prefer to keep things as simple as possible here.
>>>>
>>>> By the way, I was going back through this and notices that they are not
>>>> ifdef out in xsm/dummy.h. Are we relying on them being inlines to ensure
>>>> that they do not result in dead code?
>>>
>>> I think so. Maybe it's unhelpful that we have
>>
>> To be clear, you want to leave as is for now (or at least pick up in 
>> another patch)?
> 
> It's up to you - I can add the #ifdef there, but it doesn't look to strictly
> be needed.

Can you please make explicit which way you'd like it in order to provide an
ack, so this bug fix can go in?

Jan

On 6/18/26 10:50, Jan Beulich wrote:
> On 18.06.2026 14:51, Jan Beulich wrote:
>> On 18.06.2026 14:48, Daniel P. Smith wrote:
>>> On 6/18/26 8:42 AM, Jan Beulich wrote:
>>>> On 18.06.2026 14:34, Daniel P. Smith wrote:
>>>>> On 6/18/26 8:23 AM, Jan Beulich wrote:
>>>>>> On 18.06.2026 14:13, Andrew Cooper wrote:
>>>>>>> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>>>>>>>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>>>>>>>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>>>>>>>> hypervisors.
>>>>>>>>
>>>>>>>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>>>>>>>> those for xsm_page_offline() (where the hook pointer field already is
>>>>>>>> suitably guarded).
>>>>>>>>
>>>>>>>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>>>>>>>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>
>>>>>>> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
>>>>>>> too, which should be able to spot things like this.
>>>>>>>
>>>>>>> This is a regression vs 4.21, so does need including.
>>>>>>
>>>>>> Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
>>>>>>
>>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>>>> --- a/xen/include/xsm/xsm.h
>>>>>>>> +++ b/xen/include/xsm/xsm.h
>>>>>>>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>>>>>>>     #endif
>>>>>>>>         int (*set_target)(struct domain *d, struct domain *e);
>>>>>>>>         int (*domctl)(struct domain *d, struct xen_domctl *op);
>>>>>>>> +#ifdef CONFIG_SYSCTL
>>>>>>>>         int (*sysctl)(int cmd);
>>>>>>>>         int (*readconsole)(uint32_t clear);
>>>>>>>> +#endif
>>>>>>>
>>>>>>> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
>>>>>>>
>>>>>>> I know it will grow the diff, but can we see about collecting them into
>>>>>>> a single region, and in dummy_ops too?  It will shrink the overall
>>>>>>> result, and the order of pointers in this ops structure is uninteresting.
>>>>>>
>>>>>> I have a far more consolidating patch in the works, which is how I actually
>>>>>> noticed the issue. I'd prefer to keep things as simple as possible here.
>>>>>
>>>>> By the way, I was going back through this and notices that they are not
>>>>> ifdef out in xsm/dummy.h. Are we relying on them being inlines to ensure
>>>>> that they do not result in dead code?
>>>>
>>>> I think so. Maybe it's unhelpful that we have
>>>
>>> To be clear, you want to leave as is for now (or at least pick up in
>>> another patch)?
>>
>> It's up to you - I can add the #ifdef there, but it doesn't look to strictly
>> be needed.

It's not needed and will create minimal but extra churn on back port, so 
we can forego for now.

> Can you please make explicit which way you'd like it in order to provide an
> ack, so this bug fix can go in?

Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>

On 18/06/2026 1:23 pm, Jan Beulich wrote:
> On 18.06.2026 14:13, Andrew Cooper wrote:
>> On 18/06/2026 12:32 pm, Jan Beulich wrote:
>>> Leaving the hook pointers in struct xsm_ops when !SYSCTL would lead to
>>> the BUG_ON() in xsm_fixup_ops() triggering for respectively configured
>>> hypervisors.
>>>
>>> While moving the #ifdef for the corresponding xsm_*() wrappers, also move
>>> those for xsm_page_offline() (where the hook pointer field already is
>>> suitably guarded).
>>>
>>> Fixes: c9eabaa03a68 ("xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL")
>>> Fixes: bddd9af6049f ("xen/sysctl: wrap around XEN_SYSCTL_readconsole")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Ugly.  We probably ought to see about booting the RANDCONFIG hypervisor
>> too, which should be able to spot things like this.
>>
>> This is a regression vs 4.21, so does need including.
> Aiui it's a regression vs 4.20, i.e. will want backporting to 4.21.
>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although...
> Thanks.
>
>>> --- a/xen/include/xsm/xsm.h
>>> +++ b/xen/include/xsm/xsm.h
>>> @@ -61,8 +61,10 @@ struct xsm_ops {
>>>  #endif
>>>      int (*set_target)(struct domain *d, struct domain *e);
>>>      int (*domctl)(struct domain *d, struct xen_domctl *op);
>>> +#ifdef CONFIG_SYSCTL
>>>      int (*sysctl)(int cmd);
>>>      int (*readconsole)(uint32_t clear);
>>> +#endif
>> ... this is now the 3rd CONFIG_SYSCTL in xsm_ops.
>>
>> I know it will grow the diff, but can we see about collecting them into
>> a single region, and in dummy_ops too?  It will shrink the overall
>> result, and the order of pointers in this ops structure is uninteresting.
> I have a far more consolidating patch in the works, which is how I actually
> noticed the issue. I'd prefer to keep things as simple as possible here.

Ok.  Fine by me.

~Andrew