| 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
| ||
|
Date: Thu, 21 May 2009 20:49:00 +0900
From: Hitoshi Mitake <mitake@....info.waseda.ac.jp>
To: Hitoshi Mitake <h.mitake@...il.com>
Cc: Jeff Garzik <jeff@...zik.org>, "H. Peter Anvin" <hpa@...or.com>,
Roland Dreier <rdreier@...co.com>, Ingo Molnar <mingo@...e.hu>,
David Miller <davem@...emloft.net>,
Linus Torvalds <torvalds@...ux-foundation.org>,
tglx@...utronix.de, rpjday@...shcourse.ca,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86: Remove readq()/writeq() on 32-bit
On Thu, 21 May 2009 20:35:54 +0900
Hitoshi Mitake <h.mitake@...il.com> wrote:
> On Sun, May 17, 2009 at 17:06, Jeff Garzik <jeff@...zik.org> wrote:
> > Hitoshi Mitake wrote:
> >>
> >> On Fri, 15 May 2009 19:44:03 -0400
> >> Jeff Garzik <jeff@...zik.org> wrote:
> >>
> >>> Hitoshi Mitake wrote:
> >>>>
> >>>> On Wed, 13 May 2009 20:49:26 -0400
> >>>> Jeff Garzik <jeff@...zik.org> wrote:
> >>>>
> >>>>> H. Peter Anvin wrote:
> >>>>>>
> >>>>>> Jeff Garzik wrote:
> >>>>>>>
> >>>>>>> Judging from this thread and past, I think people will continue to
> >>>>>>> complain and get confused, even with the above.
> >>>>>>>
> >>>>>> Do you really think so? Seems unfortunate, since an API rename would
> >>>>>> be
> >>>>>> way more invasive. This is the entirety of the header patch
> >>>>>> (compile-tested using 32-bit allyesconfig).
> >>>>>
> >>>>> The header patch does not lessen the confusion, because you cannot look
> >>>>> at the code and immediately tell what is going on...
> >>>>>
> >>>>> Having a single function's behavior change based on #include selection
> >>>>> is /not/ intuitive at all, particularly for driver writers. That is unlike
> >>>>> almost every other Linux API, where functions' behavior stays constant
> >>>>> across platforms, regardless of magic "under the hood."
> >>>>>
> >>>>> That sort of trick is reserved for arch maintainers who know what they
> >>>>> are doing :)
> >>>>>
> >>>>> Jeff
> >>>>>
> >>>>>
> >>>>>
> >>>> I found another way:
> >>>> Making architecture with atomic readq/writeq provide
> >>>> HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC
> >>>> and making architecture with non-atomic readq/writeq provide
> >>>> HAVE_READQ/HAVE_WRITEQ.
> >>>> (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as
> >>>> HAVE_READQ/HAVE_WRITEQ.)
> >>>>
> >>>> So driver programmers who need atomic readq/writeq can judge existence
> >>>> of API they really need.
> >>>> If platform doesn't provide atomic readq/writeq, drivers need these can
> >>>> be disabled by Kconfig.
> >>>> And bugs Roland and David talking about will be banished.
> >>>> How about this? > Roland and David
> >>>> I wrote a test patch. Request for comments.
> >>>>
> >>>> Signed-off-by: Hitoshi Mitake <h.mitake@...il.com>
> >>>>
> >>>> ---
> >>>> arch/x86/Kconfig | 16 ++++++++++++++--
> >>>> 1 files changed, 14 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >>>> index df9e885..c94fc48 100644
> >>>> --- a/arch/x86/Kconfig
> >>>> +++ b/arch/x86/Kconfig
> >>>> @@ -19,8 +19,6 @@ config X86_64
> >>>> config X86
> >>>> def_bool y
> >>>> select HAVE_AOUT if X86_32
> >>>> - select HAVE_READQ
> >>>> - select HAVE_WRITEQ
> >>>> select HAVE_UNSTABLE_SCHED_CLOCK
> >>>> select HAVE_IDE
> >>>> select HAVE_OPROFILE
> >>>> @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP
> >>>> def_bool y
> >>>> depends on X86_32
> >>>> +config HAVE_READQ
> >>>> + def_bool y
> >>>> +
> >>>> +config HAVE_WRITEQ
> >>>> + def_bool y
> >>>> +
> >>>> +config HAVE_READQ_ATOMIC
> >>>> + def_bool y
> >>>> + depends on X86_64
> >>>> +
> >>>> +config HAVE_WRITEQ_ATOMIC
> >>>> + def_bool y
> >>>> + depends on X86_64
> >>>
> >>> If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need
> >>> HAVE_READQ -- the other relevant 32-bit platforms simply need a definition
> >>> of readq and writeq. Probably easy enough to have a common definition in
> >>> asm-generic.
> >>>
> >> That's good idea. I didn't noticed the way to use asm-generic. Thanks.
> >>
> >> How is this?
> >>
> >> Signed-off-by: Hitoshi Mitake <h.mitake@...il.com>
> >>
> >> ---
> >> arch/x86/Kconfig | 10 ++++++++--
> >> arch/x86/include/asm/io.h | 27 ++++++---------------------
> >> include/asm-generic/quadrw.h | 34 ++++++++++++++++++++++++++++++++++
> >> 3 files changed, 48 insertions(+), 23 deletions(-)
> >> create mode 100644 include/asm-generic/quadrw.h
> >
> >
> > Seems fine to me, no objections here...
> >
> >
> >
> >> +#ifndef CONFIG_HAVE_READQ_ATOMIC
> >> +static inline __u64 readq(const volatile void __iomem *addr)
> >> +{
> >> + const volatile u32 __iomem *p = addr;
> >> + u32 low, high;
> >> +
> >> + low = readl(p);
> >> + high = readl(p + 1);
> >> +
> >> + return low + ((u64)high << 32);
> >> +}
> >> +#endif /* CONFIG_HAVE_READQ_ATOMIC */
> >
> > Personally I would do
> >
> > static inline __u64 readq(const volatile void __iomem *addr)
> > {
> > __u64 low, high;
> >
> > low = readl(addr);
> > high = readl(addr + 4);
> >
> > return (high << 32) | low;
> > }
> >
> > but maybe that's just me.
> >
> > It seems inconsistent that the generic readq and writeq internals, simple as
> > they are, differ to such a degree in this implementation.
> >
> > But overall, as mentioned above, the approach seems sound to me.
> >
> > Cheers!
> >
> > Jeff
> >
> >
> >
>
I fixed readq according to Jeff's advice. I think his readq is smarter than mine.
This is new version of the patch.
So I want to hear Roland and David's opinion.
We will be able to avoid making terrible bugs with this patch.
And generic readq/writeq will be provided for the cases
without requirement of atomic readq/writeq.
This patch would make our life easily. How do you think?
Signed-off-by: Hitoshi Mitake <mitake@....info.waseda.ac.jp>
Cc: Jeff Garzik <jeff@...zik.org>
Cc: Roland Dreier <rolandd@...co.com>
Cc: David S. Miller <davem@...emloft.net>
---
arch/x86/Kconfig | 10 ++++++++--
arch/x86/include/asm/io.h | 27 ++++++---------------------
include/asm-generic/quadrw.h | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 48 insertions(+), 23 deletions(-)
create mode 100644 include/asm-generic/quadrw.h
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index a6efe0a..46ea47c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -19,8 +19,6 @@ config X86_64
config X86
def_bool y
select HAVE_AOUT if X86_32
- select HAVE_READQ
- select HAVE_WRITEQ
select HAVE_UNSTABLE_SCHED_CLOCK
select HAVE_IDE
select HAVE_OPROFILE
@@ -2035,6 +2033,14 @@ config HAVE_ATOMIC_IOMAP
def_bool y
depends on X86_32
+config HAVE_READQ_ATOMIC
+ def_bool y
+ depends on X86_64
+
+config HAVE_WRITEQ_ATOMIC
+ def_bool y
+ depends on X86_64
+
source "net/Kconfig"
source "drivers/Kconfig"
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 7373932..bad940d 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -51,32 +51,17 @@ build_mmio_write(__writel, "l", unsigned int, "r", )
build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
build_mmio_write(writeq, "q", unsigned long, "r", :"memory")
-#else
-
-static inline __u64 readq(const volatile void __iomem *addr)
-{
- const volatile u32 __iomem *p = addr;
- u32 low, high;
-
- low = readl(p);
- high = readl(p + 1);
-
- return low + ((u64)high << 32);
-}
-
-static inline void writeq(__u64 val, volatile void __iomem *addr)
-{
- writel(val, addr);
- writel(val >> 32, addr+4);
-}
-
-#endif
-
#define readq_relaxed(a) readq(a)
#define __raw_readq(a) readq(a)
#define __raw_writeq(val, addr) writeq(val, addr)
+#endif /* CONFIG_X86_64 */
+
+#ifdef CONFIG_X86_32
+#include <asm-generic/quadrw.h>
+#endif /* CONFIG_X86_32 */
+
/* Let people know that we have them */
#define readq readq
#define writeq writeq
diff --git a/include/asm-generic/quadrw.h b/include/asm-generic/quadrw.h
new file mode 100644
index 0000000..15856ac
--- /dev/null
+++ b/include/asm-generic/quadrw.h
@@ -0,0 +1,34 @@
+#ifndef GENERIC_QUADRW_H
+#define GENERIC_QUADRW_H
+
+#include <asm/io.h>
+
+/*
+ * General readq/writeq implementation.
+ * These are not atomic operations.
+ * The drivers which need atomic version readq/writeq,
+ * they should depend on HAVE_{READQ,WRITEQ}_ATOMIC in Kconfig level.
+ */
+
+#ifndef CONFIG_HAVE_READQ_ATOMIC
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+ __u64 low, high;
+
+ low = readl(addr);
+ high = readl(addr + 4);
+
+ return (high << 32) | low;
+}
+
+#endif /* CONFIG_HAVE_READQ_ATOMIC */
+
+#ifndef CONFIG_HAVE_WRITEQ_ATOMIC
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+ writel(val, addr);
+ writel(val >> 32, addr+4);
+}
+#endif /* CONFIG_HAVE_WRITEQ_ATOMIC */
+
+#endif /* GENERIC_QUADRW_H */
--
1.5.6.5
--
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/
Powered by blists - more mailing lists