Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Vineet Gupta <vineet.gupta1@synopsys.com>
To:     Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>,
        "linux-snps-arc@lists.infradead.org" 
        <linux-snps-arc@lists.infradead.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Alexey Brodkin" <alexey.brodkin@synopsys.com>
Subject: Re: [PATCH 1/2] ARCv2: LIB: memeset: fix doing prefetchw outside of
 buffer
Thread-Topic: [PATCH 1/2] ARCv2: LIB: memeset: fix doing prefetchw outside
 of buffer
Thread-Index: AQHUrBw2ZJte6r90DkSex+vHyLGaMw==
Date:   Tue, 15 Jan 2019 01:09:13 +0000
Message-ID: <C2D7FE5348E1B147BCA15975FBA2307501464272C9@US01WEMBX2.internal.synopsys.com>
References: <20190114151649.32726-1-Eugeniy.Paltsev@synopsys.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 1/14/19 7:17 AM, Eugeniy Paltsev wrote:=0A=
> Current ARCv2 memeset implementation may call 'prefetchw'=0A=
> instruction for address which lies outside of memset area.=0A=
> So we got one modified (dirty) cache line outside of memset=0A=
> area. This may lead to data corruption if this area is used=0A=
> for DMA IO.=0A=
>=0A=
> Another issue is that current ARCv2 memeset implementation=0A=
> may call 'prealloc' instruction for L1 cache line which=0A=
> doesn't fully belongs to memeset area in case of 128B L1 D$=0A=
> line length. That leads to data corruption.=0A=
>=0A=
> Fix prefetchw/prealloc instructions using in case of 64B L1 data=0A=
> cache line (default case) and don't use prefetch* instructions=0A=
> for other possible L1 data cache line lengths (32B and 128B).=0A=
>=0A=
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>=0A=
> ---=0A=
>  arch/arc/lib/memset-archs.S | 30 +++++++++++++++++++++++-------=0A=
>  1 file changed, 23 insertions(+), 7 deletions(-)=0A=
>=0A=
> diff --git a/arch/arc/lib/memset-archs.S b/arch/arc/lib/memset-archs.S=0A=
> index 62ad4bcb841a..c7717832336f 100644=0A=
> --- a/arch/arc/lib/memset-archs.S=0A=
> +++ b/arch/arc/lib/memset-archs.S=0A=
> @@ -7,11 +7,32 @@=0A=
>   */=0A=
>  =0A=
>  #include <linux/linkage.h>=0A=
> +#include <asm/cache.h>=0A=
>  =0A=
>  #undef PREALLOC_NOT_AVAIL=0A=
>  =0A=
> +/*=0A=
> + * The memset implementation below is optimized to use prefetchw and pre=
alloc=0A=
> + * instruction in case of CPU with 64B L1 data cache line (L1_CACHE_SHIF=
T =3D=3D 6)=0A=
> + * If you want to implement optimized memset for other possible L1 data =
cache=0A=
> + * line lengths (32B and 128B) you should rewrite code carefully checkin=
g=0A=
> + * we don't call any prefetchw/prealloc instruction for L1 cache lines w=
hich=0A=
> + * don't belongs to memset area.=0A=
=0A=
Good point. FWIW, it is possible to support those non common line lengths b=
y using=0A=
L1_CACHE_SHIFT etc in asm code below but I agree its not worth the trouble.=
=0A=
=0A=
> + */=0A=
> +#if L1_CACHE_SHIFT!=3D6=0A=
> +# define PREALLOC_INSTR(...)=0A=
> +# define PREFETCHW_INSTR(...)=0A=
> +#else  /* L1_CACHE_SHIFT!=3D6 */=0A=
> +# define PREFETCHW_INSTR(...)	prefetchw __VA_ARGS__=0A=
> +# ifdef PREALLOC_NOT_AVAIL=0A=
> +#  define PREALLOC_INSTR(...)	prefetchw __VA_ARGS__=0A=
> +# else=0A=
> +#  define PREALLOC_INSTR(...)	prealloc __VA_ARGS__=0A=
> +# endif=0A=
> +#endif /* L1_CACHE_SHIFT!=3D6 */=0A=
> +=0A=
>  ENTRY_CFI(memset)=0A=
> -	prefetchw [r0]		; Prefetch the write location=0A=
> +	PREFETCHW_INSTR([r0])	; Prefetch the first write location=0A=
>  	mov.f	0, r2=0A=
>  ;;; if size is zero=0A=
>  	jz.d	[blink]=0A=
> @@ -48,11 +69,7 @@ ENTRY_CFI(memset)=0A=
>  =0A=
>  	lpnz	@.Lset64bytes=0A=
>  	;; LOOP START=0A=
> -#ifdef PREALLOC_NOT_AVAIL=0A=
> -	prefetchw [r3, 64]	;Prefetch the next write location=0A=
> -#else=0A=
> -	prealloc  [r3, 64]=0A=
> -#endif=0A=
> +	PREALLOC_INSTR([r3, 64]) ;Prefetch the next write location=0A=
=0A=
These are not solving the issue - I'd break this up and move these bits to =
your=0A=
next patch.=0A=
=0A=
>  #ifdef CONFIG_ARC_HAS_LL64=0A=
>  	std.ab	r4, [r3, 8]=0A=
>  	std.ab	r4, [r3, 8]=0A=
> @@ -85,7 +102,6 @@ ENTRY_CFI(memset)=0A=
>  	lsr.f	lp_count, r2, 5 ;Last remaining  max 124 bytes=0A=
>  	lpnz	.Lset32bytes=0A=
>  	;; LOOP START=0A=
> -	prefetchw   [r3, 32]	;Prefetch the next write location=0A=
=0A=
So the real fix for issue at hand is this line. I'll keep this for the fix =
and=0A=
beef up the changelog. Thing is existing code was already skipping the last=
 64B=0A=
from the main loop (thus avoided prefetching the next line), but then reint=
roduced=0A=
prefetchw is last 32B loop, spoiling the party.  That prefetchw was pointle=
ss anyways=0A=
=0A=
-Vineet=0A=