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

["Daniel P. Smith" <dpsmith@apertussolutions.com>]

On 7/30/26 11:18 AM, Jan Beulich wrote:
> 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.
> 

Correct me if I am wrong, but we would be introducing an unused 
parameter in exchange for uniform interfaces that can be generated with 
machinery reducing hook maintenance overhead. IMHO I feel from a 
security standpoint this would be a win. Would you disagree?

>>> .{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.
> 

I agree, my preference is [] for array parameters. But this is a 
readability vs less fragile machinery. As much as I like to know that 
the parameter is meant to be an array vs a instance reference, I would 
prefer more reliability in the machinery. Since you are doing the work, 
I leave to you to decide level of effort vs the most resilient 
implementation of the 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.)
> 

I believe in 19 pci_config_permission() 5th parameter gets changed from 
1(uint8_t) to true (bool), while type4 is uint16_t.

> Thanks much for spotting.
> 

Your welcome.

>> 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.
> 

Yep, just makes review a little more fun.

>> After fixing this typo,
>>
>> Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> Thanks.
> 
> Jan