* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Andy Lutomirski @ 2015-03-27 19:58 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Bjorn Helgaas, Ville Syrjälä, 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, 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
In-Reply-To: <20150327195350.GJ5622@wotan.suse.de>
On Fri, Mar 27, 2015 at 12:53 PM, Luis R. Rodriguez <mcgrof@suse.com> 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
>> <mcgrof@do-not-panic.com> wrote:
>> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> >
>> > 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.)
--Andy
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Luis R. Rodriguez @ 2015-03-27 20:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Juergen Gross, Jan Beulich, Borislav Petkov, Suresh Siddha,
venkatesh.pallipadi, Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <CALCETrWXk0KVU__DXpGj4RSyY1K7dyjv2h5qJBrZT8psEyM-cQ@mail.gmail.com>
On Fri, Mar 20, 2015 at 04:52:18PM -0700, Andy Lutomirski wrote:
> On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> > index 8025624..8875e56 100644
> > --- a/drivers/video/fbdev/aty/atyfb_base.c
> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
> >
> > #ifdef CONFIG_MTRR
> > par->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
> >
> > @@ -2776,10 +2765,6 @@ aty_init_exit:
> > par->pll_ops->set_pll(info, &par->saved_pll);
> >
> > #ifdef CONFIG_MTRR
> > - if (par->mtrr_reg >= 0) {
> > - mtrr_del(par->mtrr_reg, 0, 0);
> > - par->mtrr_reg = -1;
> > - }
> > if (par->mtrr_aper >= 0) {
> > mtrr_del(par->mtrr_aper, 0, 0);
> > par->mtrr_aper = -1;
> > @@ -3466,7 +3451,7 @@ static int atyfb_setup_generic(struct pci_dev *pdev, struct fb_info *info,
> > }
> >
> > info->fix.mmio_start = raddr;
> > - par->ati_regbase = ioremap(info->fix.mmio_start, 0x1000);
> > + par->ati_regbase = ioremap_nocache(info->fix.mmio_start, 0x1000);
>
> Double-check me, but I think that ioremap_nocache + WC MTRR = WC.
Precicely, in this case the WC hole was obtained by using MTRR WC. This
patch removes that WC hole trick and now we can be explciit about
only wanting ioremap_nocache() on the registers, that is WC is not
desired here and is not used. The patch does not highlight the fact
that there was left in place another ioremap() call for the framebuffer:
info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);
That is the one that later after this patch we use ioremap_wc() for.
This patch just removes the hole solution. That's all.
Luis
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Luis R. Rodriguez @ 2015-03-27 20:30 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Bjorn Helgaas, Ville Syrjälä, 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, 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
In-Reply-To: <CALCETrU-EATXPNt8p21=1rsbAsWX-kJys1rwzYyZ2S1h2enVYw@mail.gmail.com>
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 <mcgrof@suse.com> 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
> >> <mcgrof@do-not-panic.com> wrote:
> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> >
> >> > 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?
Luis
^ permalink raw reply
* Re: [PATCH v1 21/47] ethernet: myri10ge: use arch_phys_wc_add()
From: Luis R. Rodriguez @ 2015-03-27 20:36 UTC (permalink / raw)
To: Hyong-Youb Kim
Cc: Luis R. Rodriguez, luto, mingo, tglx, hpa, jgross, JBeulich, bp,
suresh.b.siddha, venkatesh.pallipadi, airlied, linux-kernel,
linux-fbdev, x86, xen-devel, Ingo Molnar, Daniel Vetter,
Hyong-Youb Kim, netdev, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <20150321070759.GB8766@gtest01.arcus>
On Sat, Mar 21, 2015 at 04:08:00PM +0900, Hyong-Youb Kim wrote:
> On Fri, Mar 20, 2015 at 04:18:11PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >
> > This driver already uses ioremap_wc() on the same range
> > so when write-combining is available that will be used
> > instead.
> >
> [...]
> > --- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> > +++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> [...]
> > @@ -1984,7 +1979,6 @@ myri10ge_get_ethtool_stats(struct net_device *netdev,
> > data[i] = ((u64 *)&link_stats)[i];
> >
> > data[i++] = (unsigned int)mgp->tx_boundary;
> > - data[i++] = (unsigned int)mgp->wc_enabled;
> > data[i++] = (unsigned int)mgp->pdev->irq;
> > data[i++] = (unsigned int)mgp->msi_enabled;
> > data[i++] = (unsigned int)mgp->msix_enabled;
>
> You would have to delete "WC from myri10ge_gstrings_main_stats too.
> Something like below. Thanks.
>
> @@ -1905,7 +1905,7 @@ static const char myri10ge_gstrings_main_stats[][ETH_GSTRING_LEN] = {
> "tx_aborted_errors", "tx_carrier_errors", "tx_fifo_errors",
> "tx_heartbeat_errors", "tx_window_errors",
> /* device-specific stats */
> - "tx_boundary", "WC", "irq", "MSI", "MSIX",
> + "tx_boundary", "irq", "MSI", "MSIX",
> "read_dma_bw_MBs", "write_dma_bw_MBs", "read_write_dma_bw_MBs",
> "serial_number", "watchdog_resets",
> #ifdef CONFIG_MYRI10GE_DCA
OK great thanks. Amended.
Luis
^ permalink raw reply
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Toshi Kani @ 2015-03-27 20:40 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: luto, mingo, tglx, hpa, jgross, JBeulich, bp, suresh.b.siddha,
venkatesh.pallipadi, airlied, linux-kernel, linux-fbdev, x86,
xen-devel, Luis R. Rodriguez, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, konrad.wilk, ville.syrjala,
david.vrabel, bhelgaas, Roger Pau Monné, xen-devel
In-Reply-To: <1426893517-2511-3-git-send-email-mcgrof@do-not-panic.com>
On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
:
> @@ -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
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Andy Lutomirski @ 2015-03-27 21:21 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Juergen Gross, Jan Beulich, Borislav Petkov, Suresh Siddha,
venkatesh.pallipadi, Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <20150327201201.GL5622@wotan.suse.de>
On Fri, Mar 27, 2015 at 1:12 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Fri, Mar 20, 2015 at 04:52:18PM -0700, Andy Lutomirski wrote:
>> On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
>> <mcgrof@do-not-panic.com> wrote:
>> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
>> > index 8025624..8875e56 100644
>> > --- a/drivers/video/fbdev/aty/atyfb_base.c
>> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
>> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
>> >
>> > #ifdef CONFIG_MTRR
>> > par->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
>> >
>> > @@ -2776,10 +2765,6 @@ aty_init_exit:
>> > par->pll_ops->set_pll(info, &par->saved_pll);
>> >
>> > #ifdef CONFIG_MTRR
>> > - if (par->mtrr_reg >= 0) {
>> > - mtrr_del(par->mtrr_reg, 0, 0);
>> > - par->mtrr_reg = -1;
>> > - }
>> > if (par->mtrr_aper >= 0) {
>> > mtrr_del(par->mtrr_aper, 0, 0);
>> > par->mtrr_aper = -1;
>> > @@ -3466,7 +3451,7 @@ static int atyfb_setup_generic(struct pci_dev *pdev, struct fb_info *info,
>> > }
>> >
>> > info->fix.mmio_start = raddr;
>> > - par->ati_regbase = ioremap(info->fix.mmio_start, 0x1000);
>> > + par->ati_regbase = ioremap_nocache(info->fix.mmio_start, 0x1000);
>>
>> Double-check me, but I think that ioremap_nocache + WC MTRR = WC.
>
> Precicely, in this case the WC hole was obtained by using MTRR WC. This
> patch removes that WC hole trick and now we can be explciit about
> only wanting ioremap_nocache() on the registers, that is WC is not
> desired here and is not used. The patch does not highlight the fact
> that there was left in place another ioremap() call for the framebuffer:
>
> info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);
>
> That is the one that later after this patch we use ioremap_wc() for.
> This patch just removes the hole solution. That's all.
>
I don't understand.
If I read it right, there's a 2^n byte BAR. You're requesting WC for
the whole think using arch_phys_wc_add. On a PAT system that has no
effect and all is well. On a non-PAT system, it adds an MTRR. That
means that you need to override the MTRR somehow for the mmio regs,
and UC- won't do the trick.
Or am I missing something here?
--Andy
> Luis
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Andy Lutomirski @ 2015-03-27 21:23 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Bjorn Helgaas, Ville Syrjälä, 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, 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
In-Reply-To: <20150327203029.GM5622@wotan.suse.de>
On Fri, Mar 27, 2015 at 1:30 PM, Luis R. Rodriguez <mcgrof@suse.com> 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 <mcgrof@suse.com> 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
>> >> <mcgrof@do-not-panic.com> wrote:
>> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> >> >
>> >> > 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
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Ville Syrjälä @ 2015-03-27 21:56 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Lutomirski, Bjorn Helgaas, Luis R. Rodriguez, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Juergen Gross, Jan Beulich,
Borislav Petkov, Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Linus Torvalds, Daniel Vetter, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <20150327195759.GK5622@wotan.suse.de>
On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote:
> On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote:
> > On Fri, Mar 27, 2015 at 12:38 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrjälä wrote:
> > >> On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez wrote:
> > >> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> > >> > index 8025624..8875e56 100644
> > >> > --- a/drivers/video/fbdev/aty/atyfb_base.c
> > >> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
> > >> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
> > >> >
> > >> > #ifdef CONFIG_MTRR
> > >> > par->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);
> > >>
> > >> MTRRs need power of two size, so how is this supposed to work?
> > >
> > > As per mtrr_add_page() [0] the base and size are just supposed to be in units
> > > of 4 KiB, although the practice is to use powers of 2 in *some* drivers this
> > > is not standardized and by no means recorded as a requirement. Obviously
> > > powers of 2 will work too and you'd end up neatly aligned as well. mtrr_add()
> > > will use mtrr_check() to verify the the same requirement. Furthermore,
> > > as per my commit log message:
> >
> > Whatever the code may or may not do, the x86 architecture uses
> > power-of-two MTRR sizes. So I'm confused.
>
> There should be no confusion, I simply did not know that *was* the
> requirement for x86, if that is the case we should add a check for that
> and perhaps generalize a helper that does the power of two helper changes,
> the cleanest I found was the vesafb driver solution.
>
> Thoughts?
The vesafb solution is bad since you'll only end up covering only
the first 4MB of the framebuffer instead of the almost 8MB you want.
Which in practice will mean throwing away half the VRAM since you really
don't want the massive performance hit from accessing it as UC. And that
would mean giving up decent display resolutions as well :(
And the other option of trying to cover the remainder with multiple ever
smaller MTRRs doesn't work either since you'll run out of MTRRs very
quickly.
This is precisely why I used the hole method in atyfb in the first
place.
I don't really like the idea of any new mtrr code not supporting that
use case, especially as these things tend to be present in older machines
where PAT isn't an option.
--
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Andy Lutomirski @ 2015-03-27 22:02 UTC (permalink / raw)
To: Ville Syrjälä, Luis R. Rodriguez, Andy Lutomirski,
Bjorn Helgaas, Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, Juergen Gross, Jan Beulich, Borislav Petkov,
Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Linus Torvalds, Daniel Vetter, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <20150327215655.GA29933@sci.fi>
On Fri, Mar 27, 2015 at 2:56 PM, Ville Syrj채l채 <syrjala@sci.fi> wrote:
> On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote:
>> On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote:
>> > On Fri, Mar 27, 2015 at 12:38 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrj채l채 wrote:
>> > >> On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez wrote:
>> > >> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
>> > >> > index 8025624..8875e56 100644
>> > >> > --- a/drivers/video/fbdev/aty/atyfb_base.c
>> > >> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
>> > >> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
>> > >> >
>> > >> > #ifdef CONFIG_MTRR
>> > >> > par->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);
>> > >>
>> > >> MTRRs need power of two size, so how is this supposed to work?
>> > >
>> > > As per mtrr_add_page() [0] the base and size are just supposed to be in units
>> > > of 4 KiB, although the practice is to use powers of 2 in *some* drivers this
>> > > is not standardized and by no means recorded as a requirement. Obviously
>> > > powers of 2 will work too and you'd end up neatly aligned as well. mtrr_add()
>> > > will use mtrr_check() to verify the the same requirement. Furthermore,
>> > > as per my commit log message:
>> >
>> > Whatever the code may or may not do, the x86 architecture uses
>> > power-of-two MTRR sizes. So I'm confused.
>>
>> There should be no confusion, I simply did not know that *was* the
>> requirement for x86, if that is the case we should add a check for that
>> and perhaps generalize a helper that does the power of two helper changes,
>> the cleanest I found was the vesafb driver solution.
>>
>> Thoughts?
>
> The vesafb solution is bad since you'll only end up covering only
> the first 4MB of the framebuffer instead of the almost 8MB you want.
> Which in practice will mean throwing away half the VRAM since you really
> don't want the massive performance hit from accessing it as UC. And that
> would mean giving up decent display resolutions as well :(
>
> And the other option of trying to cover the remainder with multiple ever
> smaller MTRRs doesn't work either since you'll run out of MTRRs very
> quickly.
>
> This is precisely why I used the hole method in atyfb in the first
> place.
>
> I don't really like the idea of any new mtrr code not supporting that
> use case, especially as these things tend to be present in older machines
> where PAT isn't an option.
According to the Intel SDM, volume 3, section 11.5.2.1, table 11-6,
non-PAT CPUs that have a WC MTRR, PCD = 1, and PWT = 1 (aka UC) have
an effective memory type of UC. Hence my suggestion to add
ioremap_x86_uc and/or set_memory_x86_uc to punch a UC hole in an
otherwise WC MTRR-covered region.
ioremap_nocache is UC- (even on non-PAT unless I misunderstood how
this stuff works), so ioremap_nocache by itself isn't good enough.
--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Luis R. Rodriguez @ 2015-03-27 23:04 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Bjorn Helgaas, Ville Syrjälä, 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, 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
In-Reply-To: <CALCETrXVL=5O2_LwAgO5vFZRdEpR8WbyRE5-C-NBEER4ZQrE8Q@mail.gmail.com>
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 <mcgrof@suse.com> 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 <mcgrof@suse.com> 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
> >> >> <mcgrof@do-not-panic.com> wrote:
> >> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> >> >
> >> >> > 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.
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
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Andy Lutomirski @ 2015-03-27 23:10 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Bjorn Helgaas, Ville Syrjälä, 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, 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
In-Reply-To: <20150327230440.GO5622@wotan.suse.de>
On Fri, Mar 27, 2015 at 4:04 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> 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 <mcgrof@suse.com> 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 <mcgrof@suse.com> 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
>> >> >> <mcgrof@do-not-panic.com> wrote:
>> >> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> >> >> >
>> >> >> > 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.
>
> 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
>
> Luis
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Luis R. Rodriguez @ 2015-03-27 23:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Juergen Gross, Jan Beulich, Borislav Petkov, Suresh Siddha,
venkatesh.pallipadi, Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <CALCETrX9g6FzSnVmBow58_7cxrn21iCS+Xt8F+ThnJjukkyLmw@mail.gmail.com>
On Fri, Mar 27, 2015 at 02:21:34PM -0700, Andy Lutomirski wrote:
> On Fri, Mar 27, 2015 at 1:12 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Fri, Mar 20, 2015 at 04:52:18PM -0700, Andy Lutomirski wrote:
> >> On Fri, Mar 20, 2015 at 4:17 PM, Luis R. Rodriguez
> >> <mcgrof@do-not-panic.com> wrote:
> >> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> >> > index 8025624..8875e56 100644
> >> > --- a/drivers/video/fbdev/aty/atyfb_base.c
> >> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
> >> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
> >> >
> >> > #ifdef CONFIG_MTRR
> >> > par->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
> >> >
> >> > @@ -2776,10 +2765,6 @@ aty_init_exit:
> >> > par->pll_ops->set_pll(info, &par->saved_pll);
> >> >
> >> > #ifdef CONFIG_MTRR
> >> > - if (par->mtrr_reg >= 0) {
> >> > - mtrr_del(par->mtrr_reg, 0, 0);
> >> > - par->mtrr_reg = -1;
> >> > - }
> >> > if (par->mtrr_aper >= 0) {
> >> > mtrr_del(par->mtrr_aper, 0, 0);
> >> > par->mtrr_aper = -1;
> >> > @@ -3466,7 +3451,7 @@ static int atyfb_setup_generic(struct pci_dev *pdev, struct fb_info *info,
> >> > }
> >> >
> >> > info->fix.mmio_start = raddr;
> >> > - par->ati_regbase = ioremap(info->fix.mmio_start, 0x1000);
> >> > + par->ati_regbase = ioremap_nocache(info->fix.mmio_start, 0x1000);
> >>
> >> Double-check me, but I think that ioremap_nocache + WC MTRR = WC.
> >
> > Precicely, in this case the WC hole was obtained by using MTRR WC. This
> > patch removes that WC hole trick and now we can be explciit about
> > only wanting ioremap_nocache() on the registers, that is WC is not
> > desired here and is not used. The patch does not highlight the fact
> > that there was left in place another ioremap() call for the framebuffer:
> >
> > info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);
> >
> > That is the one that later after this patch we use ioremap_wc() for.
> > This patch just removes the hole solution. That's all.
> >
>
> I don't understand.
>
> If I read it right, there's a 2^n byte BAR. You're requesting WC for
> the whole think using arch_phys_wc_add.
I believe there is a misunderstanding of order of changes.
Let's split when we use mtrr_add() Vs arch_phys_wc_add() to avoid
confusion as in this patch we don't yet use arch_phys_wc_add(). That
is done in the next patch.
The commit log describes best the state of affairs prior to this
patch:
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.
Then this patch drops the MTRR_TYPE_UNCACHABLE rewrite thing
and instead corrects the ioremap() call for the framebuffer
to only be called for the framebuffer alone. For the MMIO
area we adjust to use then ioremap_nocache(). The MTRR left
is now only *for the framebuffer* and it should not be touching
the MMIO area. So the MMIO area has its own ioremap_nocache()
area alone, the framebuffer is left with an ioremap() followed
by an mtrr_add() call.
The next patch replaces the mtrr_add() with arch_phys_wc_add()
and then also uses ioremap_wc().
> On a PAT system that has no effect and all is well.
Yeah we're not doing arch_phys_wc_add() on the entire PCI BAR.
That was dumb, this fixes that, and on this patch mtrr_add()
is still used.
> On a non-PAT system, it adds an MTRR. That
> means that you need to override the MTRR somehow for the mmio regs,
> and UC- won't do the trick.
We don't need to solve that problem here as the MTRR should only
be for the framebuffer.
Luis
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Luis R. Rodriguez @ 2015-03-27 23:33 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Bjorn Helgaas, Ville Syrjälä, 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, 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
In-Reply-To: <CALCETrXcMC7jA2jLo_SMXVaSq+KES7_EaPN-o5xRZ+xM1dVeJQ@mail.gmail.com>
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 <mcgrof@suse.com> wrote:
> > 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 <mcgrof@suse.com> 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 <mcgrof@suse.com> 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
> >> >> >> <mcgrof@do-not-panic.com> wrote:
> >> >> >> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >> >> >> >
> >> >> >> > 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.
> >
> > 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
^ permalink raw reply
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Luis R. Rodriguez @ 2015-03-27 23:56 UTC (permalink / raw)
To: Toshi Kani
Cc: Luis R. Rodriguez, luto, mingo, tglx, hpa, jgross, JBeulich, bp,
suresh.b.siddha, venkatesh.pallipadi, airlied, linux-kernel,
linux-fbdev, x86, xen-devel, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, konrad.wilk, ville.syrjala,
david.vrabel, bhelgaas, Roger Pau Monné, xen-devel
In-Reply-To: <1427488817.31093.54.camel@misato.fc.hp.com>
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:
> :
> > @@ -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
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Luis R. Rodriguez @ 2015-03-28 0:21 UTC (permalink / raw)
To: Ville Syrjälä, Andy Lutomirski, Bjorn Helgaas,
Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Juergen Gross, Jan Beulich, Borislav Petkov, Suresh Siddha,
venkatesh.pallipadi, Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <20150327215655.GA29933@sci.fi>
On Fri, Mar 27, 2015 at 11:56:55PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote:
> > On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote:
> > > On Fri, Mar 27, 2015 at 12:38 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrjälä wrote:
> > > >> On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez wrote:
> > > >> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> > > >> > index 8025624..8875e56 100644
> > > >> > --- a/drivers/video/fbdev/aty/atyfb_base.c
> > > >> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
> > > >> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
> > > >> >
> > > >> > #ifdef CONFIG_MTRR
> > > >> > par->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);
> > > >>
> > > >> MTRRs need power of two size, so how is this supposed to work?
> > > >
> > > > As per mtrr_add_page() [0] the base and size are just supposed to be in units
> > > > of 4 KiB, although the practice is to use powers of 2 in *some* drivers this
> > > > is not standardized and by no means recorded as a requirement. Obviously
> > > > powers of 2 will work too and you'd end up neatly aligned as well. mtrr_add()
> > > > will use mtrr_check() to verify the the same requirement. Furthermore,
> > > > as per my commit log message:
> > >
> > > Whatever the code may or may not do, the x86 architecture uses
> > > power-of-two MTRR sizes. So I'm confused.
> >
> > There should be no confusion, I simply did not know that *was* the
> > requirement for x86, if that is the case we should add a check for that
> > and perhaps generalize a helper that does the power of two helper changes,
> > the cleanest I found was the vesafb driver solution.
> >
> > Thoughts?
>
> The vesafb solution is bad since you'll only end up covering only
> the first 4MB of the framebuffer instead of the almost 8MB you want.
OK so the power of 2 requirement implicates us *having* to use a large
MTRR that includes the MMIo region in the shared PCI case?
Andy, Ville, are we 100% certain about this power of two requirement?
Is that for the base and size or just the size?
Luis
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Luis R. Rodriguez @ 2015-03-28 0:28 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Ville Syrjälä, Bjorn Helgaas, Luis R. Rodriguez,
Ingo Molnar, Thomas Gleixner, H. Peter Anvin, Juergen Gross,
Jan Beulich, Borislav Petkov, Suresh Siddha, venkatesh.pallipadi,
Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <CALCETrWW9WrE_4x6SctQQBe=zXp1yMQ1i=qtsACTnHV0_Ku2iQ@mail.gmail.com>
On Fri, Mar 27, 2015 at 03:02:10PM -0700, Andy Lutomirski wrote:
> On Fri, Mar 27, 2015 at 2:56 PM, Ville Syrjälä <syrjala@sci.fi> wrote:
> > On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote:
> >> On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote:
> >> > On Fri, Mar 27, 2015 at 12:38 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> >> > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrjälä wrote:
> >> > >> On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez wrote:
> >> > >> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> >> > >> > index 8025624..8875e56 100644
> >> > >> > --- a/drivers/video/fbdev/aty/atyfb_base.c
> >> > >> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
> >> > >> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
> >> > >> >
> >> > >> > #ifdef CONFIG_MTRR
> >> > >> > par->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);
> >> > >>
> >> > >> MTRRs need power of two size, so how is this supposed to work?
> >> > >
> >> > > As per mtrr_add_page() [0] the base and size are just supposed to be in units
> >> > > of 4 KiB, although the practice is to use powers of 2 in *some* drivers this
> >> > > is not standardized and by no means recorded as a requirement. Obviously
> >> > > powers of 2 will work too and you'd end up neatly aligned as well. mtrr_add()
> >> > > will use mtrr_check() to verify the the same requirement. Furthermore,
> >> > > as per my commit log message:
> >> >
> >> > Whatever the code may or may not do, the x86 architecture uses
> >> > power-of-two MTRR sizes. So I'm confused.
> >>
> >> There should be no confusion, I simply did not know that *was* the
> >> requirement for x86, if that is the case we should add a check for that
> >> and perhaps generalize a helper that does the power of two helper changes,
> >> the cleanest I found was the vesafb driver solution.
> >>
> >> Thoughts?
> >
> > The vesafb solution is bad since you'll only end up covering only
> > the first 4MB of the framebuffer instead of the almost 8MB you want.
> > Which in practice will mean throwing away half the VRAM since you really
> > don't want the massive performance hit from accessing it as UC. And that
> > would mean giving up decent display resolutions as well :(
> >
> > And the other option of trying to cover the remainder with multiple ever
> > smaller MTRRs doesn't work either since you'll run out of MTRRs very
> > quickly.
> >
> > This is precisely why I used the hole method in atyfb in the first
> > place.
> >
> > I don't really like the idea of any new mtrr code not supporting that
> > use case, especially as these things tend to be present in older machines
> > where PAT isn't an option.
>
> According to the Intel SDM, volume 3, section 11.5.2.1, table 11-6,
> non-PAT CPUs that have a WC MTRR, PCD = 1, and PWT = 1 (aka UC) have
> an effective memory type of UC. Hence my suggestion to add
> ioremap_x86_uc and/or set_memory_x86_uc to punch a UC hole in an
> otherwise WC MTRR-covered region.
OK I think I get it now.
And I take it this would hopefully only be used for non-PAT systems?
Would there be a use case for PAT systems? I wonder if we can wrap
this under some APIs to make it clean and hide this dirty thing
behind the scenes, it seems a fragile and error prone and my hope
would be that we won't need more specialization in this area for
PAT systems.
> ioremap_nocache is UC- (even on non-PAT unless I misunderstood how
> this stuff works), so ioremap_nocache by itself isn't good enough.
Thanks for the clarification.
Luis
^ permalink raw reply
* Re: [PATCH v2 1/3] vt: add cursor blink interval escape sequence
From: Mike Frysinger @ 2015-03-28 0:35 UTC (permalink / raw)
To: Scot Doyle
Cc: Greg Kroah-Hartman, Michael Kerrisk, Jiri Slaby, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, Pavel Machek,
Geert Uytterhoeven, linux-kernel, linux-fbdev, linux-man,
linux-api
In-Reply-To: <alpine.DEB.2.11.1503261352200.2411@local>
[-- Attachment #1: Type: text/plain, Size: 610 bytes --]
On 26 Mar 2015 13:54, Scot Doyle wrote:
> Add an escape sequence to specify the current console's cursor blink
> interval. The interval is specified as a number of milliseconds until
> the next cursor display state toggle, from 50 to 65535. /proc/loadavg
> did not show a difference with a one msec interval, but the lower
> bound is set to 50 msecs since slower hardware wasn't tested.
if they want to be crazy, why not let them ? it's not like we generally prevent
the user from destroying their machine. i.e. just require the value to be > 0
(unless you want to let 0 disable things).
-mike
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/3] vt: add cursor blink interval escape sequence
From: Pavel Machek @ 2015-03-28 7:50 UTC (permalink / raw)
To: Scot Doyle, Greg Kroah-Hartman, Michael Kerrisk, Jiri Slaby,
Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Geert Uytterhoeven, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-man-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150328003503.GB27135@vapier>
On Fri 2015-03-27 20:35:03, Mike Frysinger wrote:
> On 26 Mar 2015 13:54, Scot Doyle wrote:
> > Add an escape sequence to specify the current console's cursor blink
> > interval. The interval is specified as a number of milliseconds until
> > the next cursor display state toggle, from 50 to 65535. /proc/loadavg
> > did not show a difference with a one msec interval, but the lower
> > bound is set to 50 msecs since slower hardware wasn't tested.
>
> if they want to be crazy, why not let them ? it's not like we generally prevent
> the user from destroying their machine. i.e. just require the value to be > 0
> (unless you want to let 0 disable things).
Because anyone with access to console can do this, and we only allow root
users to destroy the machine.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH v2 3/3] console_codes.4: Add CSI sequence for cursor blink interval
From: Pavel Machek @ 2015-03-28 7:51 UTC (permalink / raw)
To: Scot Doyle
Cc: Michael Kerrisk, Greg Kroah-Hartman, Jiri Slaby, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, Geert Uytterhoeven,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-man-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.DEB.2.11.1503261356490.2411@local>
On Thu 2015-03-26 13:57:44, Scot Doyle wrote:
> Add a Console Private CSI sequence to specify the current console's
> cursor blink interval. The interval is specified as a number of
> milliseconds until the next cursor display state toggle, from 50 to
> 65535.
>
> Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH v2 0/3] add cursor blink interval terminal escape sequence
From: Pavel Machek @ 2015-03-28 7:54 UTC (permalink / raw)
To: Scot Doyle
Cc: Greg Kroah-Hartman, Michael Kerrisk, Jiri Slaby, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, Geert Uytterhoeven,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-man-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.DEB.2.11.1503261341260.2164@local>
On Thu 2015-03-26 13:51:04, Scot Doyle wrote:
> v2: Add documentation to console_codes man page (man-pages repo)
>
> This patch series adds an escape sequence to specify the current console's
> cursor blink interval. The default interval is set to fbcon's currently
> hardcoded 200 msecs.
Actually... Greg, can you import console_codes.4 into kernel tree somehow?
This will bring documentation for a bunch of currently undocumented kernel
interfaces into the kernel tree... where it belongs.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* [PATCH 1/2] staging: sm7xxfb: reserve PCI resource
From: Sudip Mukherjee @ 2015-03-28 9:15 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-fbdev, devel, linux-kernel, Sudip Mukherjee
before starting to access any address inside the PCI region we should
reserve the resource and release the resource when the module exits.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/sm7xxfb/sm7xxfb.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/staging/sm7xxfb/sm7xxfb.c b/drivers/staging/sm7xxfb/sm7xxfb.c
index 5b3e614..149286e 100644
--- a/drivers/staging/sm7xxfb/sm7xxfb.c
+++ b/drivers/staging/sm7xxfb/sm7xxfb.c
@@ -776,6 +776,12 @@ static int smtcfb_pci_probe(struct pci_dev *pdev,
if (err)
return err;
+ err = pci_request_region(pdev, 0, "sm7xxfb");
+ if (err < 0) {
+ dev_err(&pdev->dev, "cannot reserve framebuffer region\n");
+ goto failed_regions;
+ }
+
sprintf(smtcfb_fix.id, "sm%Xfb", ent->device);
sfb = smtc_alloc_fb_info(pdev);
@@ -905,6 +911,9 @@ failed_fb:
smtc_free_fb_info(sfb);
failed_free:
+ pci_release_region(pdev, 0);
+
+failed_regions:
pci_disable_device(pdev);
return err;
@@ -933,6 +942,7 @@ static void smtcfb_pci_remove(struct pci_dev *pdev)
smtc_unmap_mmio(sfb);
unregister_framebuffer(&sfb->fb);
smtc_free_fb_info(sfb);
+ pci_release_region(pdev, 0);
}
#ifdef CONFIG_PM
--
1.8.1.2
^ permalink raw reply related
* [PATCH 2/2] staging: sm7xxfb: disable pci device
From: Sudip Mukherjee @ 2015-03-28 9:15 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-fbdev, devel, linux-kernel, Sudip Mukherjee
In-Reply-To: <1427533421-9751-1-git-send-email-sudipm.mukherjee@gmail.com>
disable the pci device when the module exits.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/sm7xxfb/sm7xxfb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/sm7xxfb/sm7xxfb.c b/drivers/staging/sm7xxfb/sm7xxfb.c
index 149286e..77f51a0 100644
--- a/drivers/staging/sm7xxfb/sm7xxfb.c
+++ b/drivers/staging/sm7xxfb/sm7xxfb.c
@@ -943,6 +943,7 @@ static void smtcfb_pci_remove(struct pci_dev *pdev)
unregister_framebuffer(&sfb->fb);
smtc_free_fb_info(sfb);
pci_release_region(pdev, 0);
+ pci_disable_device(pdev);
}
#ifdef CONFIG_PM
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Ville Syrjälä @ 2015-03-28 12:23 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Lutomirski, Bjorn Helgaas, Luis R. Rodriguez, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Juergen Gross, Jan Beulich,
Borislav Petkov, Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Linus Torvalds, Daniel Vetter, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <20150328002818.GT5622@wotan.suse.de>
On Sat, Mar 28, 2015 at 01:28:18AM +0100, Luis R. Rodriguez wrote:
> On Fri, Mar 27, 2015 at 03:02:10PM -0700, Andy Lutomirski wrote:
> > On Fri, Mar 27, 2015 at 2:56 PM, Ville Syrjälä <syrjala@sci.fi> wrote:
> > > On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote:
> > >> On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote:
> > >> > On Fri, Mar 27, 2015 at 12:38 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > >> > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrjälä wrote:
> > >> > >> On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez wrote:
> > >> > >> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> > >> > >> > index 8025624..8875e56 100644
> > >> > >> > --- a/drivers/video/fbdev/aty/atyfb_base.c
> > >> > >> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
> > >> > >> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
> > >> > >> >
> > >> > >> > #ifdef CONFIG_MTRR
> > >> > >> > par->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);
> > >> > >>
> > >> > >> MTRRs need power of two size, so how is this supposed to work?
> > >> > >
> > >> > > As per mtrr_add_page() [0] the base and size are just supposed to be in units
> > >> > > of 4 KiB, although the practice is to use powers of 2 in *some* drivers this
> > >> > > is not standardized and by no means recorded as a requirement. Obviously
> > >> > > powers of 2 will work too and you'd end up neatly aligned as well. mtrr_add()
> > >> > > will use mtrr_check() to verify the the same requirement. Furthermore,
> > >> > > as per my commit log message:
> > >> >
> > >> > Whatever the code may or may not do, the x86 architecture uses
> > >> > power-of-two MTRR sizes. So I'm confused.
> > >>
> > >> There should be no confusion, I simply did not know that *was* the
> > >> requirement for x86, if that is the case we should add a check for that
> > >> and perhaps generalize a helper that does the power of two helper changes,
> > >> the cleanest I found was the vesafb driver solution.
> > >>
> > >> Thoughts?
> > >
> > > The vesafb solution is bad since you'll only end up covering only
> > > the first 4MB of the framebuffer instead of the almost 8MB you want.
> > > Which in practice will mean throwing away half the VRAM since you really
> > > don't want the massive performance hit from accessing it as UC. And that
> > > would mean giving up decent display resolutions as well :(
> > >
> > > And the other option of trying to cover the remainder with multiple ever
> > > smaller MTRRs doesn't work either since you'll run out of MTRRs very
> > > quickly.
> > >
> > > This is precisely why I used the hole method in atyfb in the first
> > > place.
> > >
> > > I don't really like the idea of any new mtrr code not supporting that
> > > use case, especially as these things tend to be present in older machines
> > > where PAT isn't an option.
> >
> > According to the Intel SDM, volume 3, section 11.5.2.1, table 11-6,
> > non-PAT CPUs that have a WC MTRR, PCD = 1, and PWT = 1 (aka UC) have
> > an effective memory type of UC. Hence my suggestion to add
> > ioremap_x86_uc and/or set_memory_x86_uc to punch a UC hole in an
> > otherwise WC MTRR-covered region.
>
> OK I think I get it now.
>
> And I take it this would hopefully only be used for non-PAT systems?
> Would there be a use case for PAT systems? I wonder if we can wrap
> this under some APIs to make it clean and hide this dirty thing
> behind the scenes, it seems a fragile and error prone and my hope
> would be that we won't need more specialization in this area for
> PAT systems.
One potential complication is kernel vs. userspace mmap. MTRR applies to
the physical address, but PAT applies to the virtual address, so with
the WC MTRR you get WC for userspace "for free" as well. Also the
userspace mmaps request will have the length of smem_len (at most), so
it won't be the nice power of two in that case.
Also on PAT systems w/o a BIOS provided WC MTRR, the fbdev mmap seems
to be total crap at the moment. IIRC I have a patch to fix things a bit...
From 4e6d70d223f35953c8a11a58cf3376a8a001fa4f Mon Sep 17 00:00:00 2001
From: Ville Syrjala <syrjala@sci.fi>
Date: Fri, 15 Apr 2011 04:02:43 +0300
Subject: [PATCH] fb: writecombine fb
---
drivers/video/fbdev/core/fbmem.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0705d88..ecbde0e 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1396,6 +1396,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
unsigned long mmio_pgoff;
unsigned long start;
u32 len;
+ bool mmio = false;
if (!info)
return -ENODEV;
@@ -1426,11 +1427,20 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
vma->vm_pgoff -= mmio_pgoff;
start = info->fix.mmio_start;
len = info->fix.mmio_len;
+ mmio = true;
}
mutex_unlock(&info->mm_lock);
+ if (!mmio) {
+ vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
+ vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+
+ if (!vm_iomap_memory(vma, start, len))
+ return 0;
+ }
+
vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
- fb_pgprotect(file, vma, start);
+ vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
return vm_iomap_memory(vma, start, len);
}
Perhaps it's time I tried to send that upstream properly :P
--
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
^ permalink raw reply related
* [PATCH 70/86] fbdev/gxt4500: use uapi/linux/pci_ids.h directly
From: Michael S. Tsirkin @ 2015-03-29 13:42 UTC (permalink / raw)
To: linux-kernel
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Laurent Pinchart, Daniel Vetter, Rob Clark, Jingoo Han,
linux-fbdev
In-Reply-To: <1427635734-24786-1-git-send-email-mst@redhat.com>
Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/video/fbdev/gxt4500.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/gxt4500.c b/drivers/video/fbdev/gxt4500.c
index 135d78a..2415d3d 100644
--- a/drivers/video/fbdev/gxt4500.c
+++ b/drivers/video/fbdev/gxt4500.c
@@ -10,7 +10,7 @@
#include <linux/fb.h>
#include <linux/console.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include <linux/delay.h>
#include <linux/string.h>
--
MST
^ permalink raw reply related
* [PATCH 71/86] fbdev/i740fb: use uapi/linux/pci_ids.h directly
From: Michael S. Tsirkin @ 2015-03-29 13:42 UTC (permalink / raw)
To: linux-kernel
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Bjorn Helgaas,
Laurent Pinchart, Benoit Taine, linux-fbdev
In-Reply-To: <1427635734-24786-1-git-send-email-mst@redhat.com>
Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/video/fbdev/i740fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/i740fb.c b/drivers/video/fbdev/i740fb.c
index a2b4204..abad615 100644
--- a/drivers/video/fbdev/i740fb.c
+++ b/drivers/video/fbdev/i740fb.c
@@ -21,7 +21,7 @@
#include <linux/fb.h>
#include <linux/init.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include <linux/i2c.h>
#include <linux/i2c-algo-bit.h>
#include <linux/console.h>
--
MST
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox