| 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
| ||
|
Message-ID: <20131221185045.GA22728@google.com>
Date: Sat, 21 Dec 2013 11:50:45 -0700
From: Bjorn Helgaas <bhelgaas@...gle.com>
To: Yinghai Lu <yinghai@...nel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Gu Zheng <guz.fnst@...fujitsu.com>,
Guo Chao <yan@...ux.vnet.ibm.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel.vetter@...ll.ch>
Subject: Re: [PATCH v2 10/10] intel-gtt: Read 64bit for gmar_bus_addr
[+cc Daniel]
On Fri, Dec 20, 2013 at 05:19:38PM -0800, Yinghai Lu wrote:
> On Fri, Dec 20, 2013 at 4:27 PM, Bjorn Helgaas <bhelgaas@...gle.com> wrote:
>
> > Why are we reading these BARs directly anyway? These look like
> > standard PCI BARs (I810_GMADDR == 0x10 and I915_GMADDR == 0x18), so
> > the PCI core should already be reading them correctly, shouldn't it?
> > Can't we just use pcibios_resource_to_bus(pci_resource_start())?
> >
> > It looks like i810_setup(), i830_setup(), and i9xx_setup() have the
> > same problem and should also be using pci_resource_start() or
> > something similar.
>
> Agreed.
>
> should be sth like:
>
>
> Index: linux-2.6/drivers/char/agp/intel-gtt.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/agp/intel-gtt.c
> +++ linux-2.6/drivers/char/agp/intel-gtt.c
> @@ -608,9 +608,9 @@ static bool intel_gtt_can_wc(void)
>
> static int intel_gtt_init(void)
> {
> - u32 gma_addr;
> + struct pci_bus_region r;
> u32 gtt_map_size;
> - int ret;
> + int ret, idx;
>
> ret = intel_private.driver->setup();
> if (ret != 0)
> @@ -660,13 +660,14 @@ static int intel_gtt_init(void)
> }
>
> if (INTEL_GTT_GEN <= 2)
> - pci_read_config_dword(intel_private.pcidev, I810_GMADDR,
> - &gma_addr);
> + idx = 0; /* I810_GMADDR */
> else
> - pci_read_config_dword(intel_private.pcidev, I915_GMADDR,
> - &gma_addr);
> + idx = 2; /* I915_GMADDR */
>
> - intel_private.gma_bus_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK);
> + pcibios_resource_to_bus(intel_private.pcidev->bus, &r,
> + &intel_private.pcidev->resource[idx]);
> +
> + intel_private.gma_bus_addr = r.start;
>
> return 0;
> }
I think it's even worse than we first thought. Not only does the current
code fail on 64-bit BARs, but it also ignores the CPU/bus address
difference, and I think we want the CPU address. So I think we need
something like these:
commit 8ba262d78f9d218672486c62ba6a1c7a073bd272
Author: Bjorn Helgaas <bhelgaas@...gle.com>
Date: Sat Dec 21 10:49:58 2013 -0700
agp/intel: Rename "*_bus_addr" to "*_phys_addr"
We're dealing with CPU physical addresses here, which may be different from
bus addresses, so rename "gtt_bus_addr" to "gtt_phys_addr" and
"gma_bus_addr" to "gma_phys_addr" to avoid confusion.
No functional change.
Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.com>
diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index b8e2014cb9cb..dca04a6aa7f8 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -64,7 +64,7 @@ static struct _intel_private {
struct pci_dev *pcidev; /* device one */
struct pci_dev *bridge_dev;
u8 __iomem *registers;
- phys_addr_t gtt_bus_addr;
+ phys_addr_t gtt_phys_addr;
u32 PGETBL_save;
u32 __iomem *gtt; /* I915G */
bool clear_fake_agp; /* on first access via agp, fill with scratch */
@@ -78,7 +78,7 @@ static struct _intel_private {
int refcount;
/* Whether i915 needs to use the dmar apis or not. */
unsigned int needs_dmar : 1;
- phys_addr_t gma_bus_addr;
+ phys_addr_t gma_phys_addr;
/* Size of memory reserved for graphics by the BIOS */
unsigned int stolen_size;
/* Total number of gtt entries. */
@@ -191,7 +191,7 @@ static int i810_setup(void)
writel(virt_to_phys(gtt_table) | I810_PGETBL_ENABLED,
intel_private.registers+I810_PGETBL_CTL);
- intel_private.gtt_bus_addr = reg_addr + I810_PTE_BASE;
+ intel_private.gtt_phys_addr = reg_addr + I810_PTE_BASE;
if ((readl(intel_private.registers+I810_DRAM_CTL)
& I810_DRAM_ROW_0) == I810_DRAM_ROW_0_SDRAM) {
@@ -636,10 +636,10 @@ static int intel_gtt_init(void)
intel_private.gtt = NULL;
if (intel_gtt_can_wc())
- intel_private.gtt = ioremap_wc(intel_private.gtt_bus_addr,
+ intel_private.gtt = ioremap_wc(intel_private.gtt_phys_addr,
gtt_map_size);
if (intel_private.gtt == NULL)
- intel_private.gtt = ioremap(intel_private.gtt_bus_addr,
+ intel_private.gtt = ioremap(intel_private.gtt_phys_addr,
gtt_map_size);
if (intel_private.gtt == NULL) {
intel_private.driver->cleanup();
@@ -666,7 +666,7 @@ static int intel_gtt_init(void)
pci_read_config_dword(intel_private.pcidev, I915_GMADDR,
&gma_addr);
- intel_private.gma_bus_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK);
+ intel_private.gma_phys_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK);
return 0;
}
@@ -796,7 +796,7 @@ static int i830_setup(void)
if (!intel_private.registers)
return -ENOMEM;
- intel_private.gtt_bus_addr = reg_addr + I810_PTE_BASE;
+ intel_private.gtt_phys_addr = reg_addr + I810_PTE_BASE;
return 0;
}
@@ -821,7 +821,7 @@ static int intel_fake_agp_configure(void)
return -EIO;
intel_private.clear_fake_agp = true;
- agp_bridge->gart_bus_addr = intel_private.gma_bus_addr;
+ agp_bridge->gart_bus_addr = intel_private.gma_phys_addr;
return 0;
}
@@ -1123,13 +1123,13 @@ static int i9xx_setup(void)
case 3:
pci_read_config_dword(intel_private.pcidev,
I915_PTEADDR, >t_addr);
- intel_private.gtt_bus_addr = gtt_addr;
+ intel_private.gtt_phys_addr = gtt_addr;
break;
case 5:
- intel_private.gtt_bus_addr = reg_addr + MB(2);
+ intel_private.gtt_phys_addr = reg_addr + MB(2);
break;
default:
- intel_private.gtt_bus_addr = reg_addr + KB(512);
+ intel_private.gtt_phys_addr = reg_addr + KB(512);
break;
}
@@ -1409,7 +1409,7 @@ void intel_gtt_get(size_t *gtt_total, size_t *stolen_size,
{
*gtt_total = intel_private.gtt_total_entries << PAGE_SHIFT;
*stolen_size = intel_private.stolen_size;
- *mappable_base = intel_private.gma_bus_addr;
+ *mappable_base = intel_private.gma_phys_addr;
*mappable_end = intel_private.gtt_mappable_entries << PAGE_SHIFT;
}
EXPORT_SYMBOL(intel_gtt_get);
commit 31349de4ce32a0c2c2d14df35717544e94d56066
Author: Bjorn Helgaas <bhelgaas@...gle.com>
Date: Sat Dec 21 10:09:19 2013 -0700
intel-gtt: Use CPU address (not BAR value) for GART
There were two problems here:
1) The GMADR can be either a 32-bit or a 64-bit BAR, but we only read the
low 32 bits, so this failed if GMADR was above 4GB.
2) The value read from the BAR is a bus address, not a CPU physical
address, and these may be different.
Use pci_resource_start() instead of reading the BAR directly to remove the
BAR size and bus/CPU address issue, as gen8_gmch_probe() already does.
Reference: http://lkml.kernel.org/r/1385429290-25397-11-git-send-email-yinghai@kernel.org
Based-on-patch-by: Yinghai Lu <yinghai@...nel.org>
Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.com>
diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index dca04a6aa7f8..35169ff0ffe1 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -610,7 +610,7 @@ static int intel_gtt_init(void)
{
u32 gma_addr;
u32 gtt_map_size;
- int ret;
+ int ret, bar;
ret = intel_private.driver->setup();
if (ret != 0)
@@ -660,14 +660,12 @@ static int intel_gtt_init(void)
}
if (INTEL_GTT_GEN <= 2)
- pci_read_config_dword(intel_private.pcidev, I810_GMADDR,
- &gma_addr);
+ bar = 0; /* I810_GMADDR */
else
- pci_read_config_dword(intel_private.pcidev, I915_GMADDR,
- &gma_addr);
-
- intel_private.gma_phys_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK);
+ bar = 2; /* I915_GMADDR */
+ intel_private.gma_phys_addr = pci_resource_start(intel_private.pcidev,
+ bar);
return 0;
}
--
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/
Powered by blists - more mailing lists