From: Luis R. Rodriguez <hidden> Date: 2015-03-20 23:20:57
From: "Luis R. Rodriguez" <redacted>
When a system has PAT support enabled you don't need to be
using MTRRs. Andy had added arch_phys_wc_add() long ago to
help with this but not all drivers were converted over. We
have to take care to only convert drivers where we know that
the proper ioremap_wc() API has been used. Doing this requires
a bit of work on verifying the driver split out the ioremap'd
areas -- and if not doing that ourselves. Verifying a driver
uses the same areas can be hard but with a bit of love Coccinelle
can help with that.
We're motivated to change drivers for a few reasons:
1) Take advantage of PAT when available
2) Help with the goal of eventually using _PAGE_CACHE_UC over
_PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (de33c442e)
3) Bury MTRR code away from drivers as it is architecture specific
While working on the conversion I noticed a few things.
a) Run time disabling of MTRR
Some systems can technically have both PAT and MTRR enabled
and even if they support it, a system may end up not enabling MTRR.
There are a few reasons why this can happen but the code right now
doesn't address this well. This leads to another point: PAT code
right now is not a first class citizen on x86 -- pat_init() depends
on MTRR code so we can't actually enable PAT without building MTRR.
Doing this requires quite a bit more work so let this serve as
a starting point for conversation if we want to address that.
b) Driver work and required ioremap split
In order to take advantage of PAT device drivers that were using
MTRR must make sure that the area that was using MTRR is ioremap'd
separately. Fortunately a lot of drivers already do this, but there's
quite a bit of drivers that require some love to get that happen.
This leaves us needing to expose an last resort API to annotate this
and also avoid a regression on performance for systems that may have
PAT but can't yet move away from using MTRR. To find the drivers that
need love check out __arch_phys_wc_add(). For a good example driver
where the work was done refer to the atyfb driver fixes.
c) Missing APIs for write-combining
There's a few API calls missing to take advantage of write-combining,
this series add those.
d) Further framebuffer driver MTRR usage simplication
We can simplify MTRR usage by having the framebuffer core
add the MTRR by passing a flag when register_framebuffer()
is called, this could for instance be done on very few drivers
where the smem_len and smem_start are both used for the ioremap_wc()
and also for the arch_phys_wc_add(). Coccinelle can be easily used
to do a transformation here. I didn't do that here given that it
does not work for all device drivers *and* DRM drivers already
have something similar. Lastly this technically could also be done
on some other generic helper --- but figured its best we review that
here. One reason to *not* do this is that tons of framebuffer drivers
have mtrr options exposed -- we'd need to generalize those and provide
a port ... or deal with the fact that we are going to remove all that.
Luis R. Rodriguez (47):
x86: mtrr: annotate mtrr_type_lookup() is only implemented on
generic_mtrr_ops
x86: mtrr: generalize run time disabling of MTRR
devres: add devm_ioremap_wc()
pci: add pci_ioremap_wc_bar()
pci: add pci_iomap_wc() variants
mtrr: add __arch_phys_wc_add()
video: fbdev: atyfb: move framebuffer length fudging to helper
video: fbdev: atyfb: clarify ioremap() base and length used
vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
video: fbdev: atyfb: use arch_phys_wc_add() and ioremap_wc()
IB/qib: add acounting for MTRR
IB/qib: use arch_phys_wc_add()
IB/ipath: add counting for MTRR
IB/ipath: use __arch_phys_wc_add()
[media] media: ivtv: use __arch_phys_wc_add()
fusion: use __arch_phys_wc_add()
video: fbdev: vesafb: only support MTRR_TYPE_WRCOMB
vidoe: fbdev: vesafb: add missing mtrr_del() for added MTRR
video: fbdev: vesafb: use arch_phys_wc_add()
mtrr: avoid ifdef'ery with phys_wc_to_mtrr_index()
ethernet: myri10ge: use arch_phys_wc_add()
staging: sm750fb: use arch_phys_wc_add() and ioremap_wc()
staging: xgifb: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: arkfb: use arch_phys_wc_add() and pci_iomap_wc()
video: fbdev: radeonfb: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: gbefb: add missing mtrr_del() calls
video: fbdev: gbefb: use arch_phys_wc_add() and devm_ioremap_wc()
video: fbdev: intelfb: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: matrox: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: neofb: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: s3fb: use arch_phys_wc_add() and pci_iomap_wc()
video: fbdev: nvidia: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: savagefb: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: sisfb: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: aty: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: i810: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: i740fb: use arch_phys_wc_add() and pci_ioremap_wc_bar()
video: fbdev: kyrofb: use arch_phys_wc_add() and pci_ioremap_wc_bar()
video: fbdev: pm2fb: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: pm3fb: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: rivafb: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: tdfxfb: use arch_phys_wc_add() and ioremap_wc()
video: fbdev: vt8623fb: use arch_phys_wc_add() and pci_iomap_wc()
video: fbdev: atmel_lcdfb: use ioremap_wc() for framebuffer
video: fbdev: geode gxfb: use ioremap_wc() for framebuffer
video: fbdev: gxt4500: use pci_ioremap_wc_bar() for framebuffer
mtrr: bury MTRR - unexport mtrr_add() and mtrr_del()
Documentation/driver-model/devres.txt | 1 +
arch/x86/include/asm/io.h | 6 ++
arch/x86/include/asm/mtrr.h | 7 +-
arch/x86/kernel/cpu/mtrr/cleanup.c | 2 +-
arch/x86/kernel/cpu/mtrr/generic.c | 7 +-
arch/x86/kernel/cpu/mtrr/if.c | 3 +
arch/x86/kernel/cpu/mtrr/main.c | 73 +++++++++++++------
drivers/gpu/drm/drm_ioctl.c | 14 +---
drivers/infiniband/hw/ipath/ipath_driver.c | 7 +-
drivers/infiniband/hw/ipath/ipath_kernel.h | 4 +-
drivers/infiniband/hw/ipath/ipath_wc_x86_64.c | 47 +++++--------
drivers/infiniband/hw/qib/qib_wc_x86_64.c | 31 ++------
drivers/media/pci/ivtv/ivtvfb.c | 51 ++++----------
drivers/message/fusion/mptbase.c | 19 ++---
drivers/message/fusion/mptbase.h | 2 +-
drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 36 +++-------
drivers/pci/pci.c | 14 ++++
drivers/staging/sm750fb/sm750.c | 34 ++-------
drivers/staging/sm750fb/sm750.h | 3 -
drivers/staging/sm750fb/sm750_hw.c | 3 +-
drivers/staging/xgifb/XGI_main_26.c | 27 ++-----
drivers/video/fbdev/arkfb.c | 36 ++--------
drivers/video/fbdev/atmel_lcdfb.c | 3 +-
drivers/video/fbdev/aty/aty128fb.c | 36 ++--------
drivers/video/fbdev/aty/atyfb.h | 5 +-
drivers/video/fbdev/aty/atyfb_base.c | 90 ++++++++----------------
drivers/video/fbdev/aty/radeon_base.c | 29 ++------
drivers/video/fbdev/aty/radeonfb.h | 2 +-
drivers/video/fbdev/gbefb.c | 18 +++--
drivers/video/fbdev/geode/gxfb_core.c | 3 +-
drivers/video/fbdev/gxt4500.c | 2 +-
drivers/video/fbdev/i740fb.c | 35 ++-------
drivers/video/fbdev/i810/i810.h | 3 +-
drivers/video/fbdev/i810/i810_main.c | 11 +--
drivers/video/fbdev/i810/i810_main.h | 26 -------
drivers/video/fbdev/intelfb/intelfb.h | 4 +-
drivers/video/fbdev/intelfb/intelfbdrv.c | 38 ++--------
drivers/video/fbdev/kyro/fbdev.c | 33 +++------
drivers/video/fbdev/matrox/matroxfb_base.c | 36 ++++------
drivers/video/fbdev/matrox/matroxfb_base.h | 27 +------
drivers/video/fbdev/neofb.c | 26 ++-----
drivers/video/fbdev/nvidia/nv_type.h | 7 +-
drivers/video/fbdev/nvidia/nvidia.c | 37 ++--------
drivers/video/fbdev/pm2fb.c | 31 ++------
drivers/video/fbdev/pm3fb.c | 30 ++------
drivers/video/fbdev/riva/fbdev.c | 39 ++--------
drivers/video/fbdev/riva/rivafb.h | 4 +-
drivers/video/fbdev/s3fb.c | 35 ++-------
drivers/video/fbdev/savage/savagefb.h | 4 +-
drivers/video/fbdev/savage/savagefb_driver.c | 17 +----
drivers/video/fbdev/sis/sis.h | 2 +-
drivers/video/fbdev/sis/sis_main.c | 27 ++-----
drivers/video/fbdev/tdfxfb.c | 41 ++---------
drivers/video/fbdev/vesafb.c | 77 +++++++-------------
drivers/video/fbdev/vt8623fb.c | 31 ++------
include/asm-generic/pci_iomap.h | 14 ++++
include/linux/io.h | 12 ++++
include/linux/pci.h | 1 +
include/video/kyro.h | 4 +-
include/video/neomagic.h | 5 +-
include/video/tdfx.h | 2 +-
lib/devres.c | 29 ++++++++
lib/pci_iomap.c | 61 ++++++++++++++++
63 files changed, 463 insertions(+), 901 deletions(-)
--
2.3.2.209.gd67f9d5.dirty
@@ -50,6 +51,7 @@ extern int mtrr_trim_uncached_memory(unsigned long end_pfn);externintamd_special_default_mtrr(void);externintphys_wc_to_mtrr_index(inthandle);# else+staticconstintmtrr_enabled;staticinlineu8mtrr_type_lookup(u64addr,u64end){/*
@@ -880,7 +880,7 @@ int __init mtrr_trim_uncached_memory(unsigned long end_pfn)*Makesureweonlytrimuncachablememoryonmachinesthat*supporttheIntelMTRRarchitecture:*/-if(!is_cpu(INTEL)||disable_mtrr_trim)+if(!is_cpu(INTEL)||disable_mtrr_trim||!mtrr_enabled)return0;rdmsr(MSR_MTRRdefType,def,dummy);
@@ -116,7 +116,8 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)u8prev_match,curr_match;*repeat=0;-if(!mtrr_state_set)+/* generic_mtrr_ops is only set for generic_mtrr_ops */+if(!mtrr_state_set||!mtrr_enabled)return0xFF;if(!mtrr_state.enabled)
@@ -84,6 +85,9 @@ static int have_wrcomb(void){structpci_dev*dev;+if(!mtrr_enabled)+return0;+dev=pci_get_class(PCI_CLASS_BRIDGE_HOST<<8,NULL);if(dev!=NULL){/*
@@ -286,7 +290,7 @@ int mtrr_add_page(unsigned long base, unsigned long size,inti,replace,error;mtrr_typeltype;-if(!mtrr_if)+if(!mtrr_enabled)return-ENXIO;error=mtrr_if->validate_add_page(base,size,type);
@@ -388,6 +392,8 @@ int mtrr_add_page(unsigned long base, unsigned long size,staticintmtrr_check(unsignedlongbase,unsignedlongsize){+if(!mtrr_enabled)+return-ENODEV;if((base&(PAGE_SIZE-1))||(size&(PAGE_SIZE-1))){pr_warning("mtrr: size and base must be multiples of 4 kiB\n");pr_debug("mtrr: size: 0x%lx base: 0x%lx\n",size,base);
@@ -463,8 +469,8 @@ int mtrr_del_page(int reg, unsigned long base, unsigned long size)unsignedlonglbase,lsize;interror=-EINVAL;-if(!mtrr_if)-return-ENXIO;+if(!mtrr_enabled)+return-ENODEV;max=num_var_ranges;/* No CPU hotplug when we change MTRR entries */
@@ -523,6 +529,8 @@ int mtrr_del_page(int reg, unsigned long base, unsigned long size)*/intmtrr_del(intreg,unsignedlongbase,unsignedlongsize){+if(!mtrr_enabled)+return-ENODEV;if(mtrr_check(base,size))return-EINVAL;returnmtrr_del_page(reg,base>>PAGE_SHIFT,size>>PAGE_SHIFT);
@@ -545,7 +553,7 @@ int arch_phys_wc_add(unsigned long base, unsigned long size){intret;-if(pat_enabled)+if(pat_enabled||!mtrr_enabled)return0;/* Success! (We don't need to do anything.) */ret=mtrr_add(base,size,MTRR_TYPE_WRCOMB,true);
@@ -744,12 +753,13 @@ void __init mtrr_bp_init(void)mtrr_if->set_all();}}-}+}else+pr_info("mtrr: system does not support MTRR\n");}voidmtrr_ap_init(void){-if(!use_intel()||mtrr_aps_delayed_init)+if(!use_intel()||mtrr_aps_delayed_init||!mtrr_enabled)return;/**Ideallyweshouldholdmtrr_mutexheretoavoidmtrrentries
From: Konrad Rzeszutek Wilk <hidden> Date: 2015-03-25 20:03:02
On Fri, Mar 20, 2015 at 04:17:52PM -0700, Luis R. Rodriguez wrote:
From: "Luis R. Rodriguez" <redacted>
It is possible to enable CONFIG_MTRR and up with it
disabled at run time and yet CONFIG_X86_PAT continues
to kick through fully functionally. This can happen
s/fully/full/ ?
for instance on Xen where MTRR is not supported but
PAT is, this can happen now on Linux as of commit
47591df50 by Juergen introduced as of v3.19.
s/3.19/4.0/
Technically we should assume the proper CPU
bits would be set to disable MTRR but we can't
always rely on this. At least on the Xen Hypervisor
for instance only X86_FEATURE_MTRR was disabled
as of Xen 4.4 through Xen commit 586ab6a [0],
but not X86_FEATURE_K6_MTRR, X86_FEATURE_CENTAUR_MCR,
or X86_FEATURE_CYRIX_ARR for instance.
Oh, could you send an patch for that to Xen please?
x86 mtrr code relies on quite a bit of checks for
mtrr_if being set to check to see if MTRR did get
set up, instead of using that lets provide a generic
setter which when set we know MTRR is enabled. This
s/we know MTRR is enabled/will let us know that MTRR is enabled/
quoted hunk
also adds a few checks where they were not before
which could potentially safeguard ourselves against
incorrect usage of MTRR where this was not desirable.
Where possible match error codes as if MTRR was
disabled on arch/x86/include/asm/mtrr.h.
Lastly, since disabling MTRR can happen at run time
and we could end up with PAT enabled best record now
on our logs when MTRR is disabled.
[0] ~/devel/xen (git::stable-4.5)$ git describe --contains 586ab6a
4.4.0-rc1~18
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: venkatesh.pallipadi@intel.com
Cc: Stefan Bader <redacted>
Cc: konrad.wilk@oracle.com
Cc: ville.syrjala@linux.intel.com
Cc: david.vrabel@citrix.com
Cc: jbeulich@suse.com
Cc: toshi.kani@hp.com
Cc: bhelgaas@google.com
Cc: Roger Pau Monné <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xensource.com
Signed-off-by: Luis R. Rodriguez <redacted>
---
arch/x86/include/asm/mtrr.h | 2 ++
arch/x86/kernel/cpu/mtrr/cleanup.c | 2 +-
arch/x86/kernel/cpu/mtrr/generic.c | 5 +++--
arch/x86/kernel/cpu/mtrr/if.c | 3 +++
arch/x86/kernel/cpu/mtrr/main.c | 31 ++++++++++++++++++++++---------
5 files changed, 31 insertions(+), 12 deletions(-)
@@ -50,6 +51,7 @@ extern int mtrr_trim_uncached_memory(unsigned long end_pfn);externintamd_special_default_mtrr(void);externintphys_wc_to_mtrr_index(inthandle);# else+staticconstintmtrr_enabled;staticinlineu8mtrr_type_lookup(u64addr,u64end){/*
@@ -880,7 +880,7 @@ int __init mtrr_trim_uncached_memory(unsigned long end_pfn)*Makesureweonlytrimuncachablememoryonmachinesthat*supporttheIntelMTRRarchitecture:*/-if(!is_cpu(INTEL)||disable_mtrr_trim)+if(!is_cpu(INTEL)||disable_mtrr_trim||!mtrr_enabled)return0;rdmsr(MSR_MTRRdefType,def,dummy);
@@ -116,7 +116,8 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)u8prev_match,curr_match;*repeat=0;-if(!mtrr_state_set)+/* generic_mtrr_ops is only set for generic_mtrr_ops */+if(!mtrr_state_set||!mtrr_enabled)return0xFF;if(!mtrr_state.enabled)
@@ -84,6 +85,9 @@ static int have_wrcomb(void){structpci_dev*dev;+if(!mtrr_enabled)+return0;+dev=pci_get_class(PCI_CLASS_BRIDGE_HOST<<8,NULL);if(dev!=NULL){/*
@@ -286,7 +290,7 @@ int mtrr_add_page(unsigned long base, unsigned long size,inti,replace,error;mtrr_typeltype;-if(!mtrr_if)+if(!mtrr_enabled)return-ENXIO;error=mtrr_if->validate_add_page(base,size,type);
@@ -388,6 +392,8 @@ int mtrr_add_page(unsigned long base, unsigned long size,staticintmtrr_check(unsignedlongbase,unsignedlongsize){+if(!mtrr_enabled)+return-ENODEV;if((base&(PAGE_SIZE-1))||(size&(PAGE_SIZE-1))){pr_warning("mtrr: size and base must be multiples of 4 kiB\n");pr_debug("mtrr: size: 0x%lx base: 0x%lx\n",size,base);
@@ -463,8 +469,8 @@ int mtrr_del_page(int reg, unsigned long base, unsigned long size)unsignedlonglbase,lsize;interror=-EINVAL;-if(!mtrr_if)-return-ENXIO;+if(!mtrr_enabled)+return-ENODEV;max=num_var_ranges;/* No CPU hotplug when we change MTRR entries */
@@ -523,6 +529,8 @@ int mtrr_del_page(int reg, unsigned long base, unsigned long size)*/intmtrr_del(intreg,unsignedlongbase,unsignedlongsize){+if(!mtrr_enabled)+return-ENODEV;if(mtrr_check(base,size))return-EINVAL;returnmtrr_del_page(reg,base>>PAGE_SHIFT,size>>PAGE_SHIFT);
@@ -545,7 +553,7 @@ int arch_phys_wc_add(unsigned long base, unsigned long size){intret;-if(pat_enabled)+if(pat_enabled||!mtrr_enabled)return0;/* Success! (We don't need to do anything.) */ret=mtrr_add(base,size,MTRR_TYPE_WRCOMB,true);
@@ -744,12 +753,13 @@ void __init mtrr_bp_init(void)mtrr_if->set_all();}}-}+}else+pr_info("mtrr: system does not support MTRR\n");
'pr_warn' ?
quoted hunk
}
void mtrr_ap_init(void)
{
- if (!use_intel() || mtrr_aps_delayed_init)
+ if (!use_intel() || mtrr_aps_delayed_init || !mtrr_enabled)
return;
/*
* Ideally we should hold mtrr_mutex here to avoid mtrr entries
@@ -774,6 +784,9 @@ void mtrr_save_state(void) { int first_cpu;+ if (!mtrr_enabled)+ return;+ get_online_cpus(); first_cpu = cpumask_first(cpu_online_mask); smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
@@ -782,7 +795,7 @@ void mtrr_save_state(void) void set_mtrr_aps_delayed_init(void) {- if (!use_intel())+ if (!use_intel() || !mtrr_enabled) return; mtrr_aps_delayed_init = true;
@@ -810,7 +823,7 @@ void mtrr_aps_init(void) void mtrr_bp_restore(void) {- if (!use_intel())+ if (!use_intel() || !mtrr_enabled) return; mtrr_if->set_all();
@@ -818,7 +831,7 @@ void mtrr_bp_restore(void) static int __init mtrr_init_finialize(void) {- if (!mtrr_if)+ if (!mtrr_enabled) return 0; if (use_intel()) {
On 03/25/2015 08:59 PM, Konrad Rzeszutek Wilk wrote:
On Fri, Mar 20, 2015 at 04:17:52PM -0700, Luis R. Rodriguez wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
It is possible to enable CONFIG_MTRR and up with it
disabled at run time and yet CONFIG_X86_PAT continues
to kick through fully functionally. This can happen
s/fully/full/ ?
quoted
for instance on Xen where MTRR is not supported but
PAT is, this can happen now on Linux as of commit
47591df50 by Juergen introduced as of v3.19.
s/3.19/4.0/
No, 3.19 is correct.
Juergen
quoted
Technically we should assume the proper CPU
bits would be set to disable MTRR but we can't
always rely on this. At least on the Xen Hypervisor
for instance only X86_FEATURE_MTRR was disabled
as of Xen 4.4 through Xen commit 586ab6a [0],
but not X86_FEATURE_K6_MTRR, X86_FEATURE_CENTAUR_MCR,
or X86_FEATURE_CYRIX_ARR for instance.
Oh, could you send an patch for that to Xen please?
quoted
x86 mtrr code relies on quite a bit of checks for
mtrr_if being set to check to see if MTRR did get
set up, instead of using that lets provide a generic
setter which when set we know MTRR is enabled. This
s/we know MTRR is enabled/will let us know that MTRR is enabled/
quoted
also adds a few checks where they were not before
which could potentially safeguard ourselves against
incorrect usage of MTRR where this was not desirable.
Where possible match error codes as if MTRR was
disabled on arch/x86/include/asm/mtrr.h.
Lastly, since disabling MTRR can happen at run time
and we could end up with PAT enabled best record now
on our logs when MTRR is disabled.
[0] ~/devel/xen (git::stable-4.5)$ git describe --contains 586ab6a
4.4.0-rc1~18
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: venkatesh.pallipadi@intel.com
Cc: Stefan Bader <redacted>
Cc: konrad.wilk@oracle.com
Cc: ville.syrjala@linux.intel.com
Cc: david.vrabel@citrix.com
Cc: jbeulich@suse.com
Cc: toshi.kani@hp.com
Cc: bhelgaas@google.com
Cc: Roger Pau Monné <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xensource.com
Signed-off-by: Luis R. Rodriguez <redacted>
---
arch/x86/include/asm/mtrr.h | 2 ++
arch/x86/kernel/cpu/mtrr/cleanup.c | 2 +-
arch/x86/kernel/cpu/mtrr/generic.c | 5 +++--
arch/x86/kernel/cpu/mtrr/if.c | 3 +++
arch/x86/kernel/cpu/mtrr/main.c | 31 ++++++++++++++++++++++---------
5 files changed, 31 insertions(+), 12 deletions(-)
@@ -50,6 +51,7 @@ extern int mtrr_trim_uncached_memory(unsigned long end_pfn);externintamd_special_default_mtrr(void);externintphys_wc_to_mtrr_index(inthandle);# else+staticconstintmtrr_enabled;staticinlineu8mtrr_type_lookup(u64addr,u64end){/*
@@ -880,7 +880,7 @@ int __init mtrr_trim_uncached_memory(unsigned long end_pfn)*Makesureweonlytrimuncachablememoryonmachinesthat*supporttheIntelMTRRarchitecture:*/-if(!is_cpu(INTEL)||disable_mtrr_trim)+if(!is_cpu(INTEL)||disable_mtrr_trim||!mtrr_enabled)return0;rdmsr(MSR_MTRRdefType,def,dummy);
@@ -116,7 +116,8 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)u8prev_match,curr_match;*repeat=0;-if(!mtrr_state_set)+/* generic_mtrr_ops is only set for generic_mtrr_ops */+if(!mtrr_state_set||!mtrr_enabled)return0xFF;if(!mtrr_state.enabled)
@@ -84,6 +85,9 @@ static int have_wrcomb(void){structpci_dev*dev;+if(!mtrr_enabled)+return0;+dev=pci_get_class(PCI_CLASS_BRIDGE_HOST<<8,NULL);if(dev!=NULL){/*
@@ -286,7 +290,7 @@ int mtrr_add_page(unsigned long base, unsigned long size,inti,replace,error;mtrr_typeltype;-if(!mtrr_if)+if(!mtrr_enabled)return-ENXIO;error=mtrr_if->validate_add_page(base,size,type);
@@ -388,6 +392,8 @@ int mtrr_add_page(unsigned long base, unsigned long size,staticintmtrr_check(unsignedlongbase,unsignedlongsize){+if(!mtrr_enabled)+return-ENODEV;if((base&(PAGE_SIZE-1))||(size&(PAGE_SIZE-1))){pr_warning("mtrr: size and base must be multiples of 4 kiB\n");pr_debug("mtrr: size: 0x%lx base: 0x%lx\n",size,base);
@@ -463,8 +469,8 @@ int mtrr_del_page(int reg, unsigned long base, unsigned long size)unsignedlonglbase,lsize;interror=-EINVAL;-if(!mtrr_if)-return-ENXIO;+if(!mtrr_enabled)+return-ENODEV;max=num_var_ranges;/* No CPU hotplug when we change MTRR entries */
@@ -523,6 +529,8 @@ int mtrr_del_page(int reg, unsigned long base, unsigned long size)*/intmtrr_del(intreg,unsignedlongbase,unsignedlongsize){+if(!mtrr_enabled)+return-ENODEV;if(mtrr_check(base,size))return-EINVAL;returnmtrr_del_page(reg,base>>PAGE_SHIFT,size>>PAGE_SHIFT);
@@ -545,7 +553,7 @@ int arch_phys_wc_add(unsigned long base, unsigned long size){intret;-if(pat_enabled)+if(pat_enabled||!mtrr_enabled)return0;/* Success! (We don't need to do anything.) */ret=mtrr_add(base,size,MTRR_TYPE_WRCOMB,true);
@@ -744,12 +753,13 @@ void __init mtrr_bp_init(void)mtrr_if->set_all();}}-}+}else+pr_info("mtrr: system does not support MTRR\n");
'pr_warn' ?
quoted
}
void mtrr_ap_init(void)
{
- if (!use_intel() || mtrr_aps_delayed_init)
+ if (!use_intel() || mtrr_aps_delayed_init || !mtrr_enabled)
return;
/*
* Ideally we should hold mtrr_mutex here to avoid mtrr entries
@@ -774,6 +784,9 @@ void mtrr_save_state(void) { int first_cpu;+ if (!mtrr_enabled)+ return;+ get_online_cpus(); first_cpu = cpumask_first(cpu_online_mask); smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
@@ -782,7 +795,7 @@ void mtrr_save_state(void) void set_mtrr_aps_delayed_init(void) {- if (!use_intel())+ if (!use_intel() || !mtrr_enabled) return; mtrr_aps_delayed_init = true;
@@ -810,7 +823,7 @@ void mtrr_aps_init(void) void mtrr_bp_restore(void) {- if (!use_intel())+ if (!use_intel() || !mtrr_enabled) return; mtrr_if->set_all();
@@ -818,7 +831,7 @@ void mtrr_bp_restore(void) static int __init mtrr_init_finialize(void) {- if (!mtrr_if)+ if (!mtrr_enabled) return 0; if (use_intel()) {--
From: Luis R. Rodriguez <hidden> Date: 2015-03-26 23:36:03
On Wed, Mar 25, 2015 at 03:59:41PM -0400, Konrad Rzeszutek Wilk wrote:
On Fri, Mar 20, 2015 at 04:17:52PM -0700, Luis R. Rodriguez wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
It is possible to enable CONFIG_MTRR and up with it
disabled at run time and yet CONFIG_X86_PAT continues
to kick through fully functionally. This can happen
s/fully/full/ ?
I'll rephrase this to:
---
It is possible to enable CONFIG_MTRR and up with it
disabled at run time and yet CONFIG_X86_PAT continues
to kick through with all functionally enabled. This
can happen for instance on Xen where MTRR is not
supported but PAT is, this can happen now on Linux as
of commit 47591df50 by Juergen introduced as of v3.19.
---
Which BTW I had also mentioned on the cover letter that
this is a good time to address if we want to make PAT
then a first class citizen, to detangle it from depending
on MTRR. If so I can do that later.
quoted
Technically we should assume the proper CPU
bits would be set to disable MTRR but we can't
always rely on this. At least on the Xen Hypervisor
for instance only X86_FEATURE_MTRR was disabled
as of Xen 4.4 through Xen commit 586ab6a [0],
but not X86_FEATURE_K6_MTRR, X86_FEATURE_CENTAUR_MCR,
or X86_FEATURE_CYRIX_ARR for instance.
Oh, could you send an patch for that to Xen please?
Done.
quoted
x86 mtrr code relies on quite a bit of checks for
mtrr_if being set to check to see if MTRR did get
set up, instead of using that lets provide a generic
setter which when set we know MTRR is enabled. This
s/we know MTRR is enabled/will let us know that MTRR is enabled/
On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez [off-list ref] wrote:
I'll rephrase this to:
---
It is possible to enable CONFIG_MTRR and up with it
disabled at run time and yet CONFIG_X86_PAT continues
to kick through with all functionally enabled. This
can happen for instance on Xen where MTRR is not
supported but PAT is, this can happen now on Linux as
of commit 47591df50 by Juergen introduced as of v3.19.
I still can't parse this. What does "up with it disabled at run time"
mean? And "... continues to kick through"? Probably some idiomatic
usage I'm just too old to understand :)
Please use the conventional citation format:
47591df50512 ("xen: Support Xen pv-domains using PAT")
A one-character typo in a SHA1 makes it completely useless, so it's
nice to have the summary line both for readability and a bit of
redundancy.
Bjorn
From: Luis R. Rodriguez <hidden> Date: 2015-04-02 20:21:18
On Thu, Apr 2, 2015 at 1:13 PM, Bjorn Helgaas [off-list ref] wrote:
On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
I'll rephrase this to:
---
It is possible to enable CONFIG_MTRR and up with it
disabled at run time and yet CONFIG_X86_PAT continues
to kick through with all functionally enabled. This
can happen for instance on Xen where MTRR is not
supported but PAT is, this can happen now on Linux as
of commit 47591df50 by Juergen introduced as of v3.19.
I still can't parse this. What does "up with it disabled at run time"
mean?
It means that technically even if your CPU/BIOS/system did support
MTRR if you use a kernel with MTRR support enabled you might end up
with a situation where under one situation MTRR might be enabled and
at another run time scenario with the same exact kernel and system you
will end up with MTRR disabled. Such is the case for example when
booting with Xen, which disables the CPU bits on the hypervisor code.
If you boot the same system without Xen you'll get MTRR.
And "... continues to kick through"? Probably some idiomatic
usage I'm just too old to understand :)
That means for example that in both the above circumstances even if
MTRR went disabled at run time with Xen, the kernel went through with
getting PAT enabled.
Please use the conventional citation format:
47591df50512 ("xen: Support Xen pv-domains using PAT")
A one-character typo in a SHA1 makes it completely useless, so it's
nice to have the summary line both for readability and a bit of
redundancy.
On Thu, Apr 2, 2015 at 3:20 PM, Luis R. Rodriguez [off-list ref] wrote:
On Thu, Apr 2, 2015 at 1:13 PM, Bjorn Helgaas [off-list ref] wrote:
quoted
On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
I'll rephrase this to:
---
It is possible to enable CONFIG_MTRR and up with it
disabled at run time and yet CONFIG_X86_PAT continues
to kick through with all functionally enabled. This
can happen for instance on Xen where MTRR is not
supported but PAT is, this can happen now on Linux as
of commit 47591df50 by Juergen introduced as of v3.19.
I still can't parse this. What does "up with it disabled at run time"
mean?
It means that technically even if your CPU/BIOS/system did support
MTRR if you use a kernel with MTRR support enabled you might end up
with a situation where under one situation MTRR might be enabled and
at another run time scenario with the same exact kernel and system you
will end up with MTRR disabled. Such is the case for example when
booting with Xen, which disables the CPU bits on the hypervisor code.
If you boot the same system without Xen you'll get MTRR.
Your text is missing some words. You seem to be using "up" as a verb,
but it's not a verb. Maybe you meant "end up"? Even then, it
wouldn't make sense for CONFIG_MTRR to be "disabled at run time"
because CONFIG_MTRR is a compile-time switch. The MTRR
*functionality* could certainly be disabled at run-time, but not
CONFIG_MTRR itself.
quoted
And "... continues to kick through"? Probably some idiomatic
usage I'm just too old to understand :)
That means for example that in both the above circumstances even if
MTRR went disabled at run time with Xen, the kernel went through with
getting PAT enabled.
"CONFIG_X86_PAT continues to kick through" doesn't seem a very precise
way of describing this. But maybe it's enough for experts in this
area (which I'm not).
Bjorn
From: Luis R. Rodriguez <hidden> Date: 2015-04-02 21:03:00
On Thu, Apr 02, 2015 at 03:28:51PM -0500, Bjorn Helgaas wrote:
On Thu, Apr 2, 2015 at 3:20 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
On Thu, Apr 2, 2015 at 1:13 PM, Bjorn Helgaas [off-list ref] wrote:
quoted
On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
I'll rephrase this to:
---
It is possible to enable CONFIG_MTRR and up with it
disabled at run time and yet CONFIG_X86_PAT continues
to kick through with all functionally enabled. This
can happen for instance on Xen where MTRR is not
supported but PAT is, this can happen now on Linux as
of commit 47591df50 by Juergen introduced as of v3.19.
I still can't parse this. What does "up with it disabled at run time"
mean?
It means that technically even if your CPU/BIOS/system did support
MTRR if you use a kernel with MTRR support enabled you might end up
with a situation where under one situation MTRR might be enabled and
at another run time scenario with the same exact kernel and system you
will end up with MTRR disabled. Such is the case for example when
booting with Xen, which disables the CPU bits on the hypervisor code.
If you boot the same system without Xen you'll get MTRR.
Your text is missing some words. You seem to be using "up" as a verb,
but it's not a verb. Maybe you meant "end up"?
Indeed.
Even then, it
wouldn't make sense for CONFIG_MTRR to be "disabled at run time"
because CONFIG_MTRR is a compile-time switch. The MTRR
*functionality* could certainly be disabled at run-time, but not
CONFIG_MTRR itself.
I'll clarify.
quoted
quoted
And "... continues to kick through"? Probably some idiomatic
usage I'm just too old to understand :)
That means for example that in both the above circumstances even if
MTRR went disabled at run time with Xen, the kernel went through with
getting PAT enabled.
"CONFIG_X86_PAT continues to kick through" doesn't seem a very precise
way of describing this. But maybe it's enough for experts in this
area (which I'm not).
I've rephrased this to:
---
It is possible to enable CONFIG_MTRR and CONFIG_X86_PAT
and end up with a system with MTRR functionality disabled
PAT functionality enabled. This can happen for instance
on Xen where MTRR is not supported but PAT is. This can
happen on Linux as of commit 47591df50 ("xen: Support Xen
pv-domains using PAT") by Juergen, introduced as of v3.19.
---
Luis
On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
:
quoted hunk
@@ -734,6 +742,7 @@ void __init mtrr_bp_init(void) } if (mtrr_if) {+ mtrr_enabled = true; set_num_var_ranges(); init_table(); if (use_intel()) {
get_mtrr_state();
After setting mtrr_enabled to true, get_mtrr_state() reads
MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
MTRRs are enabled or not on the system. So, potentially, we could have
a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
to disabled when MTRRs are disabled by BIOS.
Thanks,
-Toshi
ps.
I recently cleaned up this part of the MTRR code in the patch below,
which is currently available in the -mm & -next trees.
https://lkml.org/lkml/2015/3/24/1063
From: Luis R. Rodriguez <hidden> Date: 2015-03-27 23:56:37
On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
:
quoted
@@ -734,6 +742,7 @@ void __init mtrr_bp_init(void) } if (mtrr_if) {+ mtrr_enabled = true; set_num_var_ranges(); init_table(); if (use_intel()) {
get_mtrr_state();
After setting mtrr_enabled to true, get_mtrr_state() reads
MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
MTRRs are enabled or not on the system. So, potentially, we could have
a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
to disabled when MTRRs are disabled by BIOS.
Thanks for the review, in this case then we should update mtrr_enabled to false.
ps.
I recently cleaned up this part of the MTRR code in the patch below,
which is currently available in the -mm & -next trees.
https://lkml.org/lkml/2015/3/24/1063
Great I will rebase and work with that and try to address this
consideration you have raised.
Luis
From: Luis R. Rodriguez <hidden> Date: 2015-04-02 21:49:20
On Sat, Mar 28, 2015 at 12:56:30AM +0100, Luis R. Rodriguez wrote:
On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
quoted
On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
:
quoted
@@ -734,6 +742,7 @@ void __init mtrr_bp_init(void) } if (mtrr_if) {+ mtrr_enabled = true; set_num_var_ranges(); init_table(); if (use_intel()) {
get_mtrr_state();
After setting mtrr_enabled to true, get_mtrr_state() reads
MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
MTRRs are enabled or not on the system. So, potentially, we could have
a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
to disabled when MTRRs are disabled by BIOS.
Thanks for the review, in this case then we should update mtrr_enabled to false.
quoted
ps.
I recently cleaned up this part of the MTRR code in the patch below,
which is currently available in the -mm & -next trees.
https://lkml.org/lkml/2015/3/24/1063
Great I will rebase and work with that and try to address this
consideration you have raised.
OK I'll mesh in this change as well in my next respin:
@@ -438,7 +438,7 @@ static void __init print_mtrr_state(void)}/* Grab all of the MTRR state for this CPU into *state */-void__initget_mtrr_state(void)+bool__initget_mtrr_state(void){structmtrr_var_range*vrs;unsignedlongflags;
@@ -482,6 +482,8 @@ void __init get_mtrr_state(void)post_set();local_irq_restore(flags);++return!!mtrr_state.enabled;}/* Some BIOS's are messed up and don't set all MTRRs the same! */
On Thu, 2015-04-02 at 23:49 +0200, Luis R. Rodriguez wrote:
quoted hunk
On Sat, Mar 28, 2015 at 12:56:30AM +0100, Luis R. Rodriguez wrote:
quoted
On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
quoted
On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
:
quoted
@@ -734,6 +742,7 @@ void __init mtrr_bp_init(void) } if (mtrr_if) {+ mtrr_enabled = true; set_num_var_ranges(); init_table(); if (use_intel()) {
get_mtrr_state();
After setting mtrr_enabled to true, get_mtrr_state() reads
MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
MTRRs are enabled or not on the system. So, potentially, we could have
a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
to disabled when MTRRs are disabled by BIOS.
Thanks for the review, in this case then we should update mtrr_enabled to false.
quoted
ps.
I recently cleaned up this part of the MTRR code in the patch below,
which is currently available in the -mm & -next trees.
https://lkml.org/lkml/2015/3/24/1063
Great I will rebase and work with that and try to address this
consideration you have raised.
OK I'll mesh in this change as well in my next respin:
@@ -438,7 +438,7 @@ static void __init print_mtrr_state(void)}/* Grab all of the MTRR state for this CPU into *state */-void__initget_mtrr_state(void)+bool__initget_mtrr_state(void){structmtrr_var_range*vrs;unsignedlongflags;
This should be:
return mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED;
because the MTRR_STATE_MTRR_FIXED_ENABLED flag is ignored when the
MTRR_STATE_MTRR_ENABLED flag is clear.
Thanks,
-Toshi
From: Luis R. Rodriguez <hidden> Date: 2015-04-03 01:09:08
On Thu, Apr 02, 2015 at 05:52:16PM -0600, Toshi Kani wrote:
On Thu, 2015-04-02 at 23:49 +0200, Luis R. Rodriguez wrote:
quoted
On Sat, Mar 28, 2015 at 12:56:30AM +0100, Luis R. Rodriguez wrote:
quoted
On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
quoted
On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
:
quoted
@@ -734,6 +742,7 @@ void __init mtrr_bp_init(void) } if (mtrr_if) {+ mtrr_enabled = true; set_num_var_ranges(); init_table(); if (use_intel()) {
get_mtrr_state();
After setting mtrr_enabled to true, get_mtrr_state() reads
MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
MTRRs are enabled or not on the system. So, potentially, we could have
a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
to disabled when MTRRs are disabled by BIOS.
Thanks for the review, in this case then we should update mtrr_enabled to false.
quoted
ps.
I recently cleaned up this part of the MTRR code in the patch below,
which is currently available in the -mm & -next trees.
https://lkml.org/lkml/2015/3/24/1063
Great I will rebase and work with that and try to address this
consideration you have raised.
OK I'll mesh in this change as well in my next respin:
@@ -438,7 +438,7 @@ static void __init print_mtrr_state(void)}/* Grab all of the MTRR state for this CPU into *state */-void__initget_mtrr_state(void)+bool__initget_mtrr_state(void){structmtrr_var_range*vrs;unsignedlongflags;
This should be:
return mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED;
because the MTRR_STATE_MTRR_FIXED_ENABLED flag is ignored when the
MTRR_STATE_MTRR_ENABLED flag is clear.
Thanks, I've used
return !!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED);
Amended.
Luis
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-03-20 23:50:16
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
From: "Luis R. Rodriguez" <redacted>
We have devm_ioremap_nocache() but no devm_ioremap_wc()
so add that. This will be used later.
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
From: Luis R. Rodriguez <hidden> Date: 2015-03-25 19:51:04
On Fri, Mar 20, 2015 at 04:49:51PM -0700, Andy Lutomirski wrote:
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
We have devm_ioremap_nocache() but no devm_ioremap_wc()
so add that. This will be used later.
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
From: Luis R. Rodriguez <hidden> Date: 2015-03-20 23:29:45
From: "Luis R. Rodriguez" <redacted>
This lets drivers take advanate of PAT when available. This
should help with the transition of converting video drivers over
to ioremap_wc() to help with the goal of eventually using
_PAGE_CACHE_UC over _PAGE_CACHE_UC_MINUS on x86 on
ioremap_nocache() (de33c442e)
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
drivers/pci/pci.c | 14 ++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 15 insertions(+)
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-03-20 23:50:59
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted hunk
From: "Luis R. Rodriguez" <redacted>
This lets drivers take advanate of PAT when available. This
should help with the transition of converting video drivers over
to ioremap_wc() to help with the goal of eventually using
_PAGE_CACHE_UC over _PAGE_CACHE_UC_MINUS on x86 on
ioremap_nocache() (de33c442e)
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
drivers/pci/pci.c | 14 ++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 15 insertions(+)
From: Luis R. Rodriguez <hidden> Date: 2015-03-25 20:06:41
On Fri, Mar 20, 2015 at 04:50:32PM -0700, Andy Lutomirski wrote:
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
This lets drivers take advanate of PAT when available. This
should help with the transition of converting video drivers over
to ioremap_wc() to help with the goal of eventually using
_PAGE_CACHE_UC over _PAGE_CACHE_UC_MINUS on x86 on
ioremap_nocache() (de33c442e)
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
drivers/pci/pci.c | 14 ++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 15 insertions(+)
Sure, they are equivalent however this follows the same exact style as
pci_ioremap_bar() so if we change this one might as well change the style of
pci_ioremap_bar() as well. Let me know if there is any preference. I personally
don't mind the extra line as it shortens the check.
Luis
From: Konrad Rzeszutek Wilk <hidden> Date: 2015-03-25 20:04:33
On Fri, Mar 20, 2015 at 04:17:54PM -0700, Luis R. Rodriguez wrote:
From: "Luis R. Rodriguez" <redacted>
This lets drivers take advanate of PAT when available. This
s/advanate/advantage/
should help with the transition of converting video drivers over
to ioremap_wc() to help with the goal of eventually using
_PAGE_CACHE_UC over _PAGE_CACHE_UC_MINUS on x86 on
ioremap_nocache() (de33c442e)
Please mention the title of the patch too:
"x86 PAT: fix performance drop for glx, use UC minus for ioremap(), ioremap_nocache() and pci_mmap_page_range()"
quoted hunk
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
drivers/pci/pci.c | 14 ++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 15 insertions(+)
Would it be better to use dev_warn ? That way you can see which BDF it is?
Thought WARN will give a nice stack-trace that should easily point to the
driver so perhaps not.. Either way - up to you.
From: Luis R. Rodriguez <hidden> Date: 2015-03-25 20:39:24
On Wed, Mar 25, 2015 at 04:03:46PM -0400, Konrad Rzeszutek Wilk wrote:
On Fri, Mar 20, 2015 at 04:17:54PM -0700, Luis R. Rodriguez wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
This lets drivers take advanate of PAT when available. This
s/advanate/advantage/
Amended.
quoted
should help with the transition of converting video drivers over
to ioremap_wc() to help with the goal of eventually using
_PAGE_CACHE_UC over _PAGE_CACHE_UC_MINUS on x86 on
ioremap_nocache() (de33c442e)
Please mention the title of the patch too:
"x86 PAT: fix performance drop for glx, use UC minus for ioremap(), ioremap_nocache() and pci_mmap_page_range()"
Added.
quoted
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
drivers/pci/pci.c | 14 ++++++++++++++
include/linux/pci.h | 1 +
2 files changed, 15 insertions(+)
Would it be better to use dev_warn ? That way you can see which BDF it is?
Thought WARN will give a nice stack-trace that should easily point to the
driver so perhaps not.. Either way - up to you.
I'm sticking to the style and use as with pci_ioremap_bar(). Whatever we pick
we should make both use the same. More information is always better and
since we do have dev_warn(), it would be nice to use that however within
its use on both pci_ioremap_wc_bar() and pci_ioremap_bar() we have
a use of the pdev with pci_resource_flags() and I believe if pdev is NULL
we'd get a NULL dereference (dev_driver_string() is used), so it would
seem it might be best to stick with a simple WARN_ON(). Arjan, any
preference? Obviously if pdev is NULL your driver is dumb but as folks
develop drivers this should be expected.
Luis
@@ -15,9 +15,13 @@ struct pci_dev;#ifdef CONFIG_PCI/* Create a virtual mapping cookie for a PCI BAR (memory or IO) */externvoid__iomem*pci_iomap(structpci_dev*dev,intbar,unsignedlongmax);+externvoid__iomem*pci_iomap_wc(structpci_dev*dev,intbar,unsignedlongmax);externvoid__iomem*pci_iomap_range(structpci_dev*dev,intbar,unsignedlongoffset,unsignedlongmaxlen);+externvoid__iomem*pci_iomap_wc_range(structpci_dev*dev,intbar,+unsignedlongoffset,+unsignedlongmaxlen);/* Create a virtual mapping cookie for a port on a given PCI device.*Donotcallthisdirectly,itexiststomakeiteasierforarchitectures*tooverride*/
@@ -34,12 +38,22 @@ static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned lonreturnNULL;}+staticinlinevoid__iomem*pci_iomap_wc(structpci_dev*dev,intbar,unsignedlongmax)+{+returnNULL;+}staticinlinevoid__iomem*pci_iomap_range(structpci_dev*dev,intbar,unsignedlongoffset,unsignedlongmaxlen){returnNULL;}+staticinlinevoid__iomem*pci_iomap_wc_range(structpci_dev*dev,intbar,+unsignedlongoffset,+unsignedlongmaxlen)+{+returnNULL;+}#endif#endif /* __ASM_GENERIC_IO_H */
Hi Luis,
This seems OK to me, but I'm curious about a few things.
On Fri, Mar 20, 2015 at 6:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
From: "Luis R. Rodriguez" <redacted>
This allows drivers to take advantage of write-combining
when possible. Ideally we'd have pci_read_bases() just
peg an IORESOURCE_WC flag for us
We do set IORESOURCE_PREFETCH. Do you mean something different?
but where exactly
video devices memory lie varies *largely* and at times things
are mixed with MMIO registers, sometimes we can address
the changes in drivers, other times the change requires
intrusive changes.
What does a video device address have to do with this? I do see that
if a BAR maps only a frame buffer, the device might be able to mark it
prefetchable, while if the BAR mapped both a frame buffer and some
registers, it might not be able to make it prefetchable. But that
doesn't seem like it depends on the *address*.
pci_iomap_range() already makes a cacheable mapping if
IORESOURCE_CACHEABLE; I'm guessing that you would like it to
automatically use WC if the BAR if IORESOURCE_PREFETCH, e.g.,
if (flags & IORESOURCE_CACHEABLE)
return ioremap(start, len);
if (flags & IORESOURCE_PREFETCH)
return ioremap_wc(start, len);
return ioremap_nocache(start, len);
Is there a reason not to do that?
Although there is also arch_phys_wc_add() that makes use of
architecture specific write-combinging alternatives (MTRR on
x86 when a system does not have PAT) we void polluting
pci_iomap() space with it and force drivers and subsystems
that want to use it to be explicit.
There are a few motivations for this:
a) Take advantage of PAT when available
b) Help bury MTRR code away, MTRR is architecture specific and on
x86 its replaced by PAT
c) Help with the goal of eventually using _PAGE_CACHE_UC over
_PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (de33c442e)
...
+void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
+ int bar,
+ unsigned long offset,
+ unsigned long maxlen)
+{
+ resource_size_t start = pci_resource_start(dev, bar);
+ resource_size_t len = pci_resource_len(dev, bar);
+ unsigned long flags = pci_resource_flags(dev, bar);
+
+ if (len <= offset || !start)
+ return NULL;
+ len -= offset;
+ start += offset;
+ if (maxlen && len > maxlen)
+ len = maxlen;
+ if (flags & IORESOURCE_IO)
+ return __pci_ioport_map(dev, start, len);
+ if (flags & IORESOURCE_MEM)
Should we log a note in dmesg if the BAR is *not* IORESOURCE_PREFETCH?
I know the driver might know it's safe even if the device didn't mark
the BAR as prefetchable, but it does seem like an easy way for a
driver to shoot itself in the foot.
From: Luis R. Rodriguez <hidden> Date: 2015-03-26 03:01:10
On Mon, Mar 23, 2015 at 12:20:47PM -0500, Bjorn Helgaas wrote:
Hi Luis,
This seems OK to me,
Great.
but I'm curious about a few things.
On Fri, Mar 20, 2015 at 6:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
This allows drivers to take advantage of write-combining
when possible. Ideally we'd have pci_read_bases() just
peg an IORESOURCE_WC flag for us
We do set IORESOURCE_PREFETCH. Do you mean something different?
I did not think we had a WC IORESOURCE flag. Are you saying that we can use
IORESOURCE_PREFETCH for that purpose? If so then great. As I read a PCI BAR
can have PCI_BASE_ADDRESS_MEM_PREFETCH and when that's the case we peg
IORESOURCE_PREFETCH. That seems to be what I want indeed. Questions below.
quoted
but where exactly
video devices memory lie varies *largely* and at times things
are mixed with MMIO registers, sometimes we can address
the changes in drivers, other times the change requires
intrusive changes.
What does a video device address have to do with this? I do see that
if a BAR maps only a frame buffer, the device might be able to mark it
prefetchable, while if the BAR mapped both a frame buffer and some
registers, it might not be able to make it prefetchable. But that
doesn't seem like it depends on the *address*.
I meant the offsets for each of those, either registers or framebuffer,
and that typically they are mixed (primarily on older devices), so indeed your
summary of the problem is what I meant. Let's remember that we are trying to
take advantage of PAT here when available and avoid MTRR in that case, do we
know that the same PCI BARs that have always historically used MTRRs had
IORESOURCE_PREFETCH set, is that a fair assumption ? I realize they are
different things -- but its precisely why I ask.
pci_iomap_range() already makes a cacheable mapping if
IORESOURCE_CACHEABLE; I'm guessing that you would like it to
automatically use WC if the BAR if IORESOURCE_PREFETCH, e.g.,
if (flags & IORESOURCE_CACHEABLE)
return ioremap(start, len);
if (flags & IORESOURCE_PREFETCH)
return ioremap_wc(start, len);
return ioremap_nocache(start, len);
Indeed, that's exactly what I think we should strive towards.
Is there a reason not to do that?
This depends on the exact defintion of IORESOURCE_PREFETCH and
PCI_BASE_ADDRESS_MEM_PREFETCH and how they are used all over and
accross *all devices*. This didn't look promising for starters:
include/uapi/linux/pci_regs.h:#define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08 /* prefetchable? */
PCI_BASE_ADDRESS_MEM_PREFETCH seems to be BAR specific, so a few questions:
1) Can we rest assured for instance that if we check for
PCI_BASE_ADDRESS_MEM_PREFETCH and if set that it will *only* be set on a full
PCI BAR if the full PCI BAR does want WC? If not this can regress
functionality. That seems risky. It however would not be risky if we used
another API that did look for IORESOURCE_PREFETCH and if so use ioremap_wc() --
that way only drivers we know that do use the full PCI bar would use this API.
There's a bit of a problem with this though:
2) Do we know that if a *full PCI BAR* is used for WC that
PCI_BASE_ADDRESS_MEM_PREFETCH *was* definitely set for the PCI BAR? If so then
the API usage would be restricted only to devices that we know *do* adhere to
this. That reduces the possible uses for older drivers and can create
regressions if used loosely without verification... but..
3) If from now on we get folks to commit to uset PCI_BASE_ADDRESS_MEM_PREFETCH
for full PCI BARs that do want WC perhaps newer devices / drivers will use
this very consistently ? Can we bank on that and is it worth it ?
4) If a PCI BAR *does not* have PCI_BASE_ADDRESS_MEM_PREFETCH do we know it
must not never want WC ?
If we don't have certainty on any of the above I'm afraid we can't do much
right now but perhaps we can push towards better use of PCI_BASE_ADDRESS_MEM_PREFETCH
and hope folks will only use this for the full PCI BAR only if WC is desired.
Thoughts?
quoted
Although there is also arch_phys_wc_add() that makes use of
architecture specific write-combinging alternatives (MTRR on
x86 when a system does not have PAT) we void polluting
pci_iomap() space with it and force drivers and subsystems
that want to use it to be explicit.
There are a few motivations for this:
a) Take advantage of PAT when available
b) Help bury MTRR code away, MTRR is architecture specific and on
x86 its replaced by PAT
c) Help with the goal of eventually using _PAGE_CACHE_UC over
_PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (de33c442e)
...
quoted
+void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
+ int bar,
+ unsigned long offset,
+ unsigned long maxlen)
+{
+ resource_size_t start = pci_resource_start(dev, bar);
+ resource_size_t len = pci_resource_len(dev, bar);
+ unsigned long flags = pci_resource_flags(dev, bar);
+
+ if (len <= offset || !start)
+ return NULL;
+ len -= offset;
+ start += offset;
+ if (maxlen && len > maxlen)
+ len = maxlen;
+ if (flags & IORESOURCE_IO)
+ return __pci_ioport_map(dev, start, len);
+ if (flags & IORESOURCE_MEM)
Should we log a note in dmesg if the BAR is *not* IORESOURCE_PREFETCH?
I know the driver might know it's safe even if the device didn't mark
the BAR as prefetchable, but it does seem like an easy way for a
driver to shoot itself in the foot.
You tell me. I would fear this may not be consistent and we'd end up
having bug reports open for something that has historically been a
non-issue. The above questions can help us gauge the risk of this.
Luis
On Mon, 2015-03-23 at 12:20 -0500, Bjorn Helgaas wrote:
:
pci_iomap_range() already makes a cacheable mapping if
IORESOURCE_CACHEABLE; I'm guessing that you would like it to
automatically use WC if the BAR if IORESOURCE_PREFETCH, e.g.,
if (flags & IORESOURCE_CACHEABLE)
return ioremap(start, len);
Is this supposed to be ioremap_cache()? ioremap() is the same as
ioremap_nocache() at least on x86 per arch/x86/include/asm/io.h.
if (flags & IORESOURCE_PREFETCH)
return ioremap_wc(start, len);
return ioremap_nocache(start, len);
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2015-04-21 19:25:19
On Mon, Mar 23, 2015 at 12:20:47PM -0500, Bjorn Helgaas wrote:
pci_iomap_range() already makes a cacheable mapping if
IORESOURCE_CACHEABLE; I'm guessing that you would like it to
automatically use WC if the BAR if IORESOURCE_PREFETCH, e.g.,
if (flags & IORESOURCE_CACHEABLE)
return ioremap(start, len);
if (flags & IORESOURCE_PREFETCH)
return ioremap_wc(start, len);
return ioremap_nocache(start, len);
Is there a reason not to do that?
I think that's wrong and will break a bunch of things.
PCI prefetch bit merely means bridges can combine writes and prefetch
reads. Prefetch does not affect ordering rules and does not allow
writes to be collapsed.
WC is stronger: it allows collapsing and changes ordering rules.
WC can also hurt latency as small writes are buffered.
To summarise, driver needs to know what it's doing,
we can't set WC in the pci core automatically.
--
MST
From: Konrad Rzeszutek Wilk <hidden> Date: 2015-03-25 20:11:07
On Fri, Mar 20, 2015 at 04:17:55PM -0700, Luis R. Rodriguez wrote:
From: "Luis R. Rodriguez" <redacted>
This allows drivers to take advantage of write-combining
when possible. Ideally we'd have pci_read_bases() just
peg an IORESOURCE_WC flag for us but where exactly
video devices memory lie varies *largely* and at times things
are mixed with MMIO registers, sometimes we can address
the changes in drivers, other times the change requires
intrusive changes.
Although there is also arch_phys_wc_add() that makes use of
architecture specific write-combinging alternatives (MTRR on
combinging?
quoted hunk
x86 when a system does not have PAT) we void polluting
pci_iomap() space with it and force drivers and subsystems
that want to use it to be explicit.
There are a few motivations for this:
a) Take advantage of PAT when available
b) Help bury MTRR code away, MTRR is architecture specific and on
x86 its replaced by PAT
c) Help with the goal of eventually using _PAGE_CACHE_UC over
_PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (de33c442e)
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: venkatesh.pallipadi@intel.com
Cc: Stefan Bader <redacted>
Cc: konrad.wilk@oracle.com
Cc: ville.syrjala@linux.intel.com
Cc: david.vrabel@citrix.com
Cc: jbeulich@suse.com
Cc: toshi.kani@hp.com
Cc: Roger Pau Monné <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xensource.com
Signed-off-by: Luis R. Rodriguez <redacted>
---
include/asm-generic/pci_iomap.h | 14 ++++++++++
lib/pci_iomap.c | 61 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+)
@@ -15,9 +15,13 @@ struct pci_dev;#ifdef CONFIG_PCI/* Create a virtual mapping cookie for a PCI BAR (memory or IO) */externvoid__iomem*pci_iomap(structpci_dev*dev,intbar,unsignedlongmax);+externvoid__iomem*pci_iomap_wc(structpci_dev*dev,intbar,unsignedlongmax);externvoid__iomem*pci_iomap_range(structpci_dev*dev,intbar,unsignedlongoffset,unsignedlongmaxlen);+externvoid__iomem*pci_iomap_wc_range(structpci_dev*dev,intbar,+unsignedlongoffset,+unsignedlongmaxlen);/* Create a virtual mapping cookie for a port on a given PCI device.*Donotcallthisdirectly,itexiststomakeiteasierforarchitectures*tooverride*/
@@ -34,12 +38,22 @@ static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned lonreturnNULL;}+staticinlinevoid__iomem*pci_iomap_wc(structpci_dev*dev,intbar,unsignedlongmax)+{+returnNULL;+}staticinlinevoid__iomem*pci_iomap_range(structpci_dev*dev,intbar,unsignedlongoffset,unsignedlongmaxlen){returnNULL;}+staticinlinevoid__iomem*pci_iomap_wc_range(structpci_dev*dev,intbar,+unsignedlongoffset,+unsignedlongmaxlen)+{+returnNULL;+}#endif#endif /* __ASM_GENERIC_IO_H */
+ * */
+void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
+ int bar,
+ unsigned long offset,
+ unsigned long maxlen)
+{
+ resource_size_t start = pci_resource_start(dev, bar);
+ resource_size_t len = pci_resource_len(dev, bar);
+ unsigned long flags = pci_resource_flags(dev, bar);
+
+ if (len <= offset || !start)
+ return NULL;
+ len -= offset;
+ start += offset;
+ if (maxlen && len > maxlen)
+ len = maxlen;
+ if (flags & IORESOURCE_IO)
+ return __pci_ioport_map(dev, start, len);
+ if (flags & IORESOURCE_MEM)
+ return ioremap_wc(start, len);
+ /* What? */
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(pci_iomap_wc_range);
+
+/**
* pci_iomap - create a virtual mapping cookie for a PCI BAR
* @dev: PCI device that owns the BAR
* @bar: BAR number
@@ -70,4 +110,25 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen) return pci_iomap_range(dev, bar, 0, maxlen); } EXPORT_SYMBOL(pci_iomap);++/**+ * pci_iomap_wc - create a virtual WC mapping cookie for a PCI BAR+ * @dev: PCI device that owns the BAR+ * @bar: BAR number+ * @maxlen: length of the memory to map+ *+ * Using this function you will get a __iomem address to your device BAR.+ * You can access it using ioread*() and iowrite*(). These functions hide+ * the details if this is a MMIO or PIO address space and will just do what+ * you expect from them in the correct way. When possible write combining+ * is used.+ *+ * @maxlen specifies the maximum length to map. If you want to get access to+ * the complete BAR without checking for its length first, pass %0 here.+ * */+void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)+{+ return pci_iomap_wc_range(dev, bar, 0, maxlen);+}+EXPORT_SYMBOL_GPL(pci_iomap_wc); #endif /* CONFIG_PCI */
From: Luis R. Rodriguez <hidden> Date: 2015-03-27 18:40:34
On Wed, Mar 25, 2015 at 04:07:43PM -0400, Konrad Rzeszutek Wilk wrote:
On Fri, Mar 20, 2015 at 04:17:55PM -0700, Luis R. Rodriguez wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
This allows drivers to take advantage of write-combining
when possible. Ideally we'd have pci_read_bases() just
peg an IORESOURCE_WC flag for us but where exactly
video devices memory lie varies *largely* and at times things
are mixed with MMIO registers, sometimes we can address
the changes in drivers, other times the change requires
intrusive changes.
Although there is also arch_phys_wc_add() that makes use of
architecture specific write-combinging alternatives (MTRR on
This copies the syntax of pci_iomap_range() which also uses %0, and as per
Documentation/kernel-doc-nano-HOWTO.txt % is used for constants. See:
scripts/kernel-doc -man -function pci_iomap_range lib/pci_iomap.c | nroff -man | less
Luis
From: Luis R. Rodriguez <hidden> Date: 2015-03-20 23:34:13
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata, another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
arch/x86/include/asm/io.h | 4 ++++
arch/x86/kernel/cpu/mtrr/main.c | 36 +++++++++++++++++++++++++++++-------
include/linux/io.h | 4 ++++
3 files changed, 37 insertions(+), 7 deletions(-)
@@ -538,23 +538,24 @@ int mtrr_del(int reg, unsigned long base, unsigned long size)EXPORT_SYMBOL(mtrr_del);/**-*arch_phys_wc_add-addaWCMTRRandhandleerrorsifPATisunavailable+*__arch_phys_wc_add-addaWCMTRRevenifPATisavailable*@base:Physicalbaseaddress*@size:Sizeofregion*-*IfPATisavailable,thisdoesnothing.IfPATisunavailable,it-*attemptstoaddaWCMTRRcoveringsizebytesstartingatbaseand-*logsanerrorifthisfails.+*WetypicallydonotwanttouseMTRRifPATisavailablebutthere+*aresomedriverswhichrequiresignificantworktogetthistowork+*properly.Thiscallshouldonlybeusedbythosedriverswhereitis+*clearthathardworkisrequiredtomodifythemtousearch_phys_wc_add()**Driversmuststorethereturnvaluetopasstomtrr_del_wc_if_needed,*butdriversshouldnottrytointerpretthatreturnvalue.*/-intarch_phys_wc_add(unsignedlongbase,unsignedlongsize)+int__arch_phys_wc_add(unsignedlongbase,unsignedlongsize){intret;-if(pat_enabled||!mtrr_enabled)-return0;/* Success! (We don't need to do anything.) */+if(!mtrr_enabled)+return0;ret=mtrr_add(base,size,MTRR_TYPE_WRCOMB,true);if(ret<0){
@@ -564,6 +565,27 @@ int arch_phys_wc_add(unsigned long base, unsigned long size)}returnret+MTRR_TO_PHYS_WC_OFFSET;}+EXPORT_SYMBOL_GPL(__arch_phys_wc_add);++/**+*arch_phys_wc_add-addaWCMTRRandhandleerrorsifPATisunavailable+*@base:Physicalbaseaddress+*@size:Sizeofregion+*+*IfPATisavailable,thisdoesnothing.IfPATisunavailable,it+*attemptstoaddaWCMTRRcoveringsizebytesstartingatbaseand+*logsanerrorifthisfails.+*+*Driversmuststorethereturnvaluetopasstomtrr_del_wc_if_needed,+*butdriversshouldnottrytointerpretthatreturnvalue.+*/+intarch_phys_wc_add(unsignedlongbase,unsignedlongsize)+{+if(pat_enabled||!mtrr_enabled)+return0;/* Success! (We don't need to do anything.) */++return__arch_phys_wc_add(base,size);+}EXPORT_SYMBOL(arch_phys_wc_add);/*
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-03-20 23:49:11
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata, another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
What's the exact layout that this helps? I'm sceptical that this can
ever be correct.
Is there some awful driver that has a large ioremap that's supposed to
contain multiple different memtypes? If so, can we ioremap +
set_page_xyz instead?
--Andy
From: Luis R. Rodriguez <hidden> Date: 2015-03-27 19:53:58
On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata, another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
What's the exact layout that this helps? I'm sceptical that this can
ever be correct.
Is there some awful driver that has a large ioremap that's supposed to
contain multiple different memtypes?
Yes, I cc'd you just now on one where I made changes on a driver which uses one
PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
regress those drivers by making the MTRR WC hole trick non functional.
The changes are non trivial and so in this series I supplied changes on
one driver only to show the effort required. The other drivers which
required this were:
Driver File
------------------------------------------------------------
fusion drivers/message/fusion/mptbase.c
ivtv drivers/media/pci/ivtv/ivtvfb.c
ipath drivers/infiniband/hw/ipath/ipath_driver.c
This series makes those drivers use __arch_phys_wc_add() more as a
transitory phase in hopes we can address the proper split as with the
atyfb illustrates. For ipath the changes required have a nice template
with the qib driver as they share very similar driver structure, the
qib driver *did* do the nice split.
If so, can we ioremap + set_page_xyz instead?
I'm not sure I see which call we'd use. Care to provide an example patch
alternative for the atyfb as a case in point alternative to the work required
to do the split?
Luis
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-03-27 19:58:30
On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez [off-list ref] wrote:
On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata, another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
What's the exact layout that this helps? I'm sceptical that this can
ever be correct.
Is there some awful driver that has a large ioremap that's supposed to
contain multiple different memtypes?
Yes, I cc'd you just now on one where I made changes on a driver which uses one
PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
regress those drivers by making the MTRR WC hole trick non functional.
The changes are non trivial and so in this series I supplied changes on
one driver only to show the effort required. The other drivers which
required this were:
Driver File
------------------------------------------------------------
fusion drivers/message/fusion/mptbase.c
ivtv drivers/media/pci/ivtv/ivtvfb.c
ipath drivers/infiniband/hw/ipath/ipath_driver.c
This series makes those drivers use __arch_phys_wc_add() more as a
transitory phase in hopes we can address the proper split as with the
atyfb illustrates. For ipath the changes required have a nice template
with the qib driver as they share very similar driver structure, the
qib driver *did* do the nice split.
quoted
If so, can we ioremap + set_page_xyz instead?
I'm not sure I see which call we'd use. Care to provide an example patch
alternative for the atyfb as a case in point alternative to the work required
to do the split?
I'm still confused. Would it be insufficient to ioremap_nocache the
whole thing and then call set_memory_wc on parts of it? (Sorry,
set_page_xyz was a typo.)
--Andy
--
Andy Lutomirski
AMA Capital Management, LLC
From: Luis R. Rodriguez <hidden> Date: 2015-03-27 20:30:36
On Fri, Mar 27, 2015 at 12:58:02PM -0700, Andy Lutomirski wrote:
On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata, another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
What's the exact layout that this helps? I'm sceptical that this can
ever be correct.
Is there some awful driver that has a large ioremap that's supposed to
contain multiple different memtypes?
Yes, I cc'd you just now on one where I made changes on a driver which uses one
PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
regress those drivers by making the MTRR WC hole trick non functional.
The changes are non trivial and so in this series I supplied changes on
one driver only to show the effort required. The other drivers which
required this were:
Driver File
------------------------------------------------------------
fusion drivers/message/fusion/mptbase.c
ivtv drivers/media/pci/ivtv/ivtvfb.c
ipath drivers/infiniband/hw/ipath/ipath_driver.c
This series makes those drivers use __arch_phys_wc_add() more as a
transitory phase in hopes we can address the proper split as with the
atyfb illustrates. For ipath the changes required have a nice template
with the qib driver as they share very similar driver structure, the
qib driver *did* do the nice split.
quoted
If so, can we ioremap + set_page_xyz instead?
I'm not sure I see which call we'd use. Care to provide an example patch
alternative for the atyfb as a case in point alternative to the work required
to do the split?
I'm still confused. Would it be insufficient to ioremap_nocache the
whole thing and then call set_memory_wc on parts of it? (Sorry,
set_page_xyz was a typo.)
I think that would be a sexy alternative.
In this driver's case the thing is a bit messy as it not only used
the WC MTRR for a hole but it also then used a UC MTRR on top of
it all, so since I already tried to address the split, and if we address
the power of 2 woes, I think it'd be best to try to remove the UC MTRR
and just avoid set_page_wc() in this driver's case, but for the other cases
(fusion, ivtv, ipath) I think this makes sense.
Thoughts?
Luis
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-03-27 21:23:40
On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez [off-list ref] wrote:
On Fri, Mar 27, 2015 at 12:58:02PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata, another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
What's the exact layout that this helps? I'm sceptical that this can
ever be correct.
Is there some awful driver that has a large ioremap that's supposed to
contain multiple different memtypes?
Yes, I cc'd you just now on one where I made changes on a driver which uses one
PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
regress those drivers by making the MTRR WC hole trick non functional.
The changes are non trivial and so in this series I supplied changes on
one driver only to show the effort required. The other drivers which
required this were:
Driver File
------------------------------------------------------------
fusion drivers/message/fusion/mptbase.c
ivtv drivers/media/pci/ivtv/ivtvfb.c
ipath drivers/infiniband/hw/ipath/ipath_driver.c
This series makes those drivers use __arch_phys_wc_add() more as a
transitory phase in hopes we can address the proper split as with the
atyfb illustrates. For ipath the changes required have a nice template
with the qib driver as they share very similar driver structure, the
qib driver *did* do the nice split.
quoted
If so, can we ioremap + set_page_xyz instead?
I'm not sure I see which call we'd use. Care to provide an example patch
alternative for the atyfb as a case in point alternative to the work required
to do the split?
I'm still confused. Would it be insufficient to ioremap_nocache the
whole thing and then call set_memory_wc on parts of it? (Sorry,
set_page_xyz was a typo.)
I think that would be a sexy alternative.
In this driver's case the thing is a bit messy as it not only used
the WC MTRR for a hole but it also then used a UC MTRR on top of
it all, so since I already tried to address the split, and if we address
the power of 2 woes, I think it'd be best to try to remove the UC MTRR
and just avoid set_page_wc() in this driver's case, but for the other cases
(fusion, ivtv, ipath) I think this makes sense.
Thoughts?
Once that WC MTRR is in place, I think you really need UC and not UC-
if you want to override it. Otherwise I agree with all of this.
--Andy
From: Luis R. Rodriguez <hidden> Date: 2015-03-27 23:04:49
On Fri, Mar 27, 2015 at 02:23:16PM -0700, Andy Lutomirski wrote:
On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
On Fri, Mar 27, 2015 at 12:58:02PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata, another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
What's the exact layout that this helps? I'm sceptical that this can
ever be correct.
Is there some awful driver that has a large ioremap that's supposed to
contain multiple different memtypes?
Yes, I cc'd you just now on one where I made changes on a driver which uses one
PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
regress those drivers by making the MTRR WC hole trick non functional.
The changes are non trivial and so in this series I supplied changes on
one driver only to show the effort required. The other drivers which
required this were:
Driver File
------------------------------------------------------------
fusion drivers/message/fusion/mptbase.c
ivtv drivers/media/pci/ivtv/ivtvfb.c
ipath drivers/infiniband/hw/ipath/ipath_driver.c
This series makes those drivers use __arch_phys_wc_add() more as a
transitory phase in hopes we can address the proper split as with the
atyfb illustrates. For ipath the changes required have a nice template
with the qib driver as they share very similar driver structure, the
qib driver *did* do the nice split.
quoted
If so, can we ioremap + set_page_xyz instead?
I'm not sure I see which call we'd use. Care to provide an example patch
alternative for the atyfb as a case in point alternative to the work required
to do the split?
I'm still confused. Would it be insufficient to ioremap_nocache the
whole thing and then call set_memory_wc on parts of it? (Sorry,
set_page_xyz was a typo.)
I think that would be a sexy alternative.
In this driver's case the thing is a bit messy as it not only used
the WC MTRR for a hole but it also then used a UC MTRR on top of
it all, so since I already tried to address the split, and if we address
the power of 2 woes, I think it'd be best to try to remove the UC MTRR
and just avoid set_page_wc() in this driver's case, but for the other cases
(fusion, ivtv, ipath) I think this makes sense.
Thoughts?
Once that WC MTRR is in place, I think you really need UC and not UC-
if you want to override it. Otherwise I agree with all of this.
Do you mean that the UC MTRR work around that was in place might not
have really been effective? Not quite sure what you mean. I don't think
I follow.
Luis
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-03-27 23:10:30
On Fri, Mar 27, 2015 at 4:04 PM, Luis R. Rodriguez [off-list ref] wrote:
On Fri, Mar 27, 2015 at 02:23:16PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
On Fri, Mar 27, 2015 at 12:58:02PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata, another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
What's the exact layout that this helps? I'm sceptical that this can
ever be correct.
Is there some awful driver that has a large ioremap that's supposed to
contain multiple different memtypes?
Yes, I cc'd you just now on one where I made changes on a driver which uses one
PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
regress those drivers by making the MTRR WC hole trick non functional.
The changes are non trivial and so in this series I supplied changes on
one driver only to show the effort required. The other drivers which
required this were:
Driver File
------------------------------------------------------------
fusion drivers/message/fusion/mptbase.c
ivtv drivers/media/pci/ivtv/ivtvfb.c
ipath drivers/infiniband/hw/ipath/ipath_driver.c
This series makes those drivers use __arch_phys_wc_add() more as a
transitory phase in hopes we can address the proper split as with the
atyfb illustrates. For ipath the changes required have a nice template
with the qib driver as they share very similar driver structure, the
qib driver *did* do the nice split.
quoted
If so, can we ioremap + set_page_xyz instead?
I'm not sure I see which call we'd use. Care to provide an example patch
alternative for the atyfb as a case in point alternative to the work required
to do the split?
I'm still confused. Would it be insufficient to ioremap_nocache the
whole thing and then call set_memory_wc on parts of it? (Sorry,
set_page_xyz was a typo.)
I think that would be a sexy alternative.
In this driver's case the thing is a bit messy as it not only used
the WC MTRR for a hole but it also then used a UC MTRR on top of
it all, so since I already tried to address the split, and if we address
the power of 2 woes, I think it'd be best to try to remove the UC MTRR
and just avoid set_page_wc() in this driver's case, but for the other cases
(fusion, ivtv, ipath) I think this makes sense.
Thoughts?
Once that WC MTRR is in place, I think you really need UC and not UC-
if you want to override it. Otherwise I agree with all of this.
Do you mean that the UC MTRR work around that was in place might not
have really been effective? Not quite sure what you mean. I don't think
I follow.
I mean that the UC MTRR that overrides the WC MTRR was probably fine
(I hope smaller MTRRs override larger MTRRs). But we should just
ditch UC MTRRs entirely, and setting UC in the page tables would work
on all CPUs *if we supported that*. We'd need to add a couple trivial
helpers to do that.
--Andy
From: Luis R. Rodriguez <hidden> Date: 2015-03-27 23:33:46
On Fri, Mar 27, 2015 at 04:10:03PM -0700, Andy Lutomirski wrote:
On Fri, Mar 27, 2015 at 4:04 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
On Fri, Mar 27, 2015 at 02:23:16PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
On Fri, Mar 27, 2015 at 12:58:02PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez [off-list ref] wrote:
quoted
On Fri, Mar 20, 2015 at 04:48:46PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata, another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
What's the exact layout that this helps? I'm sceptical that this can
ever be correct.
Is there some awful driver that has a large ioremap that's supposed to
contain multiple different memtypes?
Yes, I cc'd you just now on one where I made changes on a driver which uses one
PCI with mixed memtypes and uses MTRR to hole in WC. A transition to
arch_phys_wc_add() is therefore not possible if PAT is enabled as it would
regress those drivers by making the MTRR WC hole trick non functional.
The changes are non trivial and so in this series I supplied changes on
one driver only to show the effort required. The other drivers which
required this were:
Driver File
------------------------------------------------------------
fusion drivers/message/fusion/mptbase.c
ivtv drivers/media/pci/ivtv/ivtvfb.c
ipath drivers/infiniband/hw/ipath/ipath_driver.c
This series makes those drivers use __arch_phys_wc_add() more as a
transitory phase in hopes we can address the proper split as with the
atyfb illustrates. For ipath the changes required have a nice template
with the qib driver as they share very similar driver structure, the
qib driver *did* do the nice split.
quoted
If so, can we ioremap + set_page_xyz instead?
I'm not sure I see which call we'd use. Care to provide an example patch
alternative for the atyfb as a case in point alternative to the work required
to do the split?
I'm still confused. Would it be insufficient to ioremap_nocache the
whole thing and then call set_memory_wc on parts of it? (Sorry,
set_page_xyz was a typo.)
I think that would be a sexy alternative.
In this driver's case the thing is a bit messy as it not only used
the WC MTRR for a hole but it also then used a UC MTRR on top of
it all, so since I already tried to address the split, and if we address
the power of 2 woes, I think it'd be best to try to remove the UC MTRR
and just avoid set_page_wc() in this driver's case, but for the other cases
(fusion, ivtv, ipath) I think this makes sense.
Thoughts?
Once that WC MTRR is in place, I think you really need UC and not UC-
if you want to override it. Otherwise I agree with all of this.
Do you mean that the UC MTRR work around that was in place might not
have really been effective? Not quite sure what you mean. I don't think
I follow.
I mean that the UC MTRR that overrides the WC MTRR was probably fine
(I hope smaller MTRRs override larger MTRRs). But we should just
ditch UC MTRRs entirely,
Agreed, this series does that, and this patch addresses the last
UC MTRR ;)
and setting UC in the page tables would work on all CPUs *if we supported
that*. We'd need to add a couple trivial helpers to do that.
OK please check my latest reply and if you do not mind clarify what you mean
there as I am not sure if we're on the same page (no pun) here. I don't quite
follow this last statement.
Luis
On Fri, Mar 20, 2015 at 6:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata,
This probably makes sense to someone steeped in MTRR and PAT, but not
otherwise. "One exception is where drivers are known to exist on PATs
with errata"? The drivers exist, independent of PAT/MTRR/errata. Do
you mean there are drivers that can't be converted from MTRR to PAT
because some PATs are broken?
I don't really know anything about MTRR or PAT; I'm just trying to
figure out how to parse this paragraph.
another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
"transient"? Do you mean you intend to remove this API in the near future?
Bjorn
From: Luis R. Rodriguez <hidden> Date: 2015-04-02 20:55:26
On Thu, Apr 02, 2015 at 03:21:22PM -0500, Bjorn Helgaas wrote:
On Fri, Mar 20, 2015 at 6:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted
From: "Luis R. Rodriguez" <redacted>
Ideally on systems using PAT we can expect a swift
transition away from MTRR. There can be a few exceptions
to this, one is where device drivers are known to exist
on PATs with errata,
This probably makes sense to someone steeped in MTRR and PAT, but not
otherwise. "One exception is where drivers are known to exist on PATs
with errata"? The drivers exist, independent of PAT/MTRR/errata. Do
you mean there are drivers that can't be converted from MTRR to PAT
because some PATs are broken?
Well there is that but it seems we have motivation to
address the PAT broken systems so this would be one of the
lower priority reasons to consider adding this API. The
more important reason is below.
I don't really know anything about MTRR or PAT; I'm just trying to
figure out how to parse this paragraph.
Sure.
quoted
another situation is observed on
old device drivers where devices had combined MMIO
register access with whatever area they typically
later wanted to end up using MTRR for on the same
PCI BAR. This situation can still be addressed by
splitting up ioremap'd PCI BAR into two ioremap'd
calls, one for MMIO registers, and another for whatever
is desirable for write-combining -- in order to
accomplish this though quite a bit of driver
restructuring is required.
Device drivers which are known to require large
amount of re-work in order to split ioremap'd areas
can use __arch_phys_wc_add() to avoid regressions
when PAT is enabled.
For a good example driver where things are neatly
split up on a PCI BAR refer the infiniband qib
driver. For a good example of a driver where good
amount of work is required refer to the infiniband
ipath driver.
This is *only* a transitive API -- and as such no new
drivers are ever expected to use this.
"transient"? Do you mean you intend to remove this API in the near future?
That's correct, the problem is that in order to use PAT cleanly we'd need to
change these drivers to not overlap ioremap'd areas otherwise things can get
quite complex, and changing the way we do the ioremap() calls on a driver might
require a bit of work. The atyfb driver changes I did are an example of the
types of changes that are expected. In the most complex worst cases there are
MTRR "hole" tricks used, and as can be observed with the atyfb driver changes
there are a series of things to consider when this is done specially in light
of eventually making strong UC the default instead of UC-.
I might be able to work around not adding this API by reviewing the users I had
in this series again and seeing if something similar to what I will do on atyfb
can be done in the meantime by using ioremap_uc(). Its not clear to me yet.
Luis
From: Luis R. Rodriguez <hidden> Date: 2015-03-20 23:36:24
From: "Luis R. Rodriguez" <redacted>
The size of the framebuffer to be used needs to
be fudged to account for the different type of
devices that are out there. This captures what
is required to do well, we'll resuse this later.
This has no functional changes.
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
drivers/video/fbdev/aty/atyfb_base.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
From: Luis R. Rodriguez <hidden> Date: 2015-03-20 23:38:36
From: "Luis R. Rodriguez" <redacted>
This has no functional changes, it just adjusts
the ioremap() call for the framebuffer to use
the same values we later use for the framebuffer,
this will make it easier to review the next change.
The size of the framebuffer varies but since this is
for PCI we *know* this defaults to 0x800000.
atyfb_setup_generic() is *only* used on PCI probe.
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
drivers/video/fbdev/aty/atyfb_base.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Luis R. Rodriguez <hidden> Date: 2015-03-20 23:40:47
From: "Luis R. Rodriguez" <redacted>
The atyfb driver uses an MTRR work around since some
cards use the same PCI BAR for the framebuffer and MMIO.
In such cards the last page is used for MMIO, the rest for
the framebuffer, so on those cards we ioremap() the MMIO
page alone, then again ioremap() the full framebuffer
including the MMIO space *and* ___then___ use an MTRR with
MTRR_TYPE_WRCOMB on the full PCI BAR... and finally "hole"
in an MTRR_TYPE_UNCACHABLE MTRR only for MMIO.
This is a terrible fucking work around, and should by no means
be necessary however evidence through a large series of conversion
of drivers to ioremap_wc() for the framebuffer shows that around
the time MTRR started becoming popular devices did not have things
lined up for easily separating the framebuffer and MMIO register
access. In some cases a driver requires significant intrusive
changes in order to make the split for an ioremap() for MMIO registers
and another ioremap_wc() for the framebuffer, at other times a
bit of careful study of the driver suffices. This example driver
falls into the later category.
We can replace the MTRR MTRR_TYPE_UNCACHABLE
work around by using ioremap_nocache(), the length of the
MMIO space should already be correct. The other part we
need to correct is ensuring we ioremap() for the framebuffer
only the required size. Since the ioremap() happens early
on probe for PCI devices before aty_init() where we typically
adjust the length and know how to do it, we can fix this by
pegging the bus type as PCI on PCI probe, and finally fudging
and framebuffer length just as we do on aty_init().
The last thing we do must do to remain sane is ensure we
use the info->fix.smem_start and info->fix.smem_len for
the framebuffer MTRR as we know that is always well adjusted.
The *one* concern here would be if the MTRR is not in units
of 4K __but__ we already know that in the PCI case this cannot
happen, in the shared space setting the MTRR would be up to
0x7ff000 and assuming a 4K page:
; 0x7ff000 / 0x1000
2047
Also, internally when MTRR is used mtrr_add() will use mtrr_check()
and that should splat a warning when the MTRR base and size are
not compatible with what is expected for MTRR usage.
This fix lets us nuke the MTRR_TYPE_UNCACHABLE MTRR "hole".
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
drivers/video/fbdev/aty/atyfb.h | 1 -
drivers/video/fbdev/aty/atyfb_base.c | 28 ++++++----------------------
2 files changed, 6 insertions(+), 23 deletions(-)
@@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)#ifdef CONFIG_MTRRpar->mtrr_aper=-1;-par->mtrr_reg=-1;if(!nomtrr){-/* Cover the whole resource. */-par->mtrr_aper=mtrr_add(par->res_start,par->res_size,+par->mtrr_aper=mtrr_add(info->fix.smem_start,+info->fix.smem_len,MTRR_TYPE_WRCOMB,1);-if(par->mtrr_aper>=0&&!par->aux_start){-/* Make a hole for mmio. */-par->mtrr_reg=mtrr_add(par->res_start+0x800000--GUI_RESERVE,GUI_RESERVE,-MTRR_TYPE_UNCACHABLE,1);-if(par->mtrr_reg<0){-mtrr_del(par->mtrr_aper,0,0);-par->mtrr_aper=-1;-}-}}#endif
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-03-20 23:52:45
On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
[off-list ref] wrote:
quoted hunk
From: "Luis R. Rodriguez" <redacted>
The atyfb driver uses an MTRR work around since some
cards use the same PCI BAR for the framebuffer and MMIO.
In such cards the last page is used for MMIO, the rest for
the framebuffer, so on those cards we ioremap() the MMIO
page alone, then again ioremap() the full framebuffer
including the MMIO space *and* ___then___ use an MTRR with
MTRR_TYPE_WRCOMB on the full PCI BAR... and finally "hole"
in an MTRR_TYPE_UNCACHABLE MTRR only for MMIO.
This is a terrible fucking work around, and should by no means
be necessary however evidence through a large series of conversion
of drivers to ioremap_wc() for the framebuffer shows that around
the time MTRR started becoming popular devices did not have things
lined up for easily separating the framebuffer and MMIO register
access. In some cases a driver requires significant intrusive
changes in order to make the split for an ioremap() for MMIO registers
and another ioremap_wc() for the framebuffer, at other times a
bit of careful study of the driver suffices. This example driver
falls into the later category.
We can replace the MTRR MTRR_TYPE_UNCACHABLE
work around by using ioremap_nocache(), the length of the
MMIO space should already be correct. The other part we
need to correct is ensuring we ioremap() for the framebuffer
only the required size. Since the ioremap() happens early
on probe for PCI devices before aty_init() where we typically
adjust the length and know how to do it, we can fix this by
pegging the bus type as PCI on PCI probe, and finally fudging
and framebuffer length just as we do on aty_init().
The last thing we do must do to remain sane is ensure we
use the info->fix.smem_start and info->fix.smem_len for
the framebuffer MTRR as we know that is always well adjusted.
The *one* concern here would be if the MTRR is not in units
of 4K __but__ we already know that in the PCI case this cannot
happen, in the shared space setting the MTRR would be up to
0x7ff000 and assuming a 4K page:
; 0x7ff000 / 0x1000
2047
Also, internally when MTRR is used mtrr_add() will use mtrr_check()
and that should splat a warning when the MTRR base and size are
not compatible with what is expected for MTRR usage.
This fix lets us nuke the MTRR_TYPE_UNCACHABLE MTRR "hole".
Cc: Suresh Siddha <redacted>
Cc: Venkatesh Pallipadi <redacted>
Cc: Ingo Molnar <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <redacted>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <redacted>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <redacted>
---
drivers/video/fbdev/aty/atyfb.h | 1 -
drivers/video/fbdev/aty/atyfb_base.c | 28 ++++++----------------------
2 files changed, 6 insertions(+), 23 deletions(-)
@@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)#ifdef CONFIG_MTRRpar->mtrr_aper=-1;-par->mtrr_reg=-1;if(!nomtrr){-/* Cover the whole resource. */-par->mtrr_aper=mtrr_add(par->res_start,par->res_size,+par->mtrr_aper=mtrr_add(info->fix.smem_start,+info->fix.smem_len,MTRR_TYPE_WRCOMB,1);-if(par->mtrr_aper>=0&&!par->aux_start){-/* Make a hole for mmio. */-par->mtrr_reg=mtrr_add(par->res_start+0x800000--GUI_RESERVE,GUI_RESERVE,-MTRR_TYPE_UNCACHABLE,1);-if(par->mtrr_reg<0){-mtrr_del(par->mtrr_aper,0,0);-par->mtrr_aper=-1;-}-}}#endif
Double-check me, but I think that ioremap_nocache + WC MTRR = WC. I
think we might need ioremap_nocache_me_harder (or maybe ioremap_x86_uc
if you prefer that bikeshed color) for this.