Fix kdump faults on system with amd iommu

drivers/iommu/amd_iommu_init.c +17 -0

+        for_each_iommu(iommu) {
+		if (!translation_pre_enabled(iommu)) {
+			pr_err("IOMMU:%d is not pre-enabled!/n", iommu->index);
+			return -1;
+		}
+
+		if (copied)
+			continue;
+
+                lo = readl(iommu->mmio_base + MMIO_DEV_TABLE_OFFSET);
+                hi = readl(iommu->mmio_base + MMIO_DEV_TABLE_OFFSET + 4);
+                entry = (((u64) hi) << 32) + lo;
+                old_devtb_phys = entry & PAGE_MASK;
+                old_devtb = memremap(old_devtb_phys, dev_table_size, MEMREMAP_WB);
+		if (!old_devtb)
+			return -1;
+                for (devid = 0; devid <= amd_iommu_last_bdf; ++devid) {

Hi Baoquan,

drivers/iommu/amd_iommu_init.c static int get_dev_entry_bit(u16 devid, u8 bit) · line 728

Quoted 9 lines · Baoquan He

drivers/iommu/amd_iommu_init.c +9 -0

+static int copy_dev_tables(void)
+{
+	u64 entry;
+	u32 lo, hi, devid;
+	phys_addr_t old_devtb_phys;
+	struct dev_table_entry *old_devtb = NULL;
+	u16 dom_id, dte_v;
+	struct amd_iommu *iommu;
+	static int copied;

Please order this by line-length, longer lines first.

Quoted 18 lines

drivers/iommu/amd_iommu_init.c +17 -0

+        for_each_iommu(iommu) {
+		if (!translation_pre_enabled(iommu)) {
+			pr_err("IOMMU:%d is not pre-enabled!/n", iommu->index);
+			return -1;
+		}
+
+		if (copied)
+			continue;
+
+                lo = readl(iommu->mmio_base + MMIO_DEV_TABLE_OFFSET);
+                hi = readl(iommu->mmio_base + MMIO_DEV_TABLE_OFFSET + 4);
+                entry = (((u64) hi) << 32) + lo;
+                old_devtb_phys = entry & PAGE_MASK;
+                old_devtb = memremap(old_devtb_phys, dev_table_size, MEMREMAP_WB);
+		if (!old_devtb)
+			return -1;
+                for (devid = 0; devid <= amd_iommu_last_bdf; ++devid) {

drivers/iommu/amd_iommu_init.c static int get_dev_entry_bit(u16 devid, u8 bit) · line 756

Quoted 10 lines

drivers/iommu/amd_iommu_init.c +10 -0

+                        amd_iommu_dev_table[devid] = old_devtb[devid];
+                        dom_id = amd_iommu_dev_table[devid].data[1] & DEV_DOMID_MASK;
+			dte_v = amd_iommu_dev_table[devid].data[0] & DTE_FLAG_V;
+			if (!dte_v || !dom_id)
+				continue;
+                        __set_bit(dom_id, amd_iommu_pd_alloc_bitmap);
+                }
+		memunmap(old_devtb);
+		copied = 1;
+        }

This loop need more refinement and sanity checking code. I suggest using two loops and do the sanity checking in the first one. The sanity checks should do:

* Check whether all IOMMUs actually use the same device table with the same size

* Verify that the size of the old device table is the expected size.

* Also sanity check the irq-remapping information and remapping table sizes.

If any of these checks fail, just bail out of copying.

What is further needed it some more selection on what is copied from the old kernel. There is no need to copy all the GCR3 root-pointer information. If a device is set up with guest translations (DTE.GV=1), then don't copy that information but move the device over to an empty guest-cr3 table and handle the faults in the PPR log (which should just answer them with INVALID). After all these PPR faults are recoverable for the device and we should not allow the device to change old-kernels data when we don't have to.

Here several things need be done:
1) If iommu is pre-enabled in a normal kernel, just disable it and print warning.
2) If failed to copy dev table of old kernel, continue to proceed as it does in normal kernel.
3) Re-enable event/cmd buffer and install the new DTE table to reg.
4) Flush all caches

