[PATCH 2/2] Intel IOMMU Suspend/Resume Support for Queued Invalidation

[PATCH 2/2] Intel IOMMU Suspend/Resume Support for Queued Invalidation

[Fenghua Yu]

This patch supports suspend/resume for queued invalidation. During suspend/
resume, queued invalidation is disabled and then reenabled. This patch also
consolidate queued invalidation hardware operation into one function.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

---

 dmar.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
index d313039..b318bd1 100644
--- a/drivers/pci/dmar.c
+++ b/drivers/pci/dmar.c
@@ -790,14 +790,39 @@ end:
 }
 
 /*
+ * Enable queued invalidation.
+ */
+static void __dmar_enable_qi(struct intel_iommu *iommu)
+{
+	u32 cmd, sts;
+	unsigned long flags;
+	struct q_inval *qi = iommu->qi;
+
+	qi->free_head = qi->free_tail = 0;
+	qi->free_cnt = QI_LENGTH;
+
+	spin_lock_irqsave(&iommu->register_lock, flags);
+	/* write zero to the tail reg */
+	writel(0, iommu->reg + DMAR_IQT_REG);
+
+	dmar_writeq(iommu->reg + DMAR_IQA_REG, virt_to_phys(qi->desc));
+
+	cmd = iommu->gcmd | DMA_GCMD_QIE;
+	iommu->gcmd |= DMA_GCMD_QIE;
+	writel(cmd, iommu->reg + DMAR_GCMD_REG);
+
+	/* Make sure hardware complete it */
+	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, readl, (sts & DMA_GSTS_QIES), sts);
+	spin_unlock_irqrestore(&iommu->register_lock, flags);
+}
+
+/*
  * Enable Queued Invalidation interface. This is a must to support
  * interrupt-remapping. Also used by DMA-remapping, which replaces
  * register based IOTLB invalidation.
  */
 int dmar_enable_qi(struct intel_iommu *iommu)
 {
-	u32 cmd, sts;
-	unsigned long flags;
 	struct q_inval *qi;
 
 	if (!ecap_qis(iommu->ecap))
@@ -835,19 +860,7 @@ int dmar_enable_qi(struct intel_iommu *iommu)
 
 	spin_lock_init(&qi->q_lock);
 
-	spin_lock_irqsave(&iommu->register_lock, flags);
-	/* write zero to the tail reg */
-	writel(0, iommu->reg + DMAR_IQT_REG);
-
-	dmar_writeq(iommu->reg + DMAR_IQA_REG, virt_to_phys(qi->desc));
-
-	cmd = iommu->gcmd | DMA_GCMD_QIE;
-	iommu->gcmd |= DMA_GCMD_QIE;
-	writel(cmd, iommu->reg + DMAR_GCMD_REG);
-
-	/* Make sure hardware complete it */
-	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, readl, (sts & DMA_GSTS_QIES), sts);
-	spin_unlock_irqrestore(&iommu->register_lock, flags);
+	__dmar_enable_qi(iommu);
 
 	return 0;
 }
@@ -1102,3 +1115,27 @@ int __init enable_drhd_fault_handling(void)
 
 	return 0;
 }
+
+/*
+ * Re-enable Queued Invalidation interface.
+ */
+int dmar_reenable_qi(struct intel_iommu *iommu)
+{
+	if (!ecap_qis(iommu->ecap))
+		return -ENOENT;
+
+	if (!iommu->qi)
+		return -ENOENT;
+
+	/*
+	 * First disable queued invalidation.
+	 */
+	dmar_disable_qi(iommu);
+	/* Then enable queued invalidation again. Since there is no pending
+	 * invalidation requests now, it's safe to re-enable queued
+	 * invalidation.
+	 */
+	__dmar_enable_qi(iommu);
+
+	return 0;
+}

[patch 0/2] Intel IOMMU Suspend/Resume Support

[fenghua.yu]

Current Intel IOMMU does not support suspend and resume. In S3 event, kernel
crashes when IOMMU is enabled. The attached patch set implements the suspend and
resume feature for Intel IOMMU. It hooks to kernel suspend and resume interface.
When suspend happens, it saves necessary hardware registers. When resume happens
it restores the registers and restarts IOMMU.

This patch set is applied to the tip tree.

