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

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

Posted by Jan Beulich 1 year, 9 months ago

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

--- 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 */

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

Posted by Juergen Gross 1 year, 8 months ago

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

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

Posted by Jan Beulich 1 year, 8 months ago

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

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

Posted by Juergen Gross 1 year, 8 months ago

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