[PATCH 0/4] iommu/vt-d: Several cleanup patches

[PATCH 0/4] iommu/vt-d: Several cleanup patches

[Lu Baolu]

Hi,

This includes several cleanup patches which aim to make the
code more concise and easier for reading. There aren't any
functionality changes.

Best regards,
Lu Baolu

Lu Baolu (4):
  iommu: Clean up the comments for iommu_group_alloc
  iommu/vt-d: Clean up unused variable in find_or_alloc_domain
  iommu/vt-d: Clean up pasid quirk for pre-production devices
  iommu/vt-d: Remove unnecessary parentheses

 drivers/iommu/intel-iommu.c | 36 +++---------------------------------
 drivers/iommu/intel-svm.c   |  2 +-
 drivers/iommu/iommu.c       |  1 -
 include/linux/intel-iommu.h |  1 -
 4 files changed, 4 insertions(+), 36 deletions(-)

-- 
2.7.4

[PATCH 1/4] iommu: Clean up the comments for iommu_group_alloc

[Lu Baolu]

@name parameter has been removed.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d2aa2320..d87e7c2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -322,7 +322,6 @@ static struct kobj_type iommu_group_ktype = {
 
 /**
  * iommu_group_alloc - Allocate a new group
- * @name: Optional name to associate with group, visible in sysfs
  *
  * This function is called by an iommu driver to allocate a new iommu
  * group.  The iommu group represents the minimum granularity of the iommu.
-- 
2.7.4

[PATCH 2/4] iommu/vt-d: Clean up unused variable in find_or_alloc_domain

[Lu Baolu]

Remove it to make the code more concise.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 749d8f2..9064607 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2533,7 +2533,7 @@ static struct dmar_domain *find_or_alloc_domain(struct device *dev, int gaw)
 	struct device_domain_info *info = NULL;
 	struct dmar_domain *domain = NULL;
 	struct intel_iommu *iommu;
-	u16 req_id, dma_alias;
+	u16 dma_alias;
 	unsigned long flags;
 	u8 bus, devfn;
 
@@ -2541,8 +2541,6 @@ static struct dmar_domain *find_or_alloc_domain(struct device *dev, int gaw)
 	if (!iommu)
 		return NULL;
 
-	req_id = ((u16)bus << 8) | devfn;
-
 	if (dev_is_pci(dev)) {
 		struct pci_dev *pdev = to_pci_dev(dev);
 
-- 
2.7.4

[PATCH 3/4] iommu/vt-d: Clean up pasid quirk for pre-production devices

[Lu Baolu]

The pasid28 quirk is needed only for some pre-production devices.
Remove it to make the code concise.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-iommu.c | 32 ++------------------------------
 include/linux/intel-iommu.h |  1 -
 2 files changed, 2 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 9064607..10bce33 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -485,37 +485,14 @@ static int dmar_forcedac;
 static int intel_iommu_strict;
 static int intel_iommu_superpage = 1;
 static int intel_iommu_ecs = 1;
-static int intel_iommu_pasid28;
 static int iommu_identity_mapping;
 
 #define IDENTMAP_ALL		1
 #define IDENTMAP_GFX		2
 #define IDENTMAP_AZALIA		4
 
-/* Broadwell and Skylake have broken ECS support — normal so-called "second
- * level" translation of DMA requests-without-PASID doesn't actually happen
- * unless you also set the NESTE bit in an extended context-entry. Which of
- * course means that SVM doesn't work because it's trying to do nested
- * translation of the physical addresses it finds in the process page tables,
- * through the IOVA->phys mapping found in the "second level" page tables.
- *
- * The VT-d specification was retroactively changed to change the definition
- * of the capability bits and pretend that Broadwell/Skylake never happened...
- * but unfortunately the wrong bit was changed. It's ECS which is broken, but
- * for some reason it was the PASID capability bit which was redefined (from
- * bit 28 on BDW/SKL to bit 40 in future).
- *
- * So our test for ECS needs to eschew those implementations which set the old
- * PASID capabiity bit 28, since those are the ones on which ECS is broken.
- * Unless we are working around the 'pasid28' limitations, that is, by putting
- * the device into passthrough mode for normal DMA and thus masking the bug.
- */
-#define ecs_enabled(iommu) (intel_iommu_ecs && ecap_ecs(iommu->ecap) && \
-			    (intel_iommu_pasid28 || !ecap_broken_pasid(iommu->ecap)))
-/* PASID support is thus enabled if ECS is enabled and *either* of the old
- * or new capability bits are set. */
-#define pasid_enabled(iommu) (ecs_enabled(iommu) &&			\
-			      (ecap_pasid(iommu->ecap) || ecap_broken_pasid(iommu->ecap)))
+#define ecs_enabled(iommu)	(intel_iommu_ecs && ecap_ecs(iommu->ecap))
+#define pasid_enabled(iommu)	(ecs_enabled(iommu) && ecap_pasid(iommu->ecap))
 
 int intel_iommu_gfx_mapped;
 EXPORT_SYMBOL_GPL(intel_iommu_gfx_mapped);
@@ -578,11 +555,6 @@ static int __init intel_iommu_setup(char *str)
 			printk(KERN_INFO
 				"Intel-IOMMU: disable extended context table support\n");
 			intel_iommu_ecs = 0;
-		} else if (!strncmp(str, "pasid28", 7)) {
-			printk(KERN_INFO
-				"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");
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index ef169d6..1df9401 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -121,7 +121,6 @@
 #define ecap_srs(e)		((e >> 31) & 0x1)
 #define ecap_ers(e)		((e >> 30) & 0x1)
 #define ecap_prs(e)		((e >> 29) & 0x1)
-#define ecap_broken_pasid(e)	((e >> 28) & 0x1)
 #define ecap_dis(e)		((e >> 27) & 0x1)
 #define ecap_nest(e)		((e >> 26) & 0x1)
 #define ecap_mts(e)		((e >> 25) & 0x1)
-- 
2.7.4

Re: [PATCH 3/4] iommu/vt-d: Clean up pasid quirk for pre-production devices

[Chris Wilson]

Quoting Lu Baolu (2018-05-04 06:08:18)
> The pasid28 quirk is needed only for some pre-production devices.
> Remove it to make the code concise.

Every skylake mixing iommu and i915 is now inoperable on boot. Reads
by the GPU from iommu map pages return zero, writes disappear into the
void, no errors flagged.

Please revert until the matter is resolved.
-Chris

Re: [PATCH 3/4] iommu/vt-d: Clean up pasid quirk for pre-production devices

[Lu Baolu]

Hi Chris,

On 07/07/2018 03:01 PM, Chris Wilson wrote:
> Quoting Lu Baolu (2018-05-04 06:08:18)
>> The pasid28 quirk is needed only for some pre-production devices.
>> Remove it to make the code concise.
> Every skylake mixing iommu and i915 is now inoperable on boot. Reads
> by the GPU from iommu map pages return zero, writes disappear into the
> void, no errors flagged.
>
> Please revert until the matter is resolved.

Yes. I also got reports about the i915 issue.

I will submit a revert patch as soon as possible.

I am sorry for the inconvenience.

Best regards,
Lu Baolu

[PATCH 4/4] iommu/vt-d: Remove unnecessary parentheses

[Lu Baolu]

Remove unnecessary parentheses to comply with preferred coding
style.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel-svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index e8cd984..45f6e58 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -319,7 +319,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 	} else
 		pasid_max = 1 << 20;
 
-	if ((flags & SVM_FLAG_SUPERVISOR_MODE)) {
+	if (flags & SVM_FLAG_SUPERVISOR_MODE) {
 		if (!ecap_srs(iommu->ecap))
 			return -EINVAL;
 	} else if (pasid) {
-- 
2.7.4

Re: [PATCH 0/4] iommu/vt-d: Several cleanup patches

[Joerg Roedel]

On Fri, May 04, 2018 at 01:08:15PM +0800, Lu Baolu wrote:
> Lu Baolu (4):
>   iommu: Clean up the comments for iommu_group_alloc
>   iommu/vt-d: Clean up unused variable in find_or_alloc_domain
>   iommu/vt-d: Clean up pasid quirk for pre-production devices
>   iommu/vt-d: Remove unnecessary parentheses
> 
>  drivers/iommu/intel-iommu.c | 36 +++---------------------------------
>  drivers/iommu/intel-svm.c   |  2 +-
>  drivers/iommu/iommu.c       |  1 -
>  include/linux/intel-iommu.h |  1 -
>  4 files changed, 4 insertions(+), 36 deletions(-)

Applied, thanks.