-- 

[patch 1/2] Intel IOMMU Suspend/Resume Support for DMAR

[fenghua.yu]

[-- Attachment #1: iommu_sr_1.patch --]
[-- Type: text/plain, Size: 5377 bytes --]

The attached patch implements the suspend and resume feature for DMAR. It hooks
to kernel suspend and resume interface.  When suspend happens, it saves
necessary hardware registers. When resume happens it restores the registers and
restarts DMAR.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

---

 drivers/pci/intel-iommu.c   |  158 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/intel-iommu.h |    4 +
 2 files changed, 162 insertions(+)

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 49402c3..a969bc8 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -36,6 +36,8 @@
 #include <linux/iova.h>
 #include <linux/iommu.h>
 #include <linux/intel-iommu.h>
+#include <linux/sysdev.h>
+#include <asm/i8259.h>
 #include <asm/cacheflush.h>
 #include <asm/iommu.h>
 #include "pci.h"
@@ -2563,6 +2565,161 @@ static void __init init_no_remapping_devices(void)
 	}
 }
 
+#ifdef CONFIG_PM
+static int init_iommu_hw(void)
+{
+	struct dmar_drhd_unit *drhd;
+	struct intel_iommu *iommu;
+
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+
+		iommu = drhd->iommu;
+
+		if (iommu->qi)
+			dmar_reenable_qi(iommu);
+	}
+
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+
+		iommu = drhd->iommu;
+
+		iommu_flush_write_buffer(iommu);
+
+		iommu_set_root_entry(iommu);
+
+		iommu->flush.flush_context(iommu, 0, 0, 0, DMA_CCMD_GLOBAL_INVL,
+					   0);
+		iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH,
+					 0);
+		iommu_disable_protect_mem_regions(iommu);
+		iommu_enable_translation(iommu);
+	}
+
+	return 0;
+}
+
+static void iommu_flush_all(void)
+{
+	struct dmar_drhd_unit *drhd;
+	struct intel_iommu *iommu;
+
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+
+		iommu = drhd->iommu;
+		iommu->flush.flush_context(iommu, 0, 0, 0, DMA_CCMD_GLOBAL_INVL,
+					   0);
+		iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH,
+					 0);
+	}
+}
+
+static u32 iommu_state[MAX_IOMMUS][MAX_IOMMU_REGS];
+
+static int iommu_suspend(struct sys_device *dev, pm_message_t state)
+{
+	struct dmar_drhd_unit *drhd;
+	struct intel_iommu *iommu;
+	unsigned long flag;
+	int    i = 0;
+
+	iommu_flush_all();
+
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+
+		iommu = drhd->iommu;
+
+		iommu_disable_translation(iommu);
+
+		spin_lock_irqsave(&iommu->register_lock, flag);
+
+		iommu_state[i][DMAR_FECTL_REG] =
+			(u32) readl(iommu->reg + DMAR_FECTL_REG);
+		iommu_state[i][DMAR_FEDATA_REG] =
+			(u32) readl(iommu->reg + DMAR_FEDATA_REG);
+		iommu_state[i][DMAR_FEADDR_REG] =
+			(u32) readl(iommu->reg + DMAR_FEADDR_REG);
+		iommu_state[i][DMAR_FEUADDR_REG] =
+			(u32) readl(iommu->reg + DMAR_FEUADDR_REG);
+
+		spin_unlock_irqrestore(&iommu->register_lock, flag);
+		i++;
+	}
+	return 0;
+}
+
+static int iommu_resume(struct sys_device *dev)
+{
+	struct dmar_drhd_unit *drhd;
+	struct intel_iommu *iommu;
+	unsigned long flag;
+	int i = 0;
+
+	if (init_iommu_hw())
+		panic("IOMMU setup failed, DMAR can not start!\n");
+
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+
+		iommu = drhd->iommu;
+
+		spin_lock_irqsave(&iommu->register_lock, flag);
+
+		writel((u32) iommu_state[i][DMAR_FECTL_REG],
+			iommu->reg + DMAR_FECTL_REG);
+		writel((u32) iommu_state[i][DMAR_FEDATA_REG],
+			iommu->reg + DMAR_FEDATA_REG);
+		writel((u32) iommu_state[i][DMAR_FEADDR_REG],
+			iommu->reg + DMAR_FEADDR_REG);
+		writel((u32) iommu_state[i][DMAR_FEUADDR_REG],
+			iommu->reg + DMAR_FEUADDR_REG);
+
+		spin_unlock_irqrestore(&iommu->register_lock, flag);
+		i++;
+	}
+	return 0;
+}
+
+static struct sysdev_class iommu_sysclass = {
+	.name		= "iommu",
+	.resume		= iommu_resume,
+	.suspend	= iommu_suspend,
+};
+
+static struct sys_device device_iommu = {
+	.cls	= &iommu_sysclass,
+};
+
+static int __init init_iommu_sysfs(void)
+{
+	int error;
+
+	error = sysdev_class_register(&iommu_sysclass);
+	if (error)
+		return error;
+
+	error = sysdev_register(&device_iommu);
+	if (error)
+		sysdev_class_unregister(&iommu_sysclass);
+
+	return error;
+}
+
+#else
+static init __init init_iommu_sysfs(void)
+{
+	return 0;
+}
+#endif	/* CONFIG_PM */
+
 int __init intel_iommu_init(void)
 {
 	int ret = 0;
@@ -2598,6 +2755,7 @@ int __init intel_iommu_init(void)
 	init_timer(&unmap_timer);
 	force_iommu = 1;
 	dma_ops = &intel_dma_ops;
+	init_iommu_sysfs();
 
 	register_iommu(&intel_iommu_ops);
 
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 1d6c71d..5ec836b 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -205,6 +205,9 @@ static inline void dmar_writeq(void __iomem *addr, u64 val)
 /* low 64 bit */
 #define dma_frcd_page_addr(d) (d & (((u64)-1) << PAGE_SHIFT))
 
+#define MAX_IOMMUS      32
+#define MAX_IOMMU_REGS  0xc0
+
 #define IOMMU_WAIT_OP(iommu, offset, op, cond, sts)			\
 do {									\
 	cycles_t start_time = get_cycles();				\
@@ -322,6 +325,7 @@ extern int alloc_iommu(struct dmar_drhd_unit *drhd);
 extern void free_iommu(struct intel_iommu *iommu);
 extern int dmar_enable_qi(struct intel_iommu *iommu);
 extern void dmar_disable_qi(struct intel_iommu *iommu);
+extern int dmar_reenable_qi(struct intel_iommu *iommu);
 extern void qi_global_iec(struct intel_iommu *iommu);
 
 extern int qi_flush_context(struct intel_iommu *iommu, u16 did, u16 sid,

-- 

[patch 2/2] Intel IOMMU Suspend/Resume Support for Queued Invalidation.

[fenghua.yu]

[-- Attachment #1: iommu_sr_2.patch --]
[-- Type: text/plain, Size: 2652 bytes --]

This patch supports queued invalidation suspend/resume.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

---

 dmar.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
index d313039..b318bd1 100644
--- a/drivers/pci/dmar.c
+++ b/drivers/pci/dmar.c
@@ -790,14 +790,39 @@ end:
 }
 
 /*
+ * Enable queued invalidation.
+ */
+static void __dmar_enable_qi(struct intel_iommu *iommu)
+{
+	u32 cmd, sts;
+	unsigned long flags;
+	struct q_inval *qi = iommu->qi;
+
+	qi->free_head = qi->free_tail = 0;
+	qi->free_cnt = QI_LENGTH;
+
+	spin_lock_irqsave(&iommu->register_lock, flags);
+	/* write zero to the tail reg */
+	writel(0, iommu->reg + DMAR_IQT_REG);
+
+	dmar_writeq(iommu->reg + DMAR_IQA_REG, virt_to_phys(qi->desc));
+
+	cmd = iommu->gcmd | DMA_GCMD_QIE;
+	iommu->gcmd |= DMA_GCMD_QIE;
+	writel(cmd, iommu->reg + DMAR_GCMD_REG);
+
+	/* Make sure hardware complete it */
+	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, readl, (sts & DMA_GSTS_QIES), sts);
+	spin_unlock_irqrestore(&iommu->register_lock, flags);
+}
+
+/*
  * Enable Queued Invalidation interface. This is a must to support
  * interrupt-remapping. Also used by DMA-remapping, which replaces
  * register based IOTLB invalidation.
  */
 int dmar_enable_qi(struct intel_iommu *iommu)
 {
-	u32 cmd, sts;
-	unsigned long flags;
 	struct q_inval *qi;
 
 	if (!ecap_qis(iommu->ecap))
@@ -835,19 +860,7 @@ int dmar_enable_qi(struct intel_iommu *iommu)
 
 	spin_lock_init(&qi->q_lock);
 
-	spin_lock_irqsave(&iommu->register_lock, flags);
-	/* write zero to the tail reg */
-	writel(0, iommu->reg + DMAR_IQT_REG);
-
-	dmar_writeq(iommu->reg + DMAR_IQA_REG, virt_to_phys(qi->desc));
-
-	cmd = iommu->gcmd | DMA_GCMD_QIE;
-	iommu->gcmd |= DMA_GCMD_QIE;
-	writel(cmd, iommu->reg + DMAR_GCMD_REG);
-
-	/* Make sure hardware complete it */
-	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, readl, (sts & DMA_GSTS_QIES), sts);
-	spin_unlock_irqrestore(&iommu->register_lock, flags);
+	__dmar_enable_qi(iommu);
 
 	return 0;
 }
@@ -1102,3 +1115,27 @@ int __init enable_drhd_fault_handling(void)
 
 	return 0;
 }
+
+/*
+ * Re-enable Queued Invalidation interface.
+ */
+int dmar_reenable_qi(struct intel_iommu *iommu)
+{
+	if (!ecap_qis(iommu->ecap))
+		return -ENOENT;
+
+	if (!iommu->qi)
+		return -ENOENT;
+
+	/*
+	 * First disable queued invalidation.
+	 */
+	dmar_disable_qi(iommu);
+	/* Then enable queued invalidation again. Since there is no pending
+	 * invalidation requests now, it's safe to re-enable queued
+	 * invalidation.
+	 */
+	__dmar_enable_qi(iommu);
+
+	return 0;
+}

-- 

Re: [patch 1/2] Intel IOMMU Suspend/Resume Support for DMAR

[Ingo Molnar]

* fenghua.yu@intel.com <fenghua.yu@intel.com> wrote:

> The attached patch implements the suspend and resume feature for 
> DMAR. It hooks to kernel suspend and resume interface.  When 
> suspend happens, it saves necessary hardware registers. When 
> resume happens it restores the registers and restarts DMAR.

The structure looks pretty clean. I suspect David will go over the 
lowlevel details - i only have a few nitpicks:

>  #include <linux/intel-iommu.h>
> +#include <linux/sysdev.h>
> +#include <asm/i8259.h>

