Re: [PATCH v5 3/6] x86: re-work memcpy()

["Teddy Astie" <teddy.astie@xxxxxxxxxx>]

Le 05/06/2025 à 12:27, Jan Beulich a écrit :
> Move the function to its own assembly file. Having it in C just for the
> entire body to be an asm() isn't really helpful. Then have two flavors:
> A "basic" version using qword steps for the bulk of the operation, and an
> ERMS version for modern hardware, to be substituted in via alternatives
> patching.
>
> Alternatives patching, however, requires an extra precaution: It uses
> memcpy() itself, and hence the function may patch itself. Luckily the
> patched-in code only replaces the prolog of the original function. Make
> sure this remains this way.
>

We can probably workaround that by using a separate memcpy for
alternatives patching. So it wouldn't end up patching itself.

> Additionally alternatives patching, while supposedly safe via enforcing
> a control flow change when modifying already prefetched code, may not
> really be. Afaict a request is pending to drop the first of the two
> options in the SDM's "Handling Self- and Cross-Modifying Code" section.
> Insert a serializing instruction there.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> We may want to consider branching over the REP MOVSQ as well, if the
> number of qwords turns out to be zero.
> We may also want to consider using non-REP MOVS{L,W,B} for the tail.
>
> TBD: We may further need a workaround similar to Linux'es 8ca97812c3c8
>       ("x86/mce: Work around an erratum on fast string copy
>       instructions").
>
> TBD: Some older AMD CPUs have an issue with REP MOVS when source and
>       destination are misaligned with one another (modulo 32?), which may
>       require a separate memcpy() flavor.
> ---
> v5: Re-base.
> v4: Use CR2 write as serializing insn, and limit its use to boot time.
> v3: Re-base.
>
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_RETURN_THUNK) += indirect-t
>   obj-$(CONFIG_PV) += ioport_emulate.o
>   obj-y += irq.o
>   obj-$(CONFIG_KEXEC) += machine_kexec.o
> +obj-y += memcpy.o
>   obj-y += memset.o
>   obj-y += mm.o x86_64/mm.o
>   obj-$(CONFIG_VM_EVENT) += monitor.o
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -195,12 +195,16 @@ void *place_ret(void *ptr)
>    * executing.
>    *
>    * "noinline" to cause control flow change and thus invalidate I$ and
> - * cause refetch after modification.
> + * cause refetch after modification.  While the SDM continues to suggest this
> + * is sufficient, it may not be - issue a serializing insn afterwards as 
> well,
> + * unless this is for live-patching.
>    */
>   static void init_or_livepatch noinline
>   text_poke(void *addr, const void *opcode, size_t len)
>   {
>       memcpy(addr, opcode, len);
> +    if ( system_state < SYS_STATE_active )
> +        asm volatile ( "mov %%rax, %%cr2" ::: "memory" );
>   }
>
>   extern void *const __initdata_cf_clobber_start[];
> --- /dev/null
> +++ b/xen/arch/x86/memcpy.S
> @@ -0,0 +1,20 @@
> +#include <asm/asm_defns.h>
> +
> +FUNC(memcpy)
> +        mov     %rdx, %rcx
> +        mov     %rdi, %rax
> +        /*
> +         * We need to be careful here: memcpy() is involved in alternatives
> +         * patching, so the code doing the actual copying (i.e. past setting
> +         * up registers) may not be subject to patching (unless further
> +         * precautions were taken).
> +         */
> +        ALTERNATIVE "and $7, %edx; shr $3, %rcx", \
> +                    STR(rep movsb; RET), X86_FEATURE_ERMS
> +        rep movsq
> +        or      %edx, %ecx
> +        jz      1f
> +        rep movsb
> +1:
> +        RET
> +END(memcpy)
> --- a/xen/arch/x86/string.c
> +++ b/xen/arch/x86/string.c
> @@ -7,21 +7,6 @@
>
>   #include <xen/lib.h>
>
> -void *(memcpy)(void *dest, const void *src, size_t n)
> -{
> -    long d0, d1, d2;
> -
> -    asm volatile (
> -        "   rep ; movs"__OS" ; "
> -        "   mov %k4,%k3      ; "
> -        "   rep ; movsb        "
> -        : "=&c" (d0), "=&D" (d1), "=&S" (d2)
> -        : "0" (n/BYTES_PER_LONG), "r" (n%BYTES_PER_LONG), "1" (dest), "2" 
> (src)
> -        : "memory" );
> -
> -    return dest;
> -}
> -
>   void *(memmove)(void *dest, const void *src, size_t n)
>   {
>       long d0, d1, d2;
>
>

Aside that:
Reviewed-by: Teddy Astie <teddy.astie@xxxxxxxxxx>

Teddy


Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech