[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] amd iommu: Add workaround for erratum 732 & 733



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@xxxxxxx>  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@xxxxxxx>
# 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@xxxxxxx>

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: iommu_errata.patch
Description: iommu_errata.patch

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.