why is that needed?

>  #include <asm/cacheflush.h>
>  #include <asm/iommu.h>
>  #include "pci.h"
> @@ -2563,6 +2565,161 @@ static void __init init_no_remapping_devices(void)
>  	}
>  }
>  
> +#ifdef CONFIG_PM
> +static int init_iommu_hw(void)
> +{
> +	struct dmar_drhd_unit *drhd;
> +	struct intel_iommu *iommu;
> +
> +	for_each_drhd_unit(drhd) {
> +		if (drhd->ignored)
> +			continue;
> +
> +		iommu = drhd->iommu;
> +
> +		if (iommu->qi)
> +			dmar_reenable_qi(iommu);
> +	}
> +
> +	for_each_drhd_unit(drhd) {
> +		if (drhd->ignored)
> +			continue;

i suggested the following cleanliness detail on the previous 
submission a month or two ago, and it's still valid: why isnt the 
drhd->ignored skipping integrated into for_each_drhd_unit() ?

Perhaps named: for_each_active_drhd_unit().

Also, in practice, we iterate over active iommus, so this whole 
sequence:

> +	for_each_drhd_unit(drhd) {
> +		if (drhd->ignored)
> +			continue;
> +
> +		iommu = drhd->iommu;

Could perhaps be replaced with for_each_active_iommu(iommu).

> +
> +		iommu_flush_write_buffer(iommu);
> +
> +		iommu_set_root_entry(iommu);
> +
> +		iommu->flush.flush_context(iommu, 0, 0, 0, DMA_CCMD_GLOBAL_INVL,
> +					   0);
> +		iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH,
> +					 0);

just put those '0);' lines to the end of the call and ignore 
checkpatch.

> +		iommu_disable_protect_mem_regions(iommu);
> +		iommu_enable_translation(iommu);
> +	}
> +
> +	return 0;
> +}
> +
> +static void iommu_flush_all(void)
> +{
> +	struct dmar_drhd_unit *drhd;
> +	struct intel_iommu *iommu;
> +
> +	for_each_drhd_unit(drhd) {
> +		if (drhd->ignored)
> +			continue;
> +
> +		iommu = drhd->iommu;
> +		iommu->flush.flush_context(iommu, 0, 0, 0, DMA_CCMD_GLOBAL_INVL,
> +					   0);
> +		iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH,
> +					 0);

ditto.

> +	}
> +}
> +
> +static u32 iommu_state[MAX_IOMMUS][MAX_IOMMU_REGS];

