[PATCH v5 0/8] Fix kdump faults on system with amd iommu

[PATCH v5 0/8] Fix kdump faults on system with amd iommu

[Baoquan He]

This is v5 post. In fact in v3 the solution is correct. Just unluckily 
I got a AMD machine with bnx2 NIC which can't reset itself during driver
init. It made me very unconfident with my understanding about the fix.
Now with below fix the AMD machine with bnx2 NIC can also work well
to dump and there's no IO_PAGE_FAULT seen any more. Now network maintainer
has picked it up.

bnx2: Reset device during driver initialization
https://www.mail-archive.com/netdev@vger.kernel.org/msg127336.html

The principle of the fix is similar to intel iommu. Just defer the assignment
of device to domain to device driver init. But there's difference than
intel iommu. AMD iommu create protection domain and assign device to
domain in iommu driver init stage. So in this patchset I just allow the
assignment of device to domain in software level, but defer updating the
domain info, especially the pte_root to dev table entry to device driver
init stage.


Baoquan He (8):
  iommu/amd: Detect pre enabled translation
  iommu/amd: add early_enable_iommu() wrapper function
  iommu/amd: Define bit fields for DTE particularly
  iommu/amd: Add function copy_dev_tables
  iommu/amd: copy old trans table from old kernel
  iommu/amd: Do not re-enable dev table entries in kdump
  iommu/amd: Don't update domain info to dte entry at iommu init stage
  iommu/amd: Update domain into to dte entry during device driver init

 drivers/iommu/amd_iommu.c       |  49 +++++++++++++--
 drivers/iommu/amd_iommu_init.c  | 135 ++++++++++++++++++++++++++++++++++++----
 drivers/iommu/amd_iommu_proto.h |   1 +
 drivers/iommu/amd_iommu_types.h |  23 +++++--
 4 files changed, 187 insertions(+), 21 deletions(-)

-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 1/8] iommu/amd: Detect pre enabled translation

[Baoquan He]

Add functions to check whether translation is already enabled in IOMMU.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu_init.c  | 25 +++++++++++++++++++++++++
 drivers/iommu/amd_iommu_proto.h |  1 +
 drivers/iommu/amd_iommu_types.h |  4 ++++
 3 files changed, 30 insertions(+)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 59741ea..9bf1a04 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -247,6 +247,26 @@ static int amd_iommu_enable_interrupts(void);
 static int __init iommu_go_to_state(enum iommu_init_state state);
 static void init_device_table_dma(void);
 
+
+bool translation_pre_enabled(struct amd_iommu *iommu)
+{
+	return (iommu->flags & AMD_IOMMU_FLAG_TRANS_PRE_ENABLED);
+}
+
+static void clear_translation_pre_enabled(struct amd_iommu *iommu)
+{
+        iommu->flags &= ~AMD_IOMMU_FLAG_TRANS_PRE_ENABLED;
+}
+
+static void init_translation_status(struct amd_iommu *iommu)
+{
+	u32 ctrl;
+
+	ctrl = readl(iommu->mmio_base + MMIO_CONTROL_OFFSET);
+	if (ctrl & (1<<CONTROL_IOMMU_EN))
+		iommu->flags |= AMD_IOMMU_FLAG_TRANS_PRE_ENABLED;
+}
+
 static int iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
 				    u8 bank, u8 cntr, u8 fxn,
 				    u64 *value, bool is_write);
@@ -1283,6 +1303,11 @@ 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))
+		pr_warn("Translation is already enabled - trying to copy translation structures\n");
+
 	ret = init_iommu_from_acpi(iommu, h);
 	if (ret)
 		return ret;
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index 0bd9eb3..f066e01 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -98,4 +98,5 @@ static inline bool iommu_feature(struct amd_iommu *iommu, u64 f)
 	return !!(iommu->features & f);
 }
 
