From owner-svn-src-all@freebsd.org Sat Nov 17 22:55:18 2018 Return-Path: Delivered-To: svn-src-all@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 7EB84110981C; Sat, 17 Nov 2018 22:55:18 +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 D9D91846AA; Sat, 17 Nov 2018 22:55:17 +0000 (UTC) (envelope-from mat.macy@gmail.com) Received: by mail-it1-x133.google.com with SMTP id a185so1915601itc.0; Sat, 17 Nov 2018 14:55:17 -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:content-transfer-encoding; bh=E/2sZ2asVeE2a1FHqciXkaPDgzyKbDlEp12UNZshaS4=; b=OT/kRelG2yHn+I6KmTdvZIMgKMooC6ibF86BLxcHbZ1bxPWZqKGF3j41fFGaeGls+O V1Bad3MKdGGmLk8SmEU2aql4fWQxpKtPRu+lytg8K5+37lFw+ruEhqP1B4wfW4wAnjjk jOJelSoWtJeZhHOANc2HwPy9HhW0gzc6kzVmUVShbQEBVyn99gaQzRcPfcmPWPtEnQgU DYIIwWYn5p5wdUo/XxPJpvrE9e5/KSMuydpTEgZqrJdE+zy+QM3bltrHYw8ALdzarIXW ahaGAM7WQwlE7EhCiAqv+OSKdUVZWO6PDcrE6/qujAG/eDAAmvUDBZayBEVqZONWzhhb BAvg== 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:content-transfer-encoding; bh=E/2sZ2asVeE2a1FHqciXkaPDgzyKbDlEp12UNZshaS4=; b=ixUa7ukHpTHpS5DgfETJauvnn/hG8aV5JOY5tGZZaCMc+KA/MtspVPMN+/z8/h22JB nLpN1IwEDAfXMt68wYVBqZOK/CC8RIu7gKvmXnV8TJ+aAczI2kb1vlTwSkKMykXcmNnj 33K7AWUIvMTizr9of+5ljXcngm/0z/7pZicMwZEfwB3eVoCy+CNy+yiXuDB8WCe4N9uC ErFe6s79iiHi7bVpFR7j208aUwj19ScSfBeqfQMfm6wfwsdS3vu8/96W+jYD2Ovhfr3O 1BhpbzLJK3/+hE25tV55tYlJ+4W8VSXzzoKi3kuWt/xmsaUQUNQYmAQJ8jS3cAuqsJxc LOOA== X-Gm-Message-State: AGRZ1gIppp3LWjAdopc/doChSnoYF/wBFELoDwV6yrMzduIf2F3Y1Ytb ARlF7eHejt6HCJVC5jFK2KLP8PoQ6yQsUG/0tFyjANqu X-Google-Smtp-Source: AJdET5e31b0Wua6nKWWh2AXqPiuCXKX/8OLXgnnzLzMu/fpe6W/C2yT8GnfgVNCCzs7Vh0aWg31fGoTo3sqijlT6Ypw= X-Received: by 2002:a02:56d3:: with SMTP id u80-v6mr14651848jad.88.1542495316820; Sat, 17 Nov 2018 14:55:16 -0800 (PST) MIME-Version: 1.0 References: <201810222055.w9MKtZPt013627@repo.freebsd.org> In-Reply-To: From: Matthew Macy Date: Sat, 17 Nov 2018 14:55:05 -0800 Message-ID: Subject: Re: svn commit: r339618 - head/sys/compat/linuxkpi/common/include/linux To: Tijl Coosemans Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: D9D91846AA X-Spamd-Result: default: False [-4.56 / 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)[text/plain]; TO_MATCH_ENVRCPT_ALL(0.00)[]; TO_DN_SOME(0.00)[]; IP_SCORE(-2.68)[ip: (-8.93), ipnet: 2607:f8b0::/32(-2.62), asn: 15169(-1.78), country: US(-0.10)]; MX_GOOD(-0.01)[cached: alt3.gmail-smtp-in.l.google.com]; DKIM_TRACE(0.00)[gmail.com:+]; 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]; NEURAL_HAM_SHORT(-0.87)[-0.870,0]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; 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 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 17 Nov 2018 22:55:18 -0000 When looking at powerpc io.h raw and relaxed are not aliases, but it appears that on x86, they are: https://github.com/torvalds/linux/blob/master/arch/x86/include/asm/io.h Sorry for the noise. But let's starting moving the x86 specific atomic.h and io.h under asm/x86. Thanks. On Sat, Nov 17, 2018 at 2:01 PM Matthew Macy wrote: > > You should probably revert this. The implied understanding of the _relaxe= d version is incorrect. compiler_membar is there to prevent instruction reo= rdering which is possible on FreeBSD because the accesses are done in C. Th= e relaxed variants still do not permit instruction reordering. On Linux __c= ompiler_member (referred to as barrier there) isn=E2=80=99t necessary in th= e mmio accessors because they always use volatile asm which can=E2=80=99t b= e reordered. The distinction between the relaxed and non relaxed variants i= s 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 h= appen before reads can become visible in memory after they occur in the ins= truction 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-kmo= d. >> 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" clobbe= r. >> The Linux x86 implementation of read_relaxed* and write_relaxed* uses = the >> same inline asm without "memory" clobber. >> >> Implement ioread* and iowrite* in terms of read* and write* so they al= so >> 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 the= y 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 barrie= rs. */ >> + >> +#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 >>