Re: [PATCH v5 2/6] x86: re-work memset()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.06.2025 18:59, Teddy Astie wrote:
> Le 05/06/2025 à 12:26, Jan Beulich a écrit :
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -346,6 +346,12 @@ static int init_or_livepatch _apply_alte
>>           /* 0xe8/0xe9 are relative branches; fix the offset. */
>>           if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
>>               *(int32_t *)(buf + 1) += repl - orig;
>> +        else if ( IS_ENABLED(CONFIG_RETURN_THUNK) &&
>> +                  a->repl_len > 5 && buf[a->repl_len - 5] == 0xe9 &&
>> +                  ((long)repl + a->repl_len +
>> +                   *(int32_t *)(buf + a->repl_len - 4) ==
>> +                   (long)__x86_return_thunk) )
>> +            *(int32_t *)(buf + a->repl_len - 4) += repl - orig;
> 
> That looks a bit confusing, to me that probably some comment explaining 
> what instructions transform it's looking to make.

It's still the same comment ahead of the if() that applies here. This will all
become easier with Andrew's decode-light, at which point we'll be able to spot
CALL/JMP anywhere  in a blob.

>> --- /dev/null
>> +++ b/xen/arch/x86/memset.S
>> @@ -0,0 +1,30 @@
>> +#include <asm/asm_defns.h>
>> +
> 
> It would be nice to have a reminder of the calling convention (i.e what 
> register maps to what memset parameter) as it will definitely help 
> future readers.
> 
> If I understand correctly here :
> - rdi: destination (s)
> - rsi: byte to write (c)
> - rdx: number of bytes to write (n)

I don't think the (default) ABI needs re-stating for every function in every
assembly file.

>> +.macro memset
>> +        and     $7, %edx
>> +        shr     $3, %rcx
>> +        movzbl  %sil, %esi
>> +        mov     $0x0101010101010101, %rax
>> +        imul    %rsi, %rax
>> +        mov     %rdi, %r8
>> +        rep stosq
>> +        or      %edx, %ecx
>> +        jz      0f
>> +        rep stosb
>> +0:
>> +        mov     %r8, %rax
>> +        RET
>> +.endm
>> +
>> +.macro memset_erms
>> +        mov     %esi, %eax
>> +        mov     %rdi, %r8
>> +        rep stosb
>> +        mov     %r8, %rax
>> +        RET
>> +.endm
>> +
> 
> Overall, I am a bit confused on the mixing of 32-bits (edx, esi, ...) 
> and 64-bits registers (rax, ...). But it looks ok to me.

Since 64-bit forms require REX, 32-bit ones are used wherever that's sufficient
for the purpose.

Jan