linux-kernel - Re: [PATCH v2 31/32] sh: support a 2-byte smp_store

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 8 Jan 2016 09:23:09 +0200
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Rich Felker <dalias@...c.org>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org, linux-sh@...r.kernel.org,
	Rob Landley <rob@...dley.net>, Jeff Dionne <jeff@...inux.org>,
	Yoshinori Sato <ysato@...rs.sourceforge.jp>
Subject: Re: [PATCH v2 31/32] sh: support a 2-byte smp_store_mb

On Thu, Jan 07, 2016 at 11:25:05PM -0500, Rich Felker wrote:
> On Fri, Jan 08, 2016 at 12:41:35AM +0200, Michael S. Tsirkin wrote:
> > > > > It would be nice to have these in asm-generic for archs which don't
> > > > > define their own versions rather than having cruft like this repeated
> > > > > per-arch. Strictly speaking, the volatile u32 used to access the
> > > > > 32-bit word containing the u8 or u16 should be
> > > > > __attribute__((__may_alias__)) too. Is there an existing kernel type
> > > > > for a "may_alias u32" or should it perhaps be added?
> > > > > 
> > > > > Rich
> > > > 
> > > > BTW this seems suboptimal for grb and irq variants which apparently
> > > > can do things correctly.
> > > 
> > > In principle I agree, but u8/u16 xchg is mostly unused (completely
> > > unused in my builds) and unlikely to matter to performance. Also, the
> > > irq variant is only for the original sh2 which is not even produced
> > > anymore afaik. Our reimplementation of the sh2 ISA, the J2, has a
> > > cas.l instruction that will be used instead because it supports SMP
> > > where interrupt masking is insufficient to achieve atomicity.
> > > 
> > > Rich
> > 
> > Since it looks like there will soon be active maintainers
> > for this arch, I think it's best if I make the minimal possible
> > changes and then you guys can rewrite it any way you like,
> > drop irq variant or whatever.
> > 
> > The minimal change is probably the below code but
> > the grb variant is just copy paste from xchg_u8
> > with a minor tweak -
> > can you pls confirm it looks right?
> 
> I haven't had a chance to test it, but I don't see anything obviously
> wrong with it.
> 
> > I tested the llsc code on ppc and x86 and since it's
> > portable I know the logic is correct there.
> 
> Sounds good. Since it will also be needed for the cas.l variant I'd
> rather have this in the main asm/cmpxchg.h where it can be shared if
> you see an easy way to do that now, but if not I can take care of it
> later when merging cmpxchg-cas.h. Perhaps just putting __xchg_cmpxchg
> in the main asm/cmpxchg.h would suffice so that only the thin wrappers
> need to be duplicated.
> Ideally it could even be moved outside of the
> arch asm headers, but then there might be annoying header ordering
> issues to deal with.

Well it isn't possible to put it in cmpxchg.h because you get into
annoying ordering issues: __cmpxchg_u32 is needed
so it has to come after the headers, but the wrappers must
come before the headers.

I put it in a header by itself. This way it's easy to reuse,
and even the thin wrappers won't have to be duplicated.

> 
> > Will post v3 with this included but would appreciate
> > your input first.
> 
> Go ahead. Thanks!
> 
> Rich