* [PATCH 1/4] AMD IOMMU: don't BUG() when we don't have to
2013-02-15 16:13 [PATCH 0/4] AMD IOMMU: misc adjustments Jan Beulich
@ 2013-02-15 16:19 ` Jan Beulich
2013-02-15 17:34 ` Boris Ostrovsky
2013-02-15 16:20 ` [PATCH 2/4] AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0 Jan Beulich
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2013-02-15 16:19 UTC (permalink / raw)
To: xen-devel; +Cc: Boris Ostrovsky, Sherry Hurwitz
[-- Attachment #1: Type: text/plain, Size: 655 bytes --]
find_iommu_for_device() can easily return NULL instead, as all of its
callers are prepared for that.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -32,8 +32,8 @@ struct amd_iommu *find_iommu_for_device(
{
struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
- BUG_ON ( bdf >= ivrs_bdf_entries );
- return ivrs_mappings ? ivrs_mappings[bdf].iommu : NULL;
+ return ivrs_mappings && bdf < ivrs_bdf_entries ? ivrs_mappings[bdf].iommu
+ : NULL;
}
/*
[-- Attachment #2: AMD-IOMMU-replace-BUG_ON.patch --]
[-- Type: text/plain, Size: 697 bytes --]
AMD IOMMU: don't BUG() when we don't have to
find_iommu_for_device() can easily return NULL instead, as all of its
callers are prepared for that.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -32,8 +32,8 @@ struct amd_iommu *find_iommu_for_device(
{
struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
- BUG_ON ( bdf >= ivrs_bdf_entries );
- return ivrs_mappings ? ivrs_mappings[bdf].iommu : NULL;
+ return ivrs_mappings && bdf < ivrs_bdf_entries ? ivrs_mappings[bdf].iommu
+ : NULL;
}
/*
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/4] AMD IOMMU: don't BUG() when we don't have to
2013-02-15 16:19 ` [PATCH 1/4] AMD IOMMU: don't BUG() when we don't have to Jan Beulich
@ 2013-02-15 17:34 ` Boris Ostrovsky
2013-02-18 7:58 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Boris Ostrovsky @ 2013-02-15 17:34 UTC (permalink / raw)
To: Jan Beulich; +Cc: Sherry Hurwitz, xen-devel
On 02/15/2013 11:19 AM, Jan Beulich wrote:
> find_iommu_for_device() can easily return NULL instead, as all of its
> callers are prepared for that.
This patch is obsoleted by the second patch ("[PATCH 2/4] AMD IOMMU:
cover all functions of a device even if ACPI only tells us of func 0"),
isn't it?
-boris
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -32,8 +32,8 @@ struct amd_iommu *find_iommu_for_device(
> {
> struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
>
> - BUG_ON ( bdf >= ivrs_bdf_entries );
> - return ivrs_mappings ? ivrs_mappings[bdf].iommu : NULL;
> + return ivrs_mappings && bdf < ivrs_bdf_entries ? ivrs_mappings[bdf].iommu
> + : NULL;
> }
>
> /*
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/4] AMD IOMMU: don't BUG() when we don't have to
2013-02-15 17:34 ` Boris Ostrovsky
@ 2013-02-18 7:58 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2013-02-18 7:58 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: xen-devel, Sherry Hurwitz
>>> On 15.02.13 at 18:34, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> On 02/15/2013 11:19 AM, Jan Beulich wrote:
>> find_iommu_for_device() can easily return NULL instead, as all of its
>> callers are prepared for that.
>
> This patch is obsoleted by the second patch ("[PATCH 2/4] AMD IOMMU:
> cover all functions of a device even if ACPI only tells us of func 0"),
> isn't it?
Yes it is, but it is an (I think) obviously correct thing to do, and
hence much more of a backporting candidate than the other one.
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/4] AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0
2013-02-15 16:13 [PATCH 0/4] AMD IOMMU: misc adjustments Jan Beulich
2013-02-15 16:19 ` [PATCH 1/4] AMD IOMMU: don't BUG() when we don't have to Jan Beulich
@ 2013-02-15 16:20 ` Jan Beulich
2013-02-15 17:21 ` Boris Ostrovsky
2013-02-15 16:21 ` [PATCH 3/4] IOMMU, AMD Family15h Model10-1Fh erratum 746 Workaround Jan Beulich
2013-02-15 16:22 ` [PATCH 4/4] AMD IOMMU: use __ioapic_read_entry() instead of open coding it Jan Beulich
3 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2013-02-15 16:20 UTC (permalink / raw)
To: xen-devel; +Cc: Boris Ostrovsky, Sherry Hurwitz
[-- Attachment #1: Type: text/plain, Size: 5111 bytes --]
This ought to work as all functions of a device have the same place in
the bus topology, i.e. use the same IOMMU.
Also fix the type of ivrs_bdf_entries (when it's 'unsigned short' and
the last device found on a segment is ff:1f.x, it would otherwise end
up being zero).
And drop the bogus 'last_bdf' static variable, which conflicted anyway
with various functions' parameters.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -54,8 +54,6 @@ union acpi_ivhd_device {
struct acpi_ivrs_device8c special;
};
-static unsigned short __initdata last_bdf;
-
static void __init add_ivrs_mapping_entry(
u16 bdf, u16 alias_id, u8 flags, struct amd_iommu *iommu)
{
@@ -991,6 +989,7 @@ static int __init get_last_bdf_ivhd(
{
const union acpi_ivhd_device *ivhd_device;
u16 block_length, dev_length;
+ int last_bdf = 0;
if ( ivhd_block->header.length < sizeof(*ivhd_block) )
{
@@ -1051,27 +1050,34 @@ static int __init get_last_bdf_ivhd(
return -ENODEV;
}
- return 0;
+ return last_bdf;
}
static int __init get_last_bdf_acpi(struct acpi_table_header *table)
{
const struct acpi_ivrs_header *ivrs_block;
unsigned long length = sizeof(struct acpi_table_ivrs);
+ int last_bdf = 0;
while ( table->length > (length + sizeof(*ivrs_block)) )
{
ivrs_block = (struct acpi_ivrs_header *)((u8 *)table + length);
if ( table->length < (length + ivrs_block->length) )
return -ENODEV;
- if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE &&
- get_last_bdf_ivhd(
+ if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE )
+ {
+ int ret = get_last_bdf_ivhd(
container_of(ivrs_block, const struct acpi_ivrs_hardware,
- header)) != 0 )
- return -ENODEV;
+ header));
+
+ if ( ret < 0 )
+ return ret;
+ UPDATE_LAST_BDF(ret);
+ }
length += ivrs_block->length;
}
- return 0;
+
+ return last_bdf;
}
int __init amd_iommu_detect_acpi(void)
@@ -1081,8 +1087,9 @@ int __init amd_iommu_detect_acpi(void)
int __init amd_iommu_get_ivrs_dev_entries(void)
{
- acpi_table_parse(ACPI_SIG_IVRS, get_last_bdf_acpi);
- return last_bdf + 1;
+ int ret = acpi_table_parse(ACPI_SIG_IVRS, get_last_bdf_acpi);
+
+ return ret < 0 ? ret : (ret | PCI_FUNC(~0)) + 1;
}
int __init amd_iommu_update_ivrs_mapping_acpi(void)
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -35,7 +35,7 @@ static int __initdata nr_amd_iommus;
static struct tasklet amd_iommu_irq_tasklet;
-unsigned short ivrs_bdf_entries;
+unsigned int __read_mostly ivrs_bdf_entries;
static struct radix_tree_root ivrs_maps;
struct list_head amd_iommu_head;
struct table_struct device_table;
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -28,12 +28,38 @@
#include <asm/hvm/svm/amd-iommu-proto.h>
#include "../ats.h"
+static bool_t __read_mostly init_done;
+
struct amd_iommu *find_iommu_for_device(int seg, int bdf)
{
struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
- return ivrs_mappings && bdf < ivrs_bdf_entries ? ivrs_mappings[bdf].iommu
- : NULL;
+ if ( !ivrs_mappings || bdf >= ivrs_bdf_entries )
+ return NULL;
+
+ if ( unlikely(!ivrs_mappings[bdf].iommu) && likely(init_done) )
+ {
+ unsigned int bd0 = bdf & ~PCI_FUNC(~0);
+
+ if ( ivrs_mappings[bd0].iommu )
+ {
+ struct ivrs_mappings tmp = ivrs_mappings[bd0];
+
+ tmp.iommu = NULL;
+ if ( tmp.dte_requestor_id == bd0 )
+ tmp.dte_requestor_id = bdf;
+ ivrs_mappings[bdf] = tmp;
+
+ printk(XENLOG_WARNING "%04x:%02x:%02x.%u not found in ACPI tables;"
+ " using same IOMMU as function 0\n",
+ seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf));
+
+ /* write iommu field last */
+ ivrs_mappings[bdf].iommu = ivrs_mappings[bd0].iommu;
+ }
+ }
+
+ return ivrs_mappings[bdf].iommu;
}
/*
@@ -179,6 +205,8 @@ int __init amd_iov_detect(void)
return -ENODEV;
}
+ init_done = 1;
+
/*
* AMD IOMMUs don't distinguish between vectors destined for
* different cpus when doing interrupt remapping. This means
--- a/xen/include/asm-x86/amd-iommu.h
+++ b/xen/include/asm-x86/amd-iommu.h
@@ -125,7 +125,7 @@ struct ivrs_mappings {
u8 device_flags;
};
-extern unsigned short ivrs_bdf_entries;
+extern unsigned int ivrs_bdf_entries;
struct ivrs_mappings *get_ivrs_mappings(u16 seg);
int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
[-- Attachment #2: AMD-IOMMU-cover-all-functions.patch --]
[-- Type: text/plain, Size: 5190 bytes --]
AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0
This ought to work as all functions of a device have the same place in
the bus topology, i.e. use the same IOMMU.
Also fix the type of ivrs_bdf_entries (when it's 'unsigned short' and
the last device found on a segment is ff:1f.x, it would otherwise end
up being zero).
And drop the bogus 'last_bdf' static variable, which conflicted anyway
with various functions' parameters.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -54,8 +54,6 @@ union acpi_ivhd_device {
struct acpi_ivrs_device8c special;
};
-static unsigned short __initdata last_bdf;
-
static void __init add_ivrs_mapping_entry(
u16 bdf, u16 alias_id, u8 flags, struct amd_iommu *iommu)
{
@@ -991,6 +989,7 @@ static int __init get_last_bdf_ivhd(
{
const union acpi_ivhd_device *ivhd_device;
u16 block_length, dev_length;
+ int last_bdf = 0;
if ( ivhd_block->header.length < sizeof(*ivhd_block) )
{
@@ -1051,27 +1050,34 @@ static int __init get_last_bdf_ivhd(
return -ENODEV;
}
- return 0;
+ return last_bdf;
}
static int __init get_last_bdf_acpi(struct acpi_table_header *table)
{
const struct acpi_ivrs_header *ivrs_block;
unsigned long length = sizeof(struct acpi_table_ivrs);
+ int last_bdf = 0;
while ( table->length > (length + sizeof(*ivrs_block)) )
{
ivrs_block = (struct acpi_ivrs_header *)((u8 *)table + length);
if ( table->length < (length + ivrs_block->length) )
return -ENODEV;
- if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE &&
- get_last_bdf_ivhd(
+ if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE )
+ {
+ int ret = get_last_bdf_ivhd(
container_of(ivrs_block, const struct acpi_ivrs_hardware,
- header)) != 0 )
- return -ENODEV;
+ header));
+
+ if ( ret < 0 )
+ return ret;
+ UPDATE_LAST_BDF(ret);
+ }
length += ivrs_block->length;
}
- return 0;
+
+ return last_bdf;
}
int __init amd_iommu_detect_acpi(void)
@@ -1081,8 +1087,9 @@ int __init amd_iommu_detect_acpi(void)
int __init amd_iommu_get_ivrs_dev_entries(void)
{
- acpi_table_parse(ACPI_SIG_IVRS, get_last_bdf_acpi);
- return last_bdf + 1;
+ int ret = acpi_table_parse(ACPI_SIG_IVRS, get_last_bdf_acpi);
+
+ return ret < 0 ? ret : (ret | PCI_FUNC(~0)) + 1;
}
int __init amd_iommu_update_ivrs_mapping_acpi(void)
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -35,7 +35,7 @@ static int __initdata nr_amd_iommus;
static struct tasklet amd_iommu_irq_tasklet;
-unsigned short ivrs_bdf_entries;
+unsigned int __read_mostly ivrs_bdf_entries;
static struct radix_tree_root ivrs_maps;
struct list_head amd_iommu_head;
struct table_struct device_table;
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -28,12 +28,38 @@
#include <asm/hvm/svm/amd-iommu-proto.h>
#include "../ats.h"
+static bool_t __read_mostly init_done;
+
struct amd_iommu *find_iommu_for_device(int seg, int bdf)
{
struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
- return ivrs_mappings && bdf < ivrs_bdf_entries ? ivrs_mappings[bdf].iommu
- : NULL;
+ if ( !ivrs_mappings || bdf >= ivrs_bdf_entries )
+ return NULL;
+
+ if ( unlikely(!ivrs_mappings[bdf].iommu) && likely(init_done) )
+ {
+ unsigned int bd0 = bdf & ~PCI_FUNC(~0);
+
+ if ( ivrs_mappings[bd0].iommu )
+ {
+ struct ivrs_mappings tmp = ivrs_mappings[bd0];
+
+ tmp.iommu = NULL;
+ if ( tmp.dte_requestor_id == bd0 )
+ tmp.dte_requestor_id = bdf;
+ ivrs_mappings[bdf] = tmp;
+
+ printk(XENLOG_WARNING "%04x:%02x:%02x.%u not found in ACPI tables;"
+ " using same IOMMU as function 0\n",
+ seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf));
+
+ /* write iommu field last */
+ ivrs_mappings[bdf].iommu = ivrs_mappings[bd0].iommu;
+ }
+ }
+
+ return ivrs_mappings[bdf].iommu;
}
/*
@@ -179,6 +205,8 @@ int __init amd_iov_detect(void)
return -ENODEV;
}
+ init_done = 1;
+
/*
* AMD IOMMUs don't distinguish between vectors destined for
* different cpus when doing interrupt remapping. This means
--- a/xen/include/asm-x86/amd-iommu.h
+++ b/xen/include/asm-x86/amd-iommu.h
@@ -125,7 +125,7 @@ struct ivrs_mappings {
u8 device_flags;
};
-extern unsigned short ivrs_bdf_entries;
+extern unsigned int ivrs_bdf_entries;
struct ivrs_mappings *get_ivrs_mappings(u16 seg);
int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/4] AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0
2013-02-15 16:20 ` [PATCH 2/4] AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0 Jan Beulich
@ 2013-02-15 17:21 ` Boris Ostrovsky
2013-02-18 8:00 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Boris Ostrovsky @ 2013-02-15 17:21 UTC (permalink / raw)
To: Jan Beulich; +Cc: Sherry Hurwitz, xen-devel
On 02/15/2013 11:20 AM, Jan Beulich wrote:
>
> static int __init get_last_bdf_acpi(struct acpi_table_header *table)
> {
> const struct acpi_ivrs_header *ivrs_block;
> unsigned long length = sizeof(struct acpi_table_ivrs);
> + int last_bdf = 0;
>
> while ( table->length > (length + sizeof(*ivrs_block)) )
> {
> ivrs_block = (struct acpi_ivrs_header *)((u8 *)table + length);
> if ( table->length < (length + ivrs_block->length) )
> return -ENODEV;
> - if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE &&
> - get_last_bdf_ivhd(
> + if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE )
> + {
> + int ret = get_last_bdf_ivhd(
> container_of(ivrs_block, const struct acpi_ivrs_hardware,
> - header)) != 0 )
> - return -ENODEV;
> + header));
> +
> + if ( ret < 0 )
> + return ret;
> + UPDATE_LAST_BDF(ret);
Why do we need UPDATE_LAST_BDF () here? It is updated in
get_last_bdf_ivhd () above.
> + }
> length += ivrs_block->length;
> }
> - return 0;
> +
> + return last_bdf;
> }
...
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -28,12 +28,38 @@
> #include <asm/hvm/svm/amd-iommu-proto.h>
> #include "../ats.h"
>
> +static bool_t __read_mostly init_done;
> +
> struct amd_iommu *find_iommu_for_device(int seg, int bdf)
> {
> struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
>
> - return ivrs_mappings && bdf < ivrs_bdf_entries ? ivrs_mappings[bdf].iommu
> - : NULL;
> + if ( !ivrs_mappings || bdf >= ivrs_bdf_entries )
> + return NULL;
> +
> + if ( unlikely(!ivrs_mappings[bdf].iommu) && likely(init_done) )
> + {
> + unsigned int bd0 = bdf & ~PCI_FUNC(~0);
> +
> + if ( ivrs_mappings[bd0].iommu )
> + {
> + struct ivrs_mappings tmp = ivrs_mappings[bd0];
> +
> + tmp.iommu = NULL;
> + if ( tmp.dte_requestor_id == bd0 )
> + tmp.dte_requestor_id = bdf;
Is it possible to have tmp.dte_requestor_id != bd0
-boris
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/4] AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0
2013-02-15 17:21 ` Boris Ostrovsky
@ 2013-02-18 8:00 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2013-02-18 8:00 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: xen-devel, Sherry Hurwitz
>>> On 15.02.13 at 18:21, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> On 02/15/2013 11:20 AM, Jan Beulich wrote:
>>
>> static int __init get_last_bdf_acpi(struct acpi_table_header *table)
>> {
>> const struct acpi_ivrs_header *ivrs_block;
>> unsigned long length = sizeof(struct acpi_table_ivrs);
>> + int last_bdf = 0;
>>
>> while ( table->length > (length + sizeof(*ivrs_block)) )
>> {
>> ivrs_block = (struct acpi_ivrs_header *)((u8 *)table + length);
>> if ( table->length < (length + ivrs_block->length) )
>> return -ENODEV;
>> - if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE &&
>> - get_last_bdf_ivhd(
>> + if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE )
>> + {
>> + int ret = get_last_bdf_ivhd(
>> container_of(ivrs_block, const struct acpi_ivrs_hardware,
>> - header)) != 0 )
>> - return -ENODEV;
>> + header));
>> +
>> + if ( ret < 0 )
>> + return ret;
>> + UPDATE_LAST_BDF(ret);
>
> Why do we need UPDATE_LAST_BDF () here? It is updated in
> get_last_bdf_ivhd () above.
No, because "last_bdf" now is a local variable.
>> + }
>> length += ivrs_block->length;
>> }
>> - return 0;
>> +
>> + return last_bdf;
>> }
>
> ...
>
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -28,12 +28,38 @@
>> #include <asm/hvm/svm/amd-iommu-proto.h>
>> #include "../ats.h"
>>
>> +static bool_t __read_mostly init_done;
>> +
>> struct amd_iommu *find_iommu_for_device(int seg, int bdf)
>> {
>> struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
>>
>> - return ivrs_mappings && bdf < ivrs_bdf_entries ? ivrs_mappings[bdf].iommu
>> - : NULL;
>> + if ( !ivrs_mappings || bdf >= ivrs_bdf_entries )
>> + return NULL;
>> +
>> + if ( unlikely(!ivrs_mappings[bdf].iommu) && likely(init_done) )
>> + {
>> + unsigned int bd0 = bdf & ~PCI_FUNC(~0);
>> +
>> + if ( ivrs_mappings[bd0].iommu )
>> + {
>> + struct ivrs_mappings tmp = ivrs_mappings[bd0];
>> +
>> + tmp.iommu = NULL;
>> + if ( tmp.dte_requestor_id == bd0 )
>> + tmp.dte_requestor_id = bdf;
>
> Is it possible to have tmp.dte_requestor_id != bd0
Sure - when there was an alias entry for it (I assume e.g. in the
case of the device sitting behind a legacy PCI bridge).
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/4] IOMMU, AMD Family15h Model10-1Fh erratum 746 Workaround
2013-02-15 16:13 [PATCH 0/4] AMD IOMMU: misc adjustments Jan Beulich
2013-02-15 16:19 ` [PATCH 1/4] AMD IOMMU: don't BUG() when we don't have to Jan Beulich
2013-02-15 16:20 ` [PATCH 2/4] AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0 Jan Beulich
@ 2013-02-15 16:21 ` Jan Beulich
2013-02-25 21:33 ` Suravee Suthikulanit
2013-02-15 16:22 ` [PATCH 4/4] AMD IOMMU: use __ioapic_read_entry() instead of open coding it Jan Beulich
3 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2013-02-15 16:21 UTC (permalink / raw)
To: xen-devel; +Cc: Boris Ostrovsky, suravee.suthikulpanit, Sherry Hurwitz
[-- Attachment #1: Type: text/plain, Size: 2535 bytes --]
The IOMMU may stop processing page translations due to a perceived lack
of credits for writing upstream peripheral page service request (PPR)
or event logs. If the L2B miscellaneous clock gating feature is enabled
the IOMMU does not properly register credits after the log request has
completed, leading to a potential system hang.
BIOSes are supposed to disable L2B micellaneous clock gating by setting
L2_L2B_CK_GATE_CONTROL[CKGateL2BMiscDisable](D0F2xF4_x90[2]) = 1b. This
patch corrects that for those which do not enable this workaround.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -792,6 +792,42 @@ static bool_t __init set_iommu_interrupt
return 1;
}
+/*
+ * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall Translations)
+ * Workaround:
+ * BIOS should disable L2B micellaneous clock gating by setting
+ * L2_L2B_CK_GATE_CONTROL[CKGateL2BMiscDisable](D0F2xF4_x90[2]) = 1b
+ */
+static void amd_iommu_erratum_746_workaround(struct amd_iommu *iommu)
+{
+ u32 value;
+ u8 bus = PCI_BUS(iommu->bdf);
+ u8 dev = PCI_SLOT(iommu->bdf);
+ u8 func = PCI_FUNC(iommu->bdf);
+
+ if ( (boot_cpu_data.x86 != 0x15) ||
+ (boot_cpu_data.x86_model < 0x10) ||
+ (boot_cpu_data.x86_model > 0x1f) )
+ return;
+
+ pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90);
+ value = pci_conf_read32(iommu->seg, bus, dev, func, 0xf4);
+
+ if ( value & (1 << 2) )
+ return;
+
+ /* Select NB indirect register 0x90 and enable writing */
+ pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90 | (1 << 8));
+
+ pci_conf_write32(iommu->seg, bus, dev, func, 0xf4, value | (1 << 2));
+ printk(XENLOG_INFO
+ "AMD-Vi: Applying erratum 746 workaround for IOMMU at %04x:%02x:%02x.%u\n",
+ iommu->seg, bus, dev, func);
+
+ /* Clear the enable writing bit */
+ pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90);
+}
+
static void enable_iommu(struct amd_iommu *iommu)
{
unsigned long flags;
@@ -805,6 +841,8 @@ static void enable_iommu(struct amd_iomm
return;
}
+ amd_iommu_erratum_746_workaround(iommu);
+
register_iommu_dev_table_in_mmio_space(iommu);
register_iommu_cmd_buffer_in_mmio_space(iommu);
register_iommu_event_log_in_mmio_space(iommu);
[-- Attachment #2: AMD-IOMMU-erratum-746.patch --]
[-- Type: text/plain, Size: 2588 bytes --]
IOMMU, AMD Family15h Model10-1Fh erratum 746 Workaround
The IOMMU may stop processing page translations due to a perceived lack
of credits for writing upstream peripheral page service request (PPR)
or event logs. If the L2B miscellaneous clock gating feature is enabled
the IOMMU does not properly register credits after the log request has
completed, leading to a potential system hang.
BIOSes are supposed to disable L2B micellaneous clock gating by setting
L2_L2B_CK_GATE_CONTROL[CKGateL2BMiscDisable](D0F2xF4_x90[2]) = 1b. This
patch corrects that for those which do not enable this workaround.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -792,6 +792,42 @@ static bool_t __init set_iommu_interrupt
return 1;
}
+/*
+ * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall Translations)
+ * Workaround:
+ * BIOS should disable L2B micellaneous clock gating by setting
+ * L2_L2B_CK_GATE_CONTROL[CKGateL2BMiscDisable](D0F2xF4_x90[2]) = 1b
+ */
+static void amd_iommu_erratum_746_workaround(struct amd_iommu *iommu)
+{
+ u32 value;
+ u8 bus = PCI_BUS(iommu->bdf);
+ u8 dev = PCI_SLOT(iommu->bdf);
+ u8 func = PCI_FUNC(iommu->bdf);
+
+ if ( (boot_cpu_data.x86 != 0x15) ||
+ (boot_cpu_data.x86_model < 0x10) ||
+ (boot_cpu_data.x86_model > 0x1f) )
+ return;
+
+ pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90);
+ value = pci_conf_read32(iommu->seg, bus, dev, func, 0xf4);
+
+ if ( value & (1 << 2) )
+ return;
+
+ /* Select NB indirect register 0x90 and enable writing */
+ pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90 | (1 << 8));
+
+ pci_conf_write32(iommu->seg, bus, dev, func, 0xf4, value | (1 << 2));
+ printk(XENLOG_INFO
+ "AMD-Vi: Applying erratum 746 workaround for IOMMU at %04x:%02x:%02x.%u\n",
+ iommu->seg, bus, dev, func);
+
+ /* Clear the enable writing bit */
+ pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90);
+}
+
static void enable_iommu(struct amd_iommu *iommu)
{
unsigned long flags;
@@ -805,6 +841,8 @@ static void enable_iommu(struct amd_iomm
return;
}
+ amd_iommu_erratum_746_workaround(iommu);
+
register_iommu_dev_table_in_mmio_space(iommu);
register_iommu_cmd_buffer_in_mmio_space(iommu);
register_iommu_event_log_in_mmio_space(iommu);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/4] IOMMU, AMD Family15h Model10-1Fh erratum 746 Workaround
2013-02-15 16:21 ` [PATCH 3/4] IOMMU, AMD Family15h Model10-1Fh erratum 746 Workaround Jan Beulich
@ 2013-02-25 21:33 ` Suravee Suthikulanit
2013-02-26 7:42 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Suravee Suthikulanit @ 2013-02-25 21:33 UTC (permalink / raw)
To: Jan Beulich; +Cc: Boris Ostrovsky, Sherry Hurwitz, xen-devel
Jan,
This patch looks good. I have tested on the system I have and it does
what it's supposed to. Thank you for porting the patch over from Linux.
Suravee
On 2/15/2013 10:21 AM, Jan Beulich wrote:
> The IOMMU may stop processing page translations due to a perceived lack
> of credits for writing upstream peripheral page service request (PPR)
> or event logs. If the L2B miscellaneous clock gating feature is enabled
> the IOMMU does not properly register credits after the log request has
> completed, leading to a potential system hang.
>
> BIOSes are supposed to disable L2B micellaneous clock gating by setting
> L2_L2B_CK_GATE_CONTROL[CKGateL2BMiscDisable](D0F2xF4_x90[2]) = 1b. This
> patch corrects that for those which do not enable this workaround.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -792,6 +792,42 @@ static bool_t __init set_iommu_interrupt
> return 1;
> }
>
> +/*
> + * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall Translations)
> + * Workaround:
> + * BIOS should disable L2B micellaneous clock gating by setting
> + * L2_L2B_CK_GATE_CONTROL[CKGateL2BMiscDisable](D0F2xF4_x90[2]) = 1b
> + */
> +static void amd_iommu_erratum_746_workaround(struct amd_iommu *iommu)
> +{
> + u32 value;
> + u8 bus = PCI_BUS(iommu->bdf);
> + u8 dev = PCI_SLOT(iommu->bdf);
> + u8 func = PCI_FUNC(iommu->bdf);
> +
> + if ( (boot_cpu_data.x86 != 0x15) ||
> + (boot_cpu_data.x86_model < 0x10) ||
> + (boot_cpu_data.x86_model > 0x1f) )
> + return;
> +
> + pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90);
> + value = pci_conf_read32(iommu->seg, bus, dev, func, 0xf4);
> +
> + if ( value & (1 << 2) )
> + return;
> +
> + /* Select NB indirect register 0x90 and enable writing */
> + pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90 | (1 << 8));
> +
> + pci_conf_write32(iommu->seg, bus, dev, func, 0xf4, value | (1 << 2));
> + printk(XENLOG_INFO
> + "AMD-Vi: Applying erratum 746 workaround for IOMMU at %04x:%02x:%02x.%u\n",
> + iommu->seg, bus, dev, func);
> +
> + /* Clear the enable writing bit */
> + pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90);
> +}
> +
> static void enable_iommu(struct amd_iommu *iommu)
> {
> unsigned long flags;
> @@ -805,6 +841,8 @@ static void enable_iommu(struct amd_iomm
> return;
> }
>
> + amd_iommu_erratum_746_workaround(iommu);
> +
> register_iommu_dev_table_in_mmio_space(iommu);
> register_iommu_cmd_buffer_in_mmio_space(iommu);
> register_iommu_event_log_in_mmio_space(iommu);
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/4] IOMMU, AMD Family15h Model10-1Fh erratum 746 Workaround
2013-02-25 21:33 ` Suravee Suthikulanit
@ 2013-02-26 7:42 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2013-02-26 7:42 UTC (permalink / raw)
To: Suravee Suthikulanit; +Cc: Boris Ostrovsky, xen-devel, Sherry Hurwitz
>>> On 25.02.13 at 22:33, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
> This patch looks good. I have tested on the system I have and it does
> what it's supposed to. Thank you for porting the patch over from Linux.
Thanks - I take this as an ack.
Jan
> On 2/15/2013 10:21 AM, Jan Beulich wrote:
>> The IOMMU may stop processing page translations due to a perceived lack
>> of credits for writing upstream peripheral page service request (PPR)
>> or event logs. If the L2B miscellaneous clock gating feature is enabled
>> the IOMMU does not properly register credits after the log request has
>> completed, leading to a potential system hang.
>>
>> BIOSes are supposed to disable L2B micellaneous clock gating by setting
>> L2_L2B_CK_GATE_CONTROL[CKGateL2BMiscDisable](D0F2xF4_x90[2]) = 1b. This
>> patch corrects that for those which do not enable this workaround.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -792,6 +792,42 @@ static bool_t __init set_iommu_interrupt
>> return 1;
>> }
>>
>> +/*
>> + * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall
> Translations)
>> + * Workaround:
>> + * BIOS should disable L2B micellaneous clock gating by setting
>> + * L2_L2B_CK_GATE_CONTROL[CKGateL2BMiscDisable](D0F2xF4_x90[2]) = 1b
>> + */
>> +static void amd_iommu_erratum_746_workaround(struct amd_iommu *iommu)
>> +{
>> + u32 value;
>> + u8 bus = PCI_BUS(iommu->bdf);
>> + u8 dev = PCI_SLOT(iommu->bdf);
>> + u8 func = PCI_FUNC(iommu->bdf);
>> +
>> + if ( (boot_cpu_data.x86 != 0x15) ||
>> + (boot_cpu_data.x86_model < 0x10) ||
>> + (boot_cpu_data.x86_model > 0x1f) )
>> + return;
>> +
>> + pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90);
>> + value = pci_conf_read32(iommu->seg, bus, dev, func, 0xf4);
>> +
>> + if ( value & (1 << 2) )
>> + return;
>> +
>> + /* Select NB indirect register 0x90 and enable writing */
>> + pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90 | (1 << 8));
>> +
>> + pci_conf_write32(iommu->seg, bus, dev, func, 0xf4, value | (1 << 2));
>> + printk(XENLOG_INFO
>> + "AMD-Vi: Applying erratum 746 workaround for IOMMU at
> %04x:%02x:%02x.%u\n",
>> + iommu->seg, bus, dev, func);
>> +
>> + /* Clear the enable writing bit */
>> + pci_conf_write32(iommu->seg, bus, dev, func, 0xf0, 0x90);
>> +}
>> +
>> static void enable_iommu(struct amd_iommu *iommu)
>> {
>> unsigned long flags;
>> @@ -805,6 +841,8 @@ static void enable_iommu(struct amd_iomm
>> return;
>> }
>>
>> + amd_iommu_erratum_746_workaround(iommu);
>> +
>> register_iommu_dev_table_in_mmio_space(iommu);
>> register_iommu_cmd_buffer_in_mmio_space(iommu);
>> register_iommu_event_log_in_mmio_space(iommu);
>>
>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/4] AMD IOMMU: use __ioapic_read_entry() instead of open coding it
2013-02-15 16:13 [PATCH 0/4] AMD IOMMU: misc adjustments Jan Beulich
` (2 preceding siblings ...)
2013-02-15 16:21 ` [PATCH 3/4] IOMMU, AMD Family15h Model10-1Fh erratum 746 Workaround Jan Beulich
@ 2013-02-15 16:22 ` Jan Beulich
2013-02-15 17:28 ` Boris Ostrovsky
3 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2013-02-15 16:22 UTC (permalink / raw)
To: xen-devel; +Cc: Boris Ostrovsky, Sherry Hurwitz
[-- Attachment #1: Type: text/plain, Size: 837 bytes --]
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -144,7 +144,7 @@ static void update_intremap_entry_from_i
int __init amd_iommu_setup_ioapic_remapping(void)
{
- struct IO_APIC_route_entry rte = {0};
+ struct IO_APIC_route_entry rte;
unsigned long flags;
u32* entry;
int apic, pin;
@@ -159,9 +159,7 @@ int __init amd_iommu_setup_ioapic_remapp
{
for ( pin = 0; pin < nr_ioapic_entries[apic]; pin++ )
{
- *(((int *)&rte) + 1) = io_apic_read(apic, 0x11 + 2 * pin);
- *(((int *)&rte) + 0) = io_apic_read(apic, 0x10 + 2 * pin);
-
+ rte = __ioapic_read_entry(apic, pin, 1);
if ( rte.mask == 1 )
continue;
[-- Attachment #2: AMD-IOMMU-use-ioapic_read_entry.patch --]
[-- Type: text/plain, Size: 897 bytes --]
AMD IOMMU: use __ioapic_read_entry() instead of open coding it
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -144,7 +144,7 @@ static void update_intremap_entry_from_i
int __init amd_iommu_setup_ioapic_remapping(void)
{
- struct IO_APIC_route_entry rte = {0};
+ struct IO_APIC_route_entry rte;
unsigned long flags;
u32* entry;
int apic, pin;
@@ -159,9 +159,7 @@ int __init amd_iommu_setup_ioapic_remapp
{
for ( pin = 0; pin < nr_ioapic_entries[apic]; pin++ )
{
- *(((int *)&rte) + 1) = io_apic_read(apic, 0x11 + 2 * pin);
- *(((int *)&rte) + 0) = io_apic_read(apic, 0x10 + 2 * pin);
-
+ rte = __ioapic_read_entry(apic, pin, 1);
if ( rte.mask == 1 )
continue;
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 4/4] AMD IOMMU: use __ioapic_read_entry() instead of open coding it
2013-02-15 16:22 ` [PATCH 4/4] AMD IOMMU: use __ioapic_read_entry() instead of open coding it Jan Beulich
@ 2013-02-15 17:28 ` Boris Ostrovsky
0 siblings, 0 replies; 12+ messages in thread
From: Boris Ostrovsky @ 2013-02-15 17:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: Sherry Hurwitz, xen-devel
On 02/15/2013 11:22 AM, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -144,7 +144,7 @@ static void update_intremap_entry_from_i
>
> int __init amd_iommu_setup_ioapic_remapping(void)
> {
> - struct IO_APIC_route_entry rte = {0};
> + struct IO_APIC_route_entry rte;
> unsigned long flags;
> u32* entry;
> int apic, pin;
> @@ -159,9 +159,7 @@ int __init amd_iommu_setup_ioapic_remapp
> {
> for ( pin = 0; pin < nr_ioapic_entries[apic]; pin++ )
> {
> - *(((int *)&rte) + 1) = io_apic_read(apic, 0x11 + 2 * pin);
> - *(((int *)&rte) + 0) = io_apic_read(apic, 0x10 + 2 * pin);
> -
> + rte = __ioapic_read_entry(apic, pin, 1);
> if ( rte.mask == 1 )
> continue;
>
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread