Maxime,
I read your e-mail carefully and conclude that the best way forward here is
put this one to core@ for a technical decision.
Cheers,
Andrew
On Wed, Jun 03, 2020 at 08:25:32AM +0200, Maxime Villard wrote:
> Le 03/06/2020 ? 01:49, Andrew Doran a ?crit?:
> > On Tue, Jun 02, 2020 at 08:41:53AM +0200, Maxime Villard wrote:
> >
> > > Le 02/06/2020 ? 00:58, Andrew Doran a ?crit?:
> > > > Module Name:src
> > > > Committed By: ad
> > > > Date: Mon Jun 1 22:58:06 UTC 2020
> > > >
> > > > Modified Files:
> > > > src/sys/arch/amd64/amd64: cpufunc.S
> > > > src/sys/arch/amd64/include: frameasm.h
> > > >
> > > > Log Message:
> > > > Reported-by: syzbot+6dd5a230d19f0cbc7...@syzkaller.appspotmail.com
> > > >
> > > > Instrument STOS/MOVS for KMSAN to unbreak it.
> > > >
> > > >
> > > > To generate a diff of this commit:
> > > > cvs rdiff -u -r1.58 -r1.59 src/sys/arch/amd64/amd64/cpufunc.S
> > > > cvs rdiff -u -r1.49 -r1.50 src/sys/arch/amd64/include/frameasm.h
> > > >
> > > > Please note that diffs are not public domain; they are subject to the
> > > > copyright notices on the relevant files.
> > >
> > > Can you just stop ignoring the remarks that are made?
> >
> > That's up to you Maxime. If you habitually make it difficult for people to
> > come to a reasonable compromise with you, then you're asking to not be taken
> > seriously and will find yourself being ignored.
>
> You are confused. I asked for KMSAN to be unbroken, and proposed an
> alternative,
> which is atomic_load_relaxed. You were free to explain why my point was a bad
> idea or why it didn't matter, but you refused, and just added a hack on top of
> another. I'm afraid that's up to you.
>
> But whatever, it doesn't matter. Thanks for reverting the pmap.c changes, it
> is more correct now than before.
>
> > > I said explicitly
> > > that adding manual instrumentation here is _wrong_.
> >
> > I don't share your assessment in the general sense. It should be possible
> > to instrument assembly code because KMSAN is useful AND we can't get by
> > without assembly - there are some things that C just can't do (or do well
> > enough).
> >
> > On the assembly thing recall that recently you expressed a desire to remove
> > all of the amd64 assembly string functions from libc because of sanitizers -
> > I invested my time to do up a little demo to try and show you why that's not
> > a good idea:
> >
> > http://mail-index.netbsd.org/port-amd64/2020/04/19/msg003226.html
>
> I saw, yes. I answered explaining that a conversation with Ryo Shimizu had
> changed my mind a bit, and seeing your results (which as far as I can tell
> do not indicate a performance improvement significant enough to not be
> considered as noise), I asked you whether it was that relevant. You didn't
> follow up though.
>
> > The system is a balancing act. There are lots of factors to be taken into
> > account: maintainability, tooling like KMSAN, user's varying needs, the
> > capabilites of different machines, performance, feature set and so on, and
> > dare I say it even the enjoyment of the people working on the project is
> > important too. Myopic focus on one factor only to the detriment of others
> > is no good.
>
> I am well aware of that.
>
> > > The kMSan ASM fixups
> > > are limited to args/returns, and that is part of a sensical policy that
> > > _should not_ be changed without a good reason.
> > >
> > > x86_movs/x86_stos have strictly _no reason_ to exist. Of the 18
> > > conversions
> > > you made to them in pmap.c, not one is justified. memcpy/memset were all
> > > correct.
> >
> > I introduced these functions as a compromise because you were unhappy with
> > use of memcpy() to copy PDEs. See:
> >
> > http://mail-index.netbsd.org/port-amd64/2020/05/23/msg003280.html
> >
> > You wrote:
> >
> > In the [XXX] window, the PTEs could be used by userland. If you
> > copied them using memcpy(), some parts of the bytes could contain
> > stale values.
>
> Sure, I was explicitly referring to SVS, which has an unusual way of
> accessing PTEs (contrary to pmap), which is why it needs special atomic
> care that other places do not.
>
> > Live PDEs are also copied in pmap.c so I made a change there too. After
> > that I decided "why not" and used the new functions everywhere PDEs/PTEs or
> > pages are block zeroed / copied. But I'm also happy with memcpy()/memset().
> > Either way will work. In fairness I do work too fast sometimes.
> >
> > > The only reason you made these big unneeded changes is for SVS not to take
> > > the bus lock,
> >
> > There is no bus lock on x86 (not since the 1990s anyway).
> >
> > > but as was said already, atomic_load_relaxed will do what
> > > you want without the need for these functions.
> > >
> > > Please revert _both changes now_, this one and the previous one which
> > > introduced both functions, and let's use atomic_load_relaxed.
> >
> > You're focusing on only o
