Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1484284 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/8] Fix kdump faults on system with amd iommu Baoquan He <bhe@redhat.com> - 2016-09-15 17:10 +0200
    [PATCH v5 4/8] iommu/amd: Add function copy_dev_tables Baoquan He <bhe@redhat.com> - 2016-09-15 17:10 +0200
      Re: [PATCH v5 4/8] iommu/amd: Add function copy_dev_tables Joerg Roedel <joro@8bytes.org> - 2016-09-20 14:00 +0200
        Re: [PATCH v5 4/8] iommu/amd: Add function copy_dev_tables Baoquan He <bhe@redhat.com> - 2016-09-21 12:20 +0200
    [PATCH v5 7/8] iommu/amd: Don't update domain info to dte entry at iommu init stage Baoquan He <bhe@redhat.com> - 2016-09-15 17:10 +0200
      Re: [PATCH v5 7/8] iommu/amd: Don't update domain info to dte entry  at iommu init stage Joerg Roedel <joro@8bytes.org> - 2016-09-20 15:00 +0200
        Re: [PATCH v5 7/8] iommu/amd: Don't update domain info to dte entry  at iommu init stage Baoquan He <bhe@redhat.com> - 2016-09-21 12:30 +0200
          Re: [PATCH v5 7/8] iommu/amd: Don't update domain info to dte entry  at iommu init stage Baoquan He <bhe@redhat.com> - 2016-09-21 15:30 +0200
    [PATCH v5 2/8] iommu/amd: add early_enable_iommu() wrapper function Baoquan He <bhe@redhat.com> - 2016-09-15 17:10 +0200
    [PATCH v5 6/8] iommu/amd: Do not re-enable dev table entries in kdump Baoquan He <bhe@redhat.com> - 2016-09-15 17:10 +0200
      Re: [PATCH v5 6/8] iommu/amd: Do not re-enable dev table entries in  kdump Joerg Roedel <joro@8bytes.org> - 2016-09-20 14:50 +0200
        Re: [PATCH v5 6/8] iommu/amd: Do not re-enable dev table entries in  kdump Baoquan He <bhe@redhat.com> - 2016-09-21 12:30 +0200
    [PATCH v5 5/8] iommu/amd: copy old trans table from old kernel Baoquan He <bhe@redhat.com> - 2016-09-15 17:10 +0200
      Re: [PATCH v5 5/8] iommu/amd: copy old trans table from old kernel Joerg Roedel <joro@8bytes.org> - 2016-09-20 14:50 +0200
        Re: [PATCH v5 5/8] iommu/amd: copy old trans table from old kernel Baoquan He <bhe@redhat.com> - 2016-09-21 12:20 +0200
    [PATCH v5 1/8] iommu/amd: Detect pre enabled translation Baoquan He <bhe@redhat.com> - 2016-09-15 17:10 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

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

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


	Joerg

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.

> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web