+extern bool translation_pre_enabled(struct amd_iommu *iommu);
 #endif /* _ASM_X86_AMD_IOMMU_PROTO_H  */
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index caf5e38..7781953 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -384,6 +384,7 @@ extern struct kmem_cache *amd_iommu_irq_cache;
 #define APERTURE_PAGE_INDEX(a)	(((a) >> 21) & 0x3fULL)
 
 
+
 /*
  * This struct is used to pass information about
  * incoming PPR faults around.
@@ -401,6 +402,8 @@ struct amd_iommu_fault {
 struct iommu_domain;
 struct irq_domain;
 
+#define AMD_IOMMU_FLAG_TRANS_PRE_ENABLED      (1 << 0)
+
 /*
  * This structure contains generic data for  IOMMU protection domains
  * independent of their use.
@@ -524,6 +527,7 @@ struct amd_iommu {
 	struct irq_domain *ir_domain;
 	struct irq_domain *msi_domain;
 #endif
+	u32 flags;
 };
 
 #define ACPIHID_UID_LEN 256
-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 2/8] iommu/amd: add early_enable_iommu() wrapper function

[Baoquan He]

Move per iommu enabling code into a wrapper function early_enable_iommu().
This can make later kdump change easier.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu_init.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 9bf1a04..77c44c8 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1887,6 +1887,18 @@ static void iommu_apply_resume_quirks(struct amd_iommu *iommu)
 			       iommu->stored_addr_lo | 1);
 }
 
+static void early_enable_iommu(struct amd_iommu *iommu)
+{
+	iommu_disable(iommu);
+	iommu_init_flags(iommu);
+	iommu_set_device_table(iommu);
+	iommu_enable_command_buffer(iommu);
+	iommu_enable_event_buffer(iommu);
+	iommu_set_exclusion_range(iommu);
+	iommu_enable(iommu);
+	iommu_flush_all_caches(iommu);
+}
+
 /*
  * This function finally enables all IOMMUs found in the system after
  * they have been initialized
@@ -1895,16 +1907,8 @@ static void early_enable_iommus(void)
 {
 	struct amd_iommu *iommu;
 
-	for_each_iommu(iommu) {
-		iommu_disable(iommu);
-		iommu_init_flags(iommu);
-		iommu_set_device_table(iommu);
-		iommu_enable_command_buffer(iommu);
-		iommu_enable_event_buffer(iommu);
-		iommu_set_exclusion_range(iommu);
-		iommu_enable(iommu);
-		iommu_flush_all_caches(iommu);
-	}
+	for_each_iommu(iommu)
+		early_enable_iommu(iommu);
 }
 
 static void enable_iommus_v2(void)
-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 3/8] iommu/amd: Define bit fields for DTE particularly

[Baoquan He]

In amd-vi spec several bits of IO PTE fields and DTE fields are similar
so that both of them can share the same MACRO definition. However
defining their respecitve bit fields can make code more read-able. So
do it in this patch.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu.c       |  8 ++++----
 drivers/iommu/amd_iommu_types.h | 18 ++++++++++++++----
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 96de97a..995b050 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1388,9 +1388,9 @@ static int iommu_map_page(struct protection_domain *dom,
 
 	if (count > 1) {
 		__pte = PAGE_SIZE_PTE(phys_addr, page_size);
-		__pte |= PM_LEVEL_ENC(7) | IOMMU_PTE_P | IOMMU_PTE_FC;
+		__pte |= PM_LEVEL_ENC(7) | IOMMU_PTE_PR | IOMMU_PTE_FC;
 	} else
-		__pte = phys_addr | IOMMU_PTE_P | IOMMU_PTE_FC;
+		__pte = phys_addr | IOMMU_PTE_PR | IOMMU_PTE_FC;
 
 	if (prot & IOMMU_PROT_IR)
 		__pte |= IOMMU_PTE_IR;
@@ -1714,7 +1714,7 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
 
 	pte_root |= (domain->mode & DEV_ENTRY_MODE_MASK)
 		    << DEV_ENTRY_MODE_SHIFT;
-	pte_root |= IOMMU_PTE_IR | IOMMU_PTE_IW | IOMMU_PTE_P | IOMMU_PTE_TV;
+	pte_root |= DTE_FLAG_IR | DTE_FLAG_IW | DTE_FLAG_V | DTE_FLAG_TV;
 
 	flags = amd_iommu_dev_table[devid].data[1];
 
@@ -1757,7 +1757,7 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
 static void clear_dte_entry(u16 devid)
 {
 	/* remove entry from the device table seen by the hardware */
