[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] x86/string: correct memmove()'s forwarding to memcpy()

  • To: Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Wed, 3 Feb 2021 15:01:36 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=tc0U3sVt46NDiGTDiofE6JPIa/13SAjy4T3EAcDPUBE=; b=DaN/EOSFK8iuyaXuR9nj2i/TIXEHcq/gyESTGZ7Qr4E6BIlg1e5PBgn89isiIb3A7vCGErV8cR0PZzsp3PLX3NyXjxdUpom6P5TgdykkpTPsiyjcR4ItnTUfLSdkYDol6QzPvQDsZwy8yPBubeHzP0gf1OXSCUssclJ533lL3LUQULOa9ZqODiqHUHCg2pTsvkaiXr6X7dq7BVk2xwT/7m9q5tpom61gPE9ltTNMOWMFJUmWSoWYymvdk9PqjnPxJHZJuLKTieJyx6wuyBYUjqlPT9TbbHAARtGpFWDbvV/mVtm9sqOJFrsd3WRWQWlmn1oesX+0jRcFAdcbuPDwdw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=OBZpWfOkLXg9vmVLwY0p7M3OTMYuh8YKGxjGqrNg/NQY6VWv8lt532lImkuzgbodzNWesnjQM9TxsH2yLEDQaIArT8DFVANX++fhKcWyu2zPUhuuf8wT+IcuwfTF6KlWYiNYvT2BJ3Ak+ErPeFHkKrBAfqTjMDfyMUCgWEQ0dU3JDKL+qM/SUsyFQol1VusBaplp0FikxddP4luedAMsXnzyMmwK7VamM/eWylxLvcRjR9KEsjFAI/mnA2ZVE2pKrPnSh+Q/1nrroU8RB4DzHXCrF5F1PN5XG89OINv0ux89DdG4XjdAsHx9RRj7KreHvQLJoRSdk5QkI+FTCDUuBQ==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Wed, 03 Feb 2021 15:01:55 +0000
  • Ironport-sdr: dAGaf8Fk3JudgqKb7CzgBrhS1RYOSO3q2mUJZFW/mop0jdXsp0RTp81OK9riLQVXWwTZQ6shCJ V39kcAT/gGCPGg9BP1kTnBqkRKsoMmH6FDy4qTRdPuYormrbQ1UfoMVbN9Zom1ce+baMrQllCR 1iBerKVD6RqXedCZ9ke1k+MJi5Y1lZGZWXO6hB351Qb/tD2NsuvTXXKFp4ooWw942MHaP7E+kH zrPkrRiwwd686ASXJqo8oCZivPT5+/NSmgRLJDMa5cpsOQkGqKUSKhYo2pPN0CPx8Qxu1XPqkO tBo=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 03/02/2021 14:22, Jan Beulich wrote:
> With memcpy() expanding to the compiler builtin, we may not hand it
> overlapping source and destination. We strictly mean to forward to our
> own implementation (a few lines up in the same source file).
> Fixes: 78825e1c60fa ("x86/string: Clean up x86/string.h")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

I agree that the current logic is buggy, but I'm not sure this is an

You've switched from relying on GCC's builtin to operate forwards, to
relying on Xen's implementation operating forwards.

At the very least, can we get a code comment stating something like
"depends on Xen's implementation operating forwards" ?

> ---
> An alternative would be to "#undef memcpy" near the top of the file. But
> I think the way it's done now is more explicit to the reader. An #undef
> would be the only way if the macro was an object-like one.

I chose not to use undef's to avoid impacting the optimisation of other
functions in this file.  I can't remember if this made a difference in

> At least with gcc10 this does alter generated code: The builtin gets
> expanded into a tail call, while after this change our memcpy() gets
> inlined into memmove(). This would change again once we separate the 3
> functions here into their own CUs for placing them in an archive.

As (perhaps) a tangent, how do we plan to provide x86-optimised versions
in combination with the library work?  We're long overdue needing to
refresh our fast-strings support to include fast rep-mov/stosb.


> --- a/xen/arch/x86/string.c
> +++ b/xen/arch/x86/string.c
> @@ -43,7 +43,7 @@ void *(memmove)(void *dest, const void *
>          return dest;
>      if ( dest < src )
> -        return memcpy(dest, src, n);
> +        return (memcpy)(dest, src, n);
>      asm volatile (
>          "   std         ; "



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.