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

Groups > linux.kernel > #1693481 > unrolled thread

[PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled Baoquan He <bhe@redhat.com> - 2017-07-21 11:00 +0200
    Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev  table if translation pre-enabled Joerg Roedel <jroedel@suse.de> - 2017-07-27 18:00 +0200
      Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev  table if translation pre-enabled Baoquan He <bhe@redhat.com> - 2017-07-28 11:10 +0200
        Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev  table if translation pre-enabled Joerg Roedel <jroedel@suse.de> - 2017-07-28 13:20 +0200
          Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev  table if translation pre-enabled Baoquan He <bhe@redhat.com> - 2017-07-28 13:30 +0200
        Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev  table if translation pre-enabled Baoquan He <bhe@redhat.com> - 2017-07-28 13:20 +0200
        Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev  table if translation pre-enabled Joerg Roedel <jroedel@suse.de> - 2017-07-28 13:20 +0200
        Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev  table if translation pre-enabled Baoquan He <bhe@redhat.com> - 2017-07-31 12:20 +0200
          Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev  table if translation pre-enabled Baoquan He <bhe@redhat.com> - 2017-07-31 12:30 +0200
          Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev  table if translation pre-enabled Joerg Roedel <jroedel@suse.de> - 2017-07-31 12:30 +0200

#1693481 — [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

AMD pointed out it's unsafe to update the device-table while iommu
is enabled. It turns out that device-table pointer update is split
up into two 32bit writes in the IOMMU hardware. So updating it while
the IOMMU is enabled could have some nasty side effects.

The only way to work around this is to allocate the device-table below
4GB if translation is pre-enabled in kdump kernel. If allocation failed,
still use the old one.

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

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 052fa4a977d8..d7c301d0d672 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2149,11 +2149,23 @@ static void early_enable_iommu(struct amd_iommu *iommu)
  */
 static void early_enable_iommus(void)
 {
+	struct dev_table_entry *dev_tbl;
 	struct amd_iommu *iommu;
 	bool is_pre_enabled = false;
 
 	for_each_iommu(iommu) {
 		if (translation_pre_enabled(iommu)) {
+			gfp_t gfp_flag = GFP_KERNEL | __GFP_ZERO | GFP_DMA32;;
+
+			dev_tbl = (void *)__get_free_pages(gfp_flag,
+						get_order(dev_table_size));
+			if (dev_tbl != NULL) {
+				memcpy(dev_tbl, amd_iommu_dev_table, dev_table_size);
+				free_pages((unsigned long)amd_iommu_dev_table,
+						get_order(dev_table_size));
+				amd_iommu_dev_table = dev_tbl;
+			}
+
 			is_pre_enabled = true;
 			break;
 		}
-- 
2.5.5

[toc] | [next] | [standalone]

#1698119 — Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

On Fri, Jul 21, 2017 at 04:59:08PM +0800, Baoquan He wrote:
> AMD pointed out it's unsafe to update the device-table while iommu
> is enabled. It turns out that device-table pointer update is split
> up into two 32bit writes in the IOMMU hardware. So updating it while
> the IOMMU is enabled could have some nasty side effects.
> 
> The only way to work around this is to allocate the device-table below
> 4GB if translation is pre-enabled in kdump kernel. If allocation failed,
> still use the old one.

Not only for the kdump kernel. The old device table must also be below
4GB so that its pointer can be updated with a 32bit write.

If the old table is above 4GB you still need the second write to zero
the upper parts of the pointer in hardware.

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

#1698608 — Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

Hi Joerg,

On 07/27/17 at 05:55pm, Joerg Roedel wrote:
> On Fri, Jul 21, 2017 at 04:59:08PM +0800, Baoquan He wrote:
> > AMD pointed out it's unsafe to update the device-table while iommu
> > is enabled. It turns out that device-table pointer update is split
> > up into two 32bit writes in the IOMMU hardware. So updating it while
> > the IOMMU is enabled could have some nasty side effects.
> > 
> > The only way to work around this is to allocate the device-table below
> > 4GB if translation is pre-enabled in kdump kernel. If allocation failed,
> > still use the old one.
> 
> Not only for the kdump kernel. The old device table must also be below
> 4GB so that its pointer can be updated with a 32bit write.
> 
> If the old table is above 4GB you still need the second write to zero
> the upper parts of the pointer in hardware.

Do you mean the allocation of amd_iommu_dev_table in
early_amd_iommu_init() also need be addressed for 1st kernel? Seems we
don't make sure that for 1st kernel, like adding GFP_DMA32 flag when
allocate amd_iommu_dev_table in amd_iommu_dev_table
early_amd_iommu_init().

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

#1698700 — Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

On Fri, Jul 28, 2017 at 07:15:53PM +0800, Baoquan He wrote:
> On 07/28/17 at 01:14pm, Joerg Roedel wrote:
> > Yes, exactly, the first device table also needs to be allocated with
> > GFP_DMA32 so that it ends up below 4GB.
> 
> Got it, will do. Thanks!

Oh, and you also need to check in the kdump kernel whether the old
device-table is really below 4GB and abort the copy otherwise.

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

#1698707 — Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

On 07/28/17 at 01:18pm, Joerg Roedel wrote:
> On Fri, Jul 28, 2017 at 07:15:53PM +0800, Baoquan He wrote:
> > On 07/28/17 at 01:14pm, Joerg Roedel wrote:
> > > Yes, exactly, the first device table also needs to be allocated with
> > > GFP_DMA32 so that it ends up below 4GB.
> > 
> > Got it, will do. Thanks!
> 
> Oh, and you also need to check in the kdump kernel whether the old
> device-table is really below 4GB and abort the copy otherwise.

Ah, indeed. Will change it. Thanks.

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

#1698701 — Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

On 07/28/17 at 01:14pm, Joerg Roedel wrote:
> On Fri, Jul 28, 2017 at 05:06:19PM +0800, Baoquan He wrote:
> > Do you mean the allocation of amd_iommu_dev_table in
> > early_amd_iommu_init() also need be addressed for 1st kernel? Seems we
> > don't make sure that for 1st kernel, like adding GFP_DMA32 flag when
> > allocate amd_iommu_dev_table in amd_iommu_dev_table
> > early_amd_iommu_init().
> 
> Yes, exactly, the first device table also needs to be allocated with
> GFP_DMA32 so that it ends up below 4GB.

Got it, will do. Thanks!

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

#1698704 — Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

On Fri, Jul 28, 2017 at 05:06:19PM +0800, Baoquan He wrote:
> Do you mean the allocation of amd_iommu_dev_table in
> early_amd_iommu_init() also need be addressed for 1st kernel? Seems we
> don't make sure that for 1st kernel, like adding GFP_DMA32 flag when
> allocate amd_iommu_dev_table in amd_iommu_dev_table
> early_amd_iommu_init().

Yes, exactly, the first device table also needs to be allocated with
GFP_DMA32 so that it ends up below 4GB.

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

#1699898 — Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

Hi Joerg,

On 07/28/17 at 05:06pm, Baoquan He wrote:
> Hi Joerg,
> 
> On 07/27/17 at 05:55pm, Joerg Roedel wrote:
> > On Fri, Jul 21, 2017 at 04:59:08PM +0800, Baoquan He wrote:
> > > AMD pointed out it's unsafe to update the device-table while iommu
> > > is enabled. It turns out that device-table pointer update is split
> > > up into two 32bit writes in the IOMMU hardware. So updating it while
> > > the IOMMU is enabled could have some nasty side effects.
> > > 
> > > The only way to work around this is to allocate the device-table below
> > > 4GB if translation is pre-enabled in kdump kernel. If allocation failed,
> > > still use the old one.
> > 
> > Not only for the kdump kernel. The old device table must also be below
> > 4GB so that its pointer can be updated with a 32bit write.
> > 
> > If the old table is above 4GB you still need the second write to zero
> > the upper parts of the pointer in hardware.
> 
> Do you mean the allocation of amd_iommu_dev_table in
> early_amd_iommu_init() also need be addressed for 1st kernel? Seems we
> don't make sure that for 1st kernel, like adding GFP_DMA32 flag when
> allocate amd_iommu_dev_table in amd_iommu_dev_table
> early_amd_iommu_init().

I plan to add GFP_DMA32 when allocate amd_iommu_dev_table in
early_amd_iommu_init() as below. Then in kdump kernel we don't need to
worry if the old amd_iommu_dev_table could be above 4G, right? And might
not need to check if it's above 4G, right?

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 781a138..85d6445 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2436,7 +2436,8 @@ static int __init early_amd_iommu_init(void)
 
 	/* Device table - directly used by all IOMMUs */
 	ret = -ENOMEM;
-	amd_iommu_dev_table = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+	amd_iommu_dev_table = (void *)__get_free_pages(
+				      GFP_KERNEL | __GFP_ZERO | GFP_DMA32,
 				      get_order(dev_table_size));
 	if (amd_iommu_dev_table == NULL)
 		goto out;
-- 
2.9.4

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

#1699903 — Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

On 07/31/17 at 12:21pm, Joerg Roedel wrote:
> Hi Baoquan,
> 
> On Mon, Jul 31, 2017 at 06:15:30PM +0800, Baoquan He wrote:
> > I plan to add GFP_DMA32 when allocate amd_iommu_dev_table in
> > early_amd_iommu_init() as below. Then in kdump kernel we don't need to
> > worry if the old amd_iommu_dev_table could be above 4G, right? And might
> > not need to check if it's above 4G, right?
> > 
> > diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> > index 781a138..85d6445 100644
> > --- a/drivers/iommu/amd_iommu_init.c
> > +++ b/drivers/iommu/amd_iommu_init.c
> > @@ -2436,7 +2436,8 @@ static int __init early_amd_iommu_init(void)
> >  
> >  	/* Device table - directly used by all IOMMUs */
> >  	ret = -ENOMEM;
> > -	amd_iommu_dev_table = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> > +	amd_iommu_dev_table = (void *)__get_free_pages(
> > +				      GFP_KERNEL | __GFP_ZERO | GFP_DMA32,
> >  				      get_order(dev_table_size));
> >  	if (amd_iommu_dev_table == NULL)
> >  		goto out;
> 
> Yeah, adding GFP_DMA32 is right. But you still need to check it in the
> kdump path. Not checking it would mean you trust the old kernel, but
> since it paniced there is no reason to put any trust in what happened
> before.

You are right, it could be touched accidentally. It must be checked.
Thanks a lot for your answer!

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

#1699904 — Re: [PATCH v8 10/13] iommu/amd: Allocate memory below 4G for dev table if translation pre-enabled

Hi Baoquan,

On Mon, Jul 31, 2017 at 06:15:30PM +0800, Baoquan He wrote:
> I plan to add GFP_DMA32 when allocate amd_iommu_dev_table in
> early_amd_iommu_init() as below. Then in kdump kernel we don't need to
> worry if the old amd_iommu_dev_table could be above 4G, right? And might
> not need to check if it's above 4G, right?
> 
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 781a138..85d6445 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -2436,7 +2436,8 @@ static int __init early_amd_iommu_init(void)
>  
>  	/* Device table - directly used by all IOMMUs */
>  	ret = -ENOMEM;
> -	amd_iommu_dev_table = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> +	amd_iommu_dev_table = (void *)__get_free_pages(
> +				      GFP_KERNEL | __GFP_ZERO | GFP_DMA32,
>  				      get_order(dev_table_size));
>  	if (amd_iommu_dev_table == NULL)
>  		goto out;

Yeah, adding GFP_DMA32 is right. But you still need to check it in the
kdump path. Not checking it would mean you trust the old kernel, but
since it paniced there is no reason to put any trust in what happened
before.



	Joerg

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web