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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan