Re: [PATCH v3 3/7] x86: re-work memcpy()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/11/2024 2:28 pm, Jan Beulich wrote:
> 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.
>
> 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.

My feedback for patch 2 is largely applicable here too.

>
> TBD: We may further need a workaround similar to Linux'es 8ca97812c3c8
>      ("x86/mce: Work around an erratum on fast string copy
>      instructions").

Ah, so you found that erratum.  I'd say there's lower hanging fruit to
go after in the MCE logic before we get to this.

> ---
> v3: Re-base.
>
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_INDIRECT_THUNK) += indirect
>  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_HVM) += monitor.o
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -153,12 +153,14 @@ void init_or_livepatch add_nops(void *in
>   * 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.

Did you find a problem in practice, or is this just in case?

I suspect if you are seeing problems, then it's non-atomicity of the
stores into memcpy() rather than serialisation.

>   */
>  static void init_or_livepatch noinline
>  text_poke(void *addr, const void *opcode, size_t len)
>  {
>      memcpy(addr, opcode, len);
> +    cpuid_eax(0);

This whole function is buggy in a couple of ways, starting with the
comments.

The comment about noinline and control flow changes is only really
relevant to 32bit processors; we inherited that comment from Linux, and
they're not applicable to Xen.

AMD64 (both the APM, and SDM) guarantee that Self Modifying Code will be
dealt with on your behalf, with no serialisation needed.

Cross-modifying code needs far more severe serialisation than given
here.  We get away with it because alternative_{instructions,branches}()
are pre-SMP, and apply_alternatives() is on livepatches prior to them
becoming live.


I happen to know there's an AMD CPU which has an erratum regarding Self
Modifying Code and genuinely does need a serialising instruction, but I
don't know which exact CPU it is.

If we're going to put a serialising instruction, it should be a write to
CR2.  We don't care about 486 compatibility, and it's faster than CPUID
and much much faster if virtualised because it's unlikely to be
intercepted even under shadow paging.

But, it would be nice not to put serialisation in the general case to
begin with, especially not into the livepatching case.

~Andrew