Date: Sat, 17 Nov 2018 14:01:13 -0800 From: Matthew Macy <mat.macy@gmail.com> To: Tijl Coosemans <tijl@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r339618 - head/sys/compat/linuxkpi/common/include/linux Message-ID: <CAPrugNoMVDyg-CnVh5NUgrxdaHcbo3CibC3RsPQ7FVtdJ=FJdQ@mail.gmail.com> In-Reply-To: <201810222055.w9MKtZPt013627@repo.freebsd.org> References: <201810222055.w9MKtZPt013627@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
You should probably revert this. The implied understanding of the _relaxed version is incorrect. compiler_membar is there to prevent instruction reordering which is possible on FreeBSD because the accesses are done in C. The relaxed variants still do not permit instruction reordering. On Linux __compiler_member (referred to as barrier there) isn=E2=80=99t necessary in= the mmio accessors because they always use volatile asm which can=E2=80=99t be reordered. The distinction between the relaxed and non relaxed variants is that the relaxed variant lacks memory barriers (sync / lwsync / eieio on ppc, membar on sparc, etc). Most of the time we don=E2=80=99t run in to pro= blems on x86 because with TSO the only reordering possible is writes that happen before reads can become visible in memory after they occur in the instruction stream. Thanks. -M On Mon, Oct 22, 2018 at 13:55 Tijl Coosemans <tijl@freebsd.org> wrote: > Author: tijl > Date: Mon Oct 22 20:55:35 2018 > New Revision: 339618 > URL: https://svnweb.freebsd.org/changeset/base/339618 > > Log: > Define linuxkpi readq for 64-bit architectures. It is used by drm-kmod= . > Currently the compiler picks up the definition in machine/cpufunc.h. > > Add compiler memory barriers to read* and write*. The Linux x86 > implementation of these functions uses inline asm with "memory" clobber= . > The Linux x86 implementation of read_relaxed* and write_relaxed* uses t= he > same inline asm without "memory" clobber. > > Implement ioread* and iowrite* in terms of read* and write* so they als= o > have memory barriers. > > Qualify the addr parameter in write* as volatile. > > Like Linux, define macros with the same name as the inline functions. > > Only define 64-bit versions on 64-bit architectures because generally > 32-bit architectures can't do atomic 64-bit loads and stores. > > Regroup the functions a bit and add brief comments explaining what they > do: > - __raw_read*, __raw_write*: atomic, no barriers, no byte swapping > - read_relaxed*, write_relaxed*: atomic, no barriers, little-endian > - read*, write*: atomic, with barriers, little-endian > > Add a comment that says our implementation of ioread* and iowrite* > only handles MMIO and does not support port IO. > > Reviewed by: hselasky > MFC after: 3 days > > Modified: > head/sys/compat/linuxkpi/common/include/linux/io.h > > Modified: head/sys/compat/linuxkpi/common/include/linux/io.h > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- head/sys/compat/linuxkpi/common/include/linux/io.h Mon Oct 22 > 20:22:33 2018 (r339617) > +++ head/sys/compat/linuxkpi/common/include/linux/io.h Mon Oct 22 > 20:55:35 2018 (r339618) > @@ -38,153 +38,309 @@ > #include <linux/compiler.h> > #include <linux/types.h> > > +/* > + * XXX This is all x86 specific. It should be bus space access. > + */ > + > +/* Access MMIO registers atomically without barriers and byte swapping. = */ > + > +static inline uint8_t > +__raw_readb(const volatile void *addr) > +{ > + return (*(const volatile uint8_t *)addr); > +} > +#define __raw_readb(addr) __raw_readb(addr) > + > +static inline void > +__raw_writeb(uint8_t v, volatile void *addr) > +{ > + *(volatile uint8_t *)addr =3D v; > +} > +#define __raw_writeb(v, addr) __raw_writeb(v, addr) > + > +static inline uint16_t > +__raw_readw(const volatile void *addr) > +{ > + return (*(const volatile uint16_t *)addr); > +} > +#define __raw_readw(addr) __raw_readw(addr) > + > +static inline void > +__raw_writew(uint16_t v, volatile void *addr) > +{ > + *(volatile uint16_t *)addr =3D v; > +} > +#define __raw_writew(v, addr) __raw_writew(v, addr) > + > static inline uint32_t > __raw_readl(const volatile void *addr) > { > - return *(const volatile uint32_t *)addr; > + return (*(const volatile uint32_t *)addr); > } > +#define __raw_readl(addr) __raw_readl(addr) > > static inline void > -__raw_writel(uint32_t b, volatile void *addr) > +__raw_writel(uint32_t v, volatile void *addr) > { > - *(volatile uint32_t *)addr =3D b; > + *(volatile uint32_t *)addr =3D v; > } > +#define __raw_writel(v, addr) __raw_writel(v, addr) > > +#ifdef __LP64__ > static inline uint64_t > __raw_readq(const volatile void *addr) > { > - return *(const volatile uint64_t *)addr; > + return (*(const volatile uint64_t *)addr); > } > +#define __raw_readq(addr) __raw_readq(addr) > > static inline void > -__raw_writeq(uint64_t b, volatile void *addr) > +__raw_writeq(uint64_t v, volatile void *addr) > { > - *(volatile uint64_t *)addr =3D b; > + *(volatile uint64_t *)addr =3D v; > } > +#define __raw_writeq(v, addr) __raw_writeq(v, addr) > +#endif > > -/* > - * XXX This is all x86 specific. It should be bus space access. > - */ > #define mmiowb() barrier() > > -#undef writel > +/* Access little-endian MMIO registers atomically with memory barriers. = */ > + > +#undef readb > +static inline uint8_t > +readb(const volatile void *addr) > +{ > + uint8_t v; > + > + __compiler_membar(); > + v =3D *(const volatile uint8_t *)addr; > + __compiler_membar(); > + return (v); > +} > +#define readb(addr) readb(addr) > + > +#undef writeb > static inline void > -writel(uint32_t b, void *addr) > +writeb(uint8_t v, volatile void *addr) > { > - *(volatile uint32_t *)addr =3D b; > + __compiler_membar(); > + *(volatile uint8_t *)addr =3D v; > + __compiler_membar(); > } > +#define writeb(v, addr) writeb(v, addr) > > -#undef writel_relaxed > +#undef readw > +static inline uint16_t > +readw(const volatile void *addr) > +{ > + uint16_t v; > + > + __compiler_membar(); > + v =3D *(const volatile uint16_t *)addr; > + __compiler_membar(); > + return (v); > +} > +#define readw(addr) readw(addr) > + > +#undef writew > static inline void > -writel_relaxed(uint32_t b, void *addr) > +writew(uint16_t v, volatile void *addr) > { > - *(volatile uint32_t *)addr =3D b; > + __compiler_membar(); > + *(volatile uint16_t *)addr =3D v; > + __compiler_membar(); > } > +#define writew(v, addr) writew(v, addr) > > +#undef readl > +static inline uint32_t > +readl(const volatile void *addr) > +{ > + uint32_t v; > + > + __compiler_membar(); > + v =3D *(const volatile uint32_t *)addr; > + __compiler_membar(); > + return (v); > +} > +#define readl(addr) readl(addr) > + > +#undef writel > +static inline void > +writel(uint32_t v, volatile void *addr) > +{ > + __compiler_membar(); > + *(volatile uint32_t *)addr =3D v; > + __compiler_membar(); > +} > +#define writel(v, addr) writel(v, addr) > + > +#undef readq > #undef writeq > +#ifdef __LP64__ > +static inline uint64_t > +readq(const volatile void *addr) > +{ > + uint64_t v; > + > + __compiler_membar(); > + v =3D *(const volatile uint64_t *)addr; > + __compiler_membar(); > + return (v); > +} > +#define readq(addr) readq(addr) > + > static inline void > -writeq(uint64_t b, void *addr) > +writeq(uint64_t v, volatile void *addr) > { > - *(volatile uint64_t *)addr =3D b; > + __compiler_membar(); > + *(volatile uint64_t *)addr =3D v; > + __compiler_membar(); > } > +#define writeq(v, addr) writeq(v, addr) > +#endif > > -#undef writeb > +/* Access little-endian MMIO registers atomically without memory > barriers. */ > + > +#undef readb_relaxed > +static inline uint8_t > +readb_relaxed(const volatile void *addr) > +{ > + return (*(const volatile uint8_t *)addr); > +} > +#define readb_relaxed(addr) readb_relaxed(addr) > + > +#undef writeb_relaxed > static inline void > -writeb(uint8_t b, void *addr) > +writeb_relaxed(uint8_t v, volatile void *addr) > { > - *(volatile uint8_t *)addr =3D b; > + *(volatile uint8_t *)addr =3D v; > } > +#define writeb_relaxed(v, addr) writeb_relaxed(v, addr) > > -#undef writew > +#undef readw_relaxed > +static inline uint16_t > +readw_relaxed(const volatile void *addr) > +{ > + return (*(const volatile uint16_t *)addr); > +} > +#define readw_relaxed(addr) readw_relaxed(addr) > + > +#undef writew_relaxed > static inline void > -writew(uint16_t b, void *addr) > +writew_relaxed(uint16_t v, volatile void *addr) > { > - *(volatile uint16_t *)addr =3D b; > + *(volatile uint16_t *)addr =3D v; > } > +#define writew_relaxed(v, addr) writew_relaxed(v, addr) > > +#undef readl_relaxed > +static inline uint32_t > +readl_relaxed(const volatile void *addr) > +{ > + return (*(const volatile uint32_t *)addr); > +} > +#define readl_relaxed(addr) readl_relaxed(addr) > + > +#undef writel_relaxed > +static inline void > +writel_relaxed(uint32_t v, volatile void *addr) > +{ > + *(volatile uint32_t *)addr =3D v; > +} > +#define writel_relaxed(v, addr) writel_relaxed(v, addr) > + > +#undef readq_relaxed > +#undef writeq_relaxed > +#ifdef __LP64__ > +static inline uint64_t > +readq_relaxed(const volatile void *addr) > +{ > + return (*(const volatile uint64_t *)addr); > +} > +#define readq_relaxed(addr) readq_relaxed(addr) > + > +static inline void > +writeq_relaxed(uint64_t v, volatile void *addr) > +{ > + *(volatile uint64_t *)addr =3D v; > +} > +#define writeq_relaxed(v, addr) writeq_relaxed(v, addr) > +#endif > + > +/* XXX On Linux ioread and iowrite handle both MMIO and port IO. */ > + > #undef ioread8 > static inline uint8_t > ioread8(const volatile void *addr) > { > - return *(const volatile uint8_t *)addr; > + return (readb(addr)); > } > +#define ioread8(addr) ioread8(addr) > > #undef ioread16 > static inline uint16_t > ioread16(const volatile void *addr) > { > - return *(const volatile uint16_t *)addr; > + return (readw(addr)); > } > +#define ioread16(addr) ioread16(addr) > > #undef ioread16be > static inline uint16_t > ioread16be(const volatile void *addr) > { > - return be16toh(*(const volatile uint16_t *)addr); > + return (bswap16(readw(addr))); > } > +#define ioread16be(addr) ioread16be(addr) > > #undef ioread32 > static inline uint32_t > ioread32(const volatile void *addr) > { > - return *(const volatile uint32_t *)addr; > + return (readl(addr)); > } > +#define ioread32(addr) ioread32(addr) > > #undef ioread32be > static inline uint32_t > ioread32be(const volatile void *addr) > { > - return be32toh(*(const volatile uint32_t *)addr); > + return (bswap32(readl(addr))); > } > +#define ioread32be(addr) ioread32be(addr) > > #undef iowrite8 > static inline void > iowrite8(uint8_t v, volatile void *addr) > { > - *(volatile uint8_t *)addr =3D v; > + writeb(v, addr); > } > +#define iowrite8(v, addr) iowrite8(v, addr) > > #undef iowrite16 > static inline void > iowrite16(uint16_t v, volatile void *addr) > { > - *(volatile uint16_t *)addr =3D v; > + writew(v, addr); > } > +#define iowrite16 iowrite16 > > #undef iowrite32 > static inline void > iowrite32(uint32_t v, volatile void *addr) > { > - *(volatile uint32_t *)addr =3D v; > + writel(v, addr); > } > +#define iowrite32(v, addr) iowrite32(v, addr) > > #undef iowrite32be > static inline void > iowrite32be(uint32_t v, volatile void *addr) > { > - *(volatile uint32_t *)addr =3D htobe32(v); > + writel(bswap32(v), addr); > } > - > -#undef readb > -static inline uint8_t > -readb(const volatile void *addr) > -{ > - return *(const volatile uint8_t *)addr; > -} > - > -#undef readw > -static inline uint16_t > -readw(const volatile void *addr) > -{ > - return *(const volatile uint16_t *)addr; > -} > - > -#undef readl > -static inline uint32_t > -readl(const volatile void *addr) > -{ > - return *(const volatile uint32_t *)addr; > -} > +#define iowrite32be(v, addr) iowrite32be(v, addr) > > #if defined(__i386__) || defined(__amd64__) > static inline void > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPrugNoMVDyg-CnVh5NUgrxdaHcbo3CibC3RsPQ7FVtdJ=FJdQ>