[PATCH 0/4] AMD IOMMU: misc adjustments

[PATCH 0/4] AMD IOMMU: misc adjustments

[Jan Beulich]

This is to improve dealing with systems that have incorrect ACPI
tables, an erratum workaround ported over from Linux, and a
little bit of cleanup.

1: don't BUG() when we don't have to
2: cover all functions of a device even if ACPI only tells us of func 0
3: Family15h Model10-1Fh erratum 746 Workaround
4: use __ioapic_read_entry() instead of open coding it

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 1/4] AMD IOMMU: don't BUG() when we don't have to

[Jan Beulich]

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

[PATCH 2/4] AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0

[Jan Beulich]

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

[PATCH 3/4] IOMMU, AMD Family15h Model10-1Fh erratum 746 Workaround

[Jan Beulich]

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

[PATCH 4/4] AMD IOMMU: use __ioapic_read_entry() instead of open coding it

[Jan Beulich]

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

Re: [PATCH 2/4] AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0

[Boris Ostrovsky]

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

Re: [PATCH 4/4] AMD IOMMU: use __ioapic_read_entry() instead of open coding it

[Boris Ostrovsky]

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

Re: [PATCH 1/4] AMD IOMMU: don't BUG() when we don't have to

[Boris Ostrovsky]

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;
>   }
>   
>   /*
>
>
>

Re: [PATCH 1/4] AMD IOMMU: don't BUG() when we don't have to

[Jan Beulich]

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

Re: [PATCH 2/4] AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0

[Jan Beulich]

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

Re: [PATCH 3/4] IOMMU, AMD Family15h Model10-1Fh erratum 746 Workaround

[Suravee Suthikulanit]

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

Re: [PATCH 3/4] IOMMU, AMD Family15h Model10-1Fh erratum 746 Workaround

[Jan Beulich]

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