linux-kernel - Re: [PATCH 01/20] asm-generic/mmiowb: Add generic implementation of mmiowb() tracking

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:   Tue, 05 Mar 2019 10:21:42 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     linux-arch@...r.kernel.org, Michael Ellerman <mpe@...erman.id.au>,
        Will Deacon <will.deacon@....com>
Cc:     Andrea Parri <andrea.parri@...rulasolutions.com>,
        Arnd Bergmann <arnd@...db.de>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Rich Felker <dalias@...c.org>,
        David Howells <dhowells@...hat.com>,
        Daniel Lustig <dlustig@...dia.com>,
        linux-kernel@...r.kernel.org,
        "Maciej W. Rozycki" <macro@...ux-mips.org>,
        Ingo Molnar <mingo@...nel.org>,
        Palmer Dabbelt <palmer@...ive.com>,
        Paul Burton <paul.burton@...s.com>,
        "Paul E. McKenney" <paulmck@...ux.ibm.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Tony Luck <tony.luck@...el.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Yoshinori Sato <ysato@...rs.sourceforge.jp>
Subject: Re: [PATCH 01/20] asm-generic/mmiowb: Add generic implementation of
 mmiowb() tracking

Michael Ellerman's on March 4, 2019 11:01 am:
> Nicholas Piggin <npiggin@...il.com> writes:
>> Michael Ellerman's on March 3, 2019 7:26 pm:
>>> Nicholas Piggin <npiggin@...il.com> writes:
> ...
>>>> what was broken about the powerpc one, which is basically:
>>>>
>>>> static inline void mmiowb_set_pending(void)
>>>> {
>>>> 	struct mmiowb_state *ms = __mmiowb_state();
>>>> 	ms->mmiowb_pending = 1;
>>>> }
>>>>
>>>> static inline void mmiowb_spin_lock(void)
>>>> {
>>>> }
>>> 
>>> The current powerpc code clears io_sync in spin_lock().
>>> 
>>> ie, it would be equivalent to:
>>> 
>>> static inline void mmiowb_spin_lock(void)
>>> {
>>>  	ms->mmiowb_pending = 0;
>>> }
>>
>> Ah okay that's what I missed. How about we just not do that?
> 
> Yeah I thought of that too but it's not great. We'd start semi-randomly
> executing the sync in unlock depending on whether someone had done IO on
> that CPU prior to the spinlock.
> 
> eg.
> 
> 	writel(x, y);		// sets paca->io_sync
> 	...	
> 
> 	<schedule>
> 
> 	spin_lock(a);
>         ...
>         // No IO in here
>         ...
>         spin_unlock(a);		// sync() here because other task did writel().
> 
> 
> Which wouldn't be *incorrect*, but would be kind of weird.

schedule is probably okay, we could clear pending there. But you
possibly could get interrupts, or some lock free mmios that set the
flag. Does it matter that much? A random cache miss could have the
same effect.

It may matter slightly less for powerpc because we don't inline
spin locks, although I have been hoping to for a while, this might
put the nail in that.

We can always tinker with it later though so I won't insist.

Thanks,
Nick