linux-kernel - Re: [PATCH v1 05/47] pci: add pci_iomap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAErSpo5LfgTM879jmt58yLrXD6e7spsaX1wNDcc2ppU9JrMXOg@mail.gmail.com>
Date:	Mon, 23 Mar 2015 12:20:47 -0500
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	"Luis R. Rodriguez" <mcgrof@...not-panic.com>
Cc:	Andy Lutomirski <luto@...capital.net>,
	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, jgross@...e.com,
	Jan Beulich <JBeulich@...e.com>, Borislav Petkov <bp@...e.de>,
	Suresh Siddha <suresh.b.siddha@...el.com>,
	venkatesh.pallipadi@...el.com, Dave Airlie <airlied@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-fbdev@...r.kernel.org, "x86@...nel.org" <x86@...nel.org>,
	"xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
	"Luis R. Rodriguez" <mcgrof@...e.com>, Ingo Molnar <mingo@...e.hu>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Antonino Daplas <adaplas@...il.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Tomi Valkeinen <tomi.valkeinen@...com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Arnd Bergmann <arnd@...db.de>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Stefan Bader <stefan.bader@...onical.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Ville Syrjälä <ville.syrjala@...ux.intel.com>,
	David Vrabel <david.vrabel@...rix.com>,
	Toshi Kani <toshi.kani@...com>,
	Roger Pau Monné <roger.pau@...rix.com>,
	xen-devel <xen-devel@...ts.xensource.com>
Subject: Re: [PATCH v1 05/47] pci: add pci_iomap_wc() variants

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
<mcgrof@...not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@...e.com>
>
> 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.

> +               return ioremap_wc(start, len);
> +       /* What? */
> +       return NULL;
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/