-	amd_iommu_dev_table[devid].data[0]  = IOMMU_PTE_P | IOMMU_PTE_TV;
+	amd_iommu_dev_table[devid].data[0]  = DTE_FLAG_V | DTE_FLAG_TV;
 	amd_iommu_dev_table[devid].data[1] &= DTE_FLAG_MASK;
 
 	amd_iommu_apply_erratum_63(devid);
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 7781953..809944a 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -239,7 +239,7 @@
 #define PM_LEVEL_INDEX(x, a)	(((a) >> PM_LEVEL_SHIFT((x))) & 0x1ffULL)
 #define PM_LEVEL_ENC(x)		(((x) << 9) & 0xe00ULL)
 #define PM_LEVEL_PDE(x, a)	((a) | PM_LEVEL_ENC((x)) | \
-				 IOMMU_PTE_P | IOMMU_PTE_IR | IOMMU_PTE_IW)
+				 IOMMU_PTE_PR | IOMMU_PTE_IR | IOMMU_PTE_IW)
 #define PM_PTE_LEVEL(pte)	(((pte) >> 9) & 0x7ULL)
 
 #define PM_MAP_4k		0
@@ -288,13 +288,23 @@
 #define PTE_LEVEL_PAGE_SIZE(level)			\
 	(1ULL << (12 + (9 * (level))))
 
-#define IOMMU_PTE_P  (1ULL << 0)
-#define IOMMU_PTE_TV (1ULL << 1)
+/*
+ * Bit value definition for I/O PTE fields
+ */
+#define IOMMU_PTE_PR (1ULL << 0)
 #define IOMMU_PTE_U  (1ULL << 59)
 #define IOMMU_PTE_FC (1ULL << 60)
 #define IOMMU_PTE_IR (1ULL << 61)
 #define IOMMU_PTE_IW (1ULL << 62)
 
+/*
+ * Bit value definition for DTE fields
+ */
+#define DTE_FLAG_V  (1ULL << 0)
+#define DTE_FLAG_TV (1ULL << 1)
+#define DTE_FLAG_IR (1ULL << 61)
+#define DTE_FLAG_IW (1ULL << 62)
+
 #define DTE_FLAG_IOTLB	(1ULL << 32)
 #define DTE_FLAG_GV	(1ULL << 55)
 #define DTE_FLAG_MASK	(0x3ffULL << 32)
@@ -316,7 +326,7 @@
 #define GCR3_VALID		0x01ULL
 
 #define IOMMU_PAGE_MASK (((1ULL << 52) - 1) & ~0xfffULL)
-#define IOMMU_PTE_PRESENT(pte) ((pte) & IOMMU_PTE_P)
+#define IOMMU_PTE_PRESENT(pte) ((pte) & IOMMU_PTE_PR)
 #define IOMMU_PTE_PAGE(pte) (phys_to_virt((pte) & IOMMU_PAGE_MASK))
 #define IOMMU_PTE_MODE(pte) (((pte) >> 9) & 0x07)
 
-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 4/8] iommu/amd: Add function copy_dev_tables

[Baoquan He]

Add function copy_dev_tables to copy the old DEV table entry of the panicked
kernel to the new allocated DEV table. Since all iommu share the same DTE table
the copy only need be done once as long as the physical address of old DEV table
is retrieved from iommu reg. Besides the old domain id occupied in 1st kernel
need be reserved to avoid touching the old io-page tables so that on-flight DMA
can continue looking up.