Signed-off-by Baoquan He <[email protected]>

---
 drivers/iommu/amd_iommu_init.c | 44 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

drivers/iommu/amd_iommu_init.c +0 -0

index ce49641..47a8fc9 100644

drivers/iommu/amd_iommu_init.c +6 -0

 #include "amd_iommu_proto.h"
 #include "amd_iommu_types.h"
 #include "irq_remapping.h"

@@ -1344,6 +1344,12 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h)
 	iommu->int_enabled = false;
 
 	init_translation_status(iommu);
+	if (translation_pre_enabled(iommu) && !is_kdump_kernel()) {
+		iommu_disable(iommu);
+		clear_translation_pre_enabled(iommu);
+		pr_warn("Translation was enabled for IOMMU:%d but we are not in kdump mode\n",
+			iommu->index);
+	}
 
 	if (translation_pre_enabled(iommu))
 		pr_warn("Translation is already enabled - trying to copy translation structures\n");

drivers/iommu/amd_iommu_init.c +14 -0

+			is_pre_enabled = true;
+			break;
+		}
+	}
+
+	if ( !is_pre_enabled) {
+		for_each_iommu(iommu)
+			early_enable_iommu(iommu);
+	} else {
+		if (copy_dev_tables()) {
+			pr_err("Failed to copy DEV table from previous kernel.\n");
+			/*
+			 * If failed to copy dev tables from old kernel, continue to proceed
+			 * as it does in normal kernel.

drivers/iommu/amd_iommu_init.c +7 -0

+		                iommu_enable_command_buffer(iommu);
+		                iommu_enable_event_buffer(iommu);
+		                iommu_set_device_table(iommu);
+		                iommu_flush_all_caches(iommu);
+			}
+		}
+	}
 }
 
 static void enable_iommus_v2(void)

Signature · 1 line

2.5.5

Quoted 13 lines · Baoquan He

Here several things need be done:
1) If iommu is pre-enabled in a normal kernel, just disable it and print warning.
2) If failed to copy dev table of old kernel, continue to proceed as it does in normal kernel.
3) Re-enable event/cmd buffer and install the new DTE table to reg.
4) Flush all caches

Signed-off-by Baoquan He <[email protected]>

---
 drivers/iommu/amd_iommu_init.c | 44 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

drivers/iommu/amd_iommu_init.c line 37

Quoted 10 lines

drivers/iommu/amd_iommu_init.c +1 -1

index ce49641..47a8fc9 100644
@@ -34,7 +34,7 @@
 #include <asm/iommu_table.h>
 #include <asm/io_apic.h>
 #include <asm/irq_remapping.h>
-
+#include <linux/crash_dump.h>

Please keep that empty line, it is there for readability.

Quoted 23 lines

drivers/iommu/amd_iommu_init.c +7 -0

 #include "amd_iommu_proto.h"
 #include "amd_iommu_types.h"
 #include "irq_remapping.h"

@@ -1344,6 +1344,12 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h)
 	iommu->int_enabled = false;
 
 	init_translation_status(iommu);
+	if (translation_pre_enabled(iommu) && !is_kdump_kernel()) {
+		iommu_disable(iommu);
+		clear_translation_pre_enabled(iommu);
+		pr_warn("Translation was enabled for IOMMU:%d but we are not in kdump mode\n",
+			iommu->index);
+	}
 
 	if (translation_pre_enabled(iommu))
 		pr_warn("Translation is already enabled - trying to copy translation structures\n");

@@ -1946,9 +1952,41 @@ static void early_enable_iommu(struct amd_iommu *iommu)
 static void early_enable_iommus(void)
 {
 	struct amd_iommu *iommu;
+	bool is_pre_enabled=false;
 

drivers/iommu/amd_iommu_init.c static void early_enable_iommu(struct amd_iommu *iommu) · line 1958

Quoted 4 lines

drivers/iommu/amd_iommu_init.c +2 -2

-	for_each_iommu(iommu)
-		early_enable_iommu(iommu);
+	for_each_iommu(iommu) {
+		if ( translation_pre_enabled(iommu) ) {

Coding style, too many spaces. There is more of that below.

Quoted 19 lines

drivers/iommu/amd_iommu_init.c +18 -0

+			is_pre_enabled = true;
+			break;
+		}
+	}
+
+	if ( !is_pre_enabled) {
+		for_each_iommu(iommu)
+			early_enable_iommu(iommu);
+	} else {
+		if (copy_dev_tables()) {
+			pr_err("Failed to copy DEV table from previous kernel.\n");
+			/*
+			 * If failed to copy dev tables from old kernel, continue to proceed
+			 * as it does in normal kernel.
+			 */
+			for_each_iommu(iommu) {
+				clear_translation_pre_enabled(iommu);
+				early_enable_iommu(iommu);

drivers/iommu/amd_iommu_init.c static void early_enable_iommu(struct amd_iommu *iommu) · line 1982

Quoted 6 lines

drivers/iommu/amd_iommu_init.c +6 -0

+			}
+		} else {
+			pr_info("Copied DEV table from previous kernel.\n");
+			for_each_iommu(iommu) {
+				iommu_feature_disable(iommu, CONTROL_CMDBUF_EN);
+		                iommu_feature_disable(iommu, CONTROL_EVT_LOG_EN);

Could you move that into new helpers (iommu_disable_command_buffer...)?

Quoted 14 lines

drivers/iommu/amd_iommu_init.c +7 -0

+		                iommu_enable_command_buffer(iommu);
+		                iommu_enable_event_buffer(iommu);
+		                iommu_set_device_table(iommu);
+		                iommu_flush_all_caches(iommu);
+			}
+		}
+	}
 }
 
 static void enable_iommus_v2(void)

Signature · 1 line

2.5.5

Quoted 13 lines

drivers/iommu/amd_iommu_init.c +13 -0

+	if ( !is_pre_enabled) {
+		for_each_iommu(iommu)
+			early_enable_iommu(iommu);
+	} else {
+		if (copy_dev_tables()) {
+			pr_err("Failed to copy DEV table from previous kernel.\n");
+			/*
+			 * If failed to copy dev tables from old kernel, continue to proceed
+			 * as it does in normal kernel.
+			 */
+			for_each_iommu(iommu) {
+				clear_translation_pre_enabled(iommu);
+				early_enable_iommu(iommu);

drivers/iommu/amd_iommu_init.c static void early_enable_iommu(struct amd_iommu *iommu) · line 1982

Quoted 6 lines

drivers/iommu/amd_iommu_init.c +6 -0

+			}
+		} else {
+			pr_info("Copied DEV table from previous kernel.\n");
+			for_each_iommu(iommu) {
+				iommu_feature_disable(iommu, CONTROL_CMDBUF_EN);
+		                iommu_feature_disable(iommu, CONTROL_EVT_LOG_EN);

Could you move that into new helpers (iommu_disable_command_buffer...)?

Quoted 14 lines

drivers/iommu/amd_iommu_init.c +7 -0

+		                iommu_enable_command_buffer(iommu);
+		                iommu_enable_event_buffer(iommu);
+		                iommu_set_device_table(iommu);
+		                iommu_flush_all_caches(iommu);
+			}
+		}
+	}
 }
 
 static void enable_iommus_v2(void)

Signature · 1 line

2.5.5

Hi Joerg,

Quoted 13 lines · Joerg Roedel

Quoted 13 lines

drivers/iommu/amd_iommu_init.c +13 -0

