From: Wei Wang <wei.wang2@amd.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>, xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH] amd iommu: Add workaround for erratum 732 & 733
Date: Tue, 22 May 2012 17:30:38 +0200 [thread overview]
Message-ID: <4FBBB11E.5070709@amd.com> (raw)
In-Reply-To: <4FBBBA870200007800085296@nat28.tlf.novell.com>
[-- 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
next prev parent reply other threads:[~2012-05-22 15:30 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2012-05-23 6:57 ` Jan Beulich
2012-05-23 9:19 ` Wei Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4FBBB11E.5070709@amd.com \
--to=wei.wang2@amd.com \
--cc=JBeulich@suse.com \
--cc=keir@xen.org \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.