hm, this goes up to MAX_IOMMUS which you define to 32.

But dmar_drhd_unit registration in dmar_register_drhd_unit() is 
open-ended and has no such limit.

While i'm sure there's a limit in the spec, there's the chance for 
there to be more than 32 units enumerated (in future hardware) - and 
this will break silently.

It would be far cleaner to embedd the IOMMU state in the IOMMU 
driver data structure directly. That gets rid of the limit and it 
also makes this array allocated dynamically.

> +
> +static int iommu_suspend(struct sys_device *dev, pm_message_t state)
> +{
> +	struct dmar_drhd_unit *drhd;
> +	struct intel_iommu *iommu;
> +	unsigned long flag;
> +	int    i = 0;

(the tab before the 'i' looks a bit weird.)

> +
> +	iommu_flush_all();
> +
> +	for_each_drhd_unit(drhd) {
> +		if (drhd->ignored)
> +			continue;
> +
> +		iommu = drhd->iommu;
> +
> +		iommu_disable_translation(iommu);
> +
> +		spin_lock_irqsave(&iommu->register_lock, flag);
> +
> +		iommu_state[i][DMAR_FECTL_REG] =
> +			(u32) readl(iommu->reg + DMAR_FECTL_REG);
> +		iommu_state[i][DMAR_FEDATA_REG] =
> +			(u32) readl(iommu->reg + DMAR_FEDATA_REG);
> +		iommu_state[i][DMAR_FEADDR_REG] =
> +			(u32) readl(iommu->reg + DMAR_FEADDR_REG);
> +		iommu_state[i][DMAR_FEUADDR_REG] =
> +			(u32) readl(iommu->reg + DMAR_FEUADDR_REG);

no need for the cast - readl returns u32.

> +
> +		spin_unlock_irqrestore(&iommu->register_lock, flag);
> +		i++;
> +	}
> +	return 0;
> +}
> +
> +static int iommu_resume(struct sys_device *dev)
> +{
> +	struct dmar_drhd_unit *drhd;
> +	struct intel_iommu *iommu;
> +	unsigned long flag;
> +	int i = 0;
> +
> +	if (init_iommu_hw())
> +		panic("IOMMU setup failed, DMAR can not start!\n");

Please dont panic boxes ... insert a WARN() and return.

> +
> +	for_each_drhd_unit(drhd) {
> +		if (drhd->ignored)
> +			continue;
> +
> +		iommu = drhd->iommu;
> +
> +		spin_lock_irqsave(&iommu->register_lock, flag);
> +
> +		writel((u32) iommu_state[i][DMAR_FECTL_REG],
> +			iommu->reg + DMAR_FECTL_REG);
> +		writel((u32) iommu_state[i][DMAR_FEDATA_REG],
> +			iommu->reg + DMAR_FEDATA_REG);
> +		writel((u32) iommu_state[i][DMAR_FEADDR_REG],
> +			iommu->reg + DMAR_FEADDR_REG);
> +		writel((u32) iommu_state[i][DMAR_FEUADDR_REG],
> +			iommu->reg + DMAR_FEUADDR_REG);

there's no need for the (u32) case. iommu_state[] has u32 types, and 
writel takes u32.

> +
> +		spin_unlock_irqrestore(&iommu->register_lock, flag);
> +		i++;
> +	}
> +	return 0;
> +}
> +
> +static struct sysdev_class iommu_sysclass = {
> +	.name		= "iommu",
> +	.resume		= iommu_resume,
> +	.suspend	= iommu_suspend,
> +};
> +
> +static struct sys_device device_iommu = {
> +	.cls	= &iommu_sysclass,
> +};
> +
> +static int __init init_iommu_sysfs(void)
> +{
> +	int error;
> +
> +	error = sysdev_class_register(&iommu_sysclass);
> +	if (error)
> +		return error;
> +
> +	error = sysdev_register(&device_iommu);
> +	if (error)
> +		sysdev_class_unregister(&iommu_sysclass);
> +
> +	return error;
> +}
> +
> +#else
> +static init __init init_iommu_sysfs(void)
> +{
> +	return 0;
> +}
> +#endif	/* CONFIG_PM */
> +
>  int __init intel_iommu_init(void)
>  {
>  	int ret = 0;
> @@ -2598,6 +2755,7 @@ int __init intel_iommu_init(void)
>  	init_timer(&unmap_timer);
>  	force_iommu = 1;
>  	dma_ops = &intel_dma_ops;
> +	init_iommu_sysfs();
>  
>  	register_iommu(&intel_iommu_ops);
>  
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 1d6c71d..5ec836b 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -205,6 +205,9 @@ static inline void dmar_writeq(void __iomem *addr, u64 val)
>  /* low 64 bit */
>  #define dma_frcd_page_addr(d) (d & (((u64)-1) << PAGE_SHIFT))
>  
> +#define MAX_IOMMUS      32
> +#define MAX_IOMMU_REGS  0xc0

MAX_IOMMU_REGS ... would be nice to add some reference here where 
that limit comes from. Specification / documentation coordinates.

That will give those people who ever extend the hw side (and it will 
happen) a chance to notice this limit in the Linux kernel.

Also, is there a chance that MAX_IOMMU_REGS can be probed from the 
hardware directly? If that's possible and reliable then it would be 
nicer to make this limit dynamic. The registers are saved/restored 
straight and in a linear fashion - no reason to not make that 
extensible if we have a chance for it.

Thanks,

	Ingo

Re: [patch 2/2] Intel IOMMU Suspend/Resume Support for Queued Invalidation.

[Ingo Molnar]

(only small style nits.)

* fenghua.yu@intel.com <fenghua.yu@intel.com> wrote:

>  /*
> + * Enable queued invalidation.
> + */
> +static void __dmar_enable_qi(struct intel_iommu *iommu)

nice helper function.

> +{
> +	u32 cmd, sts;
> +	unsigned long flags;
> +	struct q_inval *qi = iommu->qi;
> +
> +	qi->free_head = qi->free_tail = 0;
> +	qi->free_cnt = QI_LENGTH;
> +
> +	spin_lock_irqsave(&iommu->register_lock, flags);
> +	/* write zero to the tail reg */
> +	writel(0, iommu->reg + DMAR_IQT_REG);

( small nit: critical sections are important to see at a glance, and 
  they are easier to recognize if there's a newline before and after 
  the spin_lock_irqsave() call. That will also make the comment 
  after that stand out more. )

> +
> +	dmar_writeq(iommu->reg + DMAR_IQA_REG, virt_to_phys(qi->desc));
> +
> +	cmd = iommu->gcmd | DMA_GCMD_QIE;
> +	iommu->gcmd |= DMA_GCMD_QIE;
> +	writel(cmd, iommu->reg + DMAR_GCMD_REG);
> +
> +	/* Make sure hardware complete it */
> +	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, readl, (sts & DMA_GSTS_QIES), sts);
> +	spin_unlock_irqrestore(&iommu->register_lock, flags);

(ditto.)

> +}
> +
> +/*
>   * Enable Queued Invalidation interface. This is a must to support
>   * interrupt-remapping. Also used by DMA-remapping, which replaces
>   * register based IOTLB invalidation.
>   */
>  int dmar_enable_qi(struct intel_iommu *iommu)
>  {
> -	u32 cmd, sts;
> -	unsigned long flags;
>  	struct q_inval *qi;
>  
>  	if (!ecap_qis(iommu->ecap))
> @@ -835,19 +860,7 @@ int dmar_enable_qi(struct intel_iommu *iommu)
>  
>  	spin_lock_init(&qi->q_lock);
>  
> -	spin_lock_irqsave(&iommu->register_lock, flags);
> -	/* write zero to the tail reg */
> -	writel(0, iommu->reg + DMAR_IQT_REG);
> -
> -	dmar_writeq(iommu->reg + DMAR_IQA_REG, virt_to_phys(qi->desc));
> -
> -	cmd = iommu->gcmd | DMA_GCMD_QIE;
> -	iommu->gcmd |= DMA_GCMD_QIE;
> -	writel(cmd, iommu->reg + DMAR_GCMD_REG);
> -
> -	/* Make sure hardware complete it */
> -	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, readl, (sts & DMA_GSTS_QIES), sts);
> -	spin_unlock_irqrestore(&iommu->register_lock, flags);
> +	__dmar_enable_qi(iommu);
>  
>  	return 0;
>  }
> @@ -1102,3 +1115,27 @@ int __init enable_drhd_fault_handling(void)
>  
>  	return 0;
>  }
> +
> +/*
> + * Re-enable Queued Invalidation interface.
> + */
> +int dmar_reenable_qi(struct intel_iommu *iommu)
> +{
> +	if (!ecap_qis(iommu->ecap))
> +		return -ENOENT;
> +
> +	if (!iommu->qi)
> +		return -ENOENT;

> +
> +	/*
> +	 * First disable queued invalidation.
> +	 */
> +	dmar_disable_qi(iommu);
> +	/* Then enable queued invalidation again. Since there is no pending
> +	 * invalidation requests now, it's safe to re-enable queued
> +	 * invalidation.
> +	 */
> +	__dmar_enable_qi(iommu);

the comment style looks a bit inconsistent here - the second 
one should be full winged comments as well, i.e.:

> +	/*
> +	 * First disable queued invalidation.
> +	 */
> +	dmar_disable_qi(iommu);
> +
> +	/*
> +	 * Then enable queued invalidation again. Since there is no pending
> +	 * invalidation requests now, it's safe to re-enable queued
> +	 * invalidation.
> +	 */
> +	__dmar_enable_qi(iommu);

Thanks,

	Ingo

Re: [patch 0/2] Intel IOMMU Suspend/Resume Support

[Andrew Lutomirski]

It looks like patch 1 calls dmar_reenable_qi but patch 2 defines it.

In 2.6.29, there's no dmar_disable_qi that I can see.  Can you respin
these against something a little less scary than -tip during a merge
window?  (Especially since -stable will need this soon.)

Thanks,
Andy

On Wed, Mar 25, 2009 at 2:45 PM,  <fenghua.yu@intel.com> wrote:
> Current Intel IOMMU does not support suspend and resume. In S3 event, kernel
> crashes when IOMMU is enabled. The attached patch set implements the suspend and
> resume feature for Intel IOMMU. It hooks to kernel suspend and resume interface.
> When suspend happens, it saves necessary hardware registers. When resume happens
> it restores the registers and restarts IOMMU.
>
> This patch set is applied to the tip tree.
>
> --
>
>

RE: [patch 0/2] Intel IOMMU Suspend/Resume Support

[Yu, Fenghua]

>Subject: Re: [patch 0/2] Intel IOMMU Suspend/Resume Support
>
>It looks like patch 1 calls dmar_reenable_qi but patch 2 defines it.
>
>In 2.6.29, there's no dmar_disable_qi that I can see.  Can you respin
>these against something a little less scary than -tip during a merge
>window?  (Especially since -stable will need this soon.)
>

dmar_disable_qi() is defined in tip tree already. This patch set is based on the tip tree. I do have another version of the patch set which is based on 2.6.29.

Ingo,

Do you think which tree this patch set should based on?

Thanks.

-Fenghua

Re: [patch 0/2] Intel IOMMU Suspend/Resume Support

[Ingo Molnar]

* Yu, Fenghua <fenghua.yu@intel.com> wrote:

> >Subject: Re: [patch 0/2] Intel IOMMU Suspend/Resume Support
> >
> >It looks like patch 1 calls dmar_reenable_qi but patch 2 defines it.
> >
> >In 2.6.29, there's no dmar_disable_qi that I can see.  Can you respin
> >these against something a little less scary than -tip during a merge
> >window?  (Especially since -stable will need this soon.)
> >
> 
> dmar_disable_qi() is defined in tip tree already. This patch set 
> is based on the tip tree. I do have another version of the patch 
> set which is based on 2.6.29.
> 
> Ingo,
> 
> Do you think which tree this patch set should based on?

yes, dmar_disable_qi() was part of the x2apic fix patches (for 
various crashes and hangs) from Suresh, merged by hpa, and they are 
queued up for 2.6.30:

ce4e240: x86: add x2apic_wrmsr_fence() to x2apic flush tlb paths
fa4b57c: x86, dmar: use atomic allocations for QI and Intr-remapping init
68a8ca5: x86: fix broken irq migration logic while cleaning up multiple vectors
05c3dc2: x86, ioapic: Fix non atomic allocation with interrupts disabled
29b61be: x86, x2apic: cleanup ifdef CONFIG_INTR_REMAP in io_apic code
0280f7c: x86, x2apic: cleanup the IO-APIC level migration with interrupt-remapping
cf6567f: x86, x2apic: fix clear_local_APIC() in the presence of x2apic
7c6d9f9: x86, x2apic: use virtual wire A mode in disable_IO_APIC() with interrupt-remapping
2e93456: x86, intr-remapping: fix free_irte() to clear all the IRTE entries
1531a6a: x86, dmar: start with sane state while enabling dma and interrupt-remapping
eba67e5: x86, dmar: routines for disabling queued invalidation and intr remapping
9d783ba: x86, x2apic: enable fault handling for intr-remapping
0ac2491: x86, dmar: move page fault handling code to dmar.c
4c5502b: x86, x2apic: fix lock ordering during IRQ migration
0ca0f16: Merge branches 'x86/apic', 'x86/asm', 'x86/cleanups', 'x86/debug', 'x86/kconfig', 'x86/mm', 'x86/ptrace', 'x86/setup' and 'x86/urgent'; commit 'v2.6.29-rc8' into x86/core

So it would be nice to have this based on -tip, to reduce conflicts 
- but it definitely needs David's review and acks. We can do a 
separate branch for this if needed.

	Ingo

Re: [patch 1/2] Intel IOMMU Suspend/Resume Support for DMAR

[David Woodhouse]

On Wed, 2009-03-25 at 21:12 +0100, Ingo Molnar wrote:
> 
> > +static int iommu_resume(struct sys_device *dev)
> > +{
> > +     struct dmar_drhd_unit *drhd;
> > +     struct intel_iommu *iommu;
> > +     unsigned long flag;
> > +     int i = 0;
> > +
> > +     if (init_iommu_hw())
> > +             panic("IOMMU setup failed, DMAR can not start!\n");
> 
> Please dont panic boxes ... insert a WARN() and return.

Well, the box is going to die anyway. If it was using the IOMMU before
suspend, and you fail to re-initialise the IOMMU after resume, then
you're buggered.

But if you panic() immediately during the resume, the message is
unlikely to make it out even to a serial console. If you print a warning
and limp on, I suppose there's at least a _chance_ that it might get
seen.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [patch 1/2] Intel IOMMU Suspend/Resume Support for DMAR

[Ingo Molnar]

* David Woodhouse <dwmw2@infradead.org> wrote:

> On Wed, 2009-03-25 at 21:12 +0100, Ingo Molnar wrote:
> > 
> > > +static int iommu_resume(struct sys_device *dev)
> > > +{
> > > +     struct dmar_drhd_unit *drhd;
> > > +     struct intel_iommu *iommu;
> > > +     unsigned long flag;
> > > +     int i = 0;
> > > +
> > > +     if (init_iommu_hw())
> > > +             panic("IOMMU setup failed, DMAR can not start!\n");
> > 
> > Please dont panic boxes ... insert a WARN() and return.
> 
> Well, the box is going to die anyway. If it was using the IOMMU 
> before suspend, and you fail to re-initialise the IOMMU after 
> resume, then you're buggered.

except if one is testing suspend/resume (via pm-test) without 
actually suspending the hardware.

> But if you panic() immediately during the resume, the message is 
> unlikely to make it out even to a serial console. If you print a 
> warning and limp on, I suppose there's at least a _chance_ that it 
> might get seen.

yeah.

	Ingo