And define MACRO DEV_DOMID_MASK to replace magic number 0xffffULL because
it need be reused in copy_dev_tables.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu.c       |  2 +-
 drivers/iommu/amd_iommu_init.c  | 40 ++++++++++++++++++++++++++++++++++++++++
 drivers/iommu/amd_iommu_types.h |  1 +
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 995b050..fcb69ff 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1747,7 +1747,7 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
 		flags    |= tmp;
 	}
 
-	flags &= ~(0xffffUL);
+	flags &= ~DEV_DOMID_MASK;
 	flags |= domain->id;
 
 	amd_iommu_dev_table[devid].data[1]  = flags;
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 77c44c8..ce49641 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -717,6 +717,46 @@ static int get_dev_entry_bit(u16 devid, u8 bit)
 }
 
 
+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;
+
+        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) {
+                        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;
+        }
+	return 0;
+}
+
 void amd_iommu_apply_erratum_63(u16 devid)
 {
 	int sysmgt;
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 809944a..a1ccede 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -310,6 +310,7 @@
 #define DTE_FLAG_MASK	(0x3ffULL << 32)
 #define DTE_GLX_SHIFT	(56)
 #define DTE_GLX_MASK	(3)
+#define DEV_DOMID_MASK 	0xffffULL
 
 #define DTE_GCR3_VAL_A(x)	(((x) >> 12) & 0x00007ULL)
 #define DTE_GCR3_VAL_B(x)	(((x) >> 15) & 0x0ffffULL)
-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 4/8] iommu/amd: Add function copy_dev_tables

[Baoquan He]

Hi Joerg,

Thanks for your reviewing and great suggestion!

On 09/20/16 at 01:58pm, Joerg Roedel wrote:
> Hi Baoquan,
> 
> On Thu, Sep 15, 2016 at 11:03:22PM +0800, Baoquan He wrote:
> > +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.

Will do.

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

Will do.

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

Will do. Since this need those irq DTE_IRQ_xxxx MACRO which is defined
in amd_iommu.c , I plan to move them into amd_iommu_types.h, and then do
irq-remapping. These can be made in another patch.

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

The current fix is simplest and cleanest. Because the on-flight DMAs
continue transferring data since system crash, including guest
translations, we may not need to care about it and just let it continue
flying a little more time until device is reset. Since you have suggested,
I will try to make another patch for this issue, we can see the effect.

Thanks
Baoquan

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 5/8] iommu/amd: copy old trans table from old kernel

[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 <bhe@redhat.com>
---
 drivers/iommu/amd_iommu_init.c | 44 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index ce49641..47a8fc9 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -34,7 +34,7 @@
 #include <asm/iommu_table.h>
 #include <asm/io_apic.h>
 #include <asm/irq_remapping.h>
-
+#include <linux/crash_dump.h>
 #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;
 
-	for_each_iommu(iommu)
-		early_enable_iommu(iommu);
+	for_each_iommu(iommu) {
+		if ( translation_pre_enabled(iommu) ) {
+			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);
+			}
+		} 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);
+		                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)
-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 5/8] iommu/amd: copy old trans table from old kernel

[Baoquan He]

On 09/20/16 at 02:40pm, Joerg Roedel wrote:
> On Thu, Sep 15, 2016 at 11:03:23PM +0800, Baoquan He wrote:
> > 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 <bhe@redhat.com>
> > ---
> >  drivers/iommu/amd_iommu_init.c | 44 +++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 41 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> > index ce49641..47a8fc9 100644
> > --- a/drivers/iommu/amd_iommu_init.c
> > +++ b/drivers/iommu/amd_iommu_init.c
> > @@ -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.

Thanks, will change.

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

Will change.

> 
> > +			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);
> > +			}
> > +		} 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...)?

Yes, sure, will do.

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 5/8] iommu/amd: copy old trans table from old kernel

[Baoquan He]

Hi Joerg,

