From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752665AbbC0VXk (ORCPT ); Fri, 27 Mar 2015 17:23:40 -0400 Received: from mail-lb0-f175.google.com ([209.85.217.175]:36188 "EHLO mail-lb0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752445AbbC0VXi (ORCPT ); Fri, 27 Mar 2015 17:23:38 -0400 MIME-Version: 1.0 In-Reply-To: <20150327203029.GM5622@wotan.suse.de> References: <1426893517-2511-1-git-send-email-mcgrof@do-not-panic.com> <1426893517-2511-7-git-send-email-mcgrof@do-not-panic.com> <20150327195350.GJ5622@wotan.suse.de> <20150327203029.GM5622@wotan.suse.de> From: Andy Lutomirski Date: Fri, 27 Mar 2015 14:23:16 -0700 Message-ID: Subject: Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add() To: "Luis R. Rodriguez" Cc: Bjorn Helgaas , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Mauro Carvalho Chehab , Mike Marciniszyn , "Luis R. Rodriguez" , Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" , Juergen Gross , Jan Beulich , Borislav Petkov , Suresh Siddha , venkatesh.pallipadi@intel.com, Dave Airlie , "linux-kernel@vger.kernel.org" , Linux Fbdev development list , X86 ML , "xen-devel@lists.xenproject.org" , Ingo Molnar , Daniel Vetter , Antonino Daplas , Jean-Christophe Plagniol-Villard , Tomi Valkeinen Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez wrote: > 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 wrote: >> > 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 >> >> wrote: >> >> > From: "Luis R. Rodriguez" >> >> > >> >> > 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? >> > >> >> 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 mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Lutomirski Date: Fri, 27 Mar 2015 21:23:16 +0000 Subject: Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add() Message-Id: List-Id: References: <1426893517-2511-1-git-send-email-mcgrof@do-not-panic.com> <1426893517-2511-7-git-send-email-mcgrof@do-not-panic.com> <20150327195350.GJ5622@wotan.suse.de> <20150327203029.GM5622@wotan.suse.de> In-Reply-To: <20150327203029.GM5622@wotan.suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: "Luis R. Rodriguez" Cc: Bjorn Helgaas , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Mauro Carvalho Chehab , Mike Marciniszyn , "Luis R. Rodriguez" , Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" , Juergen Gross , Jan Beulich , Borislav Petkov , Suresh Siddha , venkatesh.pallipadi@intel.com, Dave Airlie , "linux-kernel@vger.kernel.org" , Linux Fbdev development list , X86 ML , "xen-devel@lists.xenproject.org" , Ingo Molnar , Daniel Vetter , Antonino Daplas , Jean-Christophe Plagniol-Villard , Tomi Valkeinen On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez wrote: > 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 wrote: >> > 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 >> >> wrote: >> >> > From: "Luis R. Rodriguez" >> >> > >> >> > 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? >> > >> >> 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