Re: [PATCH 01/24] XSM: reduce redundancy in hook machinery

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2026 17:05, Daniel P. Smith wrote:
> On 7/28/26 9:13 AM, Jan Beulich wrote:
>> Hooks not taking xsm_default_t as first argument could of course be
>> adjusted to take one, at which point they could be covered here as well.
>> Question is why there is this difference in the first place.
> 
> I have a theory but I am not confident to write it down. I can see if I 
> can confirm with DDG if you really care that much to know the why. 
> Personally having a consistent hook interface convention would provide a 
> simpler pattern for people to follow if they are having to introdcue a 
> new hook.

Well, I don't really need to know the reason. If you agree that making
things uniform is a good move, I can simply stick a few more patches at
the end of this series.

>> .{alloc,free}_security_evtchns() and their dummy wrappers use struct
>> evtchn[] notation, while xsm_{alloc,free}_security_evtchns() use struct
>> evtchn *. Is there a reason for this inconsistency?
> 
> Person had one habit that was counter to Xen preference and missed one? 
> I have no justification for it. IMHO the interfaces should be kept 
> consistent to enable better grep-ability.

Which direction would you want it changed? Personally I like the []
notation better when arrays are meant, but the pointer notation will be
quite a bit easier with the new hooks.h machinery.

>> @@ -206,113 +117,57 @@ static inline void xsm_security_domainin
>>       alternative_vcall(xsm_ops.security_domaininfo, d, info);
>>   }
>>   
>> -static inline int xsm_domain_create(
>> -    xsm_default_t def, struct domain *d, uint32_t ssidref)
>> -{
>> -    return alternative_call(xsm_ops.domain_create, d, ssidref);
>> -}
>> +#define XSM_ALT_void alternative_vcall
>> +#define XSM_ALT_int  return alternative_call
>>   
>> -static inline int xsm_getdomaininfo(xsm_default_t def, struct domain *d)
>> -{
>> -    return alternative_call(xsm_ops.getdomaininfo, d);
>> +#define XSM_HOOK0(rtype, name) \
>> +static inline rtype xsm_ ## name(xsm_default_t def) \
>> +{ \
>> +    XSM_ALT_ ## rtype(xsm_ops.name); \
>>   }
>>   
>> -static inline int xsm_get_domain_state(xsm_default_t def, struct domain *d)
>> -{
>> -    return alternative_call(xsm_ops.get_domain_state, d);
>> +#define XSM_HOOK1(rtype, name, type1) \
>> +static inline rtype xsm_ ## name(xsm_default_t def, type1 arg1) \
>> +{ \
>> +    XSM_ALT_ ## rtype(xsm_ops.name, arg1); \
>>   }
>>   
>> -static inline int xsm_set_target(
>> -    xsm_default_t def, struct domain *d, struct domain *e)
>> -{
>> -    return alternative_call(xsm_ops.set_target, d, e);
>> +#define XSM_HOOK2(rtype, name, type1, type2) \
>> +static inline rtype xsm_ ## name( \
>> +    xsm_default_t def, type1 arg1, type2 arg2) \
>> +{ \
>> +    XSM_ALT_ ## rtype(xsm_ops.name, arg1, arg2); \
>>   }
>>   
>> -static inline int xsm_domctl(xsm_default_t def, struct domain *d,
>> -                             struct xen_domctl *op)
>> -{
>> -    return alternative_call(xsm_ops.domctl, d, op);
>> +#define XSM_HOOK3(rtype, name, type1, type2, type3) \
>> +static inline rtype xsm_ ## name( \
>> +    xsm_default_t def, type1 arg1, type2 arg2, type3 arg3) \
>> +{ \
>> +    XSM_ALT_ ## rtype(xsm_ops.name, arg1, arg2, arg3); \
>>   }
>>   
>> -#ifdef CONFIG_SYSCTL
>> -static inline int xsm_sysctl(xsm_default_t def, const struct xen_sysctl *op)
>> -{
>> -    return alternative_call(xsm_ops.sysctl, op);
>> +#define XSM_HOOK4(rtype, name, type1, type2, type3, type4) \
>> +static inline rtype xsm_ ## name( \
>> +    xsm_default_t def, type1 arg1, type2 arg2, type3 arg3, type4 arg4) \
>> +{ \
>> +    XSM_ALT_ ## rtype(xsm_ops.name, arg1, arg2, arg3, arg4); \
>>   }
>> -#endif
>>   
>> -static inline int xsm_evtchn_unbound(
>> -    xsm_default_t def, struct domain *d1, struct evtchn *chn, domid_t id2)
>> -{
>> -    return alternative_call(xsm_ops.evtchn_unbound, d1, chn, id2);
>> +#define XSM_HOOK5(rtype, name, type1, type2, type3, type4, type5) \
>> +static inline rtype xsm_ ## name( \
>> +    xsm_default_t def, type1 arg1, type2 arg2, type3 arg3, type4 arg4, \
>> +    type4 arg5) \
> 
> Looks like you have copy/paste typo?

Indeed. And the last two parameters of .pci_config_permission() sadly aren't
distinct enough to make the flaw apparent at build time. (That looks to be
the only hook with 5 parameters.)

Thanks much for spotting.

> I would just note there is quite a bit of churn in this patch, most of 
> it is mechanical, but makes it easy for these to slip through.

Right. Fortunately this needs doing only once.

> After fixing this typo,
> 
> Acked-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>

Thanks.

Jan