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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx>
> Thanks.
>
> Jan