* [PATCH 0/2] iommu: request CSR space
@ 2012-05-18 23:18 ` Bjorn Helgaas
0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2012-05-18 23:18 UTC (permalink / raw)
To: David Woodhouse; +Cc: linux-kernel, iommu, Ingo Molnar, Suresh Siddha
This adds a request_mem_region() for the DRHD registers. The AMD
code has similar code in iommu_map_mmio_space().
This helps avoid address conflicts when assigning PCI BARs.
---
Bjorn Helgaas (2):
resources: set type of new resource returned by __request_region()
iommu: Request IOMMU CSR space
drivers/iommu/dmar.c | 41 ++++++++++++++++++++++++++++++++---------
include/linux/intel-iommu.h | 2 ++
kernel/resource.c | 3 ++-
3 files changed, 36 insertions(+), 10 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 0/2] iommu: request CSR space
@ 2012-05-18 23:18 ` Bjorn Helgaas
0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2012-05-18 23:18 UTC (permalink / raw)
To: David Woodhouse
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Ingo Molnar,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Suresh Siddha
This adds a request_mem_region() for the DRHD registers. The AMD
code has similar code in iommu_map_mmio_space().
This helps avoid address conflicts when assigning PCI BARs.
---
Bjorn Helgaas (2):
resources: set type of new resource returned by __request_region()
iommu: Request IOMMU CSR space
drivers/iommu/dmar.c | 41 ++++++++++++++++++++++++++++++++---------
include/linux/intel-iommu.h | 2 ++
kernel/resource.c | 3 ++-
3 files changed, 36 insertions(+), 10 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] resources: set type of new resource returned by __request_region()
@ 2012-05-18 23:18 ` Bjorn Helgaas
0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2012-05-18 23:18 UTC (permalink / raw)
To: David Woodhouse; +Cc: linux-kernel, iommu, Ingo Molnar, Suresh Siddha
Previously we returned a new struct resource with only IORESOURCE_BUSY
set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
MEM/IO/etc. bits set. The new resource should inherit the type of
its parent.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
kernel/resource.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/kernel/resource.c b/kernel/resource.c
index 7e8ea66..04bb527 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -847,7 +847,8 @@ struct resource * __request_region(struct resource *parent,
res->name = name;
res->start = start;
res->end = start + n - 1;
- res->flags = IORESOURCE_BUSY;
+ res->flags = parent->flags;
+ res->flags |= IORESOURCE_BUSY;
res->flags |= flags;
write_lock(&resource_lock);
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] resources: set type of new resource returned by __request_region()
@ 2012-05-18 23:18 ` Bjorn Helgaas
0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2012-05-18 23:18 UTC (permalink / raw)
To: David Woodhouse
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Ingo Molnar,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Suresh Siddha
Previously we returned a new struct resource with only IORESOURCE_BUSY
set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
MEM/IO/etc. bits set. The new resource should inherit the type of
its parent.
Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
kernel/resource.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/kernel/resource.c b/kernel/resource.c
index 7e8ea66..04bb527 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -847,7 +847,8 @@ struct resource * __request_region(struct resource *parent,
res->name = name;
res->start = start;
res->end = start + n - 1;
- res->flags = IORESOURCE_BUSY;
+ res->flags = parent->flags;
+ res->flags |= IORESOURCE_BUSY;
res->flags |= flags;
write_lock(&resource_lock);
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] iommu: Request IOMMU CSR space
@ 2012-05-18 23:18 ` Bjorn Helgaas
0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2012-05-18 23:18 UTC (permalink / raw)
To: David Woodhouse; +Cc: linux-kernel, iommu, Ingo Molnar, Suresh Siddha
Request the IOMMU CSR MMIO region to keep anybody else from claiming it
while we're using it.
The bug referenced below is a crash that happened when we assigned an
address in the IOMMU CSR area to a PCI BAR. This patch just changes
the IOMMU driver, which only helps when the driver is present, so a
BIOS change is also necessary. But the driver should claim the space
it uses in any event.
Reference: https://bugzilla.novell.com/show_bug.cgi?id=760440
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/iommu/dmar.c | 41 ++++++++++++++++++++++++++++++++---------
include/linux/intel-iommu.h | 2 ++
2 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 35c1e17..e4ac23a 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -581,6 +581,7 @@ int __init detect_intel_iommu(void)
int alloc_iommu(struct dmar_drhd_unit *drhd)
{
struct intel_iommu *iommu;
+ struct resource *res;
int map_size;
u32 ver;
static int iommu_allocated = 0;
@@ -599,11 +600,20 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
iommu->seq_id = iommu_allocated++;
sprintf (iommu->name, "dmar%d", iommu->seq_id);
- iommu->reg = ioremap(drhd->reg_base_addr, VTD_PAGE_SIZE);
- if (!iommu->reg) {
- printk(KERN_ERR "IOMMU: can't map the region\n");
+ iommu->reg_base = drhd->reg_base_addr;
+ iommu->reg_size = VTD_PAGE_SIZE;
+ res = request_mem_region(iommu->reg_base, iommu->reg_size, iommu->name);
+ if (!res) {
+ printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
+ iommu->reg_base, iommu->reg_base + iommu->reg_size - 1);
goto error;
}
+
+ iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
+ if (!iommu->reg) {
+ printk(KERN_ERR "IOMMU: can't map %pR\n", res);
+ goto err_release;
+ }
iommu->cap = dmar_readq(iommu->reg + DMAR_CAP_REG);
iommu->ecap = dmar_readq(iommu->reg + DMAR_ECAP_REG);
@@ -637,17 +647,26 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
map_size = VTD_PAGE_ALIGN(map_size);
if (map_size > VTD_PAGE_SIZE) {
iounmap(iommu->reg);
- iommu->reg = ioremap(drhd->reg_base_addr, map_size);
+ release_mem_region(iommu->reg_base, iommu->reg_size);
+ iommu->reg_size = map_size;
+ res = request_mem_region(iommu->reg_base, iommu->reg_size,
+ iommu->name);
+ if (!res) {
+ printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
+ iommu->reg_base,
+ iommu->reg_base + iommu->reg_size - 1);
+ goto error;
+ }
+ iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
if (!iommu->reg) {
- printk(KERN_ERR "IOMMU: can't map the region\n");
+ printk(KERN_ERR "IOMMU: can't map %pR\n", res);
goto error;
}
}
ver = readl(iommu->reg + DMAR_VER_REG);
- pr_info("IOMMU %d: reg_base_addr %llx ver %d:%d cap %llx ecap %llx\n",
- iommu->seq_id,
- (unsigned long long)drhd->reg_base_addr,
+ pr_info("IOMMU %d: %pR ver %d:%d cap %llx ecap %llx\n",
+ iommu->seq_id, res,
DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
(unsigned long long)iommu->cap,
(unsigned long long)iommu->ecap);
@@ -659,6 +678,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
err_unmap:
iounmap(iommu->reg);
+ err_release:
+ release_mem_region(iommu->reg_base, iommu->reg_size);
error:
kfree(iommu);
return -1;
@@ -671,8 +692,10 @@ void free_iommu(struct intel_iommu *iommu)
free_dmar_iommu(iommu);
- if (iommu->reg)
+ if (iommu->reg) {
iounmap(iommu->reg);
+ release_mem_region(iommu->reg_base, iommu->reg_size);
+ }
kfree(iommu);
}
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index e6ca56d..911ded0 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -308,6 +308,8 @@ enum {
struct intel_iommu {
void __iomem *reg; /* Pointer to hardware regs, virtual addr */
+ phys_addr_t reg_base;
+ resource_size_t reg_size;
u64 cap;
u64 ecap;
u32 gcmd; /* Holds TE, EAFL. Don't need SRTP, SFL, WBF */
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] iommu: Request IOMMU CSR space
@ 2012-05-18 23:18 ` Bjorn Helgaas
0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2012-05-18 23:18 UTC (permalink / raw)
To: David Woodhouse
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Ingo Molnar,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Suresh Siddha
Request the IOMMU CSR MMIO region to keep anybody else from claiming it
while we're using it.
The bug referenced below is a crash that happened when we assigned an
address in the IOMMU CSR area to a PCI BAR. This patch just changes
the IOMMU driver, which only helps when the driver is present, so a
BIOS change is also necessary. But the driver should claim the space
it uses in any event.
Reference: https://bugzilla.novell.com/show_bug.cgi?id=760440
Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
drivers/iommu/dmar.c | 41 ++++++++++++++++++++++++++++++++---------
include/linux/intel-iommu.h | 2 ++
2 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 35c1e17..e4ac23a 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -581,6 +581,7 @@ int __init detect_intel_iommu(void)
int alloc_iommu(struct dmar_drhd_unit *drhd)
{
struct intel_iommu *iommu;
+ struct resource *res;
int map_size;
u32 ver;
static int iommu_allocated = 0;
@@ -599,11 +600,20 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
iommu->seq_id = iommu_allocated++;
sprintf (iommu->name, "dmar%d", iommu->seq_id);
- iommu->reg = ioremap(drhd->reg_base_addr, VTD_PAGE_SIZE);
- if (!iommu->reg) {
- printk(KERN_ERR "IOMMU: can't map the region\n");
+ iommu->reg_base = drhd->reg_base_addr;
+ iommu->reg_size = VTD_PAGE_SIZE;
+ res = request_mem_region(iommu->reg_base, iommu->reg_size, iommu->name);
+ if (!res) {
+ printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
+ iommu->reg_base, iommu->reg_base + iommu->reg_size - 1);
goto error;
}
+
+ iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
+ if (!iommu->reg) {
+ printk(KERN_ERR "IOMMU: can't map %pR\n", res);
+ goto err_release;
+ }
iommu->cap = dmar_readq(iommu->reg + DMAR_CAP_REG);
iommu->ecap = dmar_readq(iommu->reg + DMAR_ECAP_REG);
@@ -637,17 +647,26 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
map_size = VTD_PAGE_ALIGN(map_size);
if (map_size > VTD_PAGE_SIZE) {
iounmap(iommu->reg);
- iommu->reg = ioremap(drhd->reg_base_addr, map_size);
+ release_mem_region(iommu->reg_base, iommu->reg_size);
+ iommu->reg_size = map_size;
+ res = request_mem_region(iommu->reg_base, iommu->reg_size,
+ iommu->name);
+ if (!res) {
+ printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
+ iommu->reg_base,
+ iommu->reg_base + iommu->reg_size - 1);
+ goto error;
+ }
+ iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
if (!iommu->reg) {
- printk(KERN_ERR "IOMMU: can't map the region\n");
+ printk(KERN_ERR "IOMMU: can't map %pR\n", res);
goto error;
}
}
ver = readl(iommu->reg + DMAR_VER_REG);
- pr_info("IOMMU %d: reg_base_addr %llx ver %d:%d cap %llx ecap %llx\n",
- iommu->seq_id,
- (unsigned long long)drhd->reg_base_addr,
+ pr_info("IOMMU %d: %pR ver %d:%d cap %llx ecap %llx\n",
+ iommu->seq_id, res,
DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
(unsigned long long)iommu->cap,
(unsigned long long)iommu->ecap);
@@ -659,6 +678,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
err_unmap:
iounmap(iommu->reg);
+ err_release:
+ release_mem_region(iommu->reg_base, iommu->reg_size);
error:
kfree(iommu);
return -1;
@@ -671,8 +692,10 @@ void free_iommu(struct intel_iommu *iommu)
free_dmar_iommu(iommu);
- if (iommu->reg)
+ if (iommu->reg) {
iounmap(iommu->reg);
+ release_mem_region(iommu->reg_base, iommu->reg_size);
+ }
kfree(iommu);
}
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index e6ca56d..911ded0 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -308,6 +308,8 @@ enum {
struct intel_iommu {
void __iomem *reg; /* Pointer to hardware regs, virtual addr */
+ phys_addr_t reg_base;
+ resource_size_t reg_size;
u64 cap;
u64 ecap;
u32 gcmd; /* Holds TE, EAFL. Don't need SRTP, SFL, WBF */
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] resources: set type of new resource returned by __request_region()
@ 2012-05-21 13:04 ` Joerg Roedel
0 siblings, 0 replies; 14+ messages in thread
From: Joerg Roedel @ 2012-05-21 13:04 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: David Woodhouse, linux-kernel, iommu, Ingo Molnar, Suresh Siddha
On Fri, May 18, 2012 at 05:18:32PM -0600, Bjorn Helgaas wrote:
> Previously we returned a new struct resource with only IORESOURCE_BUSY
> set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
> MEM/IO/etc. bits set. The new resource should inherit the type of
> its parent.
Should it? What about IORESOURCE_WINDOW for example? Any particular
reason for this change to the interface of the __request_region()
function?
Joerg
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] resources: set type of new resource returned by __request_region()
@ 2012-05-21 13:04 ` Joerg Roedel
0 siblings, 0 replies; 14+ messages in thread
From: Joerg Roedel @ 2012-05-21 13:04 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Ingo Molnar,
David Woodhouse, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Suresh Siddha
On Fri, May 18, 2012 at 05:18:32PM -0600, Bjorn Helgaas wrote:
> Previously we returned a new struct resource with only IORESOURCE_BUSY
> set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
> MEM/IO/etc. bits set. The new resource should inherit the type of
> its parent.
Should it? What about IORESOURCE_WINDOW for example? Any particular
reason for this change to the interface of the __request_region()
function?
Joerg
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] iommu: Request IOMMU CSR space
@ 2012-05-21 13:15 ` Joerg Roedel
0 siblings, 0 replies; 14+ messages in thread
From: Joerg Roedel @ 2012-05-21 13:15 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: David Woodhouse, linux-kernel, iommu, Ingo Molnar, Suresh Siddha
On Fri, May 18, 2012 at 05:18:37PM -0600, Bjorn Helgaas wrote:
> Request the IOMMU CSR MMIO region to keep anybody else from claiming it
> while we're using it.
>
> The bug referenced below is a crash that happened when we assigned an
> address in the IOMMU CSR area to a PCI BAR. This patch just changes
> the IOMMU driver, which only helps when the driver is present, so a
> BIOS change is also necessary. But the driver should claim the space
> it uses in any event.
>
> Reference: https://bugzilla.novell.com/show_bug.cgi?id=760440
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Looks good.
Reviewed-by: Joerg Roedel <joro@8bytes.org>
> ---
> drivers/iommu/dmar.c | 41 ++++++++++++++++++++++++++++++++---------
> include/linux/intel-iommu.h | 2 ++
> 2 files changed, 34 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index 35c1e17..e4ac23a 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -581,6 +581,7 @@ int __init detect_intel_iommu(void)
> int alloc_iommu(struct dmar_drhd_unit *drhd)
> {
> struct intel_iommu *iommu;
> + struct resource *res;
> int map_size;
> u32 ver;
> static int iommu_allocated = 0;
> @@ -599,11 +600,20 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
> iommu->seq_id = iommu_allocated++;
> sprintf (iommu->name, "dmar%d", iommu->seq_id);
>
> - iommu->reg = ioremap(drhd->reg_base_addr, VTD_PAGE_SIZE);
> - if (!iommu->reg) {
> - printk(KERN_ERR "IOMMU: can't map the region\n");
> + iommu->reg_base = drhd->reg_base_addr;
> + iommu->reg_size = VTD_PAGE_SIZE;
> + res = request_mem_region(iommu->reg_base, iommu->reg_size, iommu->name);
> + if (!res) {
> + printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
> + iommu->reg_base, iommu->reg_base + iommu->reg_size - 1);
> goto error;
> }
> +
> + iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
> + if (!iommu->reg) {
> + printk(KERN_ERR "IOMMU: can't map %pR\n", res);
> + goto err_release;
> + }
> iommu->cap = dmar_readq(iommu->reg + DMAR_CAP_REG);
> iommu->ecap = dmar_readq(iommu->reg + DMAR_ECAP_REG);
>
> @@ -637,17 +647,26 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
> map_size = VTD_PAGE_ALIGN(map_size);
> if (map_size > VTD_PAGE_SIZE) {
> iounmap(iommu->reg);
> - iommu->reg = ioremap(drhd->reg_base_addr, map_size);
> + release_mem_region(iommu->reg_base, iommu->reg_size);
> + iommu->reg_size = map_size;
> + res = request_mem_region(iommu->reg_base, iommu->reg_size,
> + iommu->name);
> + if (!res) {
> + printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
> + iommu->reg_base,
> + iommu->reg_base + iommu->reg_size - 1);
> + goto error;
> + }
> + iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
> if (!iommu->reg) {
> - printk(KERN_ERR "IOMMU: can't map the region\n");
> + printk(KERN_ERR "IOMMU: can't map %pR\n", res);
> goto error;
> }
> }
>
> ver = readl(iommu->reg + DMAR_VER_REG);
> - pr_info("IOMMU %d: reg_base_addr %llx ver %d:%d cap %llx ecap %llx\n",
> - iommu->seq_id,
> - (unsigned long long)drhd->reg_base_addr,
> + pr_info("IOMMU %d: %pR ver %d:%d cap %llx ecap %llx\n",
> + iommu->seq_id, res,
> DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
> (unsigned long long)iommu->cap,
> (unsigned long long)iommu->ecap);
> @@ -659,6 +678,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
>
> err_unmap:
> iounmap(iommu->reg);
> + err_release:
> + release_mem_region(iommu->reg_base, iommu->reg_size);
> error:
> kfree(iommu);
> return -1;
> @@ -671,8 +692,10 @@ void free_iommu(struct intel_iommu *iommu)
>
> free_dmar_iommu(iommu);
>
> - if (iommu->reg)
> + if (iommu->reg) {
> iounmap(iommu->reg);
> + release_mem_region(iommu->reg_base, iommu->reg_size);
> + }
> kfree(iommu);
> }
>
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index e6ca56d..911ded0 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -308,6 +308,8 @@ enum {
>
> struct intel_iommu {
> void __iomem *reg; /* Pointer to hardware regs, virtual addr */
> + phys_addr_t reg_base;
> + resource_size_t reg_size;
> u64 cap;
> u64 ecap;
> u32 gcmd; /* Holds TE, EAFL. Don't need SRTP, SFL, WBF */
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] iommu: Request IOMMU CSR space
@ 2012-05-21 13:15 ` Joerg Roedel
0 siblings, 0 replies; 14+ messages in thread
From: Joerg Roedel @ 2012-05-21 13:15 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Ingo Molnar,
David Woodhouse, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Suresh Siddha
On Fri, May 18, 2012 at 05:18:37PM -0600, Bjorn Helgaas wrote:
> Request the IOMMU CSR MMIO region to keep anybody else from claiming it
> while we're using it.
>
> The bug referenced below is a crash that happened when we assigned an
> address in the IOMMU CSR area to a PCI BAR. This patch just changes
> the IOMMU driver, which only helps when the driver is present, so a
> BIOS change is also necessary. But the driver should claim the space
> it uses in any event.
>
> Reference: https://bugzilla.novell.com/show_bug.cgi?id=760440
> Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Looks good.
Reviewed-by: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> ---
> drivers/iommu/dmar.c | 41 ++++++++++++++++++++++++++++++++---------
> include/linux/intel-iommu.h | 2 ++
> 2 files changed, 34 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index 35c1e17..e4ac23a 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -581,6 +581,7 @@ int __init detect_intel_iommu(void)
> int alloc_iommu(struct dmar_drhd_unit *drhd)
> {
> struct intel_iommu *iommu;
> + struct resource *res;
> int map_size;
> u32 ver;
> static int iommu_allocated = 0;
> @@ -599,11 +600,20 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
> iommu->seq_id = iommu_allocated++;
> sprintf (iommu->name, "dmar%d", iommu->seq_id);
>
> - iommu->reg = ioremap(drhd->reg_base_addr, VTD_PAGE_SIZE);
> - if (!iommu->reg) {
> - printk(KERN_ERR "IOMMU: can't map the region\n");
> + iommu->reg_base = drhd->reg_base_addr;
> + iommu->reg_size = VTD_PAGE_SIZE;
> + res = request_mem_region(iommu->reg_base, iommu->reg_size, iommu->name);
> + if (!res) {
> + printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
> + iommu->reg_base, iommu->reg_base + iommu->reg_size - 1);
> goto error;
> }
> +
> + iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
> + if (!iommu->reg) {
> + printk(KERN_ERR "IOMMU: can't map %pR\n", res);
> + goto err_release;
> + }
> iommu->cap = dmar_readq(iommu->reg + DMAR_CAP_REG);
> iommu->ecap = dmar_readq(iommu->reg + DMAR_ECAP_REG);
>
> @@ -637,17 +647,26 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
> map_size = VTD_PAGE_ALIGN(map_size);
> if (map_size > VTD_PAGE_SIZE) {
> iounmap(iommu->reg);
> - iommu->reg = ioremap(drhd->reg_base_addr, map_size);
> + release_mem_region(iommu->reg_base, iommu->reg_size);
> + iommu->reg_size = map_size;
> + res = request_mem_region(iommu->reg_base, iommu->reg_size,
> + iommu->name);
> + if (!res) {
> + printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
> + iommu->reg_base,
> + iommu->reg_base + iommu->reg_size - 1);
> + goto error;
> + }
> + iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
> if (!iommu->reg) {
> - printk(KERN_ERR "IOMMU: can't map the region\n");
> + printk(KERN_ERR "IOMMU: can't map %pR\n", res);
> goto error;
> }
> }
>
> ver = readl(iommu->reg + DMAR_VER_REG);
> - pr_info("IOMMU %d: reg_base_addr %llx ver %d:%d cap %llx ecap %llx\n",
> - iommu->seq_id,
> - (unsigned long long)drhd->reg_base_addr,
> + pr_info("IOMMU %d: %pR ver %d:%d cap %llx ecap %llx\n",
> + iommu->seq_id, res,
> DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
> (unsigned long long)iommu->cap,
> (unsigned long long)iommu->ecap);
> @@ -659,6 +678,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
>
> err_unmap:
> iounmap(iommu->reg);
> + err_release:
> + release_mem_region(iommu->reg_base, iommu->reg_size);
> error:
> kfree(iommu);
> return -1;
> @@ -671,8 +692,10 @@ void free_iommu(struct intel_iommu *iommu)
>
> free_dmar_iommu(iommu);
>
> - if (iommu->reg)
> + if (iommu->reg) {
> iounmap(iommu->reg);
> + release_mem_region(iommu->reg_base, iommu->reg_size);
> + }
> kfree(iommu);
> }
>
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index e6ca56d..911ded0 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -308,6 +308,8 @@ enum {
>
> struct intel_iommu {
> void __iomem *reg; /* Pointer to hardware regs, virtual addr */
> + phys_addr_t reg_base;
> + resource_size_t reg_size;
> u64 cap;
> u64 ecap;
> u32 gcmd; /* Holds TE, EAFL. Don't need SRTP, SFL, WBF */
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] resources: set type of new resource returned by __request_region()
@ 2012-05-21 13:52 ` Bjorn Helgaas
0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2012-05-21 13:52 UTC (permalink / raw)
To: Joerg Roedel
Cc: David Woodhouse, linux-kernel, iommu, Ingo Molnar, Suresh Siddha
On Mon, May 21, 2012 at 7:04 AM, Joerg Roedel <joro@8bytes.org> wrote:
> On Fri, May 18, 2012 at 05:18:32PM -0600, Bjorn Helgaas wrote:
>> Previously we returned a new struct resource with only IORESOURCE_BUSY
>> set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
>> MEM/IO/etc. bits set. The new resource should inherit the type of
>> its parent.
>
> Should it? What about IORESOURCE_WINDOW for example? Any particular
> reason for this change to the interface of the __request_region()
> function?
Yes, I should have explained the connection :) The next patch prints
the resource returned from request_mem_region(). Without this
__request_region() patch, that resource has no type, so %pR can't
print it correctly.
Good question about IORESOURCE_WINDOW. Maybe it should inherit only
the IORESOURCE_TYPE_BITS part (IO/MEM/IRQ/DMA/BUS). But it seemed
like it should also inherit PREFETCH, READONLY, CACHEABLE, etc. Maybe
we should inherit everything but clear IORESOURCE_WINDOW? I'm open to
suggestions here.
Bjorn
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] resources: set type of new resource returned by __request_region()
@ 2012-05-21 13:52 ` Bjorn Helgaas
0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2012-05-21 13:52 UTC (permalink / raw)
To: Joerg Roedel
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Ingo Molnar,
David Woodhouse, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Suresh Siddha
On Mon, May 21, 2012 at 7:04 AM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, May 18, 2012 at 05:18:32PM -0600, Bjorn Helgaas wrote:
>> Previously we returned a new struct resource with only IORESOURCE_BUSY
>> set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
>> MEM/IO/etc. bits set. The new resource should inherit the type of
>> its parent.
>
> Should it? What about IORESOURCE_WINDOW for example? Any particular
> reason for this change to the interface of the __request_region()
> function?
Yes, I should have explained the connection :) The next patch prints
the resource returned from request_mem_region(). Without this
__request_region() patch, that resource has no type, so %pR can't
print it correctly.
Good question about IORESOURCE_WINDOW. Maybe it should inherit only
the IORESOURCE_TYPE_BITS part (IO/MEM/IRQ/DMA/BUS). But it seemed
like it should also inherit PREFETCH, READONLY, CACHEABLE, etc. Maybe
we should inherit everything but clear IORESOURCE_WINDOW? I'm open to
suggestions here.
Bjorn
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] resources: set type of new resource returned by __request_region()
@ 2012-05-23 10:40 ` Joerg Roedel
0 siblings, 0 replies; 14+ messages in thread
From: Joerg Roedel @ 2012-05-23 10:40 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: David Woodhouse, linux-kernel, iommu, Ingo Molnar, Suresh Siddha
On Mon, May 21, 2012 at 07:52:24AM -0600, Bjorn Helgaas wrote:
> On Mon, May 21, 2012 at 7:04 AM, Joerg Roedel <joro@8bytes.org> wrote:
> > On Fri, May 18, 2012 at 05:18:32PM -0600, Bjorn Helgaas wrote:
> >> Previously we returned a new struct resource with only IORESOURCE_BUSY
> >> set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
> >> MEM/IO/etc. bits set. The new resource should inherit the type of
> >> its parent.
> >
> > Should it? What about IORESOURCE_WINDOW for example? Any particular
> > reason for this change to the interface of the __request_region()
> > function?
>
> Yes, I should have explained the connection :) The next patch prints
> the resource returned from request_mem_region(). Without this
> __request_region() patch, that resource has no type, so %pR can't
> print it correctly.
>
> Good question about IORESOURCE_WINDOW. Maybe it should inherit only
> the IORESOURCE_TYPE_BITS part (IO/MEM/IRQ/DMA/BUS). But it seemed
> like it should also inherit PREFETCH, READONLY, CACHEABLE, etc. Maybe
> we should inherit everything but clear IORESOURCE_WINDOW? I'm open to
> suggestions here.
I am still not sure whether the inheritance is a good idea. How about
changing the define of request_mem_region to:
#define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MEM)
?
Joerg
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] resources: set type of new resource returned by __request_region()
@ 2012-05-23 10:40 ` Joerg Roedel
0 siblings, 0 replies; 14+ messages in thread
From: Joerg Roedel @ 2012-05-23 10:40 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Ingo Molnar,
David Woodhouse, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Suresh Siddha
On Mon, May 21, 2012 at 07:52:24AM -0600, Bjorn Helgaas wrote:
> On Mon, May 21, 2012 at 7:04 AM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> > On Fri, May 18, 2012 at 05:18:32PM -0600, Bjorn Helgaas wrote:
> >> Previously we returned a new struct resource with only IORESOURCE_BUSY
> >> set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
> >> MEM/IO/etc. bits set. The new resource should inherit the type of
> >> its parent.
> >
> > Should it? What about IORESOURCE_WINDOW for example? Any particular
> > reason for this change to the interface of the __request_region()
> > function?
>
> Yes, I should have explained the connection :) The next patch prints
> the resource returned from request_mem_region(). Without this
> __request_region() patch, that resource has no type, so %pR can't
> print it correctly.
>
> Good question about IORESOURCE_WINDOW. Maybe it should inherit only
> the IORESOURCE_TYPE_BITS part (IO/MEM/IRQ/DMA/BUS). But it seemed
> like it should also inherit PREFETCH, READONLY, CACHEABLE, etc. Maybe
> we should inherit everything but clear IORESOURCE_WINDOW? I'm open to
> suggestions here.
I am still not sure whether the inheritance is a good idea. How about
changing the define of request_mem_region to:
#define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MEM)
?
Joerg
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-05-23 10:40 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-18 23:18 [PATCH 0/2] iommu: request CSR space Bjorn Helgaas
2012-05-18 23:18 ` Bjorn Helgaas
2012-05-18 23:18 ` [PATCH 1/2] resources: set type of new resource returned by __request_region() Bjorn Helgaas
2012-05-18 23:18 ` Bjorn Helgaas
2012-05-21 13:04 ` Joerg Roedel
2012-05-21 13:04 ` Joerg Roedel
2012-05-21 13:52 ` Bjorn Helgaas
2012-05-21 13:52 ` Bjorn Helgaas
2012-05-23 10:40 ` Joerg Roedel
2012-05-23 10:40 ` Joerg Roedel
2012-05-18 23:18 ` [PATCH 2/2] iommu: Request IOMMU CSR space Bjorn Helgaas
2012-05-18 23:18 ` Bjorn Helgaas
2012-05-21 13:15 ` Joerg Roedel
2012-05-21 13:15 ` Joerg Roedel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.