| 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
| ||
|
Message-ID: <20130510193437.GT12292@phenom.ffwll.local>
Date: Fri, 10 May 2013 21:34:37 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Andy Lutomirski <luto@...capital.net>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
linux-fbdev@...r.kernel.org, Jerome Glisse <j.glisse@...il.com>,
Alex Deucher <alexdeucher@...il.com>,
Dave Airlie <airlied@...il.com>,
Daniel Vetter <daniel.vetter@...ll.ch>
Subject: Re: [RFC/PATCH v2 1/8] Add arch_phys_wc_{add,del} to manipulate WC
MTRRs if needed
On Fri, May 10, 2013 at 12:27:54PM -0700, Andy Lutomirski wrote:
> On Fri, May 10, 2013 at 12:09 PM, Daniel Vetter <daniel@...ll.ch> wrote:
> > On Fri, May 10, 2013 at 11:00:56AM -0700, Andy Lutomirski wrote:
> >> On Fri, May 10, 2013 at 2:19 AM, Daniel Vetter <daniel@...ll.ch> wrote:
> >> > On Thu, May 09, 2013 at 12:46:20PM -0700, Andy Lutomirski wrote:
> >> >> Several drivers currently use mtrr_add through various #ifdef guards
> >> >> and/or drm wrappers. The vast majority of them want to add WC MTRRs
> >> >> on x86 systems and don't actually need the MTRR if PAT (i.e.
> >> >> ioremap_wc, etc) are working.
> >> >>
> >> >> arch_phys_wc_add and arch_phys_wc_del are new functions, available
> >> >> on all architectures and configurations, that add WC MTRRs on x86 if
> >> >> needed (and handle errors) and do nothing at all otherwise. They're
> >> >> also easier to use than mtrr_add and mtrr_del, so the call sites can
> >> >> be simplified.
> >> >>
> >> >> As an added benefit, this will avoid wasting MTRRs and possibly
> >> >> warning pointlessly on PAT-supporting systems.
> >> >>
> >> >> Signed-off-by: Andy Lutomirski <luto@...capital.net>
> >> >> ---
> >> >> arch/x86/include/asm/io.h | 7 ++++++
> >> >> arch/x86/include/asm/mtrr.h | 5 ++++-
> >> >> arch/x86/kernel/cpu/mtrr/main.c | 48 +++++++++++++++++++++++++++++++++++++++++
> >> >> include/linux/io.h | 25 +++++++++++++++++++++
> >> >> 4 files changed, 84 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> >> >> index d8e8eef..34f69cb 100644
> >> >> --- a/arch/x86/include/asm/io.h
> >> >> +++ b/arch/x86/include/asm/io.h
> >> >> @@ -345,4 +345,11 @@ extern bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
> >> >>
> >> >> #define IO_SPACE_LIMIT 0xffff
> >> >>
> >> >> +#ifdef CONFIG_MTRR
> >> >> +extern int __must_check arch_phys_wc_add(unsigned long base,
> >> >> + unsigned long size);
> >> >> +extern void arch_phys_wc_del(int handle);
> >> >> +#define arch_phys_wc_add arch_phys_wc_add
> >> >> +#endif
> >> >> +
> >> >> #endif /* _ASM_X86_IO_H */
> >> >> diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
> >> >> index e235582..10d0fba 100644
> >> >> --- a/arch/x86/include/asm/mtrr.h
> >> >> +++ b/arch/x86/include/asm/mtrr.h
> >> >> @@ -26,7 +26,10 @@
> >> >> #include <uapi/asm/mtrr.h>
> >> >>
> >> >>
> >> >> -/* The following functions are for use by other drivers */
> >> >> +/*
> >> >> + * The following functions are for use by other drivers that cannot use
> >> >> + * arch_phys_wc_add and arch_phys_wc_del.
> >> >> + */
> >> >> # ifdef CONFIG_MTRR
> >> >> extern u8 mtrr_type_lookup(u64 addr, u64 end);
> >> >> extern void mtrr_save_fixed_ranges(void *);
> >> >> diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
> >> >> index 726bf96..23bd49a 100644
> >> >> --- a/arch/x86/kernel/cpu/mtrr/main.c
> >> >> +++ b/arch/x86/kernel/cpu/mtrr/main.c
> >> >> @@ -51,6 +51,7 @@
> >> >> #include <asm/e820.h>
> >> >> #include <asm/mtrr.h>
> >> >> #include <asm/msr.h>
> >> >> +#include <asm/pat.h>
> >> >>
> >> >> #include "mtrr.h"
> >> >>
> >> >> @@ -524,6 +525,53 @@ int mtrr_del(int reg, unsigned long base, unsigned long size)
> >> >> }
> >> >> EXPORT_SYMBOL(mtrr_del);
> >> >>
> >> >> +/**
> >> >> + * arch_phys_wc_add - add a WC MTRR and handle errors if PAT is unavailable
> >> >> + * @base: Physical base address
> >> >> + * @size: Size of region
> >> >> + *
> >> >> + * If PAT is available, this does nothing. If PAT is unavailable, it
> >> >> + * attempts to add a WC MTRR covering size bytes starting at base and
> >> >> + * logs an error if this fails.
> >> >> + *
> >> >> + * Drivers must store the return value to pass to mtrr_del_wc_if_needed,
> >> >> + * but drivers should not try to interpret that return value.
> >> >> + */
> >> >> +int arch_phys_wc_add(unsigned long base, unsigned long size)
> >> >> +{
> >> >> + int ret;
> >> >> +
> >> >> + if (pat_enabled)
> >> >> + return 0; /* Success! (We don't need to do anything.) */
> >> >
> >> > Shouldn't we #define a big number for this case since mtrr_add returns
> >> > 0-based mtrr indices? Rather unlikely that the very first mtrr is unused I
> >> > know, but still feels like a cleaner interface. And we don't need to leak
> >> > that #define out at all to users of this interface.
> >>
> >> I did something more like that in v1, but I like having the property
> >> that arch_phys_wc_del(0) does nothing as opposed to possibly breaking
> >> the system. Given that almost all of these things are stored in
> >> kzalloced space, this seems safer to me. The return value here could
> >> just as easily be -ENOSYS, but nothing should care.
> >>
> >> There is an issue, though: the drm maps interface is leaking the
> >> return values to userspace via a couple of ioctls, so I should fix it
> >> to at least return the correct value. (Why that interface includes an
> >> mtrr number is a mystery to me.)
> >
> > Old drm interfaces are horrible. Best approach is to simply stay far away
> > from them ...
> >
> >> Have I convinced you that my bike shed color is okay? I'll send out a
> >> v3 later today with a fix for the leaking-to-userspace issue and I'll
> >> fix the i915 thing.
> >
> > A comment in your arch_phys_wc_add/del functions that we do have an mtrr
> > with index 0, but won't ever get that would be fine with me. Still feels a
> > bit irky though ;-)
>
> Hmm. Maybe you missed the hack I ended up using a couple of lines
> below. I'm always using strictly positive values to indicate a real
> MTRR -- I'm adding 1000 to the result if we actually do anything. So
> a return value of 0 is genuinely safe. (I still have to fix up the
> two places in the drm code that pass that hacked-up value back to
> userspace.)
Oops, I've indeed missed that one. Only noticed the special 0 and started
hunting down the semantics of mtrr_add. With that cleared up this is
Reviewed-by: Daniel Vetter <daniel.vetter@...ll.ch>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
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