[PATCH V4 0/4] Code refine for Intel IOMMU

[PATCH V4 0/4] Code refine for Intel IOMMU

[Wei Yang]

These four patches try to refine the Intel IOMMU.                                                                                     
                                                                                                                                      
Patch 1/2 tries to make it more user friendly by add a zero-sized array in                                                            
some dmar data structure.                                                                                                             
Patch 3 move the ckeck of Register Base Address ahead to avoid cleanup when it                                                        
is NULL.                                                                                                                              
Patch 4 re-use dmar_walk_dmar_table() to make the code easy to understand.

V4:
  * add similar change for struct dmar_atsr_unit in patch 1
  * add similar change for rmrr and atsr in patch 2

V3:
  * change hdr to drhd from type acpi_dmar_header to acpi_dmar_hardware_unit 
  * add reason in changelog for the change in Patch 1

V2:
  * add patch 3 and 4

Wei Yang (4):
  iommu/vt-d: replace *hdr with {drhd/atsr}[0] in struct
    dmar_{drhd/atsr}_unit
  iommu/vt-d: use zero-sized array in DMAR related ACPI structures
  iommu/vt-d: check Register Base Address at the beginning of
    dmar_parse_one_drhd()
  iommu/vt-d: refine dmar_acpi_dev_scope_init() with
    dmar_walk_dmar_table()

 drivers/iommu/dmar.c                |  129 +++++++++++++++++------------------
 drivers/iommu/intel-iommu.c         |   26 +++----
 drivers/iommu/intel_irq_remapping.c |   10 ++-
 include/acpi/actbl2.h               |   33 +++++----
 include/linux/dmar.h                |    3 +-
 5 files changed, 102 insertions(+), 99 deletions(-)

-- 
1.7.9.5

[PATCH V4 1/4] iommu/vt-d: replace *hdr with {drhd/atsr}[0] in struct dmar_{drhd/atsr}_unit

[Wei Yang]