On 09/20/16 at 02:40pm, Joerg Roedel wrote:
> > +	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);
> > +			}
> > +		} 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.

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 5/8] iommu/amd: copy old trans table from old kernel

[Baoquan He]

On 09/28/16 at 09:37am, Baoquan He wrote:
> Hi Joerg,
> 
> On 09/20/16 at 02:40pm, Joerg Roedel wrote:
> > > +	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);
> > > +			}
> > > +		} 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.

Oh, sorry, please ignore this. It will print many lines of below message
if do not disable cmd buffer and event log here.

[    0.455642] AMD-Vi: Completion-Wait loop timed out


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 6/8] iommu/amd: Do not re-enable dev table entries in kdump

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

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu_init.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 47a8fc9..8d5db2e 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1651,7 +1651,12 @@ static int __init amd_iommu_init_pci(void)
 	 */
 	ret = amd_iommu_init_api();
 
-	init_device_table_dma();
+	for_each_iommu(iommu) {
+		if ( !translation_pre_enabled(iommu) ) {
+			init_device_table_dma();
+			break;
+		}
+	}
 
 	for_each_iommu(iommu)
 		iommu_flush_all_caches(iommu);
@@ -1829,8 +1834,7 @@ static int __init init_memory_definitions(struct acpi_table_header *table)
 }
 
 /*
- * Init the device table to not allow DMA access for devices and
- * suppress all page faults
+ * Init the device table to not allow DMA access for devices.
  */
 static void init_device_table_dma(void)
 {
-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 6/8] iommu/amd: Do not re-enable dev table entries in kdump

[Baoquan He]

On 09/20/16 at 02:42pm, Joerg Roedel wrote:
> On Thu, Sep 15, 2016 at 11:03:24PM +0800, Baoquan He wrote:
> > 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.

Yes, right. I forget people could set amd_iommu=off in 1st kernel, but
remove it in kdump kernel. Will change and merge the comment clean up
into another patch.

> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 7/8] iommu/amd: Don't update domain info to dte entry at iommu init stage

[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 <bhe@redhat.com>
---
 drivers/iommu/amd_iommu.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index fcb69ff..6c37300 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -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)
 
 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;
 	/* 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;
-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 7/8] iommu/amd: Don't update domain info to dte entry at iommu init stage

[Baoquan He]

On 09/20/16 at 02:50pm, Joerg Roedel wrote:
> On Thu, Sep 15, 2016 at 11:03:25PM +0800, Baoquan He wrote:
> > 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 <bhe@redhat.com>
> > ---
> >  drivers/iommu/amd_iommu.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > index fcb69ff..6c37300 100644
> > --- a/drivers/iommu/amd_iommu.c
> > +++ b/drivers/iommu/amd_iommu.c
> > @@ -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)
> >  
> >  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.

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 7/8] iommu/amd: Don't update domain info to dte entry at iommu init stage

[Baoquan He]

On 09/21/16 at 06:26pm, Baoquan He wrote:
> On 09/20/16 at 02:50pm, Joerg Roedel wrote:
> > On Thu, Sep 15, 2016 at 11:03:25PM +0800, Baoquan He wrote:
> > > 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 <bhe@redhat.com>
> > > ---
> > >  drivers/iommu/amd_iommu.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > > index fcb69ff..6c37300 100644
> > > --- a/drivers/iommu/amd_iommu.c
> > > +++ b/drivers/iommu/amd_iommu.c
> > > @@ -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)
> > >  
> > >  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
Here I means amd_iommu_domain_direct_map and amd_iommu_domain_enable_v2
will call set_dte_entry to change pte_root setting.

> 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.
> 
> > 
> > >  	/* 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;
> > > -- 
> > > 2.5.5
> > > 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v5 8/8] iommu/amd: Update domain into to dte entry during device driver init

[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 <bhe@redhat.com>
---
 drivers/iommu/amd_iommu.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 6c37300..00b64ee 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -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;
 
 	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);
+       }
 
 	start = address;
 	for (i = 0; i < pages; ++i) {
@@ -2470,6 +2481,9 @@ static int map_sg(struct device *dev, struct scatterlist *sglist,
 	struct scatterlist *s;
 	unsigned long address;
 	u64 dma_mask;
+	struct iommu_dev_data *dev_data = get_dev_data(dev);
+	u16 alias = amd_iommu_alias_table[dev_data->devid];
+	struct amd_iommu *iommu = amd_iommu_rlookup_table[dev_data->devid];
 
 	domain = get_domain(dev);
 	if (IS_ERR(domain))
@@ -2485,6 +2499,13 @@ static int map_sg(struct device *dev, struct scatterlist *sglist,
 		goto out_err;
 
 	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);
+       }
 
 	/* Map all sg entries */
 	for_each_sg(sglist, s, nelems, i) {
-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 8/8] iommu/amd: Update domain into to dte entry during device driver init

