[PATCH V2] x86/tboot: add an option to disable iommu force on

[PATCH V2] x86/tboot: add an option to disable iommu force on

[Shaohua Li]

IOMMU harms performance signficantly when we run very fast networking
workloads. It's 40GB networking doing XDP test. Software overhead is
almost unaware, but it's the IOTLB miss (based on our analysis) which
kills the performance. We observed the same performance issue even with
software passthrough (identity mapping), only the hardware passthrough
survives. The pps with iommu (with software passthrough) is only about
~30% of that without it. This is a limitation in hardware based on our
observation, so we'd like to disable the IOMMU force on, but we do want
to use TBOOT and we can sacrifice the DMA security bought by IOMMU. I
must admit I know nothing about TBOOT, but TBOOT guys (cc-ed) think not
eabling IOMMU is totally ok.

So introduce a new boot option to disable the force on. It's kind of
silly we need to run into intel_iommu_init even without force on, but we
need to disable TBOOT PMR registers. For system without the boot option,
nothing is changed.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
 arch/x86/kernel/tboot.c                         |  3 +++
 drivers/iommu/intel-iommu.c                     | 18 ++++++++++++++++++
 include/linux/dma_remapping.h                   |  1 +
 4 files changed, 31 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 33a3b54..8a3fb0d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1579,6 +1579,15 @@
 			extended tables themselves, and also PASID support. With
 			this option set, extended tables will not be used even
 			on hardware which claims to support them.
+		tboot_noforce [Default Off]
+			Do not force the Intel IOMMU enabled under tboot.
+			By default, tboot will force Intel IOMMU on, which
+			could harm performance of some high-throughput
+			devices like 40GBit network cards, even if identity
+			mapping is enabled.
+			Note that using this option lowers the security
+			provided by tboot because it makes the system
+			vulnerable to DMA attacks.
 
 	intel_idle.max_cstate=	[KNL,HW,ACPI,X86]
 			0	disables intel_idle and fall back on acpi_idle.
diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index d4c8011..4b17240 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -514,6 +514,9 @@ int tboot_force_iommu(void)
 	if (!tboot_enabled())
 		return 0;
 
+	if (!intel_iommu_tboot_noforce)
+		return 1;
+
 	if (no_iommu || swiotlb || dmar_disabled)
 		pr_warning("Forcing Intel-IOMMU to enabled\n");
 
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 1662288..90ab011 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -183,6 +183,7 @@ static int rwbf_quirk;
  * (used when kernel is launched w/ TXT)
  */
 static int force_on = 0;
