amd iommu: Add workaround for erratum 732 & 733

Quoted 1 line

Quoted 1 line · Wei Wang

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 <[email protected]>
# Date 1337690747 -7200
# Node ID 06aebc361de7f308b262b008153ae4549c4480c2 # Parent 592d15bd4d5ec58486d32ee9998319e7c95fcd66 amd iommu: Add workaround for erratum 733 & 733

Signed-off-by Wei Wang <[email protected]>

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

xen/drivers/passthrough/amd/iommu_init.c Tue May 22 14:45:47 2012 +0200 renamed from xen/drivers/passthrough/amd/iommu_init.c Fri May 18 16:19:21 2012 +0100 +2 -0

@@ -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",

xen/drivers/passthrough/amd/iommu_init.c Tue May 22 14:45:47 2012 +0200 renamed from xen/drivers/passthrough/amd/iommu_init.c Fri May 18 16:19:21 2012 +0100 +8 -1

+            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;

xen/drivers/passthrough/amd/iommu_init.c Tue May 22 14:45:47 2012 +0200 renamed from xen/drivers/passthrough/amd/iommu_init.c Fri May 18 16:19:21 2012 +0100 +6 -0

+            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);

xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Tue May 22 14:45:47 2012 +0200 renamed from xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Fri May 18 16:19:21 2012 +0100 +0 -0

  /* Control Register */
  #define IOMMU_CONTROL_MMIO_OFFSET            0x18

Quoted 6 lines

Quoted 1 line

Quoted 1 line · Wei Wang

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:

Quoted 6 lines

# HG changeset patch
# User Wei Wang<[email protected]>
# Date 1337690747 -7200
# Node ID 06aebc361de7f308b262b008153ae4549c4480c2 # Parent 592d15bd4d5ec58486d32ee9998319e7c95fcd66 amd iommu: Add workaround for erratum 733& 733

732& 733

Quoted 25 lines

Signed-off-by Wei Wang<[email protected]>

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

(quoted diff) +2 -0

@@ -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",

Review static void parse_event_log_entry(struct · line 584

Quoted 8 lines

(quoted diff) +2 -0

@@ -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.

Review

Quoted 13 lines

(quoted diff) +1 -0

+     * 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.

(quoted diff) +7 -0

+     */
+
+    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".

Quoted 23 lines

(quoted diff) +8 -1

+            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;

Review void parse_ppr_log_entry(struct amd_iomm · line 680

Quoted 10 lines

(quoted diff) +7 -0

+    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.

Review

Quoted 13 lines

(quoted diff) +1 -0

+     * 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.

(quoted diff) +7 -0

+     */
+
+    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".

Quoted 10 lines

(quoted diff) +6 -0

+            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);

Review void parse_ppr_log_entry(struct amd_iomm · line 716

Quoted 8 lines

(quoted diff) +2 -0

@@ -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).

Review line 307

Quoted 12 lines

--- 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

(quoted diff) +4 -0

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

Quoted 3 lines

(quoted diff) +0 -0

   /* Control Register */
   #define IOMMU_CONTROL_MMIO_OFFSET            0x18

Quoted 1 line

Quoted 1 line · Wei Wang

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.

Quoted 4 lines

Quoted 1 line

Quoted 1 line · Wei Wang

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<[email protected]>

Add missing barriers. Fix early return from parse_ppr_log_entry(). Slightly adjust comments. Strip trailing blanks.

Signed-off-by Jan Beulich<[email protected]>

xen/drivers/passthrough/amd/iommu_init.c +50 -7

@@ -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)

xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +4 -0

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