Re: [PATCH 2/3] x86/alternatives: Rework get_ideal_nops()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/06/2025 10:57 am, Jan Beulich wrote:
> On 22.05.2025 17:00, Andrew Cooper wrote:
>> The implemenation of get_ideal_nops() changes from:
>>
>>     mov    0x19bc41(%rip),%rax        # <ideal_nops>
>>     mov    %edi,%edi
>>     mov    (%rax,%rdi,8),%rax
>>     jmp    <__x86_return_thunk>
>>
>> to:
>>
>>     lea    -0x1(%rdi),%eax
>>     imul   %edi,%eax
>>     shr    %eax
>>     add    0x67fc1(%rip),%rax        # <ideal_nops>
>>     jmp    <__x86_return_thunk>
>>
>> The imul has a latency of 3 cycles on all CPUs back to the K8 and Nehalem.
>> It's better than an extra deference on all CPUs, even the older ones.
> While this is all good, what we're losing is ...
>
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -20,7 +20,7 @@
>>  #define MAX_PATCH_LEN (255-1)
>>  
>>  #ifdef K8_NOP1
>> -static const unsigned char k8nops[] init_or_livepatch_const = {
>> +static const unsigned char k8_nops[] init_or_livepatch_const = {
>>      K8_NOP1,
>>      K8_NOP2,
>>      K8_NOP3,
>> @@ -31,22 +31,10 @@ static const unsigned char k8nops[] 
>> init_or_livepatch_const = {
>>      K8_NOP8,
>>      K8_NOP9,
>>  };
>> -static const unsigned char * const k8_nops[ASM_NOP_MAX+1] 
>> init_or_livepatch_constrel = {
> ... the (at least visual) connection to ASM_NOP_MAX. Could I talk you into
> adding build time array-size checks for both arrays, to restore the
> connection?

Sorry, but I have no idea what you're asking for here.

The use of ASM_NOP_MAX was latently buggy before; it was easy to create
a NULL deference if the initialiser wasn't filled in when ASM_NOP_MAX
changed.

~Andrew