+int intel_iommu_tboot_noforce;
 
 /*
  * 0: Present
@@ -607,6 +608,10 @@ static int __init intel_iommu_setup(char *str)
 				"Intel-IOMMU: enable pre-production PASID support\n");
 			intel_iommu_pasid28 = 1;
 			iommu_identity_mapping |= IDENTMAP_GFX;
+		} else if (!strncmp(str, "tboot_noforce", 13)) {
+			printk(KERN_INFO
+				"Intel-IOMMU: not forcing on after tboot. This could expose security risk for tboot\n");
+			intel_iommu_tboot_noforce = 1;
 		}
 
 		str += strcspn(str, ",");
@@ -4851,6 +4856,19 @@ int __init intel_iommu_init(void)
 
 	if (no_iommu || dmar_disabled) {
 		/*
+		 * We exit the function here to ensure IOMMU's remapping and
+		 * mempool aren't setup, which means that the IOMMU's PMRs
+		 * won't be disabled via the call to init_dmars(). So disable
+		 * it explicitly here. The PMRs were setup by tboot prior to
+		 * calling SENTER, but the kernel is expected to reset/tear
+		 * down the PMRs.
+		 */
+		if (intel_iommu_tboot_noforce) {
+			for_each_iommu(iommu, drhd)
+				iommu_disable_protect_mem_regions(iommu);
+		}
+
+		/*
 		 * Make sure the IOMMUs are switched off, even when we
 		 * boot into a kexec kernel and the previous kernel left
 		 * them enabled
diff --git a/include/linux/dma_remapping.h b/include/linux/dma_remapping.h
index 187c102..9088407 100644
--- a/include/linux/dma_remapping.h
+++ b/include/linux/dma_remapping.h
@@ -39,6 +39,7 @@ extern int iommu_calculate_agaw(struct intel_iommu *iommu);
 extern int iommu_calculate_max_sagaw(struct intel_iommu *iommu);
 extern int dmar_disabled;
 extern int intel_iommu_enabled;
+extern int intel_iommu_tboot_noforce;
 #else
 static inline int iommu_calculate_agaw(struct intel_iommu *iommu)
 {
-- 
2.9.3

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Joerg Roedel]

On Wed, Apr 26, 2017 at 09:18:35AM -0700, Shaohua Li wrote:
> IOMMU harms performance signficantly when we run very fast networking
> workloads. It's 40GB networking doing XDP test. Software overhead is
> almost unaware, but it's the IOTLB miss (based on our analysis) which
> kills the performance. We observed the same performance issue even with
> software passthrough (identity mapping), only the hardware passthrough
> survives. The pps with iommu (with software passthrough) is only about
> ~30% of that without it. This is a limitation in hardware based on our
> observation, so we'd like to disable the IOMMU force on, but we do want
> to use TBOOT and we can sacrifice the DMA security bought by IOMMU. I
> must admit I know nothing about TBOOT, but TBOOT guys (cc-ed) think not
> eabling IOMMU is totally ok.
> 
> So introduce a new boot option to disable the force on. It's kind of
> silly we need to run into intel_iommu_init even without force on, but we
> need to disable TBOOT PMR registers. For system without the boot option,
> nothing is changed.
> 
> Signed-off-by: Shaohua Li <shli@fb.com>

Applied, thanks.

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Ingo Molnar]

* Joerg Roedel <jroedel@suse.de> wrote:

> On Wed, Apr 26, 2017 at 09:18:35AM -0700, Shaohua Li wrote:
> > IOMMU harms performance signficantly when we run very fast networking
> > workloads. It's 40GB networking doing XDP test. Software overhead is
> > almost unaware, but it's the IOTLB miss (based on our analysis) which
> > kills the performance. We observed the same performance issue even with
> > software passthrough (identity mapping), only the hardware passthrough
> > survives. The pps with iommu (with software passthrough) is only about
> > ~30% of that without it. This is a limitation in hardware based on our
> > observation, so we'd like to disable the IOMMU force on, but we do want
> > to use TBOOT and we can sacrifice the DMA security bought by IOMMU. I
> > must admit I know nothing about TBOOT, but TBOOT guys (cc-ed) think not
> > eabling IOMMU is totally ok.
> > 
> > So introduce a new boot option to disable the force on. It's kind of
> > silly we need to run into intel_iommu_init even without force on, but we
> > need to disable TBOOT PMR registers. For system without the boot option,
> > nothing is changed.
> > 
> > Signed-off-by: Shaohua Li <shli@fb.com>
> 
> Applied, thanks.

Please don't apply it yet, I posted a few review questions.

Thanks,

	Ingo

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Joerg Roedel]

Hi Ingo,

On Thu, Apr 27, 2017 at 08:52:53AM +0200, Ingo Molnar wrote:
> > Applied, thanks.
> 
> Please don't apply it yet, I posted a few review questions.

Are your questions answered with the replies by me and Shaohua?


	Joerg

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Ingo Molnar]

* Shaohua Li <shli@fb.com> wrote:

> IOMMU harms performance signficantly when we run very fast networking
> workloads. It's 40GB networking doing XDP test. Software overhead is
> almost unaware, but it's the IOTLB miss (based on our analysis) which
> kills the performance. We observed the same performance issue even with
> software passthrough (identity mapping), only the hardware passthrough
> survives. The pps with iommu (with software passthrough) is only about
> ~30% of that without it. This is a limitation in hardware based on our
> observation, so we'd like to disable the IOMMU force on, but we do want
> to use TBOOT and we can sacrifice the DMA security bought by IOMMU. I
> must admit I know nothing about TBOOT, but TBOOT guys (cc-ed) think not
> eabling IOMMU is totally ok.
> 
> So introduce a new boot option to disable the force on. It's kind of
> silly we need to run into intel_iommu_init even without force on, but we
> need to disable TBOOT PMR registers. For system without the boot option,
> nothing is changed.
> 
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
>  arch/x86/kernel/tboot.c                         |  3 +++
>  drivers/iommu/intel-iommu.c                     | 18 ++++++++++++++++++
>  include/linux/dma_remapping.h                   |  1 +
>  4 files changed, 31 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 33a3b54..8a3fb0d 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1579,6 +1579,15 @@
>  			extended tables themselves, and also PASID support. With
>  			this option set, extended tables will not be used even
>  			on hardware which claims to support them.
> +		tboot_noforce [Default Off]
> +			Do not force the Intel IOMMU enabled under tboot.
> +			By default, tboot will force Intel IOMMU on, which
> +			could harm performance of some high-throughput
> +			devices like 40GBit network cards, even if identity
> +			mapping is enabled.
> +			Note that using this option lowers the security
> +			provided by tboot because it makes the system
> +			vulnerable to DMA attacks.

So what's the purpose of this kernel option?

It sure isn't the proper solution for correctly architectured hardware/firmware 
(which can just choose not to expose the IOMMU!), and for one-time hacks for 
special embedded systems or for debugging why not just add an iommu=off option to 
force it off?

This just increases complexity for no good reason.

Thanks,

	Ingo

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Joerg Roedel]

On Thu, Apr 27, 2017 at 08:51:42AM +0200, Ingo Molnar wrote:
> > +		tboot_noforce [Default Off]
> > +			Do not force the Intel IOMMU enabled under tboot.
> > +			By default, tboot will force Intel IOMMU on, which
> > +			could harm performance of some high-throughput
> > +			devices like 40GBit network cards, even if identity
> > +			mapping is enabled.
> > +			Note that using this option lowers the security
> > +			provided by tboot because it makes the system
> > +			vulnerable to DMA attacks.
> 
> So what's the purpose of this kernel option?
> 
> It sure isn't the proper solution for correctly architectured hardware/firmware 
> (which can just choose not to expose the IOMMU!), and for one-time hacks for 
> special embedded systems or for debugging why not just add an iommu=off option to 
> force it off?

I guess that tboot requires an IOMMU to be present in order to work. It
will do initial IOMMU setup and hands the hardware over to Linux later
on.

The problem solved here is that someone wants tboot for security
reasons, but doesn't want the performance penalty of having the IOMMU
enabled and can live with the risk of an DMA attack.


Regards,

	Joerg

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Shaohua Li]

On Thu, Apr 27, 2017 at 10:42:07AM +0200, Joerg Roedel wrote:
> On Thu, Apr 27, 2017 at 08:51:42AM +0200, Ingo Molnar wrote:
> > > +		tboot_noforce [Default Off]
> > > +			Do not force the Intel IOMMU enabled under tboot.
> > > +			By default, tboot will force Intel IOMMU on, which
> > > +			could harm performance of some high-throughput
> > > +			devices like 40GBit network cards, even if identity
> > > +			mapping is enabled.
> > > +			Note that using this option lowers the security
> > > +			provided by tboot because it makes the system
> > > +			vulnerable to DMA attacks.
> > 
> > So what's the purpose of this kernel option?
> > 
> > It sure isn't the proper solution for correctly architectured hardware/firmware 
> > (which can just choose not to expose the IOMMU!), and for one-time hacks for 
> > special embedded systems or for debugging why not just add an iommu=off option to 
> > force it off?
> 
> I guess that tboot requires an IOMMU to be present in order to work. It
> will do initial IOMMU setup and hands the hardware over to Linux later
> on.
> 
> The problem solved here is that someone wants tboot for security
> reasons, but doesn't want the performance penalty of having the IOMMU
> enabled and can live with the risk of an DMA attack.

This is exactly the usage for us. And please note, not everybody should
sacrifice the DMA security. It is only required when the pcie device hits iommu
hardware limitation. In our enviroment, normal network workloads (as high as
60k pps) are completely ok with iommu enabled. Only the XDP workload, which can
do around 200k pps, is suffering from the problem. So completely forcing iommu
off for some workloads without the performance issue isn't good because of the
DMA security.

Thanks,
Shaohua

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Joerg Roedel]

On Thu, Apr 27, 2017 at 07:49:02AM -0700, Shaohua Li wrote:
> This is exactly the usage for us. And please note, not everybody should
> sacrifice the DMA security. It is only required when the pcie device hits iommu
> hardware limitation. In our enviroment, normal network workloads (as high as
> 60k pps) are completely ok with iommu enabled. Only the XDP workload, which can
> do around 200k pps, is suffering from the problem. So completely forcing iommu
> off for some workloads without the performance issue isn't good because of the
> DMA security.

How big are the packets in your XDP workload? I also run pps tests for
performance measurement on older desktop-class hardware
(Xeon E5-1620 v2 and AMD FX 6100) and 10GBit network
hardware, and easily get over the 200k pps mark with IOMMU enabled. The
Intel system can receive >900k pps and the AMD system is still at
~240k pps.

But my tests only send IPv4/UDP packets with 8bytes of payload, so that
is probably different to your setup.



	Joerg

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Shaohua Li]

On Thu, Apr 27, 2017 at 05:18:55PM +0200, Joerg Roedel wrote:
> On Thu, Apr 27, 2017 at 07:49:02AM -0700, Shaohua Li wrote:
> > This is exactly the usage for us. And please note, not everybody should
> > sacrifice the DMA security. It is only required when the pcie device hits iommu
> > hardware limitation. In our enviroment, normal network workloads (as high as
> > 60k pps) are completely ok with iommu enabled. Only the XDP workload, which can
> > do around 200k pps, is suffering from the problem. So completely forcing iommu
> > off for some workloads without the performance issue isn't good because of the
> > DMA security.
> 
> How big are the packets in your XDP workload? I also run pps tests for
> performance measurement on older desktop-class hardware
> (Xeon E5-1620 v2 and AMD FX 6100) and 10GBit network
> hardware, and easily get over the 200k pps mark with IOMMU enabled. The
> Intel system can receive >900k pps and the AMD system is still at
> ~240k pps.
> 
> But my tests only send IPv4/UDP packets with 8bytes of payload, so that
> is probably different to your setup.

Sorry, I wrote the wrong data. With iommu the pps is 6M pps, and without it, we
can get around 20M pps. XDP is much faster than normal network workloads. The
test uses 64 bytes. We tried other sizes in the machine (not 8 bytes though),
but pps doesn't change significantly. With different package size, the peek pps
is around 7M with iommu, then the NIC starts to drop package. CPU util is very
low as I said before. Without iommu, the peek pps is around 22M.

Thanks,
Shaohua

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Joerg Roedel]

On Thu, Apr 27, 2017 at 08:41:20AM -0700, Shaohua Li wrote:
> Sorry, I wrote the wrong data. With iommu the pps is 6M pps, and without it, we
> can get around 20M pps. XDP is much faster than normal network workloads. The
> test uses 64 bytes. We tried other sizes in the machine (not 8 bytes though),
> but pps doesn't change significantly. With different package size, the peek pps
> is around 7M with iommu, then the NIC starts to drop package. CPU util is very
> low as I said before. Without iommu, the peek pps is around 22M.

Ah, these numbers makes more sense.

Thanks,

	Joerg

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Ingo Molnar]

* Joerg Roedel <jroedel@suse.de> wrote:

> On Thu, Apr 27, 2017 at 08:51:42AM +0200, Ingo Molnar wrote:
> > > +		tboot_noforce [Default Off]
> > > +			Do not force the Intel IOMMU enabled under tboot.
> > > +			By default, tboot will force Intel IOMMU on, which
> > > +			could harm performance of some high-throughput
> > > +			devices like 40GBit network cards, even if identity
> > > +			mapping is enabled.
> > > +			Note that using this option lowers the security
> > > +			provided by tboot because it makes the system
> > > +			vulnerable to DMA attacks.
> > 
> > So what's the purpose of this kernel option?
> > 
> > It sure isn't the proper solution for correctly architectured hardware/firmware 
> > (which can just choose not to expose the IOMMU!), and for one-time hacks for 
> > special embedded systems or for debugging why not just add an iommu=off option to 
> > force it off?
> 
> I guess that tboot requires an IOMMU to be present in order to work. It
> will do initial IOMMU setup and hands the hardware over to Linux later
> on.
> 
> The problem solved here is that someone wants tboot for security
> reasons, but doesn't want the performance penalty of having the IOMMU
> enabled and can live with the risk of an DMA attack.

Yes, that makes sense - but in this case it would be far more user friendly to 
make it a sysctl, not a boot option. This is also much more manageable for 
distributions and also allows it to be more easily turned into a security policy 
feature.

New boot options should be for debugging hacks in essence - any serious hardware 
configuration should be done via more user-friendly methods.

Thanks,

	Ingo

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Joerg Roedel]

Hi Ingo,

On Fri, May 05, 2017 at 08:59:20AM +0200, Ingo Molnar wrote:
> * Joerg Roedel <jroedel@suse.de> wrote:

> > The problem solved here is that someone wants tboot for security
> > reasons, but doesn't want the performance penalty of having the IOMMU
> > enabled and can live with the risk of an DMA attack.
> 
> Yes, that makes sense - but in this case it would be far more user friendly to 
> make it a sysctl, not a boot option. This is also much more manageable for 
> distributions and also allows it to be more easily turned into a security policy 
> feature.
> 
> New boot options should be for debugging hacks in essence - any serious hardware 
> configuration should be done via more user-friendly methods.

I agree in general that a sysctl would be more user-friendly. But the
problem is that enabling/disabling the IOMMU is a boot-time option that
can't be changed at runtime.

That is because this decission defines how the bus addresses are mapped
to physical addresses through the dma-api. When the iommu is disabled,
it is just a 1-1 mapping, but when it is enabled a physical address
could end up on any address in the bus address space.

Once drivers are loaded that allocate those addresses we can't change
the mappings anymore as disabling the iommu would do.


Regards,

	Joerg

Re: [PATCH V2] x86/tboot: add an option to disable iommu force on

[Ingo Molnar]

* Joerg Roedel <jroedel@suse.de> wrote:

> Hi Ingo,
> 
> On Fri, May 05, 2017 at 08:59:20AM +0200, Ingo Molnar wrote:
> > * Joerg Roedel <jroedel@suse.de> wrote:
> 
> > > The problem solved here is that someone wants tboot for security
> > > reasons, but doesn't want the performance penalty of having the IOMMU
> > > enabled and can live with the risk of an DMA attack.
> > 
> > Yes, that makes sense - but in this case it would be far more user friendly to 
> > make it a sysctl, not a boot option. This is also much more manageable for 
> > distributions and also allows it to be more easily turned into a security policy 
> > feature.
> > 
> > New boot options should be for debugging hacks in essence - any serious hardware 
> > configuration should be done via more user-friendly methods.
> 
> I agree in general that a sysctl would be more user-friendly. But the
> problem is that enabling/disabling the IOMMU is a boot-time option that
> can't be changed at runtime.
> 
> That is because this decission defines how the bus addresses are mapped
> to physical addresses through the dma-api. When the iommu is disabled,
> it is just a 1-1 mapping, but when it is enabled a physical address
> could end up on any address in the bus address space.
> 
> Once drivers are loaded that allocate those addresses we can't change
> the mappings anymore as disabling the iommu would do.

Ok - that makes sense - I withdraw my objections:

  Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo