Re: [PATCH for-4.22] XSM: guard .sysctl() and .readconsole() hooks

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxxx>, 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