[Baoquan He]

On 09/20/16 at 02:53pm, Joerg Roedel wrote:
> On Thu, Sep 15, 2016 at 11:03:26PM +0800, Baoquan He wrote:
> > 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 <bhe@redhat.com>
> > ---
> >  drivers/iommu/amd_iommu.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > index 6c37300..00b64ee 100644
> > --- a/drivers/iommu/amd_iommu.c
> > +++ b/drivers/iommu/amd_iommu.c
> > @@ -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;
> >  
> >  	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.

Very earlier you mentioned this, and I tried, but failed. I guess that
is because of the bnx2 NIC resetting problem. Let me try it again. In
theory it should be better. Just sometime people probably don't call
set_dma_mask explicitly, then default dma mask is used. This could be
seen in those simple device. For those complicated device like sata disk
and ethernet NIC, set_dma_mask should be called. So I would like to try
and use set_dma_mask. Thanks for your great suggestion.

Thanks
Baoquan


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v5 8/8] iommu/amd: Update domain into to dte entry during device driver init

[Baoquan He]

On 09/20/16 at 02:53pm, Joerg Roedel wrote:
> 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.

Hi Joerg,

I tried hooking this into set_dma_mask call-back, but on my local
machine with amd iommu v2, an ohci pci device doesn't call set_dma_mask.
Then IO_PAGE_FAULT printing flooded.

00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB OHCI Controller (rev 11)

Below is the patch I made using set_dma_mask call-back. So should we do
as the original method, or use set_dma_mask method and add set_dma_mask
calling in those driver which missed calling it?


From 1ca66f886a46839cb937fd1e6a1d84b6719f23c4 Mon Sep 17 00:00:00 2001
From: Baoquan He <bhe@redhat.com>
Date: Mon, 12 Sep 2016 08:05:15 +0800
Subject: [PATCH] iommu/amd: Update domain into to dte entry during device
 driver init

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. Usually
device driver initialization will call set_dma_mask to set the
limitation of dma address. Do it in set_dma_mask call-back is a good
chance.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 drivers/iommu/amd_iommu.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 6c37300..2a0b1ce 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2653,6 +2653,27 @@ static int amd_iommu_dma_supported(struct device *dev, u64 mask)
 	return check_device(dev);
 }
 
+static int set_dma_mask(struct device *dev, u64 mask)
+{
+	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];
+
+	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);
+       }
+
+	if (!dev->dma_mask || !amd_iommu_dma_supported(dev, mask))
+		return -EIO;
+	*dev->dma_mask = mask;
+	return 0;
+}
+
 static struct dma_map_ops amd_iommu_dma_ops = {
 	.alloc		= alloc_coherent,
 	.free		= free_coherent,
@@ -2661,6 +2682,7 @@ static struct dma_map_ops amd_iommu_dma_ops = {
 	.map_sg		= map_sg,
 	.unmap_sg	= unmap_sg,
 	.dma_supported	= amd_iommu_dma_supported,
+	.set_dma_mask	= set_dma_mask,
 };
 
 static int init_reserved_iova_ranges(void)
-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec