Re: [PATCH] x86: rework hypercall argument count table instantiation & use

[Juergen Gross <jgross@xxxxxxxx>]

On 15.08.22 17:20, Jan Beulich wrote:
> On 15.08.2022 17:13, Juergen Gross wrote:
> > On 27.07.22 18:00, Jan Beulich wrote:
> > > The initial observation were duplicate symbols that our checking warns
> > > about. Instead of merely renaming one or both pair(s) of symbols,
> > > reduce #ifdef-ary at the same time by moving the instantiation of the
> > > arrays into macros (a native and a 32-bit one each, where likely more
> > > redundancy could be eliminated, if we really wanted to). While doing the
> > > conversion also stop open-coding array_access_nospec().
> > >
> > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
> Thanks.
>
> > With one small nit ...
> >
> > > --- a/xen/arch/x86/include/asm/hypercall.h
> > > +++ b/xen/arch/x86/include/asm/hypercall.h
> > > @@ -43,16 +43,6 @@ compat_common_vcpu_op(
> > >    
> > >    #endif /* CONFIG_COMPAT */
> > >    
> > > -#ifndef NDEBUG
> > > -static inline unsigned int _get_nargs(const unsigned char *tbl, unsigned int c)
> > > -{
> > > -    return tbl[c];
> > > -}
> > > -#define get_nargs(t, c) _get_nargs(t, array_index_nospec(c, ARRAY_SIZE(t)))
> > > -#else
> > > -#define get_nargs(tbl, c) 0
> > > -#endif
> > > -
> > >    static inline void clobber_regs(struct cpu_user_regs *regs,
> > >                                    unsigned int nargs)
> > >    {
> > > @@ -69,6 +59,11 @@ static inline void clobber_regs(struct c
> > >    #endif
> > >    }
> > >    
> > > +#define clobber_regs(r, n, t) ({ \
> > > +    static const unsigned char t[] = hypercall_args_ ## t; \
> > > +    clobber_regs(r, array_access_nospec(t, n)); \
> > > +})
> > > +
> > ... could I talk you into not overloading the names of the inline
> > functions with macros? You are changing all the call sites anyway.
> I do, but the call sites should use the present name, so if anything
> it would be the inline function that gets renamed. Yet I'm
> deliberately using the same name, to kind of "hide" the actual
> function (like we do elsewhere in a few cases). The effect you're
> after would be implicitly had by going the route described in the
> post-commit-message remark. I'd be happy to switch to that model, if
> that suites you better.
Would be fine with me.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature