linux-kernel - Re: lib/iomap.c mmio_{in,out}s* vs. __raw

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:	Sun, 05 Nov 2006 14:55:53 +1100
From:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:	Linus Torvalds <torvalds@...l.org>
Cc:	Russell King <rmk+lkml@....linux.org.uk>,
	Linux Kernel list <linux-kernel@...r.kernel.org>,
	Jeff Garzik <jgarzik@...ox.com>, Andrew Morton <akpm@...l.org>,
	"David S. Miller" <davem@...emloft.net>,
	Paul Mackerras <paulus@...ba.org>
Subject: Re: lib/iomap.c mmio_{in,out}s* vs. __raw_* accessors

On Sat, 2006-11-04 at 19:46 -0800, Linus Torvalds wrote:
> 
> On Sun, 5 Nov 2006, Benjamin Herrenschmidt wrote:
> >
> > Make the generic lib/iomap.c use arch provided MMIO accessors when
> > available for big endian and repeat operations. Also while at it,
> > fix the *_be version which are currently broken for PIO
> 
> Just rip the _be versions out, methinks.
> 
> Also, what does your "writesb()" actually look like? I assume it's the 
> exact same thing as the generic one, with just another barrier. No? 

With our old naming (we used _ins* for the base operations, the PIO ones
just calling them with a magic offset) :

void _insb(const volatile u8 __iomem *port, void *buf, long count)
{
        u8 *tbuf = buf;
        u8 tmp;

        if (unlikely(count <= 0))
                return;
        asm volatile("sync");
        do {
                tmp = *port;
                asm volatile("eieio");
                *tbuf++ = tmp;
        } while (--count != 0);
        asm volatile("twi 0,%0,0; isync" : : "r" (tmp));
}
EXPORT_SYMBOL(_insb);

void _outsb(volatile u8 __iomem *port, const void *buf, long count)
{
        const u8 *tbuf = buf;

        if (unlikely(count <= 0))
                return;
        asm volatile("sync");
        do {
                *port = *tbuf++;
        } while (--count != 0);
        asm volatile("sync");
}
EXPORT_SYMBOL(_outsb);

I'm not even entirely sure that the insb one is correct, maybe it should
or-in all the values to get a proper data dependency that acutally
depends on all previous loads to pass to the twi instructions (it's a
conditional trap instruction with the condition set as never, it forces
the CPU to think the data has been consumed and thus, in conjunction
with the subsequent isync, forces the loads to be performed before any
further code can execute).

The store also has two barriers as you can see. That's mostly related to
the need for very synchronous IOs to please drivers, though the second
one could be replaced with

	get_paca()->io_sync = 1;	

(Which is a new mecanism we introduced recently to tell spin_unlock that
one or previous MMIO stores might need a sync before we unlock. Looks
like we didn't update the "s" functions when adding that).

Ben.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/