+	if ( !is_pre_enabled) {
+		for_each_iommu(iommu)
+			early_enable_iommu(iommu);
+	} else {
+		if (copy_dev_tables()) {
+			pr_err("Failed to copy DEV table from previous kernel.\n");
+			/*
+			 * If failed to copy dev tables from old kernel, continue to proceed
+			 * as it does in normal kernel.
+			 */
+			for_each_iommu(iommu) {
+				clear_translation_pre_enabled(iommu);
+				early_enable_iommu(iommu);

Quoted 8 lines

drivers/iommu/amd_iommu_init.c static void early_enable_iommu(struct amd_iommu *iommu) · line 1982

Quoted 6 lines

drivers/iommu/amd_iommu_init.c +6 -0

+			}
+		} else {
+			pr_info("Copied DEV table from previous kernel.\n");
+			for_each_iommu(iommu) {
+				iommu_feature_disable(iommu, CONTROL_CMDBUF_EN);
+		                iommu_feature_disable(iommu, CONTROL_EVT_LOG_EN);

Could you move that into new helpers (iommu_disable_command_buffer...)?

Did you mean wraping iommu_feature_disable(iommu, CONTROL_CMDBUF_EN) into a helper function like iommu_disable_command_buffer(), and wraping iommu_feature_disable(iommu, CONTROL_EVT_LOG_EN) into iommu_disable_event_buffer()?

I retest with not disabling command buffer and event log here, it works on amd iommu v1 and v2 systems. So if I understand your comment correctly, there's no need to add "iommu_feature_disable(iommu, CONTROL_CMDBUF_EN)" and "iommu_feature_disable(iommu, CONTROL_EVT_LOG_EN)" here. I remember I added them here because more IO_PAGE_FAULT messages are printed without them. But now it seems not to related to them. So I will remove above two lines of code.

This enabling should have been done in normal kernel. It's unnecessary to enable it again in kdump kernel.

And clean up the function comments of init_device_table_dma.

Quoted 4 lines · Baoquan He

This enabling should have been done in normal kernel. It's unnecessary to enable it again in kdump kernel.

And clean up the function comments of init_device_table_dma.

Well, no. We don't want to make any assumptions on what the previous kernel did and what it did not. The init_device_table_dma() code should run anyway.

AMD iommu creates protection domain and assign each device to it during iommu driver initialization stage. This happened just after system pci bus scanning stage, and much earlier than device driver init stage. So at this time if in kdump kernel the domain info, especially pte_root, can't be updated to dte entry. We should wait until device driver init stage.

Signed-off-by Baoquan He <[email protected]>

---
 drivers/iommu/amd_iommu.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/iommu/amd_iommu.c +10 -0

index fcb69ff..6c37300 100644
@@ -137,6 +137,7 @@ struct iommu_dev_data {
 	bool pri_tlp;			  /* PASID TLB required for
 					     PPR completions */
 	u32 errata;			  /* Bitmap for errata to apply */
+	bool domain_updated;
 };
 
 /*

@@ -1708,6 +1709,15 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
 {
 	u64 pte_root = 0;
 	u64 flags = 0;
+	struct iommu_dev_data *dev_data;
+	struct amd_iommu *iommu = amd_iommu_rlookup_table[devid];
+
+	dev_data = find_dev_data(devid);
+        if (!dev_data)
+                return;
+
+	if (translation_pre_enabled(iommu) && !dev_data->domain_updated)
+		return;
 
 	if (domain->mode != PAGE_MODE_NONE)
 		pte_root = virt_to_phys(domain->pt_root);

@@ -1756,6 +1766,14 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
 

drivers/iommu/amd_iommu.c +0 -0

 	/* remove entry from the device table seen by the hardware */
 	amd_iommu_dev_table[devid].data[0]  = DTE_FLAG_V | DTE_FLAG_TV;
 	amd_iommu_dev_table[devid].data[1] &= DTE_FLAG_MASK;

Signature · 1 line

2.5.5

Quoted 42 lines · Baoquan He

AMD iommu creates protection domain and assign each device to it during iommu driver initialization stage. This happened just after system pci bus scanning stage, and much earlier than device driver init stage. So at this time if in kdump kernel the domain info, especially pte_root, can't be updated to dte entry. We should wait until device driver init stage.

Signed-off-by Baoquan He <[email protected]>

---
 drivers/iommu/amd_iommu.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/iommu/amd_iommu.c +10 -0

index fcb69ff..6c37300 100644
@@ -137,6 +137,7 @@ struct iommu_dev_data {
 	bool pri_tlp;			  /* PASID TLB required for
 					     PPR completions */
 	u32 errata;			  /* Bitmap for errata to apply */
+	bool domain_updated;
 };
 
 /*

@@ -1708,6 +1709,15 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
 {
 	u64 pte_root = 0;
 	u64 flags = 0;
+	struct iommu_dev_data *dev_data;
+	struct amd_iommu *iommu = amd_iommu_rlookup_table[devid];
+
+	dev_data = find_dev_data(devid);
+        if (!dev_data)
+                return;
+
+	if (translation_pre_enabled(iommu) && !dev_data->domain_updated)
+		return;
 
 	if (domain->mode != PAGE_MODE_NONE)
 		pte_root = virt_to_phys(domain->pt_root);

@@ -1756,6 +1766,14 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
 

drivers/iommu/amd_iommu.c static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats) · line 1776

Quoted 10 lines

drivers/iommu/amd_iommu.c +8 -0

 static void clear_dte_entry(u16 devid)
 {
+	struct iommu_dev_data *dev_data;
+	struct amd_iommu *iommu = amd_iommu_rlookup_table[devid];
+
+	dev_data = find_dev_data(devid);
+        if (!dev_data)
+                return;
+	if (translation_pre_enabled(iommu) && !dev_data->domain_updated)
+		return;

This should be moved to do_attach/do_detach. There you also already have the dev_data you need here.

Quoted 7 lines

drivers/iommu/amd_iommu.c +0 -0

 	/* remove entry from the device table seen by the hardware */
 	amd_iommu_dev_table[devid].data[0]  = DTE_FLAG_V | DTE_FLAG_TV;
 	amd_iommu_dev_table[devid].data[1] &= DTE_FLAG_MASK;

Signature · 1 line

2.5.5

Quoted 43 lines · Joerg Roedel

Quoted 42 lines · Baoquan He

AMD iommu creates protection domain and assign each device to it during iommu driver initialization stage. This happened just after system pci bus scanning stage, and much earlier than device driver init stage. So at this time if in kdump kernel the domain info, especially pte_root, can't be updated to dte entry. We should wait until device driver init stage.

Signed-off-by Baoquan He <[email protected]>

---
 drivers/iommu/amd_iommu.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/iommu/amd_iommu.c +10 -0

index fcb69ff..6c37300 100644
@@ -137,6 +137,7 @@ struct iommu_dev_data {
 	bool pri_tlp;			  /* PASID TLB required for
 					     PPR completions */
 	u32 errata;			  /* Bitmap for errata to apply */
+	bool domain_updated;
 };
 
 /*

@@ -1708,6 +1709,15 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
 {
 	u64 pte_root = 0;
 	u64 flags = 0;
+	struct iommu_dev_data *dev_data;
+	struct amd_iommu *iommu = amd_iommu_rlookup_table[devid];
+
+	dev_data = find_dev_data(devid);
+        if (!dev_data)
+                return;
+
+	if (translation_pre_enabled(iommu) && !dev_data->domain_updated)
+		return;
 
 	if (domain->mode != PAGE_MODE_NONE)
 		pte_root = virt_to_phys(domain->pt_root);

@@ -1756,6 +1766,14 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
 

Quoted 13 lines

drivers/iommu/amd_iommu.c static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats) · line 1776

Quoted 10 lines

drivers/iommu/amd_iommu.c +8 -0

 static void clear_dte_entry(u16 devid)
 {
+	struct iommu_dev_data *dev_data;
+	struct amd_iommu *iommu = amd_iommu_rlookup_table[devid];
+
+	dev_data = find_dev_data(devid);
+        if (!dev_data)
+                return;
+	if (translation_pre_enabled(iommu) && !dev_data->domain_updated)
+		return;

This should be moved to do_attach/do_detach. There you also already have the dev_data you need here.

For amd-vi v1, checking in do_attach/do_detach is enough. But for v2 amd_iommu_domain_direct_map and amd_iommu_domain_enable_v2 also call it

to install pte_root into dev table. So finally, I move them into these two lowest level functions to prevent any pte_root changing during iommu init stage. It involves the least code change.

Quoted 7 lines

Quoted 7 lines

drivers/iommu/amd_iommu.c +0 -0

 	/* remove entry from the device table seen by the hardware */
 	amd_iommu_dev_table[devid].data[0]  = DTE_FLAG_V | DTE_FLAG_TV;
 	amd_iommu_dev_table[devid].data[1] &= DTE_FLAG_MASK;

Signature · 1 line

2.5.5

All devices are supposed to reset themselves at device driver initialization stage. At this time if in kdump kernel those on-flight DMA will be stopped because of device reset. It's best time to update the protection domain info, especially pte_root, to dte entry which the device relates to.

Signed-off-by Baoquan He <[email protected]>

---
 drivers/iommu/amd_iommu.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

drivers/iommu/amd_iommu.c +4 -0

index 6c37300..00b64ee 100644
@@ -2310,6 +2310,10 @@ static dma_addr_t __map_single(struct device *dev,
 	unsigned int pages;
 	int prot = 0;
 	int i;
+	struct iommu_dev_data *dev_data = get_dev_data(dev);
+	struct protection_domain *domain = get_domain(dev);
+	u16 alias = amd_iommu_alias_table[dev_data->devid];
+	struct amd_iommu *iommu = amd_iommu_rlookup_table[dev_data->devid];
 
 	pages = iommu_num_pages(paddr, size, PAGE_SIZE);
 	paddr &= PAGE_MASK;

Quoted 28 lines · Baoquan He

All devices are supposed to reset themselves at device driver initialization stage. At this time if in kdump kernel those on-flight DMA will be stopped because of device reset. It's best time to update the protection domain info, especially pte_root, to dte entry which the device relates to.

Signed-off-by Baoquan He <[email protected]>

---
 drivers/iommu/amd_iommu.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

drivers/iommu/amd_iommu.c +4 -0

index 6c37300..00b64ee 100644
@@ -2310,6 +2310,10 @@ static dma_addr_t __map_single(struct device *dev,
 	unsigned int pages;
 	int prot = 0;
 	int i;
+	struct iommu_dev_data *dev_data = get_dev_data(dev);
+	struct protection_domain *domain = get_domain(dev);
+	u16 alias = amd_iommu_alias_table[dev_data->devid];
+	struct amd_iommu *iommu = amd_iommu_rlookup_table[dev_data->devid];
 
 	pages = iommu_num_pages(paddr, size, PAGE_SIZE);
 	paddr &= PAGE_MASK;

@@ -2319,6 +2323,13 @@ static dma_addr_t __map_single(struct device *dev,
 		goto out;
 

drivers/iommu/amd_iommu.c static dma_addr_t __map_single(struct device *dev, · line 2332

Quoted 8 lines

drivers/iommu/amd_iommu.c +7 -0

 	prot = dir2prot(direction);
+	if (translation_pre_enabled(iommu) && !dev_data->domain_updated) {
+               dev_data->domain_updated = true;
+               set_dte_entry(dev_data->devid, domain, dev_data->ats.enabled);
+               if (alias != dev_data->devid)
+                       set_dte_entry(alias, domain, dev_data->ats.enabled);
+               device_flush_dte(dev_data);
+       }

Hmm, have you tried hooking this into the set_dma_mask call-back? Every driver should call it for its device, so that should be a better indicator to now map a new domain.

Hmm, have you tried hooking this into the set_dma_mask call-back? Every driver should call it for its device, so that should be a better indicator to now map a new domain.

From 1ca66f886a46839cb937fd1e6a1d84b6719f23c4 Mon Sep 17 00:00:00 2001