lkml.org 
[lkml]   [2013]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC/PATCH v2 1/8] Add arch_phys_wc_{add,del} to manipulate WC MTRRs if needed
    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
    >

    --
    Daniel Vetter
    Software Engineer, Intel Corporation
    +41 (0) 79 365 57 48 - http://blog.ffwll.ch


    \
     
     \ /
      Last update: 2013-05-10 12:01    [W:3.415 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site