From owner-svn-src-head@freebsd.org Sat Nov 17 22:01:26 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6288C1107BFB; Sat, 17 Nov 2018 22:01:26 +0000 (UTC) (envelope-from mat.macy@gmail.com) Received: from mail-it1-x133.google.com (mail-it1-x133.google.com [IPv6:2607:f8b0:4864:20::133]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 9B27A82750; Sat, 17 Nov 2018 22:01:25 +0000 (UTC) (envelope-from mat.macy@gmail.com) Received: by mail-it1-x133.google.com with SMTP id g85so1468408ita.3; Sat, 17 Nov 2018 14:01:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=P8KqskL6gW1NQvOzqGKhomib6Lq4rJjo/qAwnaHp0zo=; b=D0NZwIxTTycZaqJma2wODZ8BJeac2dNOgoS7/JGxwAnMonh9Vrx+n5i4WnJeiMQbCP v7oeJizK9rzBH5by1t65eIvIXeo17o54ka8K3tRYVRlk/UwxBrhDgZGpwGu1I7pZNTJK X5UHb+XHNCwMvZbZLtQ+vMbTu9orNhwKTTtFThGQAnsMXwl2/RltKqWKwzTyatBZ4AcL wpmF3RoBYSthPRyVfuYqFnHnXZLbV3/NWl5PdezwGrJxfc5h5ppXhAtL2Fl0a+9+5lBi lakzQbh+1jFjRZpNkhqxn4EnsDk313siCVEbFRXchuHbi10VIvDwrsBqc55uEuymhVqI FgVA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=P8KqskL6gW1NQvOzqGKhomib6Lq4rJjo/qAwnaHp0zo=; b=MJAa3PfAGQd9vasBiZlAGj7G0a0q/QT6zety3ZdHXCMqc8bROjZMZ7BzN0HulZtsww 2VblEHb+CtFs6Y4aA+bKm1H7Rq4qru8ooH8OwT23L92DvA56uW8hc1pynosnwJj4f/sS jsvLqodwlzQbnbEGh1kUWKlOGKRvpasjZTu/1XFw910Uk3Ontr9kXFoqbIqI7lvwHXIu 35yp05KQwNaUE7ckL7n019qZbaKs2vG7cdZ1VLBEnFWdjaWKYV1Qn6ttCemQeDNs2YgF Bf/C7/hymamb+lIzu1ljo6KXHs2JNsmhVVt9wQrEf/Dn4UsvxditlNRypQpuHrBx4QA5 yzlQ== X-Gm-Message-State: AGRZ1gL3V6xC/AtLKM3XAWSdnihQf9h/Sh4ddKO5Pe2cjep0Ji9/spo5 FeJLJbyV0ACZUd0drtRW5E7bqTgHKMp9+rgwBi+k2Q== X-Google-Smtp-Source: AFSGD/W1pGgWb3/WmSojuXxjKC0o4Am78X7Zvq+pg8xevCNle5IqzpToR3p29TslFYLDpfmRoV0XYmAK/eHAIUrSwxs= X-Received: by 2002:a05:660c:11d8:: with SMTP id p24mr3455987itm.20.1542492084563; Sat, 17 Nov 2018 14:01:24 -0800 (PST) MIME-Version: 1.0 References: <201810222055.w9MKtZPt013627@repo.freebsd.org> In-Reply-To: <201810222055.w9MKtZPt013627@repo.freebsd.org> From: Matthew Macy Date: Sat, 17 Nov 2018 14:01:13 -0800 Message-ID: Subject: Re: svn commit: r339618 - head/sys/compat/linuxkpi/common/include/linux To: Tijl Coosemans Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-Rspamd-Queue-Id: 9B27A82750 X-Spamd-Result: default: False [-4.49 / 15.00]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[gmail.com]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36]; FREEMAIL_FROM(0.00)[gmail.com]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; TO_MATCH_ENVRCPT_ALL(0.00)[]; TO_DN_SOME(0.00)[]; IP_SCORE(-2.65)[ip: (-8.77), ipnet: 2607:f8b0::/32(-2.61), asn: 15169(-1.78), country: US(-0.10)]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; RCVD_IN_DNSWL_NONE(0.00)[3.3.1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.b.8.f.7.0.6.2.list.dnswl.org : 127.0.5.0]; MX_GOOD(-0.01)[cached: alt3.gmail-smtp-in.l.google.com]; NEURAL_HAM_SHORT(-0.83)[-0.826,0]; FROM_EQ_ENVFROM(0.00)[]; RCVD_TLS_LAST(0.00)[]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; TAGGED_FROM(0.00)[]; RCVD_COUNT_TWO(0.00)[2] X-Rspamd-Server: mx1.freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 17 Nov 2018 22:01:26 -0000 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 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 > #include > > +/* > + * 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 > >