Re: [stos] Re: CVS commit: src/sys/arch
Le 28/05/2020 à 23:58, Andrew Doran a écrit : 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? The functions that begin by __msan_* are called from the compiler-generated instrumentation directly, not from the kernel (even though they could). Adding calls to this function from asm would be a big hack that I don't want in the kernel, and it wouldn't be addressing the real problem, which is, that the introduction of x86_movs/x86_stos is unnecessary in the first place, and the way they are used right now is just wrong -- memcpy/memset should have stayed in place. The whole change you made is for svs_pdir_switch() to use quads, but why not use atomic_load_relaxed? With that it should fetch quads without taking the bus lock, and we don't have to resort to ugly ASM functions. Thanks, Maxime
Re: [stos] Re: CVS commit: src/sys/arch
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
Re: [stos] Re: CVS commit: src/sys/arch
Le 28/05/2020 à 19:06, Maxime Villard a écrit : 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. Thanks, Maxime "MI" -> "MD"
Re: [stos] Re: CVS commit: src/sys/arch
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. Thanks, Maxime
[stos] Re: CVS commit: src/sys/arch
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