Before commit <6b1972493a84> ("iommu/vt-d: Implement DMAR unit hotplug
framework"), dmaru->hdr just points to the memory region of DMA remapping
hardware definition. In this case, it would have no difference to where we
put hdr.

After this commit, DMA remapping hardware definition is copied and
attach to the end of dmaru structure. By replacing a pointer with a
zero-sized array, that would save some space for this structure.

This patch replace *hdr with drhd[0] in struct dmar_drhd_unit and change
the type from acpi_dmar_header to acpi_dmar_hardware_unit. By doing so, it
reflects the real data type contained in dmar_drhd_unit and avoid some type
cast between them.

The same thing happens to struct dmar_atsr_unit, so this patch does similar
change for this structure.

Besides this, this patch includes another change:
* remove redundant type cast to the same type in dmar_table_detect()

Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/dmar.c                |   17 +++++------------
 drivers/iommu/intel-iommu.c         |   18 +++++++++---------
 drivers/iommu/intel_irq_remapping.c |   10 ++++------
 include/linux/dmar.h                |    3 ++-
 4 files changed, 20 insertions(+), 28 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 80e3c17..80199b3 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -292,8 +292,7 @@ static int dmar_pci_bus_add_dev(struct dmar_pci_notify_info *info)
 		if (dmaru->include_all)
 			continue;
 
-		drhd = container_of(dmaru->hdr,
-				    struct acpi_dmar_hardware_unit, header);
+		drhd = dmaru->drhd;
 		ret = dmar_insert_dev_scope(info, (void *)(drhd + 1),
 				((void *)drhd) + drhd->header.length,
 				dmaru->segment,
@@ -390,8 +389,7 @@ static int dmar_parse_one_drhd(struct acpi_dmar_header *header, void *arg)
 	 * If header is allocated from slab by ACPI _DSM method, we need to
 	 * copy the content because the memory buffer will be freed on return.
 	 */
-	dmaru->hdr = (void *)(dmaru + 1);
-	memcpy(dmaru->hdr, header, header->length);
+	memcpy(dmaru->drhd, drhd, drhd->header.length);
 	dmaru->reg_base_addr = drhd->address;
 	dmaru->segment = drhd->segment;
 	dmaru->include_all = drhd->flags & 0x1; /* BIT0: INCLUDE_ALL */
@@ -529,8 +527,7 @@ static int __init dmar_table_detect(void)
 
 	/* if we could find DMAR table, then there are DMAR devices */
 	status = acpi_get_table_with_size(ACPI_SIG_DMAR, 0,
-				(struct acpi_table_header **)&dmar_tbl,
-				&dmar_tbl_size);
+				&dmar_tbl, &dmar_tbl_size);
 
 	if (ACPI_SUCCESS(status) && !dmar_tbl) {
 		pr_warn("Unable to map DMAR\n");
@@ -663,9 +660,7 @@ dmar_find_matched_drhd_unit(struct pci_dev *dev)
 
 	rcu_read_lock();
 	for_each_drhd_unit(dmaru) {
-		drhd = container_of(dmaru->hdr,
-				    struct acpi_dmar_hardware_unit,
-				    header);
+		drhd = dmaru->drhd;
 
 		if (dmaru->include_all &&
 		    drhd->segment == pci_domain_nr(dev->bus))
@@ -693,9 +688,7 @@ static void __init dmar_acpi_insert_dev_scope(u8 device_number,
 	struct acpi_dmar_pci_path *path;
 
 	for_each_drhd_unit(dmaru) {
-		drhd = container_of(dmaru->hdr,
-				    struct acpi_dmar_hardware_unit,
-				    header);
+		drhd = dmaru->drhd;
 
 		for (scope = (void *)(drhd + 1);
 		     (unsigned long)scope < ((unsigned long)drhd) + drhd->header.length;
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ac73876..ab03f79 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -442,10 +442,11 @@ struct dmar_rmrr_unit {
 
 struct dmar_atsr_unit {
 	struct list_head list;		/* list of ATSR units */
-	struct acpi_dmar_header *hdr;	/* ACPI header */
 	struct dmar_dev_scope *devices;	/* target devices */
 	int devices_cnt;		/* target device count */
 	u8 include_all:1;		/* include all ports */
+	struct acpi_dmar_atsr atsr[0];	/* Root Port ATS Capability
+					   Reporting Structure */
 };
 
 static LIST_HEAD(dmar_atsr_units);
@@ -4089,7 +4090,7 @@ static struct dmar_atsr_unit *dmar_find_atsr(struct acpi_dmar_atsr *atsr)
 	struct acpi_dmar_atsr *tmp;
 
 	list_for_each_entry_rcu(atsru, &dmar_atsr_units, list) {
-		tmp = (struct acpi_dmar_atsr *)atsru->hdr;
+		tmp = atsru->atsr;
 		if (atsr->segment != tmp->segment)
 			continue;
 		if (atsr->header.length != tmp->header.length)
@@ -4109,7 +4110,7 @@ int dmar_parse_one_atsr(struct acpi_dmar_header *hdr, void *arg)
 	if (system_state != SYSTEM_BOOTING && !intel_iommu_enabled)
 		return 0;
 
-	atsr = container_of(hdr, struct acpi_dmar_atsr, header);
+	atsr = (struct acpi_dmar_atsr *)hdr;
 	atsru = dmar_find_atsr(atsr);
 	if (atsru)
 		return 0;
@@ -4123,8 +4124,7 @@ int dmar_parse_one_atsr(struct acpi_dmar_header *hdr, void *arg)
 	 * copy the memory content because the memory buffer will be freed
 	 * on return.
 	 */
-	atsru->hdr = (void *)(atsru + 1);
-	memcpy(atsru->hdr, hdr, hdr->length);
+	memcpy(atsru->atsr, hdr, hdr->length);
 	atsru->include_all = atsr->flags & 0x1;
 	if (!atsru->include_all) {
 		atsru->devices = dmar_alloc_dev_scope((void *)(atsr + 1),
@@ -4152,7 +4152,7 @@ int dmar_release_one_atsr(struct acpi_dmar_header *hdr, void *arg)
 	struct acpi_dmar_atsr *atsr;
 	struct dmar_atsr_unit *atsru;
 
-	atsr = container_of(hdr, struct acpi_dmar_atsr, header);
+	atsr = (struct acpi_dmar_atsr *)hdr;
 	atsru = dmar_find_atsr(atsr);
 	if (atsru) {
 		list_del_rcu(&atsru->list);
@@ -4170,7 +4170,7 @@ int dmar_check_one_atsr(struct acpi_dmar_header *hdr, void *arg)
 	struct acpi_dmar_atsr *atsr;
 	struct dmar_atsr_unit *atsru;
 
-	atsr = container_of(hdr, struct acpi_dmar_atsr, header);
+	atsr = (struct acpi_dmar_atsr *)hdr;
 	atsru = dmar_find_atsr(atsr);
 	if (!atsru)
 		return 0;
@@ -4328,7 +4328,7 @@ int dmar_find_matched_atsr_unit(struct pci_dev *dev)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(atsru, &dmar_atsr_units, list) {
-		atsr = container_of(atsru->hdr, struct acpi_dmar_atsr, header);
+		atsr = atsru->atsr;
 		if (atsr->segment != pci_domain_nr(dev->bus))
 			continue;
 
@@ -4377,7 +4377,7 @@ int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)
 		if (atsru->include_all)
 			continue;
 
-		atsr = container_of(atsru->hdr, struct acpi_dmar_atsr, header);
+		atsr = atsru->atsr;
 		if (info->event == BUS_NOTIFY_ADD_DEVICE) {
 			ret = dmar_insert_dev_scope(info, (void *)(atsr + 1),
 					(void *)atsr + atsr->header.length,
diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index 1fae188..e35062e 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -886,17 +886,15 @@ static int ir_parse_one_ioapic_scope(struct acpi_dmar_device_scope *scope,
 	return 0;
 }
 
-static int ir_parse_ioapic_hpet_scope(struct acpi_dmar_header *header,
+static int ir_parse_ioapic_hpet_scope(struct acpi_dmar_hardware_unit *drhd,
 				      struct intel_iommu *iommu)
 {
 	int ret = 0;
-	struct acpi_dmar_hardware_unit *drhd;
 	struct acpi_dmar_device_scope *scope;
 	void *start, *end;
 
-	drhd = (struct acpi_dmar_hardware_unit *)header;
 	start = (void *)(drhd + 1);
-	end = ((void *)drhd) + header->length;
+	end = ((void *)drhd) + drhd->header.length;
 
 	while (start < end && ret == 0) {
 		scope = start;
@@ -940,7 +938,7 @@ static int __init parse_ioapics_under_ir(void)
 		if (!ecap_ir_support(iommu->ecap))
 			continue;
 
-		ret = ir_parse_ioapic_hpet_scope(drhd->hdr, iommu);
+		ret = ir_parse_ioapic_hpet_scope(drhd->drhd, iommu);
 		if (ret)
 			return ret;
 
@@ -1420,7 +1418,7 @@ static int dmar_ir_add(struct dmar_drhd_unit *dmaru, struct intel_iommu *iommu)
 		return -ENODEV;
 	}
 
-	if (ir_parse_ioapic_hpet_scope(dmaru->hdr, iommu)) {
+	if (ir_parse_ioapic_hpet_scope(dmaru->drhd, iommu)) {
 		pr_warn("DRHD %Lx: failed to parse managed IOAPIC/HPET\n",
 			iommu->reg_phys);
 		return -ENODEV;
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index e9bc929..70ef3cf 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -52,7 +52,6 @@ struct dmar_dev_scope {
 extern struct acpi_table_header *dmar_tbl;
 struct dmar_drhd_unit {
 	struct list_head list;		/* list of drhd units	*/
-	struct  acpi_dmar_header *hdr;	/* ACPI header		*/
 	u64	reg_base_addr;		/* register base address*/
 	struct	dmar_dev_scope *devices;/* target device array	*/
 	int	devices_cnt;		/* target device count	*/
@@ -60,6 +59,8 @@ struct dmar_drhd_unit {
 	u8	ignored:1; 		/* ignore drhd		*/
 	u8	include_all:1;
 	struct intel_iommu *iommu;
+	struct  acpi_dmar_hardware_unit drhd[0];
+					/* ACPI Hardware Unit Definition */
 };
 
 struct dmar_pci_path {
-- 
1.7.9.5

[PATCH V4 2/4] iommu/vt-d: use zero-sized array in DMAR related ACPI structures

[Wei Yang]

1. DMAR table has variable number of remapping entries
2. DMAR hardware unit has variable number of device scope
3. DMAR device scope has variable number of pci path
4. DMAR reserved memory has variable number device scope
5. DMAR ATS capability reporting structure has variable number device scope

In current implementation, we use (head + 1) to access these variable
number elements, which may not be obvious for audience. Zero-sized array is
usually used for this purpose.

This patch adds zero-sized array for variable elements in DMAR ACPI
structures, which tries to make the code more audience friendly.

Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/dmar.c        |   22 ++++++++++------------
 drivers/iommu/intel-iommu.c |    8 ++++----
 include/acpi/actbl2.h       |   33 +++++++++++++++++++--------------
 3 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 80199b3..6bff602 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -225,7 +225,6 @@ int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
 	int i, level;
 	struct device *tmp, *dev = &info->dev->dev;
 	struct acpi_dmar_device_scope *scope;
-	struct acpi_dmar_pci_path *path;
 
 	if (segment != info->seg)
 		return 0;
@@ -236,9 +235,8 @@ int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
 		    scope->entry_type != ACPI_DMAR_SCOPE_TYPE_BRIDGE)
 			continue;
 
-		path = (struct acpi_dmar_pci_path *)(scope + 1);
-		level = (scope->length - sizeof(*scope)) / sizeof(*path);
-		if (!dmar_match_pci_path(info, scope->bus, path, level))
+		level = (scope->length - sizeof(*scope)) / sizeof(*scope->path);
+		if (!dmar_match_pci_path(info, scope->bus, scope->path, level))
 			continue;
 
 		if ((scope->entry_type == ACPI_DMAR_SCOPE_TYPE_ENDPOINT) ^
@@ -393,7 +391,7 @@ static int dmar_parse_one_drhd(struct acpi_dmar_header *header, void *arg)
 	dmaru->reg_base_addr = drhd->address;
 	dmaru->segment = drhd->segment;
 	dmaru->include_all = drhd->flags & 0x1; /* BIT0: INCLUDE_ALL */
-	dmaru->devices = dmar_alloc_dev_scope((void *)(drhd + 1),
+	dmaru->devices = dmar_alloc_dev_scope(drhd->dev_scope,
 					      ((void *)drhd) + drhd->header.length,
 					      &dmaru->devices_cnt);
 	if (dmaru->devices_cnt && dmaru->devices == NULL) {
@@ -579,7 +577,7 @@ static int dmar_walk_remapping_entries(struct acpi_dmar_header *start,
 static inline int dmar_walk_dmar_table(struct acpi_table_dmar *dmar,
 				       struct dmar_res_callback *cb)
 {
-	return dmar_walk_remapping_entries((void *)(dmar + 1),
+	return dmar_walk_remapping_entries(dmar->remapping_entries,
 			dmar->header.length - sizeof(*dmar), cb);
 }
 
@@ -685,12 +683,11 @@ static void __init dmar_acpi_insert_dev_scope(u8 device_number,
 	struct acpi_dmar_device_scope *scope;
 	struct device *tmp;
 	int i;
-	struct acpi_dmar_pci_path *path;
 
 	for_each_drhd_unit(dmaru) {
 		drhd = dmaru->drhd;
 
-		for (scope = (void *)(drhd + 1);
+		for (scope = drhd->dev_scope;
 		     (unsigned long)scope < ((unsigned long)drhd) + drhd->header.length;
 		     scope = ((void *)scope) + scope->length) {
 			if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_NAMESPACE)
@@ -698,15 +695,16 @@ static void __init dmar_acpi_insert_dev_scope(u8 device_number,
 			if (scope->enumeration_id != device_number)
 				continue;
 
-			path = (void *)(scope + 1);
 			pr_info("ACPI device \"%s\" under DMAR at %llx as %02x:%02x.%d\n",
 				dev_name(&adev->dev), dmaru->reg_base_addr,
-				scope->bus, path->device, path->function);
+				scope->bus, scope->path->device,
+				scope->path->function);
 			for_each_dev_scope(dmaru->devices, dmaru->devices_cnt, i, tmp)
 				if (tmp == NULL) {
 					dmaru->devices[i].bus = scope->bus;
-					dmaru->devices[i].devfn = PCI_DEVFN(path->device,
-									    path->function);
+					dmaru->devices[i].devfn =
+					  PCI_DEVFN(scope->path->device,
+						    scope->path->function);
 					rcu_assign_pointer(dmaru->devices[i].dev,
 							   get_device(&adev->dev));
 					return;
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ab03f79..e3061d3 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4071,7 +4071,7 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
 	rmrr = (struct acpi_dmar_reserved_memory *)header;
 	rmrru->base_address = rmrr->base_address;
 	rmrru->end_address = rmrr->end_address;
-	rmrru->devices = dmar_alloc_dev_scope((void *)(rmrr + 1),
+	rmrru->devices = dmar_alloc_dev_scope((void *)(rmrr->dev_scope),
 				((void *)rmrr) + rmrr->header.length,
 				&rmrru->devices_cnt);
 	if (rmrru->devices_cnt && rmrru->devices == NULL) {
@@ -4127,7 +4127,7 @@ int dmar_parse_one_atsr(struct acpi_dmar_header *hdr, void *arg)
 	memcpy(atsru->atsr, hdr, hdr->length);
 	atsru->include_all = atsr->flags & 0x1;
 	if (!atsru->include_all) {
-		atsru->devices = dmar_alloc_dev_scope((void *)(atsr + 1),
+		atsru->devices = dmar_alloc_dev_scope((void *)(atsr->dev_scope),
 				(void *)atsr + atsr->header.length,
 				&atsru->devices_cnt);
 		if (atsru->devices_cnt && atsru->devices == NULL) {
@@ -4361,7 +4361,7 @@ int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)
 		rmrr = container_of(rmrru->hdr,
 				    struct acpi_dmar_reserved_memory, header);
 		if (info->event == BUS_NOTIFY_ADD_DEVICE) {
-			ret = dmar_insert_dev_scope(info, (void *)(rmrr + 1),
+			ret = dmar_insert_dev_scope(info, (void *)(rmrr->dev_scope),
 				((void *)rmrr) + rmrr->header.length,
 				rmrr->segment, rmrru->devices,
 				rmrru->devices_cnt);
@@ -4379,7 +4379,7 @@ int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)
 
 		atsr = atsru->atsr;
 		if (info->event == BUS_NOTIFY_ADD_DEVICE) {
-			ret = dmar_insert_dev_scope(info, (void *)(atsr + 1),
+			ret = dmar_insert_dev_scope(info, (void *)(atsr->dev_scope),
 					(void *)atsr + atsr->header.length,
 					atsr->segment, atsru->devices,
 					atsru->devices_cnt);
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 6e28f54..e03df1d 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -403,17 +403,6 @@ struct acpi_table_dbgp {
  *
  ******************************************************************************/
 
-struct acpi_table_dmar {
-	struct acpi_table_header header;	/* Common ACPI table header */
-	u8 width;		/* Host Address Width */
-	u8 flags;
-	u8 reserved[10];
-};
-
-/* Masks for Flags field above */
-
-#define ACPI_DMAR_INTR_REMAP        (1)
-
 /* DMAR subtable header */
 
 struct acpi_dmar_header {
@@ -434,12 +423,18 @@ enum acpi_dmar_type {
 
 /* DMAR Device Scope structure */
 
+struct acpi_dmar_pci_path {
+	u8 device;
+	u8 function;
+};
+
 struct acpi_dmar_device_scope {
 	u8 entry_type;
 	u8 length;
 	u16 reserved;
 	u8 enumeration_id;
 	u8 bus;
+	struct acpi_dmar_pci_path path[0];
 };
 
 /* Values for entry_type in struct acpi_dmar_device_scope - device types */
@@ -454,11 +449,18 @@ enum acpi_dmar_scope_type {
 	ACPI_DMAR_SCOPE_TYPE_RESERVED = 6	/* 6 and greater are reserved */
 };
 
-struct acpi_dmar_pci_path {
-	u8 device;
-	u8 function;
+struct acpi_table_dmar {
+	struct acpi_table_header header;	/* Common ACPI table header */
+	u8 width;		/* Host Address Width */
+	u8 flags;
+	u8 reserved[10];
+	struct acpi_dmar_header remapping_entries[0];
 };
 
+/* Masks for Flags field above */
+
+#define ACPI_DMAR_INTR_REMAP        (1)
+
 /*
  * DMAR Subtables, correspond to Type in struct acpi_dmar_header
  */
@@ -471,6 +473,7 @@ struct acpi_dmar_hardware_unit {
 	u8 reserved;
 	u16 segment;
 	u64 address;		/* Register Base Address */
+	struct acpi_dmar_device_scope dev_scope[0];
 };
 
 /* Masks for Flags field above */
@@ -485,6 +488,7 @@ struct acpi_dmar_reserved_memory {
 	u16 segment;
 	u64 base_address;	/* 4K aligned base address */
 	u64 end_address;	/* 4K aligned limit address */
+	struct acpi_dmar_device_scope dev_scope[0];
 };
 
 /* Masks for Flags field above */
@@ -498,6 +502,7 @@ struct acpi_dmar_atsr {
 	u8 flags;
 	u8 reserved;
 	u16 segment;
+	struct acpi_dmar_device_scope dev_scope[0];
 };
 
 /* Masks for Flags field above */
-- 
1.7.9.5

[PATCH V4 3/4] iommu/vt-d: check Register Base Address at the beginning of dmar_parse_one_drhd()

[Wei Yang]

A NULL value of Register Base Address in a Hardware Unit Definition means
it is an invalid dmar. Current implementation checks this value in
alloc_iommu(), by when it has already allocated memory to store itself and
device scope.

This patch moves the check at the beginning of dmar_parse_one_drhd(), so
that it notices this is an invalid dmar and avoids related setup jobs.

Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/dmar.c |   34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 6bff602..35fc8fb 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -363,6 +363,18 @@ dmar_find_dmaru(struct acpi_dmar_hardware_unit *drhd)
 	return NULL;
 }
 
+static void warn_invalid_dmar(u64 addr, const char *message)
+{
+	WARN_TAINT_ONCE(
+		1, TAINT_FIRMWARE_WORKAROUND,
+		"Your BIOS is broken; DMAR reported at address %llx%s!\n"
+		"BIOS vendor: %s; Ver: %s; Product Version: %s\n",
+		addr, message,
+		dmi_get_system_info(DMI_BIOS_VENDOR),
+		dmi_get_system_info(DMI_BIOS_VERSION),
+		dmi_get_system_info(DMI_PRODUCT_VERSION));
+}
+
 /**
  * dmar_parse_one_drhd - parses exactly one DMA remapping hardware definition
  * structure which uniquely represent one DMA remapping hardware unit
@@ -379,6 +391,11 @@ static int dmar_parse_one_drhd(struct acpi_dmar_header *header, void *arg)
 	if (dmaru)
 		goto out;
 
+	if (!drhd->address) {
+		warn_invalid_dmar(0, "");
+		return -EINVAL;
+	}
+
 	dmaru = kzalloc(sizeof(*dmaru) + header->length, GFP_KERNEL);
 	if (!dmaru)
 		return -ENOMEM;
@@ -808,18 +825,6 @@ int __init dmar_table_init(void)
 	return dmar_table_initialized < 0 ? dmar_table_initialized : 0;
 }
 
-static void warn_invalid_dmar(u64 addr, const char *message)
-{
-	WARN_TAINT_ONCE(
-		1, TAINT_FIRMWARE_WORKAROUND,
-		"Your BIOS is broken; DMAR reported at address %llx%s!\n"
-		"BIOS vendor: %s; Ver: %s; Product Version: %s\n",
-		addr, message,
-		dmi_get_system_info(DMI_BIOS_VENDOR),
-		dmi_get_system_info(DMI_BIOS_VERSION),
-		dmi_get_system_info(DMI_PRODUCT_VERSION));
-}
-
 static int __ref
 dmar_validate_one_drhd(struct acpi_dmar_header *entry, void *arg)
 {
@@ -995,11 +1000,6 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 	int msagaw = 0;
 	int err;
 
-	if (!drhd->reg_base_addr) {
-		warn_invalid_dmar(0, "");
-		return -EINVAL;
-	}
-
 	iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
 	if (!iommu)
 		return -ENOMEM;
-- 
1.7.9.5

[PATCH V4 4/4] iommu/vt-d: refine dmar_acpi_dev_scope_init() with dmar_walk_dmar_table()

[Wei Yang]

dmar_acpi_dev_scope_init() iterates on the remapping structure and just do
proper job for ANDD structure. This is the what dmar_walk_dmar_table()
does.

This patch improves the code with dmar_walk_dmar_table().

Signed-off-by: Wei Yang <richard.weiyang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/dmar.c |   56 ++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 35fc8fb..6281b45 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -733,36 +733,44 @@ static void __init dmar_acpi_insert_dev_scope(u8 device_number,
 		device_number, dev_name(&adev->dev));
 }
 
-static int __init dmar_acpi_dev_scope_init(void)
+static int __dmar_acpi_dev_scope_init(struct acpi_dmar_header *header,
+				      void *arg)
 {
 	struct acpi_dmar_andd *andd;
+	acpi_handle h;
+	struct acpi_device *adev;
+
+	andd = (struct acpi_dmar_andd *)header;
+	if (!ACPI_SUCCESS(acpi_get_handle(ACPI_ROOT_OBJECT,
+					  andd->device_name,
+					  &h))) {
+		pr_err("Failed to find handle for ACPI object %s\n",
+		       andd->device_name);
+		return 0;
+	}
+	if (acpi_bus_get_device(h, &adev)) {
+		pr_err("Failed to get device for ACPI object %s\n",
+		       andd->device_name);
+		return 0;
+	}
+	dmar_acpi_insert_dev_scope(andd->device_number, adev);
+	return 0;
+}
+
+static int __init dmar_acpi_dev_scope_init(void)
+{
+	struct acpi_table_dmar *dmar;
+	struct dmar_res_callback cb = {
+		.print_entry = false,
+		.ignore_unhandled = true,
+		.cb[ACPI_DMAR_TYPE_NAMESPACE] = &__dmar_acpi_dev_scope_init,
+	};
 
 	if (dmar_tbl == NULL)
 		return -ENODEV;
 
-	for (andd = (void *)dmar_tbl + sizeof(struct acpi_table_dmar);
-	     ((unsigned long)andd) < ((unsigned long)dmar_tbl) + dmar_tbl->length;
-	     andd = ((void *)andd) + andd->header.length) {
-		if (andd->header.type == ACPI_DMAR_TYPE_NAMESPACE) {
-			acpi_handle h;
-			struct acpi_device *adev;
-
-			if (!ACPI_SUCCESS(acpi_get_handle(ACPI_ROOT_OBJECT,
-							  andd->device_name,
-							  &h))) {
-				pr_err("Failed to find handle for ACPI object %s\n",
-				       andd->device_name);
-				continue;
-			}
-			if (acpi_bus_get_device(h, &adev)) {
-				pr_err("Failed to get device for ACPI object %s\n",
-				       andd->device_name);
-				continue;
-			}
-			dmar_acpi_insert_dev_scope(andd->device_number, adev);
-		}
-	}
-	return 0;
+	dmar = (struct acpi_table_dmar *)dmar_tbl;
+	return dmar_walk_dmar_table(dmar, &cb);
 }
 
 int __init dmar_dev_scope_init(void)
-- 
1.7.9.5

Re: [PATCH V4 0/4] Code refine for Intel IOMMU

[Wei Yang]

Ping~

On Thu, Apr 14, 2016 at 02:55:06PM +0000, Wei Yang wrote:
>These four patches try to refine the Intel IOMMU.                                                                                     
>                                                                                                                                      
>Patch 1/2 tries to make it more user friendly by add a zero-sized array in                                                            
>some dmar data structure.                                                                                                             
>Patch 3 move the ckeck of Register Base Address ahead to avoid cleanup when it                                                        
>is NULL.                                                                                                                              
>Patch 4 re-use dmar_walk_dmar_table() to make the code easy to understand.
>
>V4:
>  * add similar change for struct dmar_atsr_unit in patch 1
>  * add similar change for rmrr and atsr in patch 2
>
>V3:
>  * change hdr to drhd from type acpi_dmar_header to acpi_dmar_hardware_unit 
>  * add reason in changelog for the change in Patch 1
>
>V2:
>  * add patch 3 and 4
>
>Wei Yang (4):
>  iommu/vt-d: replace *hdr with {drhd/atsr}[0] in struct
>    dmar_{drhd/atsr}_unit
>  iommu/vt-d: use zero-sized array in DMAR related ACPI structures
>  iommu/vt-d: check Register Base Address at the beginning of
>    dmar_parse_one_drhd()
>  iommu/vt-d: refine dmar_acpi_dev_scope_init() with
>    dmar_walk_dmar_table()
>
> drivers/iommu/dmar.c                |  129 +++++++++++++++++------------------
> drivers/iommu/intel-iommu.c         |   26 +++----
> drivers/iommu/intel_irq_remapping.c |   10 ++-
> include/acpi/actbl2.h               |   33 +++++----
> include/linux/dmar.h                |    3 +-
> 5 files changed, 102 insertions(+), 99 deletions(-)
>
>-- 
>1.7.9.5

-- 
Wei Yang
Help you, Help me

Re: [PATCH V4 0/4] Code refine for Intel IOMMU

[Joerg Roedel]

On Sun, May 08, 2016 at 01:22:53PM +0000, Wei Yang wrote:
> >Wei Yang (4):
> >  iommu/vt-d: replace *hdr with {drhd/atsr}[0] in struct
> >    dmar_{drhd/atsr}_unit
> >  iommu/vt-d: use zero-sized array in DMAR related ACPI structures
> >  iommu/vt-d: check Register Base Address at the beginning of
> >    dmar_parse_one_drhd()
> >  iommu/vt-d: refine dmar_acpi_dev_scope_init() with
> >    dmar_walk_dmar_table()

Okay, I've ignored this so far as I don't see where you are going with
this refactoring. The patches as-is don't make much sense to me other
than creating conflicts with other vt-d driver changes.



	Joerg

Re: [PATCH V4 0/4] Code refine for Intel IOMMU

[Wei Yang]

On Mon, May 09, 2016 at 01:24:02PM +0200, Joerg Roedel wrote:
>On Sun, May 08, 2016 at 01:22:53PM +0000, Wei Yang wrote:
>> >Wei Yang (4):
>> >  iommu/vt-d: replace *hdr with {drhd/atsr}[0] in struct
>> >    dmar_{drhd/atsr}_unit
>> >  iommu/vt-d: use zero-sized array in DMAR related ACPI structures
>> >  iommu/vt-d: check Register Base Address at the beginning of
>> >    dmar_parse_one_drhd()
>> >  iommu/vt-d: refine dmar_acpi_dev_scope_init() with
>> >    dmar_walk_dmar_table()
>
>Okay, I've ignored this so far as I don't see where you are going with
>this refactoring. The patches as-is don't make much sense to me other
>than creating conflicts with other vt-d driver changes.
>

Hi, Joerg

Thanks for your comments. 

Generally, the purpose of these patches is to make the code more audience
friendly. Below is my thoughts for each patch.

For Patch 1, the zero-sized drhd/atsr would save some space in
dmar_{drhd/atsr}_unit. As in the change log explained, before commit
<6b1972493a84> ("iommu/vt-d: Implement DMAR unit hotplug framework"), it is
necessary to have a pointer. While after this commit, we just need a place
holder.

For Patch 2, I add a zero-sized array at the end of related structures to
represent variable length elements. By doing so, our code could use this field
to be a parameter for a function instead of casting and add 1. I think this
would let audience understand the variable elements is used here instead of go
through SPEC to check which part it means, which is not that reader friendly.

For Patch 3, when you go through the code, Register Base Address will be
checked to see whether this is a valid dmar. Since this is what we have to do,
I move this step a little bit ahead, so that we can avoid related setups at
the first place.

For Patch 4, dmar_acpi_dev_scope_init() initialize dev_scope by iterate on the
remapping structure. We can see this is just what dmar_walk_dmar_table() does.
So I reuse the code in this place.

In my mind, those changes are dmar internal changes, which will not be seen
outside. Would you mind pointing me the conflicts you saw? Maybe I missed some
thing :-)

Thanks, have a good day.

>
>
>	Joerg

-- 
Wei Yang
Help you, Help me