* [PATCH] amd iommu: Add workaround for erratum 732 & 733
@ 2012-05-22 13:56 Wei Wang
2012-05-22 14:10 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Wei Wang @ 2012-05-22 13:56 UTC (permalink / raw)
To: Jan Beulich, Keir Fraser; +Cc: xen-devel
Hi Jan
This patch implements the suggested workaround for erratum 732 & 733. It
is mostly derived from the Linux patch recently posted. Please review it.
Thanks,
Wei
# HG changeset patch
# User Wei Wang <wei.wang2@amd.com>
# Date 1337690747 -7200
# Node ID 06aebc361de7f308b262b008153ae4549c4480c2
# Parent 592d15bd4d5ec58486d32ee9998319e7c95fcd66
amd iommu: Add workaround for erratum 733 & 733
Signed-off-by: Wei Wang <wei.wang2@amd.com>
diff -r 592d15bd4d5e -r 06aebc361de7
xen/drivers/passthrough/amd/iommu_init.c
--- a/xen/drivers/passthrough/amd/iommu_init.c Fri May 18 16:19:21
2012 +0100
+++ b/xen/drivers/passthrough/amd/iommu_init.c Tue May 22 14:45:47
2012 +0200
@@ -29,6 +29,7 @@
#include <asm/hvm/svm/amd-iommu-proto.h>
#include <asm-x86/fixmap.h>
#include <mach_apic.h>
+#include <xen/delay.h>
static int __initdata nr_amd_iommus;
@@ -566,6 +567,7 @@ static void parse_event_log_entry(struct
u16 domain_id, device_id, bdf, cword;
u32 code;
u64 *addr;
+ int count = 0;
char * event_str[] = {"ILLEGAL_DEV_TABLE_ENTRY",
"IO_PAGE_FAULT",
"DEV_TABLE_HW_ERROR",
@@ -575,9 +577,27 @@ static void parse_event_log_entry(struct
"IOTLB_INV_TIMEOUT",
"INVALID_DEV_REQUEST"};
+retry:
code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
IOMMU_EVENT_CODE_SHIFT);
+ /* Workaround for erratum 732.
+ * it can happen that the tail pointer is updated before the actual
entry
+ * is written. Suggested by RevGuide, we initialize the event log
buffer to
+ * all zeros and write event log entries to zero after they are
processed.
+ */
+
+ if ( code == 0 )
+ {
+ if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
+ {
+ AMD_IOMMU_DEBUG("AMD-Vi: No event written to event log\n");
+ return;
+ }
+ udelay(1);
+ goto retry;
+ }
+
if ( (code > IOMMU_EVENT_INVALID_DEV_REQUEST) ||
(code < IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY) )
{
@@ -615,6 +635,8 @@ static void parse_event_log_entry(struct
AMD_IOMMU_DEBUG("event 0x%08x 0x%08x 0x%08x 0x%08x\n", entry[0],
entry[1], entry[2], entry[3]);
}
+
+ memset(entry, 0, IOMMU_EVENT_LOG_ENTRY_SIZE);
}
static void iommu_check_event_log(struct amd_iommu *iommu)
@@ -646,10 +668,32 @@ void parse_ppr_log_entry(struct amd_iomm
{
u16 device_id;
- u8 bus, devfn;
+ u8 bus, devfn, code;
struct pci_dev *pdev;
struct domain *d;
+ int count = 0;
+retry:
+ code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
+ IOMMU_PPR_LOG_CODE_SHIFT);
+
+ /* Workaround for erratum 733.
+ * it can happen that the tail pointer is updated before the actual
entry
+ * is written. Suggested by RevGuide, we initialize the ppr log
buffer to
+ * all zeros and write ppr log entries to zero after they are
processed.
+ */
+
+ if ( code == 0 )
+ {
+ if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
+ {
+ AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to ppr log\n");
+ return;
+ }
+ udelay(1);
+ goto retry;
+ }
+
/* here device_id is physical value */
device_id = iommu_get_devid_from_cmd(entry[0]);
bus = PCI_BUS(device_id);
@@ -665,6 +709,8 @@ void parse_ppr_log_entry(struct amd_iomm
d = pdev->domain;
guest_iommu_add_ppr_log(d, entry);
+
+ memset(entry, 0, IOMMU_PPR_LOG_ENTRY_SIZE);
}
static void iommu_check_ppr_log(struct amd_iommu *iommu)
diff -r 592d15bd4d5e -r 06aebc361de7
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Fri May 18
16:19:21 2012 +0100
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Tue May 22
14:45:47 2012 +0200
@@ -301,6 +301,10 @@
#define IOMMU_PPR_LOG_TAIL_OFFSET 0x2038
#define IOMMU_PPR_LOG_DEVICE_ID_MASK 0x0000FFFF
#define IOMMU_PPR_LOG_DEVICE_ID_SHIFT 0
+#define IOMMU_PPR_LOG_CODE_MASK 0xF0000000
+#define IOMMU_PPR_LOG_CODE_SHIFT 28
+
+#define IOMMU_LOG_ENTRY_TIMEOUT 100000
/* Control Register */
#define IOMMU_CONTROL_MMIO_OFFSET 0x18
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] amd iommu: Add workaround for erratum 732 & 733
2012-05-22 13:56 [PATCH] amd iommu: Add workaround for erratum 732 & 733 Wei Wang
@ 2012-05-22 14:10 ` Jan Beulich
2012-05-22 15:30 ` Wei Wang
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2012-05-22 14:10 UTC (permalink / raw)
To: Wei Wang; +Cc: Keir Fraser, xen-devel
>>> On 22.05.12 at 15:56, Wei Wang <wei.wang2@amd.com> wrote:
> Hi Jan
> This patch implements the suggested workaround for erratum 732 & 733. It
> is mostly derived from the Linux patch recently posted. Please review it.
> Thanks,
> Wei
Looks good in principle, but came out with newline damage. Can
you please resend as attachment, ideally at once fixing a few
(mostly cosmetic) things:
> # HG changeset patch
> # User Wei Wang <wei.wang2@amd.com>
> # Date 1337690747 -7200
> # Node ID 06aebc361de7f308b262b008153ae4549c4480c2
> # Parent 592d15bd4d5ec58486d32ee9998319e7c95fcd66
> amd iommu: Add workaround for erratum 733 & 733
732 & 733
> Signed-off-by: Wei Wang <wei.wang2@amd.com>
>
> diff -r 592d15bd4d5e -r 06aebc361de7
> xen/drivers/passthrough/amd/iommu_init.c
> --- a/xen/drivers/passthrough/amd/iommu_init.c Fri May 18 16:19:21
> 2012 +0100
> +++ b/xen/drivers/passthrough/amd/iommu_init.c Tue May 22 14:45:47
> 2012 +0200
> @@ -29,6 +29,7 @@
> #include <asm/hvm/svm/amd-iommu-proto.h>
> #include <asm-x86/fixmap.h>
> #include <mach_apic.h>
> +#include <xen/delay.h>
>
> static int __initdata nr_amd_iommus;
>
> @@ -566,6 +567,7 @@ static void parse_event_log_entry(struct
> u16 domain_id, device_id, bdf, cword;
> u32 code;
> u64 *addr;
> + int count = 0;
> char * event_str[] = {"ILLEGAL_DEV_TABLE_ENTRY",
> "IO_PAGE_FAULT",
> "DEV_TABLE_HW_ERROR",
> @@ -575,9 +577,27 @@ static void parse_event_log_entry(struct
> "IOTLB_INV_TIMEOUT",
> "INVALID_DEV_REQUEST"};
>
> +retry:
> code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
> IOMMU_EVENT_CODE_SHIFT);
>
> + /* Workaround for erratum 732.
/*
* Workaround for erratum 732.
> + * it can happen that the tail pointer is updated before the actual
> entry
> + * is written. Suggested by RevGuide, we initialize the event log
> buffer to
> + * all zeros and write event log entries to zero after they are
> processed.
> + */
> +
> + if ( code == 0 )
> + {
> + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
> + {
> + AMD_IOMMU_DEBUG("AMD-Vi: No event written to event log\n");
Perhaps remove the second "event".
> + return;
> + }
> + udelay(1);
> + goto retry;
> + }
> +
> if ( (code > IOMMU_EVENT_INVALID_DEV_REQUEST) ||
> (code < IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY) )
> {
> @@ -615,6 +635,8 @@ static void parse_event_log_entry(struct
> AMD_IOMMU_DEBUG("event 0x%08x 0x%08x 0x%08x 0x%08x\n", entry[0],
> entry[1], entry[2], entry[3]);
> }
> +
> + memset(entry, 0, IOMMU_EVENT_LOG_ENTRY_SIZE);
> }
>
> static void iommu_check_event_log(struct amd_iommu *iommu)
> @@ -646,10 +668,32 @@ void parse_ppr_log_entry(struct amd_iomm
> {
>
> u16 device_id;
> - u8 bus, devfn;
> + u8 bus, devfn, code;
> struct pci_dev *pdev;
> struct domain *d;
> + int count = 0;
>
> +retry:
> + code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
> + IOMMU_PPR_LOG_CODE_SHIFT);
> +
> + /* Workaround for erratum 733.
See above.
> + * it can happen that the tail pointer is updated before the actual
> entry
> + * is written. Suggested by RevGuide, we initialize the ppr log
> buffer to
> + * all zeros and write ppr log entries to zero after they are
> processed.
> + */
> +
> + if ( code == 0 )
> + {
> + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
> + {
> + AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to ppr log\n");
Perhaps remove the second "ppr".
> + return;
> + }
> + udelay(1);
> + goto retry;
> + }
> +
> /* here device_id is physical value */
> device_id = iommu_get_devid_from_cmd(entry[0]);
> bus = PCI_BUS(device_id);
> @@ -665,6 +709,8 @@ void parse_ppr_log_entry(struct amd_iomm
> d = pdev->domain;
>
> guest_iommu_add_ppr_log(d, entry);
> +
> + memset(entry, 0, IOMMU_PPR_LOG_ENTRY_SIZE);
> }
>
> static void iommu_check_ppr_log(struct amd_iommu *iommu)
I'd personally also prefer the loops to be written as such (i.e.
without goto-s).
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Fri May 18
> 16:19:21 2012 +0100
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Tue May 22
> 14:45:47 2012 +0200
> @@ -301,6 +301,10 @@
> #define IOMMU_PPR_LOG_TAIL_OFFSET 0x2038
> #define IOMMU_PPR_LOG_DEVICE_ID_MASK 0x0000FFFF
> #define IOMMU_PPR_LOG_DEVICE_ID_SHIFT 0
> +#define IOMMU_PPR_LOG_CODE_MASK 0xF0000000
> +#define IOMMU_PPR_LOG_CODE_SHIFT 28
> +
> +#define IOMMU_LOG_ENTRY_TIMEOUT 100000
That's rather long a timeout (100ms) for a busy loop - is that
really necessary?
Jan
>
> /* Control Register */
> #define IOMMU_CONTROL_MMIO_OFFSET 0x18
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] amd iommu: Add workaround for erratum 732 & 733
2012-05-22 14:10 ` Jan Beulich
@ 2012-05-22 15:30 ` Wei Wang
2012-05-23 6:57 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Wei Wang @ 2012-05-22 15:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: Keir Fraser, xen-devel
[-- Attachment #1: Type: text/plain, Size: 5678 bytes --]
Hi Jan,
Thanks for review it. New version has been attached. It should have
fixed issues you mentioned. We don't have a particular number for loop
count, so I cut it to 1000, it should be enough. Please have a look.
Thanks,
Wei
On 05/22/2012 04:10 PM, Jan Beulich wrote:
>>>> On 22.05.12 at 15:56, Wei Wang<wei.wang2@amd.com> wrote:
>> Hi Jan
>> This patch implements the suggested workaround for erratum 732& 733. It
>> is mostly derived from the Linux patch recently posted. Please review it.
>> Thanks,
>> Wei
>
> Looks good in principle, but came out with newline damage. Can
> you please resend as attachment, ideally at once fixing a few
> (mostly cosmetic) things:
>
>> # HG changeset patch
>> # User Wei Wang<wei.wang2@amd.com>
>> # Date 1337690747 -7200
>> # Node ID 06aebc361de7f308b262b008153ae4549c4480c2
>> # Parent 592d15bd4d5ec58486d32ee9998319e7c95fcd66
>> amd iommu: Add workaround for erratum 733& 733
>
> 732& 733
>
>> Signed-off-by: Wei Wang<wei.wang2@amd.com>
>>
>> diff -r 592d15bd4d5e -r 06aebc361de7
>> xen/drivers/passthrough/amd/iommu_init.c
>> --- a/xen/drivers/passthrough/amd/iommu_init.c Fri May 18 16:19:21
>> 2012 +0100
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c Tue May 22 14:45:47
>> 2012 +0200
>> @@ -29,6 +29,7 @@
>> #include<asm/hvm/svm/amd-iommu-proto.h>
>> #include<asm-x86/fixmap.h>
>> #include<mach_apic.h>
>> +#include<xen/delay.h>
>>
>> static int __initdata nr_amd_iommus;
>>
>> @@ -566,6 +567,7 @@ static void parse_event_log_entry(struct
>> u16 domain_id, device_id, bdf, cword;
>> u32 code;
>> u64 *addr;
>> + int count = 0;
>> char * event_str[] = {"ILLEGAL_DEV_TABLE_ENTRY",
>> "IO_PAGE_FAULT",
>> "DEV_TABLE_HW_ERROR",
>> @@ -575,9 +577,27 @@ static void parse_event_log_entry(struct
>> "IOTLB_INV_TIMEOUT",
>> "INVALID_DEV_REQUEST"};
>>
>> +retry:
>> code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
>> IOMMU_EVENT_CODE_SHIFT);
>>
>> + /* Workaround for erratum 732.
>
> /*
> * Workaround for erratum 732.
>
>> + * it can happen that the tail pointer is updated before the actual
>> entry
>> + * is written. Suggested by RevGuide, we initialize the event log
>> buffer to
>> + * all zeros and write event log entries to zero after they are
>> processed.
>> + */
>> +
>> + if ( code == 0 )
>> + {
>> + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
>> + {
>> + AMD_IOMMU_DEBUG("AMD-Vi: No event written to event log\n");
>
> Perhaps remove the second "event".
>
>> + return;
>> + }
>> + udelay(1);
>> + goto retry;
>> + }
>> +
>> if ( (code> IOMMU_EVENT_INVALID_DEV_REQUEST) ||
>> (code< IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY) )
>> {
>> @@ -615,6 +635,8 @@ static void parse_event_log_entry(struct
>> AMD_IOMMU_DEBUG("event 0x%08x 0x%08x 0x%08x 0x%08x\n", entry[0],
>> entry[1], entry[2], entry[3]);
>> }
>> +
>> + memset(entry, 0, IOMMU_EVENT_LOG_ENTRY_SIZE);
>> }
>>
>> static void iommu_check_event_log(struct amd_iommu *iommu)
>> @@ -646,10 +668,32 @@ void parse_ppr_log_entry(struct amd_iomm
>> {
>>
>> u16 device_id;
>> - u8 bus, devfn;
>> + u8 bus, devfn, code;
>> struct pci_dev *pdev;
>> struct domain *d;
>> + int count = 0;
>>
>> +retry:
>> + code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
>> + IOMMU_PPR_LOG_CODE_SHIFT);
>> +
>> + /* Workaround for erratum 733.
>
> See above.
>
>> + * it can happen that the tail pointer is updated before the actual
>> entry
>> + * is written. Suggested by RevGuide, we initialize the ppr log
>> buffer to
>> + * all zeros and write ppr log entries to zero after they are
>> processed.
>> + */
>> +
>> + if ( code == 0 )
>> + {
>> + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
>> + {
>> + AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to ppr log\n");
>
> Perhaps remove the second "ppr".
>
>> + return;
>> + }
>> + udelay(1);
>> + goto retry;
>> + }
>> +
>> /* here device_id is physical value */
>> device_id = iommu_get_devid_from_cmd(entry[0]);
>> bus = PCI_BUS(device_id);
>> @@ -665,6 +709,8 @@ void parse_ppr_log_entry(struct amd_iomm
>> d = pdev->domain;
>>
>> guest_iommu_add_ppr_log(d, entry);
>> +
>> + memset(entry, 0, IOMMU_PPR_LOG_ENTRY_SIZE);
>> }
>>
>> static void iommu_check_ppr_log(struct amd_iommu *iommu)
>
> I'd personally also prefer the loops to be written as such (i.e.
> without goto-s).
>
>> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Fri May 18
>> 16:19:21 2012 +0100
>> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Tue May 22
>> 14:45:47 2012 +0200
>> @@ -301,6 +301,10 @@
>> #define IOMMU_PPR_LOG_TAIL_OFFSET 0x2038
>> #define IOMMU_PPR_LOG_DEVICE_ID_MASK 0x0000FFFF
>> #define IOMMU_PPR_LOG_DEVICE_ID_SHIFT 0
>> +#define IOMMU_PPR_LOG_CODE_MASK 0xF0000000
>> +#define IOMMU_PPR_LOG_CODE_SHIFT 28
>> +
>> +#define IOMMU_LOG_ENTRY_TIMEOUT 100000
>
> That's rather long a timeout (100ms) for a busy loop - is that
> really necessary?
>
> Jan
>
>>
>> /* Control Register */
>> #define IOMMU_CONTROL_MMIO_OFFSET 0x18
>
>
>
[-- Attachment #2: iommu_errata.patch --]
[-- Type: text/x-patch, Size: 4316 bytes --]
# HG changeset patch
# Parent 592d15bd4d5ec58486d32ee9998319e7c95fcd66
# User Wei Wang <wei.wang2@amd.com>
amd iommu: Add workaround for erratum 732 & 733
Signed-off-by: Wei Wang <wei.wang2@amd.com>
diff -r 592d15bd4d5e xen/drivers/passthrough/amd/iommu_init.c
--- a/xen/drivers/passthrough/amd/iommu_init.c Fri May 18 16:19:21 2012 +0100
+++ b/xen/drivers/passthrough/amd/iommu_init.c Tue May 22 17:04:57 2012 +0200
@@ -29,6 +29,7 @@
#include <asm/hvm/svm/amd-iommu-proto.h>
#include <asm-x86/fixmap.h>
#include <mach_apic.h>
+#include <xen/delay.h>
static int __initdata nr_amd_iommus;
@@ -566,6 +567,7 @@ static void parse_event_log_entry(struct
u16 domain_id, device_id, bdf, cword;
u32 code;
u64 *addr;
+ int count = 0;
char * event_str[] = {"ILLEGAL_DEV_TABLE_ENTRY",
"IO_PAGE_FAULT",
"DEV_TABLE_HW_ERROR",
@@ -578,6 +580,25 @@ static void parse_event_log_entry(struct
code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
IOMMU_EVENT_CODE_SHIFT);
+ /*
+ * Workaround for erratum 732.
+ * it can happen that the tail pointer is updated before the actual entry
+ * is written. Suggested by RevGuide, we initialize the event log buffer to
+ * all zeros and write event log entries to zero after they are processed.
+ */
+
+ while ( code == 0 )
+ {
+ if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
+ {
+ AMD_IOMMU_DEBUG("AMD-Vi: No event written to log\n");
+ return;
+ }
+ udelay(1);
+ code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
+ IOMMU_EVENT_CODE_SHIFT);
+ }
+
if ( (code > IOMMU_EVENT_INVALID_DEV_REQUEST) ||
(code < IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY) )
{
@@ -615,6 +636,8 @@ static void parse_event_log_entry(struct
AMD_IOMMU_DEBUG("event 0x%08x 0x%08x 0x%08x 0x%08x\n", entry[0],
entry[1], entry[2], entry[3]);
}
+
+ memset(entry, 0, IOMMU_EVENT_LOG_ENTRY_SIZE);
}
static void iommu_check_event_log(struct amd_iommu *iommu)
@@ -646,10 +669,33 @@ void parse_ppr_log_entry(struct amd_iomm
{
u16 device_id;
- u8 bus, devfn;
+ u8 bus, devfn, code;
struct pci_dev *pdev;
struct domain *d;
+ int count = 0;
+ code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
+ IOMMU_PPR_LOG_CODE_SHIFT);
+
+ /*
+ * Workaround for erratum 733.
+ * it can happen that the tail pointer is updated before the actual entry
+ * is written. Suggested by RevGuide, we initialize the ppr log buffer to
+ * all zeros and write ppr log entries to zero after they are processed.
+ */
+
+ while ( code == 0 )
+ {
+ if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
+ {
+ AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to log\n");
+ return;
+ }
+ udelay(1);
+ code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
+ IOMMU_PPR_LOG_CODE_SHIFT);
+ }
+
/* here device_id is physical value */
device_id = iommu_get_devid_from_cmd(entry[0]);
bus = PCI_BUS(device_id);
@@ -665,6 +711,8 @@ void parse_ppr_log_entry(struct amd_iomm
d = pdev->domain;
guest_iommu_add_ppr_log(d, entry);
+
+ memset(entry, 0, IOMMU_PPR_LOG_ENTRY_SIZE);
}
static void iommu_check_ppr_log(struct amd_iommu *iommu)
diff -r 592d15bd4d5e xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Fri May 18 16:19:21 2012 +0100
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Tue May 22 17:04:57 2012 +0200
@@ -301,6 +301,10 @@
#define IOMMU_PPR_LOG_TAIL_OFFSET 0x2038
#define IOMMU_PPR_LOG_DEVICE_ID_MASK 0x0000FFFF
#define IOMMU_PPR_LOG_DEVICE_ID_SHIFT 0
+#define IOMMU_PPR_LOG_CODE_MASK 0xF0000000
+#define IOMMU_PPR_LOG_CODE_SHIFT 28
+
+#define IOMMU_LOG_ENTRY_TIMEOUT 1000
/* Control Register */
#define IOMMU_CONTROL_MMIO_OFFSET 0x18
[-- 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] 5+ messages in thread
* Re: [PATCH] amd iommu: Add workaround for erratum 732 & 733
2012-05-22 15:30 ` Wei Wang
@ 2012-05-23 6:57 ` Jan Beulich
2012-05-23 9:19 ` Wei Wang
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2012-05-23 6:57 UTC (permalink / raw)
To: Wei Wang; +Cc: Keir Fraser, xen-devel
>>> On 22.05.12 at 17:30, Wei Wang <wei.wang2@amd.com> wrote:
> Thanks for review it. New version has been attached. It should have
> fixed issues you mentioned. We don't have a particular number for loop
> count, so I cut it to 1000, it should be enough. Please have a look.
I adjusted it further to address a few things I noticed while
pulling it into my local tree. Please let me know if any of the
adjustments I did are in error.
I'll also send a half-way related (and dependent in terms of
being able to cleanly apply) follow-on patch in a minute.
Jan
**************************************************
amd iommu: Add workaround for erratum 732 & 733
Signed-off-by: Wei Wang <wei.wang2@amd.com>
Add missing barriers. Fix early return from parse_ppr_log_entry().
Slightly adjust comments. Strip trailing blanks.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -29,6 +29,7 @@
#include <asm/hvm/svm/amd-iommu-proto.h>
#include <asm-x86/fixmap.h>
#include <mach_apic.h>
+#include <xen/delay.h>
static int __initdata nr_amd_iommus;
@@ -566,6 +567,7 @@ static void parse_event_log_entry(struct
u16 domain_id, device_id, bdf, cword;
u32 code;
u64 *addr;
+ int count = 0;
char * event_str[] = {"ILLEGAL_DEV_TABLE_ENTRY",
"IO_PAGE_FAULT",
"DEV_TABLE_HW_ERROR",
@@ -578,6 +580,25 @@ static void parse_event_log_entry(struct
code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
IOMMU_EVENT_CODE_SHIFT);
+ /*
+ * Workaround for erratum 732:
+ * It can happen that the tail pointer is updated before the actual entry
+ * got written. As suggested by RevGuide, we initialize the event log
+ * buffer to all zeros and clear event log entries after processing them.
+ */
+ while ( code == 0 )
+ {
+ if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
+ {
+ AMD_IOMMU_DEBUG("AMD-Vi: No event written to log\n");
+ return;
+ }
+ udelay(1);
+ rmb();
+ code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
+ IOMMU_EVENT_CODE_SHIFT);
+ }
+
if ( (code > IOMMU_EVENT_INVALID_DEV_REQUEST) ||
(code < IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY) )
{
@@ -615,6 +636,8 @@ static void parse_event_log_entry(struct
AMD_IOMMU_DEBUG("event 0x%08x 0x%08x 0x%08x 0x%08x\n", entry[0],
entry[1], entry[2], entry[3]);
}
+
+ memset(entry, 0, IOMMU_EVENT_LOG_ENTRY_SIZE);
}
static void iommu_check_event_log(struct amd_iommu *iommu)
@@ -646,9 +669,31 @@ void parse_ppr_log_entry(struct amd_iomm
{
u16 device_id;
- u8 bus, devfn;
+ u8 bus, devfn, code;
struct pci_dev *pdev;
- struct domain *d;
+ int count = 0;
+
+ code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
+ IOMMU_PPR_LOG_CODE_SHIFT);
+
+ /*
+ * Workaround for erratum 733:
+ * It can happen that the tail pointer is updated before the actual entry
+ * got written. As suggested by RevGuide, we initialize the event log
+ * buffer to all zeros and clear ppr log entries after processing them.
+ */
+ while ( code == 0 )
+ {
+ if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
+ {
+ AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to log\n");
+ return;
+ }
+ udelay(1);
+ rmb();
+ code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
+ IOMMU_PPR_LOG_CODE_SHIFT);
+ }
/* here device_id is physical value */
device_id = iommu_get_devid_from_cmd(entry[0]);
@@ -659,12 +704,10 @@ void parse_ppr_log_entry(struct amd_iomm
pdev = pci_get_pdev(iommu->seg, bus, devfn);
spin_unlock(&pcidevs_lock);
- if ( pdev == NULL )
- return;
-
- d = pdev->domain;
+ if ( pdev )
+ guest_iommu_add_ppr_log(pdev->domain, entry);
- guest_iommu_add_ppr_log(d, entry);
+ memset(entry, 0, IOMMU_PPR_LOG_ENTRY_SIZE);
}
static void iommu_check_ppr_log(struct amd_iommu *iommu)
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -301,6 +301,10 @@
#define IOMMU_PPR_LOG_TAIL_OFFSET 0x2038
#define IOMMU_PPR_LOG_DEVICE_ID_MASK 0x0000FFFF
#define IOMMU_PPR_LOG_DEVICE_ID_SHIFT 0
+#define IOMMU_PPR_LOG_CODE_MASK 0xF0000000
+#define IOMMU_PPR_LOG_CODE_SHIFT 28
+
+#define IOMMU_LOG_ENTRY_TIMEOUT 1000
/* Control Register */
#define IOMMU_CONTROL_MMIO_OFFSET 0x18
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] amd iommu: Add workaround for erratum 732 & 733
2012-05-23 6:57 ` Jan Beulich
@ 2012-05-23 9:19 ` Wei Wang
0 siblings, 0 replies; 5+ messages in thread
From: Wei Wang @ 2012-05-23 9:19 UTC (permalink / raw)
To: Jan Beulich; +Cc: Keir Fraser, xen-devel
On 05/23/2012 08:57 AM, Jan Beulich wrote:
>>>> On 22.05.12 at 17:30, Wei Wang<wei.wang2@amd.com> wrote:
>> Thanks for review it. New version has been attached. It should have
>> fixed issues you mentioned. We don't have a particular number for loop
>> count, so I cut it to 1000, it should be enough. Please have a look.
>
> I adjusted it further to address a few things I noticed while
> pulling it into my local tree. Please let me know if any of the
> adjustments I did are in error.
>
> I'll also send a half-way related (and dependent in terms of
> being able to cleanly apply) follow-on patch in a minute.
Both look great to me. Acked.
Thanks,
Wei
> Jan
>
> **************************************************
>
> amd iommu: Add workaround for erratum 732& 733
>
> Signed-off-by: Wei Wang<wei.wang2@amd.com>
>
> Add missing barriers. Fix early return from parse_ppr_log_entry().
> Slightly adjust comments. Strip trailing blanks.
>
> Signed-off-by: Jan Beulich<jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -29,6 +29,7 @@
> #include<asm/hvm/svm/amd-iommu-proto.h>
> #include<asm-x86/fixmap.h>
> #include<mach_apic.h>
> +#include<xen/delay.h>
>
> static int __initdata nr_amd_iommus;
>
> @@ -566,6 +567,7 @@ static void parse_event_log_entry(struct
> u16 domain_id, device_id, bdf, cword;
> u32 code;
> u64 *addr;
> + int count = 0;
> char * event_str[] = {"ILLEGAL_DEV_TABLE_ENTRY",
> "IO_PAGE_FAULT",
> "DEV_TABLE_HW_ERROR",
> @@ -578,6 +580,25 @@ static void parse_event_log_entry(struct
> code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
> IOMMU_EVENT_CODE_SHIFT);
>
> + /*
> + * Workaround for erratum 732:
> + * It can happen that the tail pointer is updated before the actual entry
> + * got written. As suggested by RevGuide, we initialize the event log
> + * buffer to all zeros and clear event log entries after processing them.
> + */
> + while ( code == 0 )
> + {
> + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
> + {
> + AMD_IOMMU_DEBUG("AMD-Vi: No event written to log\n");
> + return;
> + }
> + udelay(1);
> + rmb();
> + code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
> + IOMMU_EVENT_CODE_SHIFT);
> + }
> +
> if ( (code> IOMMU_EVENT_INVALID_DEV_REQUEST) ||
> (code< IOMMU_EVENT_ILLEGAL_DEV_TABLE_ENTRY) )
> {
> @@ -615,6 +636,8 @@ static void parse_event_log_entry(struct
> AMD_IOMMU_DEBUG("event 0x%08x 0x%08x 0x%08x 0x%08x\n", entry[0],
> entry[1], entry[2], entry[3]);
> }
> +
> + memset(entry, 0, IOMMU_EVENT_LOG_ENTRY_SIZE);
> }
>
> static void iommu_check_event_log(struct amd_iommu *iommu)
> @@ -646,9 +669,31 @@ void parse_ppr_log_entry(struct amd_iomm
> {
>
> u16 device_id;
> - u8 bus, devfn;
> + u8 bus, devfn, code;
> struct pci_dev *pdev;
> - struct domain *d;
> + int count = 0;
> +
> + code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
> + IOMMU_PPR_LOG_CODE_SHIFT);
> +
> + /*
> + * Workaround for erratum 733:
> + * It can happen that the tail pointer is updated before the actual entry
> + * got written. As suggested by RevGuide, we initialize the event log
> + * buffer to all zeros and clear ppr log entries after processing them.
> + */
> + while ( code == 0 )
> + {
> + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
> + {
> + AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to log\n");
> + return;
> + }
> + udelay(1);
> + rmb();
> + code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
> + IOMMU_PPR_LOG_CODE_SHIFT);
> + }
>
> /* here device_id is physical value */
> device_id = iommu_get_devid_from_cmd(entry[0]);
> @@ -659,12 +704,10 @@ void parse_ppr_log_entry(struct amd_iomm
> pdev = pci_get_pdev(iommu->seg, bus, devfn);
> spin_unlock(&pcidevs_lock);
>
> - if ( pdev == NULL )
> - return;
> -
> - d = pdev->domain;
> + if ( pdev )
> + guest_iommu_add_ppr_log(pdev->domain, entry);
>
> - guest_iommu_add_ppr_log(d, entry);
> + memset(entry, 0, IOMMU_PPR_LOG_ENTRY_SIZE);
> }
>
> static void iommu_check_ppr_log(struct amd_iommu *iommu)
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -301,6 +301,10 @@
> #define IOMMU_PPR_LOG_TAIL_OFFSET 0x2038
> #define IOMMU_PPR_LOG_DEVICE_ID_MASK 0x0000FFFF
> #define IOMMU_PPR_LOG_DEVICE_ID_SHIFT 0
> +#define IOMMU_PPR_LOG_CODE_MASK 0xF0000000
> +#define IOMMU_PPR_LOG_CODE_SHIFT 28
> +
> +#define IOMMU_LOG_ENTRY_TIMEOUT 1000
>
> /* Control Register */
> #define IOMMU_CONTROL_MMIO_OFFSET 0x18
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-05-23 9:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-22 13:56 [PATCH] amd iommu: Add workaround for erratum 732 & 733 Wei Wang
2012-05-22 14:10 ` Jan Beulich
2012-05-22 15:30 ` Wei Wang
2012-05-23 6:57 ` Jan Beulich
2012-05-23 9:19 ` Wei Wang
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.