| Message ID | cc65eb5f-310e-b51b-71c6-17b93cbe7d0d@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: rework hypercall argument count table instantiation & use | expand |
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@suse.com> Reviewed-by: Juergen Gross <jgross@suse.com> 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. Juergen
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@suse.com> > > Reviewed-by: Juergen Gross <jgross@suse.com> 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
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@suse.com> >> >> Reviewed-by: Juergen Gross <jgross@suse.com> > > 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
--- a/xen/arch/x86/hvm/hypercall.c +++ b/xen/arch/x86/hvm/hypercall.c @@ -111,11 +111,6 @@ long hvm_physdev_op(int cmd, XEN_GUEST_H return compat_physdev_op(cmd, arg); } -#ifndef NDEBUG -static const unsigned char hypercall_args_64[] = hypercall_args_hvm64; -static const unsigned char hypercall_args_32[] = hypercall_args_hvm32; -#endif - int hvm_hypercall(struct cpu_user_regs *regs) { struct vcpu *curr = current; @@ -177,7 +172,7 @@ int hvm_hypercall(struct cpu_user_regs * regs->r10, regs->r8); if ( !curr->hcall_preempted && regs->rax != -ENOSYS ) - clobber_regs(regs, get_nargs(hypercall_args_64, eax)); + clobber_regs(regs, eax, hvm64); } else { @@ -190,7 +185,7 @@ int hvm_hypercall(struct cpu_user_regs * curr->hcall_compat = false; if ( !curr->hcall_preempted && regs->eax != -ENOSYS ) - clobber_regs32(regs, get_nargs(hypercall_args_32, eax)); + clobber_regs32(regs, eax, hvm32); } hvmemul_cache_restore(curr, token); --- 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)); \ +}) + static inline void clobber_regs32(struct cpu_user_regs *regs, unsigned int nargs) { @@ -85,4 +80,9 @@ static inline void clobber_regs32(struct #endif } +#define clobber_regs32(r, n, t) ({ \ + static const unsigned char t[] = hypercall_args_ ## t; \ + clobber_regs32(r, array_access_nospec(t, n)); \ +}) + #endif /* __ASM_X86_HYPERCALL_H__ */ --- a/xen/arch/x86/pv/hypercall.c +++ b/xen/arch/x86/pv/hypercall.c @@ -27,13 +27,6 @@ #include <asm/multicall.h> #include <irq_vectors.h> -#ifndef NDEBUG -static const unsigned char hypercall_args_64[] = hypercall_args_pv64; -#ifdef CONFIG_PV32 -static const unsigned char hypercall_args_32[] = hypercall_args_pv32; -#endif -#endif - /* Forced inline to cause 'compat' to be evaluated at compile time. */ static void always_inline _pv_hypercall(struct cpu_user_regs *regs, bool compat) @@ -65,7 +58,7 @@ _pv_hypercall(struct cpu_user_regs *regs call_handlers_pv64(eax, regs->rax, rdi, rsi, rdx, r10, r8); if ( !curr->hcall_preempted && regs->rax != -ENOSYS ) - clobber_regs(regs, get_nargs(hypercall_args_64, eax)); + clobber_regs(regs, eax, pv64); } #ifdef CONFIG_PV32 else @@ -90,7 +83,7 @@ _pv_hypercall(struct cpu_user_regs *regs curr->hcall_compat = false; if ( !curr->hcall_preempted && regs->eax != -ENOSYS ) - clobber_regs32(regs, get_nargs(hypercall_args_32, eax)); + clobber_regs32(regs, eax, pv32); } #endif /* CONFIG_PV32 */
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@suse.com> --- A tentative variant with yet less redundancy (only a single macro needed) would be #define clobber_regs(r, n, t, b) ({ \ static const unsigned char t ## b[] = hypercall_args_ ## t ## b; \ clobber_regs ## b(r, array_access_nospec(t ## b, n)); \ }) with invocations then being e.g. clobber_regs(regs, eax, hvm, 64); and with the clobber_regs() inline function renamed to clobber_regs64().