[PATCH] iommu: fix amd_iommu=force_isolation

[PATCH] iommu: fix amd_iommu=force_isolation

[Yu Zhao]

The parameter is still there but it's ignored. We need to check its
value before deciding to go into passthrough mode for AMD IOMMU.

Fixes: aafd8ba0ca74 ("iommu/amd: Implement add_device and remove_device")

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 drivers/iommu/amd_iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 1167ff0416cf..3e4219e6cff0 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2195,7 +2195,8 @@ static int amd_iommu_add_device(struct device *dev)
 
 	BUG_ON(!dev_data);
 
-	if (iommu_pass_through || dev_data->iommu_v2)
+	if (iommu_pass_through ||
+	    (!amd_iommu_force_isolation && dev_data->iommu_v2))
 		iommu_request_dm_for_dev(dev);
 
 	/* Domains are initialized for this device - have a look what we ended up with */
-- 
2.20.0.rc1.387.gf8505762e3-goog

Re: [PATCH] iommu: fix amd_iommu=force_isolation

[Joerg Roedel]

On Tue, Dec 04, 2018 at 03:37:16PM -0700, Yu Zhao wrote:
> The parameter is still there but it's ignored. We need to check its
> value before deciding to go into passthrough mode for AMD IOMMU.
> 
> Fixes: aafd8ba0ca74 ("iommu/amd: Implement add_device and remove_device")
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  drivers/iommu/amd_iommu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 1167ff0416cf..3e4219e6cff0 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2195,7 +2195,8 @@ static int amd_iommu_add_device(struct device *dev)
>  
>  	BUG_ON(!dev_data);
>  
> -	if (iommu_pass_through || dev_data->iommu_v2)
> +	if (iommu_pass_through ||
> +	    (!amd_iommu_force_isolation && dev_data->iommu_v2))

This breaks the iommu_v2 use-case, as it needs a direct mapping for the
devices that support it.

I think the force_isolation parameter does not make sense anymore today
and should be removed.


Regards,

	Joerg

Re: [PATCH] iommu: fix amd_iommu=force_isolation

[Yu Zhao]

On Wed, Dec 05, 2018 at 05:09:55PM +0100, Joerg Roedel wrote:
> On Tue, Dec 04, 2018 at 03:37:16PM -0700, Yu Zhao wrote:
> > The parameter is still there but it's ignored. We need to check its
> > value before deciding to go into passthrough mode for AMD IOMMU.
> > 
> > Fixes: aafd8ba0ca74 ("iommu/amd: Implement add_device and remove_device")
> > 
> > Signed-off-by: Yu Zhao <yuzhao@google.com>
> > ---
> >  drivers/iommu/amd_iommu.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > index 1167ff0416cf..3e4219e6cff0 100644
> > --- a/drivers/iommu/amd_iommu.c
> > +++ b/drivers/iommu/amd_iommu.c
> > @@ -2195,7 +2195,8 @@ static int amd_iommu_add_device(struct device *dev)
> >  
> >  	BUG_ON(!dev_data);
> >  
> > -	if (iommu_pass_through || dev_data->iommu_v2)
> > +	if (iommu_pass_through ||
> > +	    (!amd_iommu_force_isolation && dev_data->iommu_v2))
> 
> This breaks the iommu_v2 use-case, as it needs a direct mapping for the
> devices that support it.

Actually this is what we want. We occasionally use this parameter
to isolate v2 device when debugging memory corruption that we
suspect is caused by DMA. It helped us before. Now with the patch,
we caught another v2 device doing DMA write to memory areas that
it's not supposed to.

> I think the force_isolation parameter does not make sense anymore today
> and should be removed.

Could you please suggest an alternative? Otherwise, we won't be able
to force v2 device out of direct mapping for the reason mentioned
above.

Thank you.

[PATCH v2] iommu: fix amd_iommu=force_isolation

[Yu Zhao]

The parameter is still there but it's ignored. We need to check its
value before deciding to go into passthrough mode for AMD IOMMU v2
capable device.

We occasionally use this parameter to force v2 capable device into
translation mode to debug memory corruption that we suspect is
caused by DMA writes.

To address the following comment from Joerg Roedel on the first
version, v2 capability of device is completely ignored.
> This breaks the iommu_v2 use-case, as it needs a direct mapping for the
> devices that support it.

And from Documentation/admin-guide/kernel-parameters.txt:
  This option does not override iommu=pt

Fixes: aafd8ba0ca74 ("iommu/amd: Implement add_device and remove_device")

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 drivers/iommu/amd_iommu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 1167ff0416cf..325f3bad118b 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -438,7 +438,14 @@ static int iommu_init_device(struct device *dev)
 
 	dev_data->alias = get_alias(dev);
 
-	if (dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
+	/*
+	 * By default we use passthrough mode for IOMMUv2 capable device.
+	 * But if amd_iommu=force_isolation is set (e.g. to debug DMA to
+	 * invalid address), we ignore the capability for the device so
+	 * it'll be forced to go into translation mode.
+	 */
+	if ((iommu_pass_through || !amd_iommu_force_isolation) &&
+	    dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
 		struct amd_iommu *iommu;
 
 		iommu = amd_iommu_rlookup_table[dev_data->devid];
-- 
2.20.0.rc2.403.gdbc3b29805-goog

Re: [PATCH v2] iommu: fix amd_iommu=force_isolation

[Joerg Roedel]

On Thu, Dec 06, 2018 at 02:39:15PM -0700, Yu Zhao wrote:
> Fixes: aafd8ba0ca74 ("iommu/amd: Implement add_device and remove_device")
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  drivers/iommu/amd_iommu.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Applied, thanks.