Re: [stos] Re: CVS commit: src/sys/arch

[Andrew Doran <ad%netbsd.org@localhost>]

On Thu, May 28, 2020 at 07:06:04PM +0200, Maxime Villard wrote:
> Le 27/05/2020 ? 21:58, Maxime Villard a ?crit?:
> > Le 27/05/2020 ? 21:33, Andrew Doran a ?crit?:
> > > Module Name:??? src
> > > Committed By:??? ad
> > > Date:??????? Wed May 27 19:33:40 UTC 2020
> > > 
> > > Modified Files:
> > > ????src/sys/arch/amd64/amd64: cpufunc.S locore.S
> > > ????src/sys/arch/i386/i386: cpufunc.S locore.S
> > > ????src/sys/arch/x86/include: pmap.h
> > > ????src/sys/arch/x86/x86: pmap.c
> > > 
> > > Log Message:
> > > - Add a couple of wrapper functions around STOS and MOVS and use them to zero
> > > ?? and copy PTEs in preference to memset()/memcpy().
> > > 
> > > - Remove related SSE / pageidlezero stuff.
> > > 
> > > 
> > > To generate a diff of this commit:
> > > cvs rdiff -u -r1.56 -r1.57 src/sys/arch/amd64/amd64/cpufunc.S
> > > cvs rdiff -u -r1.208 -r1.209 src/sys/arch/amd64/amd64/locore.S
> > > cvs rdiff -u -r1.42 -r1.43 src/sys/arch/i386/i386/cpufunc.S
> > > cvs rdiff -u -r1.184 -r1.185 src/sys/arch/i386/i386/locore.S
> > > cvs rdiff -u -r1.121 -r1.122 src/sys/arch/x86/include/pmap.h
> > > cvs rdiff -u -r1.395 -r1.396 src/sys/arch/x86/x86/pmap.c
> > > 
> > > Please note that diffs are not public domain; they are subject to the
> > > copyright notices on the relevant files.
> > 
> >  ????-END(x86_stos)
> >  ????+END(x86_movs)
> > 
> > The naming convention should also be more explicit I think, because movs
> > does not say if it is b/w/l/q. Don't have anything meaningful to suggest
> > though
> 
> I see that syzbot-kMSan is failing because of this change. Looking at it more
> closely:
> 
>  - Having MI ASM copy functions is unwanted, because the sanitizers won't be
>    able to sanitize the accesses. kMSan misses the initialization and reports
>    false positives. As well kASan will miss memory corruptions and kCSan will
>    miss races.
> 
>  - It appears you replaced memcpy/memset by x86_movs/x86_stos in places where
>    it is unnecessary and unwanted. Eg the majority of the changes in pmap.c
>    are unwanted and should remain memcpy/memset.
>
> Please revert this change promptly in order to unbreak kMSan. We really need
> to have a clear policy on which function should be in ASM, and not introduce
> wild MI things like that which not only are rarely justified but also are a
> big PITA when sanitizers come into play.

Very good.  I see a function in subr_msan.c that looks like it does the
needful here:

	void __msan_instrument_asm_store(void *addr, size_t size)

I don't see any uses in tree.  Is there a reason for that?

Thanks,
Andrew