| Message ID | 97346b603822ffd8ab446316fc08fbf7686c9d50.1368128020.git.luto@amacapital.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@amacapital.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. -Daniel > + > + ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true); > + if (ret < 0) { > + pr_warn("Failed to add WC MTRR for [%p-%p]; performance may suffer.", > + (void *)base, (void *)(base + size - 1)); > + return ret; > + } > + return ret + 1000; > +} > +EXPORT_SYMBOL(arch_phys_wc_add); > + > +/* > + * arch_phys_wc_del - undoes arch_phys_wc_add > + * @handle: Return value from arch_phys_wc_add > + * > + * This cleans up after mtrr_add_wc_if_needed. > + * > + * The API guarantees that mtrr_del_wc_if_needed(error code) and > + * mtrr_del_wc_if_needed(0) do nothing. > + */ > +extern void arch_phys_wc_del(int handle) > +{ > + if (handle >= 1) { > + WARN_ON(handle < 1000); > + mtrr_del(handle - 1000, 0, 0); > + } > +} > +EXPORT_SYMBOL(arch_phys_wc_del); > + > /* > * HACK ALERT! > * These should be called implicitly, but we can't yet until all the initcall > diff --git a/include/linux/io.h b/include/linux/io.h > index 069e407..f4f42fa 100644 > --- a/include/linux/io.h > +++ b/include/linux/io.h > @@ -76,4 +76,29 @@ void devm_ioremap_release(struct device *dev, void *res); > #define arch_has_dev_port() (1) > #endif > > +/* > + * Some systems (x86 without PAT) have a somewhat reliable way to mark a > + * physical address range such that uncached mappings will actually > + * end up write-combining. This facility should be used in conjunction > + * with pgprot_writecombine, ioremap-wc, or set_memory_wc, since it has > + * no effect if the per-page mechanisms are functional. > + * (On x86 without PAT, these functions manipulate MTRRs.) > + * > + * arch_phys_del_wc(0) or arch_phys_del_wc(any error code) is guaranteed > + * to have no effect. > + */ > +#ifndef arch_phys_wc_add > +static inline int __must_check arch_phys_wc_add(unsigned long base, > + unsigned long size) > +{ > + return 0; /* It worked (i.e. did nothing). */ > +} > + > +static inline void arch_phys_wc_del(int handle) > +{ > +} > + > +#define arch_phys_wc_add arch_phys_wc_add > +#endif > + > #endif /* _LINUX_IO_H */ > -- > 1.8.1.4 >
On Fri, May 10, 2013 at 2:19 AM, Daniel Vetter <daniel@ffwll.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@amacapital.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.) 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. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@ffwll.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@amacapital.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 ;-) Cheers, Daniel
On Fri, May 10, 2013 at 12:09 PM, Daniel Vetter <daniel@ffwll.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@ffwll.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@amacapital.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.) --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@ffwll.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@ffwll.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@amacapital.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@ffwll.ch>
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.) */ + + ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true); + if (ret < 0) { + pr_warn("Failed to add WC MTRR for [%p-%p]; performance may suffer.", + (void *)base, (void *)(base + size - 1)); + return ret; + } + return ret + 1000; +} +EXPORT_SYMBOL(arch_phys_wc_add); + +/* + * arch_phys_wc_del - undoes arch_phys_wc_add + * @handle: Return value from arch_phys_wc_add + * + * This cleans up after mtrr_add_wc_if_needed. + * + * The API guarantees that mtrr_del_wc_if_needed(error code) and + * mtrr_del_wc_if_needed(0) do nothing. + */ +extern void arch_phys_wc_del(int handle) +{ + if (handle >= 1) { + WARN_ON(handle < 1000); + mtrr_del(handle - 1000, 0, 0); + } +} +EXPORT_SYMBOL(arch_phys_wc_del); + /* * HACK ALERT! * These should be called implicitly, but we can't yet until all the initcall diff --git a/include/linux/io.h b/include/linux/io.h index 069e407..f4f42fa 100644 --- a/include/linux/io.h +++ b/include/linux/io.h @@ -76,4 +76,29 @@ void devm_ioremap_release(struct device *dev, void *res); #define arch_has_dev_port() (1) #endif +/* + * Some systems (x86 without PAT) have a somewhat reliable way to mark a + * physical address range such that uncached mappings will actually + * end up write-combining. This facility should be used in conjunction + * with pgprot_writecombine, ioremap-wc, or set_memory_wc, since it has + * no effect if the per-page mechanisms are functional. + * (On x86 without PAT, these functions manipulate MTRRs.) + * + * arch_phys_del_wc(0) or arch_phys_del_wc(any error code) is guaranteed + * to have no effect. + */ +#ifndef arch_phys_wc_add +static inline int __must_check arch_phys_wc_add(unsigned long base, + unsigned long size) +{ + return 0; /* It worked (i.e. did nothing). */ +} + +static inline void arch_phys_wc_del(int handle) +{ +} + +#define arch_phys_wc_add arch_phys_wc_add +#endif + #endif /* _LINUX_IO_H */
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@amacapital.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(-)