Consolidate ACPI PCI root common code into ACPI core

Introduce common interface acpi_pci_root_create() and related data structures to create PCI root bus for ACPI PCI host bridges. It will be used to kill duplicated arch specific code for IA64 and x86. It may also help ARM64 in future.

Reviewed-by Lorenzo Pieralisi <[email protected]>

Tested-by Tony Luck <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
 drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |   24 ++++++
 2 files changed, 228 insertions(+)

drivers/acpi/pci_root.c +204 -0

index 393706a5261b..850d7bf0c873 100644
@@ -652,6 +652,210 @@ static void acpi_pci_root_remove(struct acpi_device *device)
 	kfree(root);
 }
 
+/*
+ * Following code to support acpi_pci_root_create() is copied from
+ * arch/x86/pci/acpi.c and modified so it could be reused by x86, IA64
+ * and ARM64.
+ */
+static void acpi_pci_root_validate_resources(struct device *dev,
+					     struct list_head *resources,
+					     unsigned long type)
+{
+	LIST_HEAD(list);
+	struct resource *res1, *res2, *root = NULL;
+	struct resource_entry *tmp, *entry, *entry2;
+
+	BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
+	root = (type & IORESOURCE_MEM) ? &iomem_resource : &ioport_resource;
+
+	list_splice_init(resources, &list);
+	resource_list_for_each_entry_safe(entry, tmp, &list) {
+		bool free = false;
+		resource_size_t end;
+
+		res1 = entry->res;
+		if (!(res1->flags & type))
+			goto next;
+
+		/* Exclude non-addressable range or non-addressable portion */
+		end = min(res1->end, root->end);
+		if (end <= res1->start) {
+			dev_info(dev, "host bridge window %pR (ignored, not CPU addressable)\n",
+				 res1);
+			free = true;
+			goto next;
+		} else if (res1->end != end) {
+			dev_info(dev, "host bridge window %pR ([%#llx-%#llx] ignored, not CPU addressable)\n",
+				 res1, (unsigned long long)end + 1,
+				 (unsigned long long)res1->end);
+			res1->end = end;
+		}
+
+		resource_list_for_each_entry(entry2, resources) {
+			res2 = entry2->res;
+			if (!(res2->flags & type))
+				continue;
+
+			/*
+			 * I don't like throwing away windows because then
+			 * our resources no longer match the ACPI _CRS, but
+			 * the kernel resource tree doesn't allow overlaps.
+			 */
+			if (resource_overlaps(res1, res2)) {
+				res2->start = min(res1->start, res2->start);
+				res2->end = max(res1->end, res2->end);
+				dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
+					 res2, res1);
+				free = true;
+				goto next;
+			}
+		}
+
+next:
+		resource_list_del(entry);
+		if (free)
+			resource_list_free_entry(entry);
+		else
+			resource_list_add_tail(entry, resources);
+	}
+}
+
+int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
+{
+	int ret;
+	struct list_head *list = &info->resources;
+	struct acpi_device *device = info->bridge;
+	struct resource_entry *entry, *tmp;
+	unsigned long flags;
+
+	flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_MEM_8AND16BIT;
+	ret = acpi_dev_get_resources(device, list,
+				     acpi_dev_filter_resource_type_cb,
+				     (void *)flags);
+	if (ret < 0)
+		dev_warn(&device->dev,
+			 "failed to parse _CRS method, error code %d\n", ret);
+	else if (ret == 0)
+		dev_dbg(&device->dev,
+			"no IO and memory resources present in _CRS\n");
+	else {
+		resource_list_for_each_entry_safe(entry, tmp, list) {
+			if (entry->res->flags & IORESOURCE_DISABLED)
+				resource_list_destroy_entry(entry);
+			else
+				entry->res->name = info->name;
+		}
+		acpi_pci_root_validate_resources(&device->dev, list,
+						 IORESOURCE_MEM);
+		acpi_pci_root_validate_resources(&device->dev, list,
+						 IORESOURCE_IO);
+	}
+
+	return ret;
+}
+
+static void pci_acpi_root_add_resources(struct acpi_pci_root_info *info)
+{
+	struct resource_entry *entry, *tmp;
+	struct resource *res, *conflict, *root = NULL;
+
+	resource_list_for_each_entry_safe(entry, tmp, &info->resources) {
+		res = entry->res;
+		if (res->flags & IORESOURCE_MEM)
+			root = &iomem_resource;
+		else if (res->flags & IORESOURCE_IO)
+			root = &ioport_resource;
+		else
+			continue;
+
+		conflict = insert_resource_conflict(root, res);
+		if (conflict) {
+			dev_info(&info->bridge->dev,
+				 "ignoring host bridge window %pR (conflicts with %s %pR)\n",
+				 res, conflict->name, conflict);
+			resource_list_destroy_entry(entry);
+		}
+	}
+}
+
+static void __acpi_pci_root_release_info(struct acpi_pci_root_info *info)
+{
+	struct resource *res;
+	struct resource_entry *entry, *tmp;
+
+	if (!info)
+		return;
+
+	resource_list_for_each_entry_safe(entry, tmp, &info->resources) {
+		res = entry->res;
+		if (res->parent &&
+		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
+			release_resource(res);
+		resource_list_destroy_entry(entry);
+	}
+
+	info->ops->release_info(info);
+}
+
+static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
+{
+	struct resource *res;
+	struct resource_entry *entry;
+
+	resource_list_for_each_entry(entry, &bridge->windows) {
+		res = entry->res;
+		if (res->parent &&
+		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
+			release_resource(res);
+	}
+	__acpi_pci_root_release_info(bridge->release_data);
+}
+
+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
+				     struct acpi_pci_root_ops *ops,
+				     struct acpi_pci_root_info *info,
+				     void *sysdata)
+{
+	int ret, busnum = root->secondary.start;
+	struct acpi_device *device = root->device;
+	int node = acpi_get_node(device->handle);
+	struct pci_bus *bus;
+
+	info->root = root;
+	info->bridge = device;
+	info->ops = ops;
+	INIT_LIST_HEAD(&info->resources);
+	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
+		 root->segment, busnum);
+
+	if (ops->init_info && ops->init_info(info))
+		goto out_release_info;
+	if (ops->prepare_resources)
+		ret = ops->prepare_resources(info);
+	else
+		ret = acpi_pci_probe_root_resources(info);
+	if (ret < 0)
+		goto out_release_info;
+
+	pci_acpi_root_add_resources(info);
+	pci_add_resource(&info->resources, &root->secondary);
+	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+				  sysdata, &info->resources);
+	if (!bus)
+		goto out_release_info;
+
+	pci_scan_child_bus(bus);
+	pci_set_host_bridge_release(to_pci_host_bridge(bus->bridge),
+				    acpi_pci_root_release_info, info);
+	if (node != NUMA_NO_NODE)
+		dev_printk(KERN_DEBUG, &bus->dev, "on NUMA node %d\n", node);
+	return bus;
+
+out_release_info:
+	__acpi_pci_root_release_info(info);
+	return NULL;
+}
+
 void __init acpi_pci_root_init(void)
 {
 	acpi_hest_init();

include/linux/pci-acpi.h +24 -0

index a965efa52152..89ab0572dbc6 100644
@@ -52,6 +52,30 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
 	return ACPI_HANDLE(dev);
 }
 
+struct acpi_pci_root;
+struct acpi_pci_root_ops;
+
+struct acpi_pci_root_info {
+	struct acpi_pci_root		*root;
+	struct acpi_device		*bridge;
+	struct acpi_pci_root_ops	*ops;
+	struct list_head		resources;
+	char				name[16];
+};
+
+struct acpi_pci_root_ops {
+	struct pci_ops *pci_ops;
+	int (*init_info)(struct acpi_pci_root_info *info);
+	void (*release_info)(struct acpi_pci_root_info *info);
+	int (*prepare_resources)(struct acpi_pci_root_info *info);
+};
+
+extern int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info);
+extern struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
+					    struct acpi_pci_root_ops *ops,
+					    struct acpi_pci_root_info *info,
+					    void *sd);
+
 void acpi_pci_add_bus(struct pci_bus *bus);
 void acpi_pci_remove_bus(struct pci_bus *bus);
 

Signature · 6 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Introduce common interface acpi_pci_root_create() and related data structures to create PCI root bus for ACPI PCI host bridges. It will be used to kill duplicated arch specific code for IA64 and x86. It may also help ARM64 in future.

Reviewed-by Lorenzo Pieralisi <[email protected]>

Tested-by Tony Luck <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
  drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/pci-acpi.h |   24 ++++++
  2 files changed, 228 insertions(+)

drivers/acpi/pci_root.c +21 -0

+
+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
+				     struct acpi_pci_root_ops *ops,
+				     struct acpi_pci_root_info *info,
+				     void *sysdata)
+{
+	int ret, busnum = root->secondary.start;
+	struct acpi_device *device = root->device;
+	int node = acpi_get_node(device->handle);
+	struct pci_bus *bus;
+
+	info->root = root;
+	info->bridge = device;
+	info->ops = ops;
+	INIT_LIST_HEAD(&info->resources);
+	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
+		 root->segment, busnum);
+
+	if (ops->init_info && ops->init_info(info))
+		goto out_release_info;
+	if (ops->prepare_resources)

Quoted 14 lines · Jiang Liu

Introduce common interface acpi_pci_root_create() and related data structures to create PCI root bus for ACPI PCI host bridges. It will be used to kill duplicated arch specific code for IA64 and x86. It may also help ARM64 in future.

Reviewed-by Lorenzo Pieralisi <[email protected]>

Tested-by Tony Luck <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
 drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |   24 ++++++
 2 files changed, 228 insertions(+)

[...]

Quoted 21 lines

drivers/acpi/pci_root.c +21 -0

+
+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
+				     struct acpi_pci_root_ops *ops,
+				     struct acpi_pci_root_info *info,
+				     void *sysdata)
+{
+	int ret, busnum = root->secondary.start;
+	struct acpi_device *device = root->device;
+	int node = acpi_get_node(device->handle);
+	struct pci_bus *bus;
+
+	info->root = root;
+	info->bridge = device;
+	info->ops = ops;
+	INIT_LIST_HEAD(&info->resources);
+	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
+		 root->segment, busnum);
+
+	if (ops->init_info && ops->init_info(info))
+		goto out_release_info;
+	if (ops->prepare_resources)

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 843

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+		ret = ops->prepare_resources(info);
+	else
+		ret = acpi_pci_probe_root_resources(info);
+	if (ret < 0)
+		goto out_release_info;
+
+	pci_acpi_root_add_resources(info);
+	pci_add_resource(&info->resources, &root->secondary);
+	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+				  sysdata, &info->resources);

Thank a lot for this cleanup!!

I recall you already considered passing segment (domain nr) to pci_create_root_bus, right? Can you please remind me why we gave up on this?

I am asking because currently I can not find the way to retrieve domain number from pci_bus_assign_domain_nr (for those platforms which choose PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the part of pci_create_root_bus.

Regards,
Tomasz

Quoted 55 lines · Tomasz Nowicki

Quoted 14 lines · Jiang Liu

Introduce common interface acpi_pci_root_create() and related data structures to create PCI root bus for ACPI PCI host bridges. It will be used to kill duplicated arch specific code for IA64 and x86. It may also help ARM64 in future.

Reviewed-by Lorenzo Pieralisi <[email protected]>

Tested-by Tony Luck <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
  drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/pci-acpi.h |   24 ++++++
  2 files changed, 228 insertions(+)

[...]

Quoted 21 lines

drivers/acpi/pci_root.c +21 -0

+
+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
+				     struct acpi_pci_root_ops *ops,
+				     struct acpi_pci_root_info *info,
+				     void *sysdata)
+{
+	int ret, busnum = root->secondary.start;
+	struct acpi_device *device = root->device;
+	int node = acpi_get_node(device->handle);
+	struct pci_bus *bus;
+
+	info->root = root;
+	info->bridge = device;
+	info->ops = ops;
+	INIT_LIST_HEAD(&info->resources);
+	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
+		 root->segment, busnum);
+
+	if (ops->init_info && ops->init_info(info))
+		goto out_release_info;
+	if (ops->prepare_resources)

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 843

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+		ret = ops->prepare_resources(info);
+	else
+		ret = acpi_pci_probe_root_resources(info);
+	if (ret < 0)
+		goto out_release_info;
+
+	pci_acpi_root_add_resources(info);
+	pci_add_resource(&info->resources, &root->secondary);
+	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+				  sysdata, &info->resources);

Thank a lot for this cleanup!!

I recall you already considered passing segment (domain nr) to pci_create_root_bus, right? Can you please remind me why we gave up on this?

Quoted 4 lines

I am asking because currently I can not find the way to retrieve domain number from pci_bus_assign_domain_nr (for those platforms which choose PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the part of pci_create_root_bus.

Not sure I fully understand your question, but pci_bus_assign_domain_nr() will put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC. Do you want to override that value with the segment nr from MCFG?

Quoted 56 lines · Liviu Dudau

Quoted 55 lines · Tomasz Nowicki

Quoted 14 lines · Jiang Liu

Introduce common interface acpi_pci_root_create() and related data structures to create PCI root bus for ACPI PCI host bridges. It will be used to kill duplicated arch specific code for IA64 and x86. It may also help ARM64 in future.

Reviewed-by Lorenzo Pieralisi <[email protected]>

Tested-by Tony Luck <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
 drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |   24 ++++++
 2 files changed, 228 insertions(+)

[...]

Quoted 21 lines

drivers/acpi/pci_root.c +21 -0

+
+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
+				     struct acpi_pci_root_ops *ops,
+				     struct acpi_pci_root_info *info,
+				     void *sysdata)
+{
+	int ret, busnum = root->secondary.start;
+	struct acpi_device *device = root->device;
+	int node = acpi_get_node(device->handle);
+	struct pci_bus *bus;
+
+	info->root = root;
+	info->bridge = device;
+	info->ops = ops;
+	INIT_LIST_HEAD(&info->resources);
+	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
+		 root->segment, busnum);
+
+	if (ops->init_info && ops->init_info(info))
+		goto out_release_info;
+	if (ops->prepare_resources)

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 843

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+		ret = ops->prepare_resources(info);
+	else
+		ret = acpi_pci_probe_root_resources(info);
+	if (ret < 0)
+		goto out_release_info;
+
+	pci_acpi_root_add_resources(info);
+	pci_add_resource(&info->resources, &root->secondary);
+	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+				  sysdata, &info->resources);

Thank a lot for this cleanup!!

I recall you already considered passing segment (domain nr) to pci_create_root_bus, right? Can you please remind me why we gave up on this?

Quoted 9 lines

Quoted 4 lines

I am asking because currently I can not find the way to retrieve domain number from pci_bus_assign_domain_nr (for those platforms which choose PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the part of pci_create_root_bus.

Not sure I fully understand your question, but pci_bus_assign_domain_nr() will put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC. Do you want to override that value with the segment nr from MCFG?

Let me give ACPI ARM64 example:

1. We parse MCFG table and get segment nr assigned to root bridge
2. Then PCI host bridge calls acpi_pci_root_create -> pci_create_root_bus -> pci_bus_assign_domain_nr
3. At this point we cannot get segment nr for ACPI

So I would like to assign MCFG segment nr to bus->domain_nr being in pci_bus_assign_domain_nr giving we have scenario above.

Quoted 69 lines · Tomasz Nowicki

Quoted 56 lines · Liviu Dudau

Quoted 55 lines · Tomasz Nowicki

Quoted 14 lines · Jiang Liu

Introduce common interface acpi_pci_root_create() and related data structures to create PCI root bus for ACPI PCI host bridges. It will be used to kill duplicated arch specific code for IA64 and x86. It may also help ARM64 in future.

Reviewed-by Lorenzo Pieralisi <[email protected]>

Tested-by Tony Luck <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
  drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/pci-acpi.h |   24 ++++++
  2 files changed, 228 insertions(+)

[...]

Quoted 21 lines

drivers/acpi/pci_root.c +21 -0

+
+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
+				     struct acpi_pci_root_ops *ops,
+				     struct acpi_pci_root_info *info,
+				     void *sysdata)
+{
+	int ret, busnum = root->secondary.start;
+	struct acpi_device *device = root->device;
+	int node = acpi_get_node(device->handle);
+	struct pci_bus *bus;
+
+	info->root = root;
+	info->bridge = device;
+	info->ops = ops;
+	INIT_LIST_HEAD(&info->resources);
+	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
+		 root->segment, busnum);
+
+	if (ops->init_info && ops->init_info(info))
+		goto out_release_info;
+	if (ops->prepare_resources)

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 843

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+		ret = ops->prepare_resources(info);
+	else
+		ret = acpi_pci_probe_root_resources(info);
+	if (ret < 0)
+		goto out_release_info;
+
+	pci_acpi_root_add_resources(info);
+	pci_add_resource(&info->resources, &root->secondary);
+	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+				  sysdata, &info->resources);

Thank a lot for this cleanup!!

I recall you already considered passing segment (domain nr) to pci_create_root_bus, right? Can you please remind me why we gave up on this?

Quoted 9 lines

Quoted 4 lines

I am asking because currently I can not find the way to retrieve domain number from pci_bus_assign_domain_nr (for those platforms which choose PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the part of pci_create_root_bus.

Not sure I fully understand your question, but pci_bus_assign_domain_nr() will put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC. Do you want to override that value with the segment nr from MCFG?

Let me give ACPI ARM64 example:

Quoted 7 lines

1. We parse MCFG table and get segment nr assigned to root bridge
2. Then PCI host bridge calls acpi_pci_root_create -> pci_create_root_bus -> pci_bus_assign_domain_nr
3. At this point we cannot get segment nr for ACPI

So I would like to assign MCFG segment nr to bus->domain_nr being in pci_bus_assign_domain_nr giving we have scenario above.

I do not understand what you mean by "assign MCFG segment" here, please explain. The MCFG segment group number is used to look-up the configuration space for a given host bridge, not to assign a domain number to it.

The domain_nr above is the value returned by the _SEG object for the host bridge device.

Quoted 56 lines · Liviu Dudau

Quoted 55 lines · Tomasz Nowicki

Quoted 14 lines · Jiang Liu

Introduce common interface acpi_pci_root_create() and related data structures to create PCI root bus for ACPI PCI host bridges. It will be used to kill duplicated arch specific code for IA64 and x86. It may also help ARM64 in future.

Reviewed-by Lorenzo Pieralisi <[email protected]>

Tested-by Tony Luck <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
 drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |   24 ++++++
 2 files changed, 228 insertions(+)

[...]

Quoted 21 lines

drivers/acpi/pci_root.c +21 -0

+
+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
+				     struct acpi_pci_root_ops *ops,
+				     struct acpi_pci_root_info *info,
+				     void *sysdata)
+{
+	int ret, busnum = root->secondary.start;
+	struct acpi_device *device = root->device;
+	int node = acpi_get_node(device->handle);
+	struct pci_bus *bus;
+
+	info->root = root;
+	info->bridge = device;
+	info->ops = ops;
+	INIT_LIST_HEAD(&info->resources);
+	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
+		 root->segment, busnum);
+
+	if (ops->init_info && ops->init_info(info))
+		goto out_release_info;
+	if (ops->prepare_resources)

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 843

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+		ret = ops->prepare_resources(info);
+	else
+		ret = acpi_pci_probe_root_resources(info);
+	if (ret < 0)
+		goto out_release_info;
+
+	pci_acpi_root_add_resources(info);
+	pci_add_resource(&info->resources, &root->secondary);
+	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+				  sysdata, &info->resources);

Thank a lot for this cleanup!!

I recall you already considered passing segment (domain nr) to pci_create_root_bus, right? Can you please remind me why we gave up on this?

Quoted 9 lines

Quoted 4 lines

I am asking because currently I can not find the way to retrieve domain number from pci_bus_assign_domain_nr (for those platforms which choose PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the part of pci_create_root_bus.

Not sure I fully understand your question, but pci_bus_assign_domain_nr() will put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC. Do you want to override that value with the segment nr from MCFG?

Let me give ACPI ARM64 example:

1. We parse MCFG table and get segment nr assigned to root bridge
2. Then PCI host bridge calls acpi_pci_root_create -> pci_create_root_bus -> pci_bus_assign_domain_nr
3. At this point we cannot get segment nr for ACPI

So I would like to assign MCFG segment nr to bus->domain_nr being in pci_bus_assign_domain_nr giving we have scenario above.

Thanks,
Tomasz

Quoted 64 lines · Liviu Dudau

Quoted 57 lines · Tomasz Nowicki

Quoted 15 lines · Jiang Liu

Introduce common interface acpi_pci_root_create() and related data structures to create PCI root bus for ACPI PCI host bridges. It will be used to kill duplicated arch specific code for IA64 and x86. It may also help ARM64 in future.

Reviewed-by Lorenzo Pieralisi <[email protected]>

Tested-by Tony Luck <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
  drivers/acpi/pci_root.c  |  204
++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/pci-acpi.h |   24 ++++++
  2 files changed, 228 insertions(+)

[...]

Quoted 21 lines

drivers/acpi/pci_root.c +21 -0

+
+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
+                     struct acpi_pci_root_ops *ops,
+                     struct acpi_pci_root_info *info,
+                     void *sysdata)
+{
+    int ret, busnum = root->secondary.start;
+    struct acpi_device *device = root->device;
+    int node = acpi_get_node(device->handle);
+    struct pci_bus *bus;
+
+    info->root = root;
+    info->bridge = device;
+    info->ops = ops;
+    INIT_LIST_HEAD(&info->resources);
+    snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
+         root->segment, busnum);
+
+    if (ops->init_info && ops->init_info(info))
+        goto out_release_info;
+    if (ops->prepare_resources)

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 843

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+        ret = ops->prepare_resources(info);
+    else
+        ret = acpi_pci_probe_root_resources(info);
+    if (ret < 0)
+        goto out_release_info;
+
+    pci_acpi_root_add_resources(info);
+    pci_add_resource(&info->resources, &root->secondary);
+    bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+                  sysdata, &info->resources);

Thank a lot for this cleanup!!

I recall you already considered passing segment (domain nr) to pci_create_root_bus, right? Can you please remind me why we gave up on this?

Quoted 5 lines

I am asking because currently I can not find the way to retrieve domain number from pci_bus_assign_domain_nr (for those platforms which choose PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the
part of pci_create_root_bus.

Quoted 6 lines

Not sure I fully understand your question, but pci_bus_assign_domain_nr() will
put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC.
Do you want to override that value with the segment nr from MCFG?

Let me give ACPI ARM64 example:

1. We parse MCFG table and get segment nr assigned to root bridge
2. Then PCI host bridge calls acpi_pci_root_create -> pci_create_root_bus -> pci_bus_assign_domain_nr
3. At this point we cannot get segment nr for ACPI

So I would like to assign MCFG segment nr to bus->domain_nr being in pci_bus_assign_domain_nr giving we have scenario above.

Thanks,
Tomasz

Signature · 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 74 lines · Tomasz Nowicki

Quoted 64 lines · Liviu Dudau

Quoted 57 lines · Tomasz Nowicki

Quoted 15 lines · Jiang Liu

Introduce common interface acpi_pci_root_create() and related data structures to create PCI root bus for ACPI PCI host bridges. It will be used to kill duplicated arch specific code for IA64 and x86. It may also help ARM64 in future.

Reviewed-by Lorenzo Pieralisi <[email protected]>

Tested-by Tony Luck <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
  drivers/acpi/pci_root.c  |  204
++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/pci-acpi.h |   24 ++++++
  2 files changed, 228 insertions(+)

[...]

Quoted 21 lines

drivers/acpi/pci_root.c +21 -0

+
+struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
+                     struct acpi_pci_root_ops *ops,
+                     struct acpi_pci_root_info *info,
+                     void *sysdata)
+{
+    int ret, busnum = root->secondary.start;
+    struct acpi_device *device = root->device;
+    int node = acpi_get_node(device->handle);
+    struct pci_bus *bus;
+
+    info->root = root;
+    info->bridge = device;
+    info->ops = ops;
+    INIT_LIST_HEAD(&info->resources);
+    snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
+         root->segment, busnum);
+
+    if (ops->init_info && ops->init_info(info))
+        goto out_release_info;
+    if (ops->prepare_resources)

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 843

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+        ret = ops->prepare_resources(info);
+    else
+        ret = acpi_pci_probe_root_resources(info);
+    if (ret < 0)
+        goto out_release_info;
+
+    pci_acpi_root_add_resources(info);
+    pci_add_resource(&info->resources, &root->secondary);
+    bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+                  sysdata, &info->resources);

Thank a lot for this cleanup!!

I recall you already considered passing segment (domain nr) to pci_create_root_bus, right? Can you please remind me why we gave up on this?

Quoted 5 lines

I am asking because currently I can not find the way to retrieve domain number from pci_bus_assign_domain_nr (for those platforms which choose PCI_DOMAINS_GENERIC and want to use segment nr from MCFG table) which is the
part of pci_create_root_bus.

Quoted 6 lines

Not sure I fully understand your question, but pci_bus_assign_domain_nr() will
put the assigned domain number in bus->domain_nr if you chose PCI_DOMAINS_GENERIC.
Do you want to override that value with the segment nr from MCFG?

Let me give ACPI ARM64 example:

Quoted 7 lines

1. We parse MCFG table and get segment nr assigned to root bridge
2. Then PCI host bridge calls acpi_pci_root_create -> pci_create_root_bus -> pci_bus_assign_domain_nr
3. At this point we cannot get segment nr for ACPI

So I would like to assign MCFG segment nr to bus->domain_nr being in pci_bus_assign_domain_nr giving we have scenario above.

Please use sysdata for that, IA64 and x86 are making use of sysdata to store such information:
struct pci_sysdata {

        int             domain;         /* PCI domain */
        int             node;           /* NUMA node */

#ifdef CONFIG_ACPI
struct acpi_device *companion; /* ACPI companion device */
#endif
#ifdef CONFIG_X86_64
void *iommu; /* IOMMU private data */
#endif
};

Hanjun once tried to introduce struct pci_sysdata for ARM64, but seems it has been rejected.

Quoted 9 lines

Thanks,
Tomasz

Signature · 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Introduce common interface acpi_pci_root_create() and related data structures to create PCI root bus for ACPI PCI host bridges. It will be used to kill duplicated arch specific code for IA64 and x86. It may also help ARM64 in future.

Reviewed-by Lorenzo Pieralisi <[email protected]>

Tested-by Tony Luck <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Signed-off-by Liu Jiang <[email protected]>

---
  drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/pci-acpi.h |   24 ++++++
  2 files changed, 228 insertions(+)

drivers/acpi/pci_root.c +87 -0

index 393706a5261b..850d7bf0c873 100644
@@ -652,6 +652,210 @@ static void acpi_pci_root_remove(struct acpi_device *device)
  	kfree(root);
  }
 
+/*
+ * Following code to support acpi_pci_root_create() is copied from
+ * arch/x86/pci/acpi.c and modified so it could be reused by x86, IA64
+ * and ARM64.
+ */
+static void acpi_pci_root_validate_resources(struct device *dev,
+					     struct list_head *resources,
+					     unsigned long type)
+{
+	LIST_HEAD(list);
+	struct resource *res1, *res2, *root = NULL;
+	struct resource_entry *tmp, *entry, *entry2;
+
+	BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
+	root = (type & IORESOURCE_MEM) ? &iomem_resource : &ioport_resource;
+
+	list_splice_init(resources, &list);
+	resource_list_for_each_entry_safe(entry, tmp, &list) {
+		bool free = false;
+		resource_size_t end;
+
+		res1 = entry->res;
+		if (!(res1->flags & type))
+			goto next;
+
+		/* Exclude non-addressable range or non-addressable portion */
+		end = min(res1->end, root->end);
+		if (end <= res1->start) {
+			dev_info(dev, "host bridge window %pR (ignored, not CPU addressable)\n",
+				 res1);
+			free = true;
+			goto next;
+		} else if (res1->end != end) {
+			dev_info(dev, "host bridge window %pR ([%#llx-%#llx] ignored, not CPU addressable)\n",
+				 res1, (unsigned long long)end + 1,
+				 (unsigned long long)res1->end);
+			res1->end = end;
+		}
+
+		resource_list_for_each_entry(entry2, resources) {
+			res2 = entry2->res;
+			if (!(res2->flags & type))
+				continue;
+
+			/*
+			 * I don't like throwing away windows because then
+			 * our resources no longer match the ACPI _CRS, but
+			 * the kernel resource tree doesn't allow overlaps.
+			 */
+			if (resource_overlaps(res1, res2)) {
+				res2->start = min(res1->start, res2->start);
+				res2->end = max(res1->end, res2->end);
+				dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
+					 res2, res1);
+				free = true;
+				goto next;
+			}
+		}
+
+next:
+		resource_list_del(entry);
+		if (free)
+			resource_list_free_entry(entry);
+		else
+			resource_list_add_tail(entry, resources);
+	}
+}
+
+int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
+{
+	int ret;
+	struct list_head *list = &info->resources;
+	struct acpi_device *device = info->bridge;
+	struct resource_entry *entry, *tmp;
+	unsigned long flags;
+
+	flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_MEM_8AND16BIT;
+	ret = acpi_dev_get_resources(device, list,
+				     acpi_dev_filter_resource_type_cb,
+				     (void *)flags);
+	if (ret < 0)
+		dev_warn(&device->dev,
+			 "failed to parse _CRS method, error code %d\n", ret);
+	else if (ret == 0)
+		dev_dbg(&device->dev,
+			"no IO and memory resources present in _CRS\n");
+	else {

On 14.10.2015 08:29, Jiang Liu wrote:

Quoted 82 lines

drivers/acpi/pci_root.c +82 -0

+static void acpi_pci_root_validate_resources(struct device *dev,
+					     struct list_head *resources,
+					     unsigned long type)
+{
+	LIST_HEAD(list);
+	struct resource *res1, *res2, *root = NULL;
+	struct resource_entry *tmp, *entry, *entry2;
+
+	BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
+	root = (type & IORESOURCE_MEM) ? &iomem_resource : &ioport_resource;
+
+	list_splice_init(resources, &list);
+	resource_list_for_each_entry_safe(entry, tmp, &list) {
+		bool free = false;
+		resource_size_t end;
+
+		res1 = entry->res;
+		if (!(res1->flags & type))
+			goto next;
+
+		/* Exclude non-addressable range or non-addressable portion */
+		end = min(res1->end, root->end);
+		if (end <= res1->start) {
+			dev_info(dev, "host bridge window %pR (ignored, not CPU addressable)\n",
+				 res1);
+			free = true;
+			goto next;
+		} else if (res1->end != end) {
+			dev_info(dev, "host bridge window %pR ([%#llx-%#llx] ignored, not CPU addressable)\n",
+				 res1, (unsigned long long)end + 1,
+				 (unsigned long long)res1->end);
+			res1->end = end;
+		}
+
+		resource_list_for_each_entry(entry2, resources) {
+			res2 = entry2->res;
+			if (!(res2->flags & type))
+				continue;
+
+			/*
+			 * I don't like throwing away windows because then
+			 * our resources no longer match the ACPI _CRS, but
+			 * the kernel resource tree doesn't allow overlaps.
+			 */
+			if (resource_overlaps(res1, res2)) {
+				res2->start = min(res1->start, res2->start);
+				res2->end = max(res1->end, res2->end);
+				dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
+					 res2, res1);
+				free = true;
+				goto next;
+			}
+		}
+
+next:
+		resource_list_del(entry);
+		if (free)
+			resource_list_free_entry(entry);
+		else
+			resource_list_add_tail(entry, resources);
+	}
+}
+
+int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
+{
+	int ret;
+	struct list_head *list = &info->resources;
+	struct acpi_device *device = info->bridge;
+	struct resource_entry *entry, *tmp;
+	unsigned long flags;
+
+	flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_MEM_8AND16BIT;
+	ret = acpi_dev_get_resources(device, list,
+				     acpi_dev_filter_resource_type_cb,
+				     (void *)flags);
+	if (ret < 0)
+		dev_warn(&device->dev,
+			 "failed to parse _CRS method, error code %d\n", ret);
+	else if (ret == 0)
+		dev_dbg(&device->dev,
+			"no IO and memory resources present in _CRS\n");
+	else {

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 751

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+		resource_list_for_each_entry_safe(entry, tmp, list) {
+			if (entry->res->flags & IORESOURCE_DISABLED)
+				resource_list_destroy_entry(entry);
+			else
+				entry->res->name = info->name;
+		}
+		acpi_pci_root_validate_resources(&device->dev, list,
+						 IORESOURCE_MEM);
+		acpi_pci_root_validate_resources(&device->dev, list,
+						 IORESOURCE_IO);

It is not clear to me why we need these two calls above ^^^. We are using pci_acpi_root_add_resources(info) later. Is it not enough?

Also, I cannot use acpi_pci_probe_root_resources() in my ARM64 PCI driver. It is because acpi_dev_get_resources is adding translation_offset to IO ranges start address and then:
acpi_pci_root_validate_resources(&device->dev, list,

				 IORESOURCE_IO);

rejects that IO regions as it is out of my 0x0-SZ_16M window.

Does acpi_pci_probe_root_resources meant to be x86 specific and I should avoid using it?

Quoted 1 line · Tomasz Nowicki

On 14.10.2015 08:29, Jiang Liu wrote:

[...]

Quoted 100 lines

Quoted 82 lines

drivers/acpi/pci_root.c +82 -0

+static void acpi_pci_root_validate_resources(struct device *dev,
+					     struct list_head *resources,
+					     unsigned long type)
+{
+	LIST_HEAD(list);
+	struct resource *res1, *res2, *root = NULL;
+	struct resource_entry *tmp, *entry, *entry2;
+
+	BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
+	root = (type & IORESOURCE_MEM) ? &iomem_resource : &ioport_resource;
+
+	list_splice_init(resources, &list);
+	resource_list_for_each_entry_safe(entry, tmp, &list) {
+		bool free = false;
+		resource_size_t end;
+
+		res1 = entry->res;
+		if (!(res1->flags & type))
+			goto next;
+
+		/* Exclude non-addressable range or non-addressable portion */
+		end = min(res1->end, root->end);
+		if (end <= res1->start) {
+			dev_info(dev, "host bridge window %pR (ignored, not CPU addressable)\n",
+				 res1);
+			free = true;
+			goto next;
+		} else if (res1->end != end) {
+			dev_info(dev, "host bridge window %pR ([%#llx-%#llx] ignored, not CPU addressable)\n",
+				 res1, (unsigned long long)end + 1,
+				 (unsigned long long)res1->end);
+			res1->end = end;
+		}
+
+		resource_list_for_each_entry(entry2, resources) {
+			res2 = entry2->res;
+			if (!(res2->flags & type))
+				continue;
+
+			/*
+			 * I don't like throwing away windows because then
+			 * our resources no longer match the ACPI _CRS, but
+			 * the kernel resource tree doesn't allow overlaps.
+			 */
+			if (resource_overlaps(res1, res2)) {
+				res2->start = min(res1->start, res2->start);
+				res2->end = max(res1->end, res2->end);
+				dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
+					 res2, res1);
+				free = true;
+				goto next;
+			}
+		}
+
+next:
+		resource_list_del(entry);
+		if (free)
+			resource_list_free_entry(entry);
+		else
+			resource_list_add_tail(entry, resources);
+	}
+}
+
+int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
+{
+	int ret;
+	struct list_head *list = &info->resources;
+	struct acpi_device *device = info->bridge;
+	struct resource_entry *entry, *tmp;
+	unsigned long flags;
+
+	flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_MEM_8AND16BIT;
+	ret = acpi_dev_get_resources(device, list,
+				     acpi_dev_filter_resource_type_cb,
+				     (void *)flags);
+	if (ret < 0)
+		dev_warn(&device->dev,
+			 "failed to parse _CRS method, error code %d\n", ret);
+	else if (ret == 0)
+		dev_dbg(&device->dev,
+			"no IO and memory resources present in _CRS\n");
+	else {

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 751

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+		resource_list_for_each_entry_safe(entry, tmp, list) {
+			if (entry->res->flags & IORESOURCE_DISABLED)
+				resource_list_destroy_entry(entry);
+			else
+				entry->res->name = info->name;
+		}
+		acpi_pci_root_validate_resources(&device->dev, list,
+						 IORESOURCE_MEM);
+		acpi_pci_root_validate_resources(&device->dev, list,
+						 IORESOURCE_IO);

It is not clear to me why we need these two calls above ^^^. We are using pci_acpi_root_add_resources(info) later. Is it not enough?

Also, I cannot use acpi_pci_probe_root_resources() in my ARM64 PCI driver. It is because acpi_dev_get_resources is adding translation_offset to IO ranges start address and then:
acpi_pci_root_validate_resources(&device->dev, list,

Quoted 5 lines

				 IORESOURCE_IO);

rejects that IO regions as it is out of my 0x0-SZ_16M window.

Does acpi_pci_probe_root_resources meant to be x86 specific and I should avoid using it?

IIUC, you _have_ to have the proper translation_offset to map the bridge window into the IO address space:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348708.html

Then, using the offset, you should do something ia64 does, namely, retrieve the CPU address corresponding to IO space (see arch/ia64/pci/pci.c
- add_io_space()) and map it in the physical address space by using pci_remap_iospace(), it is similar to what we have to do with DT.

It is extremely confusing and I am not sure I got it right myself, I am still grokking ia64 code to understand what it really does.

So basically, the IO bridge window coming from acpi_dev_get_resource() should represent the IO space in 0 - 16M, IIUC.

By using the offset (that was initialized using translation_offset) and the resource->start, you can retrieve the cpu address that you need to actually map the IO space, since that's what we do on ARM (ie the IO resource is an offset into the virtual address space set aside for IO).

Confusing, to say the least. Jiang, did I get it right ?

Quoted 1 line · Tomasz Nowicki

On 14.10.2015 08:29, Jiang Liu wrote:

[...]

Quoted 100 lines

Quoted 82 lines

drivers/acpi/pci_root.c +82 -0

+static void acpi_pci_root_validate_resources(struct device *dev,
+					     struct list_head *resources,
+					     unsigned long type)
+{
+	LIST_HEAD(list);
+	struct resource *res1, *res2, *root = NULL;
+	struct resource_entry *tmp, *entry, *entry2;
+
+	BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
+	root = (type & IORESOURCE_MEM) ? &iomem_resource : &ioport_resource;
+
+	list_splice_init(resources, &list);
+	resource_list_for_each_entry_safe(entry, tmp, &list) {
+		bool free = false;
+		resource_size_t end;
+
+		res1 = entry->res;
+		if (!(res1->flags & type))
+			goto next;
+
+		/* Exclude non-addressable range or non-addressable portion */
+		end = min(res1->end, root->end);
+		if (end <= res1->start) {
+			dev_info(dev, "host bridge window %pR (ignored, not CPU addressable)\n",
+				 res1);
+			free = true;
+			goto next;
+		} else if (res1->end != end) {
+			dev_info(dev, "host bridge window %pR ([%#llx-%#llx] ignored, not CPU addressable)\n",
+				 res1, (unsigned long long)end + 1,
+				 (unsigned long long)res1->end);
+			res1->end = end;
+		}
+
+		resource_list_for_each_entry(entry2, resources) {
+			res2 = entry2->res;
+			if (!(res2->flags & type))
+				continue;
+
+			/*
+			 * I don't like throwing away windows because then
+			 * our resources no longer match the ACPI _CRS, but
+			 * the kernel resource tree doesn't allow overlaps.
+			 */
+			if (resource_overlaps(res1, res2)) {
+				res2->start = min(res1->start, res2->start);
+				res2->end = max(res1->end, res2->end);
+				dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
+					 res2, res1);
+				free = true;
+				goto next;
+			}
+		}
+
+next:
+		resource_list_del(entry);
+		if (free)
+			resource_list_free_entry(entry);
+		else
+			resource_list_add_tail(entry, resources);
+	}
+}
+
+int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
+{
+	int ret;
+	struct list_head *list = &info->resources;
+	struct acpi_device *device = info->bridge;
+	struct resource_entry *entry, *tmp;
+	unsigned long flags;
+
+	flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_MEM_8AND16BIT;
+	ret = acpi_dev_get_resources(device, list,
+				     acpi_dev_filter_resource_type_cb,
+				     (void *)flags);
+	if (ret < 0)
+		dev_warn(&device->dev,
+			 "failed to parse _CRS method, error code %d\n", ret);
+	else if (ret == 0)
+		dev_dbg(&device->dev,
+			"no IO and memory resources present in _CRS\n");
+	else {

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 751

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+		resource_list_for_each_entry_safe(entry, tmp, list) {
+			if (entry->res->flags & IORESOURCE_DISABLED)
+				resource_list_destroy_entry(entry);
+			else
+				entry->res->name = info->name;
+		}
+		acpi_pci_root_validate_resources(&device->dev, list,
+						 IORESOURCE_MEM);
+		acpi_pci_root_validate_resources(&device->dev, list,
+						 IORESOURCE_IO);

It is not clear to me why we need these two calls above ^^^. We are using pci_acpi_root_add_resources(info) later. Is it not enough?

Also, I cannot use acpi_pci_probe_root_resources() in my ARM64 PCI driver. It is because acpi_dev_get_resources is adding translation_offset to IO ranges start address and then:
acpi_pci_root_validate_resources(&device->dev, list,

Quoted 5 lines

				 IORESOURCE_IO);

rejects that IO regions as it is out of my 0x0-SZ_16M window.

Does acpi_pci_probe_root_resources meant to be x86 specific and I should avoid using it?

IIUC, you _have_ to have the proper translation_offset to map the bridge window into the IO address space:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348708.html

Then, using the offset, you should do something ia64 does, namely, retrieve the CPU address corresponding to IO space (see arch/ia64/pci/pci.c
- add_io_space()) and map it in the physical address space by using pci_remap_iospace(), it is similar to what we have to do with DT.

It is extremely confusing and I am not sure I got it right myself, I am still grokking ia64 code to understand what it really does.

So basically, the IO bridge window coming from acpi_dev_get_resource() should represent the IO space in 0 - 16M, IIUC.

By using the offset (that was initialized using translation_offset) and the resource->start, you can retrieve the cpu address that you need to actually map the IO space, since that's what we do on ARM (ie the IO resource is an offset into the virtual address space set aside for IO).

Confusing, to say the least. Jiang, did I get it right ?

Lorenzo

Quoted 127 lines · Lorenzo Pieralisi

Quoted 1 line · Tomasz Nowicki

On 14.10.2015 08:29, Jiang Liu wrote:

[...]

Quoted 100 lines

Quoted 82 lines

drivers/acpi/pci_root.c +82 -0

+static void acpi_pci_root_validate_resources(struct device *dev,
+					     struct list_head *resources,
+					     unsigned long type)
+{
+	LIST_HEAD(list);
+	struct resource *res1, *res2, *root = NULL;
+	struct resource_entry *tmp, *entry, *entry2;
+
+	BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
+	root = (type & IORESOURCE_MEM) ? &iomem_resource : &ioport_resource;
+
+	list_splice_init(resources, &list);
+	resource_list_for_each_entry_safe(entry, tmp, &list) {
+		bool free = false;
+		resource_size_t end;
+
+		res1 = entry->res;
+		if (!(res1->flags & type))
+			goto next;
+
+		/* Exclude non-addressable range or non-addressable portion */
+		end = min(res1->end, root->end);
+		if (end <= res1->start) {
+			dev_info(dev, "host bridge window %pR (ignored, not CPU addressable)\n",
+				 res1);
+			free = true;
+			goto next;
+		} else if (res1->end != end) {
+			dev_info(dev, "host bridge window %pR ([%#llx-%#llx] ignored, not CPU addressable)\n",
+				 res1, (unsigned long long)end + 1,
+				 (unsigned long long)res1->end);
+			res1->end = end;
+		}
+
+		resource_list_for_each_entry(entry2, resources) {
+			res2 = entry2->res;
+			if (!(res2->flags & type))
+				continue;
+
+			/*
+			 * I don't like throwing away windows because then
+			 * our resources no longer match the ACPI _CRS, but
+			 * the kernel resource tree doesn't allow overlaps.
+			 */
+			if (resource_overlaps(res1, res2)) {
+				res2->start = min(res1->start, res2->start);
+				res2->end = max(res1->end, res2->end);
+				dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
+					 res2, res1);
+				free = true;
+				goto next;
+			}
+		}
+
+next:
+		resource_list_del(entry);
+		if (free)
+			resource_list_free_entry(entry);
+		else
+			resource_list_add_tail(entry, resources);
+	}
+}
+
+int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
+{
+	int ret;
+	struct list_head *list = &info->resources;
+	struct acpi_device *device = info->bridge;
+	struct resource_entry *entry, *tmp;
+	unsigned long flags;
+
+	flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_MEM_8AND16BIT;
+	ret = acpi_dev_get_resources(device, list,
+				     acpi_dev_filter_resource_type_cb,
+				     (void *)flags);
+	if (ret < 0)
+		dev_warn(&device->dev,
+			 "failed to parse _CRS method, error code %d\n", ret);
+	else if (ret == 0)
+		dev_dbg(&device->dev,
+			"no IO and memory resources present in _CRS\n");
+	else {

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 751

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+		resource_list_for_each_entry_safe(entry, tmp, list) {
+			if (entry->res->flags & IORESOURCE_DISABLED)
+				resource_list_destroy_entry(entry);
+			else
+				entry->res->name = info->name;
+		}
+		acpi_pci_root_validate_resources(&device->dev, list,
+						 IORESOURCE_MEM);
+		acpi_pci_root_validate_resources(&device->dev, list,
+						 IORESOURCE_IO);

It is not clear to me why we need these two calls above ^^^. We are using pci_acpi_root_add_resources(info) later. Is it not enough?

Also, I cannot use acpi_pci_probe_root_resources() in my ARM64 PCI driver. It is because acpi_dev_get_resources is adding translation_offset to IO ranges start address and then:
acpi_pci_root_validate_resources(&device->dev, list,

Quoted 5 lines

				 IORESOURCE_IO);

rejects that IO regions as it is out of my 0x0-SZ_16M window.

Does acpi_pci_probe_root_resources meant to be x86 specific and I should avoid using it?

IIUC, you _have_ to have the proper translation_offset to map the bridge window into the IO address space:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348708.html

Then, using the offset, you should do something ia64 does, namely, retrieve the CPU address corresponding to IO space (see arch/ia64/pci/pci.c
- add_io_space()) and map it in the physical address space by using pci_remap_iospace(), it is similar to what we have to do with DT.

It is extremely confusing and I am not sure I got it right myself, I am still grokking ia64 code to understand what it really does.

So basically, the IO bridge window coming from acpi_dev_get_resource() should represent the IO space in 0 - 16M, IIUC.

Quoted 7 lines

By using the offset (that was initialized using translation_offset) and the resource->start, you can retrieve the cpu address that you need to actually map the IO space, since that's what we do on ARM (ie the IO resource is an offset into the virtual address space set aside for IO).

Confusing, to say the least. Jiang, did I get it right ?

Hi Lorenzo and Tomasz,
With a cup of coffee, I got myself awake eventually:)
Now we are going to talk about IO port on IA64, really a little complex:( Actually there are two types of translation.
1) A PCI domain has a 24-bit IO port address space, there may be multiple IO port address spaces in systems with multiple PCI domains. So the first type of translation is to translate domain specific IO port address into system global IO port address (iomem_resource) by
res->start = acpi_des->start + acpi_des->translation_offset

2) IA64 needs to map IO port address spaces into MMIO address space because it has no instructions to access IO ports directly. So IA64 has reserved a MMIO range to map IO port address spaces. This type of translation relies on architecture specific information instead of ACPI descriptors.

On the other hand, ACPI specification has defined "I/O to Memory Translation" flag and "Memory to I/O Translation" flag in ACPI Extended Address Space Descriptor,

doesn't really support such a use case. So we need to find a way out here. Could you please help to provide more information about PCI host bridge resource descriptor implementation details on ARM64?

Quoted 160 lines · Jiang Liu

Quoted 135 lines · Lorenzo Pieralisi

Quoted 1 line · Tomasz Nowicki

On 14.10.2015 08:29, Jiang Liu wrote:

[...]

Quoted 106 lines

Quoted 88 lines

drivers/acpi/pci_root.c +10 -0

+static void acpi_pci_root_validate_resources(struct device *dev,
+                         struct list_head *resources,
+                         unsigned long type)
+{
+    LIST_HEAD(list);
+    struct resource *res1, *res2, *root = NULL;
+    struct resource_entry *tmp, *entry, *entry2;
+
+    BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
+    root = (type & IORESOURCE_MEM) ? &iomem_resource :

&ioport_resource;

drivers/acpi/pci_root.c +11 -0

+
+    list_splice_init(resources, &list);
+    resource_list_for_each_entry_safe(entry, tmp, &list) {
+        bool free = false;
+        resource_size_t end;
+
+        res1 = entry->res;
+        if (!(res1->flags & type))
+            goto next;
+
+        /* Exclude non-addressable range or non-addressable

portion */

drivers/acpi/pci_root.c +3 -0

+        end = min(res1->end, root->end);
+        if (end <= res1->start) {
+            dev_info(dev, "host bridge window %pR (ignored, not

CPU addressable)\n",

drivers/acpi/pci_root.c +5 -0

+                 res1);
+            free = true;
+            goto next;
+        } else if (res1->end != end) {
+            dev_info(dev, "host bridge window %pR ([%#llx-%#llx]

ignored, not CPU addressable)\n",

drivers/acpi/pci_root.c +19 -0

+                 res1, (unsigned long long)end + 1,
+                 (unsigned long long)res1->end);
+            res1->end = end;
+        }
+
+        resource_list_for_each_entry(entry2, resources) {
+            res2 = entry2->res;
+            if (!(res2->flags & type))
+                continue;
+
+            /*
+             * I don't like throwing away windows because then
+             * our resources no longer match the ACPI _CRS, but
+             * the kernel resource tree doesn't allow overlaps.
+             */
+            if (resource_overlaps(res1, res2)) {
+                res2->start = min(res1->start, res2->start);
+                res2->end = max(res1->end, res2->end);
+                dev_info(dev, "host bridge window expanded to %pR;

%pR ignored\n",

drivers/acpi/pci_root.c +24 -0

+                     res2, res1);
+                free = true;
+                goto next;
+            }
+        }
+
+next:
+        resource_list_del(entry);
+        if (free)
+            resource_list_free_entry(entry);
+        else
+            resource_list_add_tail(entry, resources);
+    }
+}
+
+int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
+{
+    int ret;
+    struct list_head *list = &info->resources;
+    struct acpi_device *device = info->bridge;
+    struct resource_entry *entry, *tmp;
+    unsigned long flags;
+
+    flags = IORESOURCE_IO | IORESOURCE_MEM |

IORESOURCE_MEM_8AND16BIT;

drivers/acpi/pci_root.c +10 -0

+    ret = acpi_dev_get_resources(device, list,
+                     acpi_dev_filter_resource_type_cb,
+                     (void *)flags);
+    if (ret < 0)
+        dev_warn(&device->dev,
+             "failed to parse _CRS method, error code %d\n", ret);
+    else if (ret == 0)
+        dev_dbg(&device->dev,
+            "no IO and memory resources present in _CRS\n");
+    else {

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 751

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+        resource_list_for_each_entry_safe(entry, tmp, list) {
+            if (entry->res->flags & IORESOURCE_DISABLED)
+                resource_list_destroy_entry(entry);
+            else
+                entry->res->name = info->name;
+        }
+        acpi_pci_root_validate_resources(&device->dev, list,
+                         IORESOURCE_MEM);
+        acpi_pci_root_validate_resources(&device->dev, list,
+                         IORESOURCE_IO);

It is not clear to me why we need these two calls above ^^^. We are using pci_acpi_root_add_resources(info) later. Is it not enough?

Also, I cannot use acpi_pci_probe_root_resources() in my ARM64 PCI driver. It is because acpi_dev_get_resources is adding translation_offset to IO ranges start address and then:
acpi_pci_root_validate_resources(&device->dev, list,

Quoted 5 lines

                 IORESOURCE_IO);

rejects that IO regions as it is out of my 0x0-SZ_16M window.

Does acpi_pci_probe_root_resources meant to be x86 specific and I should avoid using it?

IIUC, you _have_ to have the proper translation_offset to map the bridge window into the IO address space:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348708.html

Then, using the offset, you should do something ia64 does, namely, retrieve the CPU address corresponding to IO space (see arch/ia64/pci/pci.c
- add_io_space()) and map it in the physical address space by using pci_remap_iospace(), it is similar to what we have to do with DT.

It is extremely confusing and I am not sure I got it right myself, I am still grokking ia64 code to understand what it really does.

So basically, the IO bridge window coming from acpi_dev_get_resource() should represent the IO space in 0 - 16M, IIUC.

Quoted 7 lines

By using the offset (that was initialized using translation_offset) and the resource->start, you can retrieve the cpu address that you need to actually map the IO space, since that's what we do on ARM (ie the IO resource is an offset into the virtual address space set aside for IO).

Confusing, to say the least. Jiang, did I get it right ?

Hi Lorenzo and Tomasz,
With a cup of coffee, I got myself awake eventually:)
Now we are going to talk about IO port on IA64, really a little complex:( Actually there are two types of translation.
1) A PCI domain has a 24-bit IO port address space, there may be multiple IO port address spaces in systems with multiple PCI domains. So the first type of translation is to translate domain specific IO port address into system global IO port address (iomem_resource) by
res->start = acpi_des->start + acpi_des->translation_offset

2) IA64 needs to map IO port address spaces into MMIO address space because it has no instructions to access IO ports directly. So IA64 has reserved a MMIO range to map IO port address spaces. This type of translation relies on architecture specific information instead of ACPI descriptors.

Quoted 3 lines

On the other hand, ACPI specification has defined "I/O to Memory Translation" flag and "Memory to I/O Translation" flag in ACPI Extended Address Space Descriptor,

Based on 2) and above, does it mean IA64 should use "ACPI Extended Address Space Descriptor" for its PCI bridge IO windows only?

but current implementation

Quoted 5 lines

doesn't really support such a use case. So we need to find a way out here. Could you please help to provide more information about PCI host bridge resource descriptor implementation details on ARM64?

Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

         0x00000000,         // Granularity
         0x00000000,         // Range Minimum
         0x0000FFFF,         // Range Maximum
         0x3EFF0000,         // Translation Offset
         0x00010000,         // Length

,, , TypeStatic)

You can also have a look at my implementation example in mail to Lorenzo.

Thanks,
Tomasz

Quoted 175 lines · Tomasz Nowicki

Quoted 160 lines · Jiang Liu

Quoted 135 lines · Lorenzo Pieralisi

Quoted 1 line · Tomasz Nowicki

On 14.10.2015 08:29, Jiang Liu wrote:

[...]

Quoted 106 lines

Quoted 88 lines

drivers/acpi/pci_root.c +10 -0

+static void acpi_pci_root_validate_resources(struct device *dev,
+                         struct list_head *resources,
+                         unsigned long type)
+{
+    LIST_HEAD(list);
+    struct resource *res1, *res2, *root = NULL;
+    struct resource_entry *tmp, *entry, *entry2;
+
+    BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
+    root = (type & IORESOURCE_MEM) ? &iomem_resource :

&ioport_resource;

drivers/acpi/pci_root.c +11 -0

+
+    list_splice_init(resources, &list);
+    resource_list_for_each_entry_safe(entry, tmp, &list) {
+        bool free = false;
+        resource_size_t end;
+
+        res1 = entry->res;
+        if (!(res1->flags & type))
+            goto next;
+
+        /* Exclude non-addressable range or non-addressable

portion */

drivers/acpi/pci_root.c +3 -0

+        end = min(res1->end, root->end);
+        if (end <= res1->start) {
+            dev_info(dev, "host bridge window %pR (ignored, not

CPU addressable)\n",

drivers/acpi/pci_root.c +5 -0

+                 res1);
+            free = true;
+            goto next;
+        } else if (res1->end != end) {
+            dev_info(dev, "host bridge window %pR ([%#llx-%#llx]

ignored, not CPU addressable)\n",

drivers/acpi/pci_root.c +19 -0

+                 res1, (unsigned long long)end + 1,
+                 (unsigned long long)res1->end);
+            res1->end = end;
+        }
+
+        resource_list_for_each_entry(entry2, resources) {
+            res2 = entry2->res;
+            if (!(res2->flags & type))
+                continue;
+
+            /*
+             * I don't like throwing away windows because then
+             * our resources no longer match the ACPI _CRS, but
+             * the kernel resource tree doesn't allow overlaps.
+             */
+            if (resource_overlaps(res1, res2)) {
+                res2->start = min(res1->start, res2->start);
+                res2->end = max(res1->end, res2->end);
+                dev_info(dev, "host bridge window expanded to %pR;

%pR ignored\n",

drivers/acpi/pci_root.c +24 -0

+                     res2, res1);
+                free = true;
+                goto next;
+            }
+        }
+
+next:
+        resource_list_del(entry);
+        if (free)
+            resource_list_free_entry(entry);
+        else
+            resource_list_add_tail(entry, resources);
+    }
+}
+
+int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
+{
+    int ret;
+    struct list_head *list = &info->resources;
+    struct acpi_device *device = info->bridge;
+    struct resource_entry *entry, *tmp;
+    unsigned long flags;
+
+    flags = IORESOURCE_IO | IORESOURCE_MEM |

IORESOURCE_MEM_8AND16BIT;

drivers/acpi/pci_root.c +10 -0

+    ret = acpi_dev_get_resources(device, list,
+                     acpi_dev_filter_resource_type_cb,
+                     (void *)flags);
+    if (ret < 0)
+        dev_warn(&device->dev,
+             "failed to parse _CRS method, error code %d\n", ret);
+    else if (ret == 0)
+        dev_dbg(&device->dev,
+            "no IO and memory resources present in _CRS\n");
+    else {

drivers/acpi/pci_root.c static void acpi_pci_root_remove(struct acpi_device *device) · line 751

Quoted 10 lines

drivers/acpi/pci_root.c +10 -0

+        resource_list_for_each_entry_safe(entry, tmp, list) {
+            if (entry->res->flags & IORESOURCE_DISABLED)
+                resource_list_destroy_entry(entry);
+            else
+                entry->res->name = info->name;
+        }
+        acpi_pci_root_validate_resources(&device->dev, list,
+                         IORESOURCE_MEM);
+        acpi_pci_root_validate_resources(&device->dev, list,
+                         IORESOURCE_IO);

It is not clear to me why we need these two calls above ^^^. We are using pci_acpi_root_add_resources(info) later. Is it not enough?

Also, I cannot use acpi_pci_probe_root_resources() in my ARM64 PCI driver. It is because acpi_dev_get_resources is adding translation_offset to IO ranges start address and then:
acpi_pci_root_validate_resources(&device->dev, list,

Quoted 5 lines

                  IORESOURCE_IO);

rejects that IO regions as it is out of my 0x0-SZ_16M window.

Does acpi_pci_probe_root_resources meant to be x86 specific and I should avoid using it?

IIUC, you _have_ to have the proper translation_offset to map the bridge window into the IO address space:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348708.html

Then, using the offset, you should do something ia64 does, namely, retrieve the CPU address corresponding to IO space (see arch/ia64/pci/pci.c
- add_io_space()) and map it in the physical address space by using pci_remap_iospace(), it is similar to what we have to do with DT.

It is extremely confusing and I am not sure I got it right myself, I am still grokking ia64 code to understand what it really does.

So basically, the IO bridge window coming from acpi_dev_get_resource() should represent the IO space in 0 - 16M, IIUC.

Quoted 7 lines

By using the offset (that was initialized using translation_offset) and the resource->start, you can retrieve the cpu address that you need to actually map the IO space, since that's what we do on ARM (ie the IO resource is an offset into the virtual address space set aside for IO).

Confusing, to say the least. Jiang, did I get it right ?

Hi Lorenzo and Tomasz,
With a cup of coffee, I got myself awake eventually:)
Now we are going to talk about IO port on IA64, really a little complex:( Actually there are two types of translation.
1) A PCI domain has a 24-bit IO port address space, there may be multiple IO port address spaces in systems with multiple PCI domains. So the first type of translation is to translate domain specific IO port address into system global IO port address (iomem_resource) by

    res->start = acpi_des->start + acpi_des->translation_offset

2) IA64 needs to map IO port address spaces into MMIO address space because it has no instructions to access IO ports directly. So IA64 has reserved a MMIO range to map IO port address spaces. This type of translation relies on architecture specific information instead of ACPI descriptors.

Quoted 3 lines

On the other hand, ACPI specification has defined "I/O to Memory Translation" flag and "Memory to I/O Translation" flag in ACPI Extended Address Space Descriptor,

Based on 2) and above, does it mean IA64 should use "ACPI Extended Address Space Descriptor" for its PCI bridge IO windows only?

but current implementation

Quoted 5 lines

doesn't really support such a use case. So we need to find a way out here. Could you please help to provide more information about PCI host bridge resource descriptor implementation details on ARM64?

Quoted 8 lines

Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

          0x00000000,         // Granularity
          0x00000000,         // Range Minimum
          0x0000FFFF,         // Range Maximum
          0x3EFF0000,         // Translation Offset
          0x00010000,         // Length

,, , TypeStatic)

The above DWordIO resource descriptor doesn't confirm to the ACPI spec. According to my understanding, ARM/ARM64 has no concept of IO port address space, so the PCI host bridge will map IO port on PCI side onto MMIO on host side. In other words, PCI host bridge on ARM64 implement a IO Port->MMIO translation instead of a IO Port->IO Port translation. If that's true, it should use 'TypeTranslation' instead of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't support 'TypeTranslation' yet, so we need to find a solution for it.

Quoted 10 lines · Jiang Liu

Quoted 9 lines · Tomasz Nowicki

On 06.11.2015 09:52, Jiang Liu wrote:
Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

          0x00000000,         // Granularity
          0x00000000,         // Range Minimum
          0x0000FFFF,         // Range Maximum
          0x3EFF0000,         // Translation Offset
          0x00010000,         // Length

,, , TypeStatic)

Quoted 8 lines

The above DWordIO resource descriptor doesn't confirm to the ACPI spec. According to my understanding, ARM/ARM64 has no concept of IO port address space, so the PCI host bridge will map IO port on PCI side onto MMIO on host side. In other words, PCI host bridge on ARM64 implement a IO Port->MMIO translation instead of a IO Port->IO Port translation. If that's true, it should use 'TypeTranslation' instead of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't support 'TypeTranslation' yet, so we need to find a solution for it.

I think you are right, we need TypeTranslation flag for ARM64 DWordIO descriptors and an extra kernel patch to support it.

Thanks,
Tomasz
From 51f5cddd8c4301b731805074ebc3e3a6c7dbaf59 Mon Sep 17 00:00:00 2001

Quoted 20 lines · Tomasz Nowicki

Quoted 18 lines · Jiang Liu

Quoted 9 lines · Tomasz Nowicki

On 06.11.2015 09:52, Jiang Liu wrote:
Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

          0x00000000,         // Granularity
          0x00000000,         // Range Minimum
          0x0000FFFF,         // Range Maximum
          0x3EFF0000,         // Translation Offset
          0x00010000,         // Length

,, , TypeStatic)

The above DWordIO resource descriptor doesn't confirm to the ACPI spec. According to my understanding, ARM/ARM64 has no concept of IO port address space, so the PCI host bridge will map IO port on PCI side onto MMIO on host side. In other words, PCI host bridge on ARM64 implement a IO Port->MMIO translation instead of a IO Port->IO Port translation. If that's true, it should use 'TypeTranslation' instead of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't support 'TypeTranslation' yet, so we need to find a solution for it.

Quoted 2 lines

I think you are right, we need TypeTranslation flag for ARM64 DWordIO descriptors and an extra kernel patch to support it.

How about the attached to patch to support TypeTranslation? It only passes compilation:)

Quoted 21 lines · Jiang Liu

Quoted 20 lines · Tomasz Nowicki

Quoted 18 lines · Jiang Liu

Quoted 9 lines · Tomasz Nowicki

On 06.11.2015 09:52, Jiang Liu wrote:
Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

          0x00000000,         // Granularity
          0x00000000,         // Range Minimum
          0x0000FFFF,         // Range Maximum
          0x3EFF0000,         // Translation Offset
          0x00010000,         // Length

,, , TypeStatic)

The above DWordIO resource descriptor doesn't confirm to the ACPI spec. According to my understanding, ARM/ARM64 has no concept of IO port address space, so the PCI host bridge will map IO port on PCI side onto MMIO on host side. In other words, PCI host bridge on ARM64 implement a IO Port->MMIO translation instead of a IO Port->IO Port translation. If that's true, it should use 'TypeTranslation' instead of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't support 'TypeTranslation' yet, so we need to find a solution for it.

Quoted 4 lines

Quoted 2 lines

I think you are right, we need TypeTranslation flag for ARM64 DWordIO descriptors and an extra kernel patch to support it.

How about the attached to patch to support TypeTranslation? It only passes compilation:)

Eh, hopefully there are not any ACPI tables out there with that bit set that work _today_ and would not work with the patch attached :)

My question is still there: do we want to handle the same problem as ia64 has in a different manner ? Certainly we won't be able to update ia64 platforms ACPI tables, so we would end up with two platforms handling IO resources in different ways unless I am missing something here.

BTW, why would we add offset to res->start only if TypeTranslation is clear ? Is not that something we would do just to make things "work" ? That flag has no bearing on the offset, only on the resource type AFAIK.

This without taking into account ARM64 systems shipping with ACPI tables that does not set the TypeTranslation at present.

On top of that, I noticed that core ACPI code handles Sparse Translation (ie _TRS), that should be considered meaningful only if _TTP is set (and that's not checked).

Thoughts ?

Thanks,
Lorenzo

Quoted 27 lines · Lorenzo Pieralisi

Quoted 21 lines · Jiang Liu

Quoted 20 lines · Tomasz Nowicki

Quoted 18 lines · Jiang Liu

Quoted 9 lines · Tomasz Nowicki

On 06.11.2015 09:52, Jiang Liu wrote:
Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

          0x00000000,         // Granularity
          0x00000000,         // Range Minimum
          0x0000FFFF,         // Range Maximum
          0x3EFF0000,         // Translation Offset
          0x00010000,         // Length

,, , TypeStatic)

The above DWordIO resource descriptor doesn't confirm to the ACPI spec. According to my understanding, ARM/ARM64 has no concept of IO port address space, so the PCI host bridge will map IO port on PCI side onto MMIO on host side. In other words, PCI host bridge on ARM64 implement a IO Port->MMIO translation instead of a IO Port->IO Port translation. If that's true, it should use 'TypeTranslation' instead of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't support 'TypeTranslation' yet, so we need to find a solution for it.

Quoted 4 lines

Quoted 2 lines

I think you are right, we need TypeTranslation flag for ARM64 DWordIO descriptors and an extra kernel patch to support it.

How about the attached to patch to support TypeTranslation? It only passes compilation:)

Quoted 8 lines

Eh, hopefully there are not any ACPI tables out there with that bit set that work _today_ and would not work with the patch attached :)

My question is still there: do we want to handle the same problem as ia64 has in a different manner ? Certainly we won't be able to update ia64 platforms ACPI tables, so we would end up with two platforms handling IO resources in different ways unless I am missing something here.

There are some difference between IA64 and ARM64.
On IA64, it supports 16M IO address space per PCI domain and 256 PCI domains at max. So the system IO address space is 16M * 256 = 4G. So it does two level translations to support IO port
1) translate PCI bus local IO port address into system global IO port address by adding acpi_des->translation_offset.
2) translate the 4G system IO port address space into MMIO address. IA64 has reserved a 4G space for IO port mapping. This translation is done by arch specific method.
In other word, IA64 needs two level translation, but ACPI only provides on (trans_type, trans_offset) pair for encoding, so it's used for step 1).

For ARM64, I think currently it only needs step 2).

Quoted 4 lines

BTW, why would we add offset to res->start only if TypeTranslation is clear ? Is not that something we would do just to make things "work" ? That flag has no bearing on the offset, only on the resource type AFAIK.

It's not a hack, but a way to interpret ACPI spec:)

With current linux resource management framework, we need to allocate both MMIO and IO port address space range for an ACPI resource of type 'TypeTranslation'. And struct resource could be either IO port or MMIO, not both. So the choice is to keep the resource as IO port, and let arch code to build the special MMIO mapping for it. Otherwise it will break too many things if we convert the resource as MMIO.

That said, we need to add translation_offset to convert bus local IO port address into system global IO port address if it's type of TypeStatic, because ioresource_ioport uses system global IO port address.

For an ACPI resource of type TypeTranslation, system global IO port address equals bus local IO port address, and the translation_offset is used to translate IO port address into MMIO address, so we shouldn't add translation_offset to the IO port resource descriptor.

Thanks,
Gerry

Quoted 7 lines

This without taking into account ARM64 systems shipping with ACPI tables that does not set the TypeTranslation at present.

On top of that, I noticed that core ACPI code handles Sparse Translation (ie _TRS), that should be considered meaningful only if _TTP is set (and that's not checked).

Yes, that's a flaw:(

Quoted 6 lines

Thoughts ?

Thanks,
Lorenzo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 68 lines · Jiang Liu

Quoted 27 lines · Lorenzo Pieralisi

Quoted 21 lines · Jiang Liu

Quoted 20 lines · Tomasz Nowicki

Quoted 18 lines · Jiang Liu

Quoted 9 lines · Tomasz Nowicki

On 06.11.2015 09:52, Jiang Liu wrote:
Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

          0x00000000,         // Granularity
          0x00000000,         // Range Minimum
          0x0000FFFF,         // Range Maximum
          0x3EFF0000,         // Translation Offset
          0x00010000,         // Length

,, , TypeStatic)

The above DWordIO resource descriptor doesn't confirm to the ACPI spec. According to my understanding, ARM/ARM64 has no concept of IO port address space, so the PCI host bridge will map IO port on PCI side onto MMIO on host side. In other words, PCI host bridge on ARM64 implement a IO Port->MMIO translation instead of a IO Port->IO Port translation. If that's true, it should use 'TypeTranslation' instead of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't support 'TypeTranslation' yet, so we need to find a solution for it.

Quoted 4 lines

Quoted 2 lines

I think you are right, we need TypeTranslation flag for ARM64 DWordIO descriptors and an extra kernel patch to support it.

How about the attached to patch to support TypeTranslation? It only passes compilation:)

Quoted 8 lines

Eh, hopefully there are not any ACPI tables out there with that bit set that work _today_ and would not work with the patch attached :)

My question is still there: do we want to handle the same problem as ia64 has in a different manner ? Certainly we won't be able to update ia64 platforms ACPI tables, so we would end up with two platforms handling IO resources in different ways unless I am missing something here.

There are some difference between IA64 and ARM64.
On IA64, it supports 16M IO address space per PCI domain and 256 PCI domains at max. So the system IO address space is 16M * 256 = 4G. So it does two level translations to support IO port
1) translate PCI bus local IO port address into system global IO port address by adding acpi_des->translation_offset.
2) translate the 4G system IO port address space into MMIO address. IA64 has reserved a 4G space for IO port mapping. This translation is done by arch specific method.
In other word, IA64 needs two level translation, but ACPI only provides on (trans_type, trans_offset) pair for encoding, so it's used for step 1).

For ARM64, I think currently it only needs step 2).

Quoted 4 lines

BTW, why would we add offset to res->start only if TypeTranslation is clear ? Is not that something we would do just to make things "work" ? That flag has no bearing on the offset, only on the resource type AFAIK.

It's not a hack, but a way to interpret ACPI spec:)

With current linux resource management framework, we need to allocate both MMIO and IO port address space range for an ACPI resource of type 'TypeTranslation'. And struct resource could be either IO port or MMIO, not both. So the choice is to keep the resource as IO port, and let arch code to build the special MMIO mapping for it. Otherwise it will break too many things if we convert the resource as MMIO.

That said, we need to add translation_offset to convert bus local IO port address into system global IO port address if it's type of TypeStatic, because ioresource_ioport uses system global IO port address.

Quoted 4 lines

For an ACPI resource of type TypeTranslation, system global IO port address equals bus local IO port address, and the translation_offset is used to translate IO port address into MMIO address, so we shouldn't add translation_offset to the IO port resource descriptor.

One note for the TypeTranslation case, the arch code needs to reset resource_win->offset to zero after setting up the MMIO map. Sample code as below:
va = ioremap(resource_win->offset + res->start, resource_size(res)); resource_win->offset = 0;

Otherwise it will break pcibios_resource_to_bus() etc.

Quoted 16 lines

Thanks,
Gerry

Quoted 7 lines

This without taking into account ARM64 systems shipping with ACPI tables that does not set the TypeTranslation at present.

On top of that, I noticed that core ACPI code handles Sparse Translation (ie _TRS), that should be considered meaningful only if _TTP is set (and that's not checked).

Yes, that's a flaw:(

Quoted 3 lines

Thoughts ?

Quoted 9 lines

Quoted 3 lines

Thanks,
Lorenzo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 20 lines · Tomasz Nowicki

Quoted 18 lines · Jiang Liu

Quoted 9 lines · Tomasz Nowicki

On 06.11.2015 09:52, Jiang Liu wrote:
Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

           0x00000000,         // Granularity
           0x00000000,         // Range Minimum
           0x0000FFFF,         // Range Maximum
           0x3EFF0000,         // Translation Offset
           0x00010000,         // Length

,, , TypeStatic)

The above DWordIO resource descriptor doesn't confirm to the ACPI spec. According to my understanding, ARM/ARM64 has no concept of IO port address space, so the PCI host bridge will map IO port on PCI side onto MMIO on host side. In other words, PCI host bridge on ARM64 implement a IO Port->MMIO translation instead of a IO Port->IO Port translation. If that's true, it should use 'TypeTranslation' instead of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't support 'TypeTranslation' yet, so we need to find a solution for it.

Quoted 2 lines

I think you are right, we need TypeTranslation flag for ARM64 DWordIO descriptors and an extra kernel patch to support it.

How about the attached to patch to support TypeTranslation? It only passes compilation:)

Quoted 21 lines · Jiang Liu

Quoted 20 lines · Tomasz Nowicki

Quoted 18 lines · Jiang Liu

Quoted 9 lines · Tomasz Nowicki

On 06.11.2015 09:52, Jiang Liu wrote:
Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

          0x00000000,         // Granularity
          0x00000000,         // Range Minimum
          0x0000FFFF,         // Range Maximum
          0x3EFF0000,         // Translation Offset
          0x00010000,         // Length

,, , TypeStatic)

The above DWordIO resource descriptor doesn't confirm to the ACPI spec. According to my understanding, ARM/ARM64 has no concept of IO port address space, so the PCI host bridge will map IO port on PCI side onto MMIO on host side. In other words, PCI host bridge on ARM64 implement a IO Port->MMIO translation instead of a IO Port->IO Port translation. If that's true, it should use 'TypeTranslation' instead of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't support 'TypeTranslation' yet, so we need to find a solution for it.

Quoted 4 lines

Quoted 2 lines

I think you are right, we need TypeTranslation flag for ARM64 DWordIO descriptors and an extra kernel patch to support it.

How about the attached to patch to support TypeTranslation? It only passes compilation:)

Based on the further discussion, your draft patch looks good to me. Lorenzo, do you agree?

Quoted 22 lines · Tomasz Nowicki

Quoted 21 lines · Jiang Liu

Quoted 20 lines · Tomasz Nowicki

Quoted 18 lines · Jiang Liu

Quoted 9 lines · Tomasz Nowicki

On 06.11.2015 09:52, Jiang Liu wrote:
Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

          0x00000000,         // Granularity
          0x00000000,         // Range Minimum
          0x0000FFFF,         // Range Maximum
          0x3EFF0000,         // Translation Offset
          0x00010000,         // Length

,, , TypeStatic)

The above DWordIO resource descriptor doesn't confirm to the ACPI spec. According to my understanding, ARM/ARM64 has no concept of IO port address space, so the PCI host bridge will map IO port on PCI side onto MMIO on host side. In other words, PCI host bridge on ARM64 implement a IO Port->MMIO translation instead of a IO Port->IO Port translation. If that's true, it should use 'TypeTranslation' instead of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't support 'TypeTranslation' yet, so we need to find a solution for it.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

I think you are right, we need TypeTranslation flag for ARM64 DWordIO descriptors and an extra kernel patch to support it.

How about the attached to patch to support TypeTranslation? It only passes compilation:)

Based on the further discussion, your draft patch looks good to me. Lorenzo, do you agree?

No, because I still do not understand the difference between ia64 and arm64 (they both drive IO ports cycles through MMIO so the resource descriptors content must be the same or better they must mean the same thing). On top of that, this is something that was heavily debated for DT:

http://www.spinics.net/lists/arm-kernel/msg345633.html

and I would like to get Arnd and Bjorn opinion on this because we should not "interpret" ACPI specifications, we should understand what they are supposed to describe and write kernel code accordingly.

In particular, I would like to understand, for an eg DWordIO descriptor, what Range Minimum, Range Maximum and Translation Offset represent, they can't mean different things depending on the SW parsing them, this totally defeats the purpose.

By the way, ia64 ioremaps the translation_offset (ie new_space()), so basically that's the CPU physical address at which the PCI host bridge map the IO space transactions), I do not think ia64 is any different from arm64 in this respect, if it is please provide an HW description here from the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge tables would help).

Quoted 38 lines · On Monday 09 November 2015 17:10:43 Lorenzo Pieralisi wrote:

Quoted 22 lines · Tomasz Nowicki

Quoted 21 lines · Jiang Liu

Quoted 20 lines · Tomasz Nowicki

Quoted 18 lines · Jiang Liu

Quoted 9 lines · Tomasz Nowicki

On 06.11.2015 09:52, Jiang Liu wrote:
Sure, ARM64 (0-16M IO space) QEMU example:
DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,

          0x00000000,         // Granularity
          0x00000000,         // Range Minimum
          0x0000FFFF,         // Range Maximum
          0x3EFF0000,         // Translation Offset
          0x00010000,         // Length

,, , TypeStatic)

The above DWordIO resource descriptor doesn't confirm to the ACPI spec. According to my understanding, ARM/ARM64 has no concept of IO port address space, so the PCI host bridge will map IO port on PCI side onto MMIO on host side. In other words, PCI host bridge on ARM64 implement a IO Port->MMIO translation instead of a IO Port->IO Port translation. If that's true, it should use 'TypeTranslation' instead of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't support 'TypeTranslation' yet, so we need to find a solution for it.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

I think you are right, we need TypeTranslation flag for ARM64 DWordIO descriptors and an extra kernel patch to support it.

How about the attached to patch to support TypeTranslation? It only passes compilation:)

Based on the further discussion, your draft patch looks good to me. Lorenzo, do you agree?

No, because I still do not understand the difference between ia64 and arm64 (they both drive IO ports cycles through MMIO so the resource descriptors content must be the same or better they must mean the same thing). On top of that, this is something that was heavily debated for DT:

http://www.spinics.net/lists/arm-kernel/msg345633.html

Quoted 8 lines

and I would like to get Arnd and Bjorn opinion on this because we should not "interpret" ACPI specifications, we should understand what they are supposed to describe and write kernel code accordingly.

In particular, I would like to understand, for an eg DWordIO descriptor, what Range Minimum, Range Maximum and Translation Offset represent, they can't mean different things depending on the SW parsing them, this totally defeats the purpose.

I have no clue about what those mean in ACPI though.

Generally speaking, each PCI domain is expected to have a (normally 64KB) range of CPU addresses that gets translated into PCI I/O space the same way that config space and memory space are handled.
This is true for almost every architecture except for x86, which uses different CPU instructions for I/O space compared to the other spaces.

Quoted 6 lines

By the way, ia64 ioremaps the translation_offset (ie new_space()), so basically that's the CPU physical address at which the PCI host bridge map the IO space transactions), I do not think ia64 is any different from arm64 in this respect, if it is please provide an HW description here from the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge tables would help).

The main difference between ia64 and a lot of the other architectures (e.g. sparc is different again) is that ia64 defines a logical address range in terms of having a small number for each I/O space followed by the offset within that space as a 'port number' and uses a mapping function that is defined as

static inline void *__ia64_mk_io_addr (unsigned long port)

{
        struct io_space *space = &io_space[IO_SPACE_NR(port)];
        return (space->mmio_base | IO_SPACE_PORT(port););
}
static inline unsigned int inl(unsigned long port)
{
        return *__ia64_mk_io_addr(port);
}

Most architectures allow only one I/O port range and put it at a fixed virtual address so that inl() simply becomes

static inline u32 inl(unsigned long addr)

{
        return readl(PCI_IOBASE + addr);
}

which noticeably reduces code size.

On some architectures (powerpc, arm, arm64), we then get the same simplified definition with a fixed virtual address, and use pci_ioremap_io() or something like that to to map a physical address range into this virtual address window at the correct io_offset;

From 6521d3c5303b1d9830eecede4c1b778deffe17b0 Mon Sep 17 00:00:00 2001

Quoted 44 lines

Quoted 4 lines

In particular, I would like to understand, for an eg DWordIO descriptor, what Range Minimum, Range Maximum and Translation Offset represent, they can't mean different things depending on the SW parsing them, this totally defeats the purpose.

I have no clue about what those mean in ACPI though.

Generally speaking, each PCI domain is expected to have a (normally 64KB) range of CPU addresses that gets translated into PCI I/O space the same way that config space and memory space are handled.
This is true for almost every architecture except for x86, which uses different CPU instructions for I/O space compared to the other spaces.

Quoted 6 lines

By the way, ia64 ioremaps the translation_offset (ie new_space()), so basically that's the CPU physical address at which the PCI host bridge map the IO space transactions), I do not think ia64 is any different from arm64 in this respect, if it is please provide an HW description here from the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge tables would help).

The main difference between ia64 and a lot of the other architectures (e.g. sparc is different again) is that ia64 defines a logical address range in terms of having a small number for each I/O space followed by the offset within that space as a 'port number' and uses a mapping function that is defined as

static inline void *__ia64_mk_io_addr (unsigned long port)

{
        struct io_space *space = &io_space[IO_SPACE_NR(port)];
        return (space->mmio_base | IO_SPACE_PORT(port););
}
static inline unsigned int inl(unsigned long port)
{
        return *__ia64_mk_io_addr(port);
}

Most architectures allow only one I/O port range and put it at a fixed virtual address so that inl() simply becomes

static inline u32 inl(unsigned long addr)

{
        return readl(PCI_IOBASE + addr);
}

Quoted 6 lines

which noticeably reduces code size.

On some architectures (powerpc, arm, arm64), we then get the same simplified definition with a fixed virtual address, and use pci_ioremap_io() or something like that to to map a physical address range into this virtual address window at the correct io_offset;

Hi all,
Thanks for explanation, I found a way to make the ACPI resource
parsing interface arch neutral, it should help to address Lorenzo's concern. Please refer to the attached patch. (It's still RFC, not tested yet).

On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:

[...]

Quoted 50 lines

Quoted 44 lines

Quoted 4 lines

In particular, I would like to understand, for an eg DWordIO descriptor, what Range Minimum, Range Maximum and Translation Offset represent, they can't mean different things depending on the SW parsing them, this totally defeats the purpose.

I have no clue about what those mean in ACPI though.

Generally speaking, each PCI domain is expected to have a (normally 64KB) range of CPU addresses that gets translated into PCI I/O space the same way that config space and memory space are handled.
This is true for almost every architecture except for x86, which uses different CPU instructions for I/O space compared to the other spaces.

Quoted 6 lines

By the way, ia64 ioremaps the translation_offset (ie new_space()), so basically that's the CPU physical address at which the PCI host bridge map the IO space transactions), I do not think ia64 is any different from arm64 in this respect, if it is please provide an HW description here from the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge tables would help).

The main difference between ia64 and a lot of the other architectures (e.g. sparc is different again) is that ia64 defines a logical address range in terms of having a small number for each I/O space followed by the offset within that space as a 'port number' and uses a mapping function that is defined as

static inline void *__ia64_mk_io_addr (unsigned long port)

{
        struct io_space *space = &io_space[IO_SPACE_NR(port)];
        return (space->mmio_base | IO_SPACE_PORT(port););
}
static inline unsigned int inl(unsigned long port)
{
        return *__ia64_mk_io_addr(port);
}

Most architectures allow only one I/O port range and put it at a fixed virtual address so that inl() simply becomes

static inline u32 inl(unsigned long addr)

{
        return readl(PCI_IOBASE + addr);
}

Quoted 6 lines

which noticeably reduces code size.

On some architectures (powerpc, arm, arm64), we then get the same simplified definition with a fixed virtual address, and use pci_ioremap_io() or something like that to to map a physical address range into this virtual address window at the correct io_offset;

Quoted 5 lines

Hi all,
Thanks for explanation, I found a way to make the ACPI resource
parsing interface arch neutral, it should help to address Lorenzo's concern. Please refer to the attached patch. (It's still RFC, not tested yet).

If we go with this approach though, you are not adding the offset to the resource when parsing the memory spaces in acpi_decode_space(), are we sure that's what we really want ?

In DT, a host bridge range has a:

- CPU physical address
- PCI bus address

We use that to compute the offset between primary bus (ie CPU physical address) and secondary bus (ie PCI bus address).

The value ending up in the PCI resource struct (for memory space) is the CPU physical address, if you do not add the offset in acpi_decode_space that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI, am I wrong ?

Overall I think the point is related to ioport_resource and its check in acpi_pci_root_validate_resources() which basically that's the problem that started this thread.

On arm64, IO_SPACE_LIMIT is 16M, which, AFAIK is a kernel limit, not a HW one.

Comparing the resources parsed from the PCI bridge _CRS against the range 0..IO_SPACE_LIMIT is not necessarily meaningful (or at least not meaningful in its current form), on ia64 it works because IO_SPACE_LIMIT is bumped up to 4G, that's the reason why adding the offset to the ACPI IO resources work on ia64 as far as I understand.

And that's why I pulled Arnd in this discussion since he knows better than me: what does ioport_resource _really_ represent on ARM64 ? It seems to me that it is a range of IO ports values (ie a window that defines the allowed offset in the virtual address space allocated to PCI IO) that has _nothing_ to do with the CPU physical address at which the IO space is actually mapped.

To sum it up for a, say, DWordIo/Memory descriptor:

- AddressMinimum, AddressMaximum represent the PCI bus addresses defining the resource start..end
- AddressTranslation is the offset that has to be added to AddressMinimum and AddressMaximum to get the window in CPU physical address space

So:

- Either we go with the patch attached (but please check my comment on the memory spaces)
- Or we patch acpi_pci_root_validate_resources() to amend the way IORESOURCE_IO is currently checked against ioport_resource, it can't work on arm64 at present, I described why above

Thoughts appreciated it is time we got this sorted and thanks for the patch.

Thanks,
Lorenzo

On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
If we go with this approach though, you are not adding the offset to the resource when parsing the memory spaces in acpi_decode_space(), are we sure that's what we really want ?

In DT, a host bridge range has a:

- CPU physical address
- PCI bus address

We use that to compute the offset between primary bus (ie CPU physical address) and secondary bus (ie PCI bus address).

The value ending up in the PCI resource struct (for memory space) is the CPU physical address, if you do not add the offset in acpi_decode_space that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI, am I wrong ?

On arm64, IO_SPACE_LIMIT is 16M, which, AFAIK is a kernel limit, not a HW one. Comparing the resources parsed from the PCI bridge _CRS against the range 0..IO_SPACE_LIMIT is not necessarily meaningful (or at least not meaningful in its current form), on ia64 it works because IO_SPACE_LIMIT is bumped up to 4G, that's the reason why adding the offset to the ACPI IO resources work on ia64 as far as I understand.

And that's why I pulled Arnd in this discussion since he knows better than me: what does ioport_resource _really_ represent on ARM64 ? It seems to me that it is a range of IO ports values (ie a window that defines the allowed offset in the virtual address space allocated to PCI IO) that has _nothing_ to do with the CPU physical address at which the IO space is actually mapped.

To sum it up for a, say, DWordIo/Memory descriptor:

- AddressMinimum, AddressMaximum represent the PCI bus addresses defining the resource start..end
- AddressTranslation is the offset that has to be added to AddressMinimum and AddressMaximum to get the window in CPU physical address space

So:

- Either we go with the patch attached (but please check my comment on the memory spaces)
- Or we patch acpi_pci_root_validate_resources() to amend the way IORESOURCE_IO is currently checked against ioport_resource, it can't work on arm64 at present, I described why above

Thoughts appreciated it is time we got this sorted and thanks for the patch.

Quoted 13 lines · On Wednesday 11 November 2015 17:46:47 Lorenzo Pieralisi wrote:

On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:
If we go with this approach though, you are not adding the offset to the resource when parsing the memory spaces in acpi_decode_space(), are we sure that's what we really want ?

In DT, a host bridge range has a:

- CPU physical address
- PCI bus address

We use that to compute the offset between primary bus (ie CPU physical address) and secondary bus (ie PCI bus address).

Quoted 4 lines

The value ending up in the PCI resource struct (for memory space) is the CPU physical address, if you do not add the offset in acpi_decode_space that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI, am I wrong ?

Sinan Kaya pointed out that SBSA mandates a 1:1 mapping for memory space.

Quoted 9 lines

To sum it up for a, say, DWordIo/Memory descriptor:

- AddressMinimum, AddressMaximum represent the PCI bus addresses defining the resource start..end
- AddressTranslation is the offset that has to be added to AddressMinimum and AddressMaximum to get the window in CPU physical address space

So:

Quoted 8 lines

- Either we go with the patch attached (but please check my comment on the memory spaces)
- Or we patch acpi_pci_root_validate_resources() to amend the way IORESOURCE_IO is currently checked against ioport_resource, it can't work on arm64 at present, I described why above

Thoughts appreciated it is time we got this sorted and thanks for the patch.

The easiest way would be to assume that nobody is building a server system that has multiple I/O spaces. SBSA explicitly makes I/O space optional, and really the only reason anyone includes that feature these days is for initializing GPUs through the BIOS POST method, so that is not relevant for servers. Even when you do have multiple PCIe host controllers that all support I/O space, the most logical implementation would be to share one common space.

If that fails, there are still two cases you have to care about, and it really depends on what hardware makers decide to use here (and whether we have any influence over them). The easier way for us to do this is if every hardware sets up the mapping between CPU physical and PCI I/O in a way that the I/O space numbers are non-overlapping between the host controllers. That way we can still make Linux ioport_resource addresses the same as PCI I/O space addresses (using io_offset=0), with the downside being that only the first PCIe host (as enumerated by the bootloader) can have I/O space addresses below 1024 that may be required for ISA compatibility on some hardware.

On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:

[...]

Quoted 50 lines

Quoted 44 lines

Quoted 4 lines

In particular, I would like to understand, for an eg DWordIO descriptor, what Range Minimum, Range Maximum and Translation Offset represent, they can't mean different things depending on the SW parsing them, this totally defeats the purpose.

I have no clue about what those mean in ACPI though.

Generally speaking, each PCI domain is expected to have a (normally 64KB) range of CPU addresses that gets translated into PCI I/O space the same way that config space and memory space are handled.
This is true for almost every architecture except for x86, which uses different CPU instructions for I/O space compared to the other spaces.

Quoted 6 lines

By the way, ia64 ioremaps the translation_offset (ie new_space()), so basically that's the CPU physical address at which the PCI host bridge map the IO space transactions), I do not think ia64 is any different from arm64 in this respect, if it is please provide an HW description here from the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge tables would help).

The main difference between ia64 and a lot of the other architectures (e.g. sparc is different again) is that ia64 defines a logical address range in terms of having a small number for each I/O space followed by the offset within that space as a 'port number' and uses a mapping function that is defined as

static inline void *__ia64_mk_io_addr (unsigned long port)

{
        struct io_space *space = &io_space[IO_SPACE_NR(port)];
        return (space->mmio_base | IO_SPACE_PORT(port););
}
static inline unsigned int inl(unsigned long port)
{
        return *__ia64_mk_io_addr(port);
}

Most architectures allow only one I/O port range and put it at a fixed virtual address so that inl() simply becomes

static inline u32 inl(unsigned long addr)

{
        return readl(PCI_IOBASE + addr);
}

Quoted 6 lines

which noticeably reduces code size.

On some architectures (powerpc, arm, arm64), we then get the same simplified definition with a fixed virtual address, and use pci_ioremap_io() or something like that to to map a physical address range into this virtual address window at the correct io_offset;

Quoted 5 lines

Hi all,
Thanks for explanation, I found a way to make the ACPI resource
parsing interface arch neutral, it should help to address Lorenzo's concern. Please refer to the attached patch. (It's still RFC, not tested yet).

If we go with this approach though, you are not adding the offset to the resource when parsing the memory spaces in acpi_decode_space(), are we sure that's what we really want ?

In DT, a host bridge range has a:

- CPU physical address
- PCI bus address

We use that to compute the offset between primary bus (ie CPU physical address) and secondary bus (ie PCI bus address).

The value ending up in the PCI resource struct (for memory space) is the CPU physical address, if you do not add the offset in acpi_decode_space that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI, am I wrong ?

Overall I think the point is related to ioport_resource and its check in acpi_pci_root_validate_resources() which basically that's the problem that started this thread.

On arm64, IO_SPACE_LIMIT is 16M, which, AFAIK is a kernel limit, not a HW one. Comparing the resources parsed from the PCI bridge _CRS against the range 0..IO_SPACE_LIMIT is not necessarily meaningful (or at least not meaningful in its current form), on ia64 it works because IO_SPACE_LIMIT is bumped up to 4G, that's the reason why adding the offset to the ACPI IO resources work on ia64 as far as I understand.

And that's why I pulled Arnd in this discussion since he knows better than me: what does ioport_resource _really_ represent on ARM64 ? It seems to me that it is a range of IO ports values (ie a window that defines the allowed offset in the virtual address space allocated to PCI IO) that has _nothing_ to do with the CPU physical address at which the IO space is actually mapped.

To sum it up for a, say, DWordIo/Memory descriptor:

- AddressMinimum, AddressMaximum represent the PCI bus addresses defining the resource start..end
- AddressTranslation is the offset that has to be added to AddressMinimum and AddressMaximum to get the window in CPU physical address space

So:

- Either we go with the patch attached (but please check my comment on the memory spaces)
- Or we patch acpi_pci_root_validate_resources() to amend the way IORESOURCE_IO is currently checked against ioport_resource, it can't work on arm64 at present, I described why above

Thoughts appreciated it is time we got this sorted and thanks for the patch.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 69 lines · Lorenzo Pieralisi

On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:

[...]

Quoted 50 lines

Quoted 44 lines

Quoted 4 lines

In particular, I would like to understand, for an eg DWordIO descriptor, what Range Minimum, Range Maximum and Translation Offset represent, they can't mean different things depending on the SW parsing them, this totally defeats the purpose.

I have no clue about what those mean in ACPI though.

Generally speaking, each PCI domain is expected to have a (normally 64KB) range of CPU addresses that gets translated into PCI I/O space the same way that config space and memory space are handled.
This is true for almost every architecture except for x86, which uses different CPU instructions for I/O space compared to the other spaces.

Quoted 6 lines

By the way, ia64 ioremaps the translation_offset (ie new_space()), so basically that's the CPU physical address at which the PCI host bridge map the IO space transactions), I do not think ia64 is any different from arm64 in this respect, if it is please provide an HW description here from the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge tables would help).

The main difference between ia64 and a lot of the other architectures (e.g. sparc is different again) is that ia64 defines a logical address range in terms of having a small number for each I/O space followed by the offset within that space as a 'port number' and uses a mapping function that is defined as

static inline void *__ia64_mk_io_addr (unsigned long port)

{
         struct io_space *space = &io_space[IO_SPACE_NR(port)];
         return (space->mmio_base | IO_SPACE_PORT(port););
}
static inline unsigned int inl(unsigned long port)
{
         return *__ia64_mk_io_addr(port);
}

Most architectures allow only one I/O port range and put it at a fixed virtual address so that inl() simply becomes

static inline u32 inl(unsigned long addr)

{
         return readl(PCI_IOBASE + addr);
}

Quoted 6 lines

which noticeably reduces code size.

On some architectures (powerpc, arm, arm64), we then get the same simplified definition with a fixed virtual address, and use pci_ioremap_io() or something like that to to map a physical address range into this virtual address window at the correct io_offset;

Quoted 5 lines

Hi all,
Thanks for explanation, I found a way to make the ACPI resource
parsing interface arch neutral, it should help to address Lorenzo's concern. Please refer to the attached patch. (It's still RFC, not tested yet).

If we go with this approach though, you are not adding the offset to the resource when parsing the memory spaces in acpi_decode_space(), are we sure that's what we really want ?

In DT, a host bridge range has a:

- CPU physical address
- PCI bus address

Quoted 7 lines

We use that to compute the offset between primary bus (ie CPU physical address) and secondary bus (ie PCI bus address).

The value ending up in the PCI resource struct (for memory space) is the CPU physical address, if you do not add the offset in acpi_decode_space that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI, am I wrong ?

Hi Lorenzo,
I may have found the divergence between us about the design here. You
treat it as a one-stage translation but I treat it as a
two-stage translation as below:
stage 1: map(translate) per-PCI-domain IO port address[0, 16M) into system global IO port address. Here system global IO port address is ioport_resource[0, IO_SPACE_LIMIT).
stage 2: map system IO port address into system memory address.

We need two objects of struct resource_win to support above two-stage translation. One object, type of IORESOURCE_IO, is used to support stage one, and it will also used to allocate IO port resources for PCI devices. Another object, type of IORESOURCE_MMIO, is used to allocate resource from iomem_resource and setup MMIO mapping to actually access IO ports.

For ARM64, it doesn't support multiple per-PCI-domain(bus local) IO port address space yet, so stage one seems to be optional becomes the offset between bus local IO port address and system IO port address is always 0. But we still need two objects of struct resource_win. The first object is

	{
		offset:0,
		start:AddressMinimum,
		end:AddressMaximum,
		flags:IORESOURCE_IO
	}

Here it's type of IORESOURCE_IO and offset must be zero because pcibios_resource_to_bus() will access it translate system IO port address into bus local IO port address. With my patch, the struct resource_win object created by the ACPI core will be reused for this.

The second object is:

	{
		offset:Translation_Offset,
		start:AddressMinimum + Translation_Offset,
		end:AddressMaximum + Translation_Offset,
		flags:IORESOURCE_MMIO
	}

Arch code need to create the second struct resource_win object and actually setup the MMIO mapping.

But there's really another bug need to get fixed, funciton acpi_dev_ioresource_flags() assumes bus local IO port address space is size of 64K, which is wrong for IA64 and ARM64.

Quoted 135 lines · Jiang Liu

Quoted 85 lines · Lorenzo Pieralisi

On Tue, Nov 10, 2015 at 01:50:46PM +0800, Jiang Liu wrote:

[...]

Quoted 61 lines

Quoted 53 lines

Quoted 5 lines

In particular, I would like to understand, for an eg DWordIO descriptor,
what Range Minimum, Range Maximum and Translation Offset represent, they can't mean different things depending on the SW parsing them, this totally defeats the purpose.

I have no clue about what those mean in ACPI though.

Generally speaking, each PCI domain is expected to have a (normally 64KB)
range of CPU addresses that gets translated into PCI I/O space the same
way that config space and memory space are handled.
This is true for almost every architecture except for x86, which uses different CPU instructions for I/O space compared to the other spaces.

Quoted 10 lines

By the way, ia64 ioremaps the translation_offset (ie new_space()), so basically that's the CPU physical address at which the PCI host bridge
map the IO space transactions), I do not think ia64 is any different from
arm64 in this respect, if it is please provide an HW description here from
the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
tables would help).

The main difference between ia64 and a lot of the other architectures (e.g.
sparc is different again) is that ia64 defines a logical address range in terms of having a small number for each I/O space followed by the offset within that space as a 'port number' and uses a mapping function
that is defined as

static inline void *__ia64_mk_io_addr (unsigned long port)

{
         struct io_space *space = &io_space[IO_SPACE_NR(port)];
         return (space->mmio_base | IO_SPACE_PORT(port););
}
static inline unsigned int inl(unsigned long port)
{
         return *__ia64_mk_io_addr(port);
}

Most architectures allow only one I/O port range and put it at a fixed virtual address so that inl() simply becomes

static inline u32 inl(unsigned long addr)

{
         return readl(PCI_IOBASE + addr);
}

Quoted 8 lines

which noticeably reduces code size.

On some architectures (powerpc, arm, arm64), we then get the same simplified
definition with a fixed virtual address, and use pci_ioremap_io() or something like that to to map a physical address range into this virtual
address window at the correct io_offset;

Quoted 6 lines

Hi all,
Thanks for explanation, I found a way to make the ACPI resource
parsing interface arch neutral, it should help to address Lorenzo's concern. Please refer to the attached patch. (It's still RFC, not tested
yet).

If we go with this approach though, you are not adding the offset to the resource when parsing the memory spaces in acpi_decode_space(), are we
sure that's what we really want ?

In DT, a host bridge range has a:

- CPU physical address
- PCI bus address

We use that to compute the offset between primary bus (ie CPU physical address) and secondary bus (ie PCI bus address).

Quoted 6 lines

The value ending up in the PCI resource struct (for memory space) is the CPU physical address, if you do not add the offset in acpi_decode_space
that does not hold true on platforms where CPU<->PCI offset != 0 on ACPI,
am I wrong ?

Hi Lorenzo,
I may have found the divergence between us about the design here. You
treat it as a one-stage translation but I treat it as a two-stage translation as below:
stage 1: map(translate) per-PCI-domain IO port address[0, 16M) into system global IO port address. Here system global IO port address is ioport_resource[0, IO_SPACE_LIMIT).
stage 2: map system IO port address into system memory address.

We need two objects of struct resource_win to support above two-stage translation. One object, type of IORESOURCE_IO, is used to support stage one, and it will also used to allocate IO port resources for PCI devices. Another object, type of IORESOURCE_MMIO, is used to allocate resource from iomem_resource and setup MMIO mapping to actually access IO ports.

For ARM64, it doesn't support multiple per-PCI-domain(bus local) IO port address space yet, so stage one seems to be optional becomes the offset between bus local IO port address and system IO port address is always 0. But we still need two objects of struct resource_win. The first object is

    {
        offset:0,
        start:AddressMinimum,
        end:AddressMaximum,
        flags:IORESOURCE_IO
    }

Here it's type of IORESOURCE_IO and offset must be zero because pcibios_resource_to_bus() will access it translate system IO port address into bus local IO port address. With my patch, the struct resource_win object created by the ACPI core will be reused for this.

The second object is:
{
offset:Translation_Offset, start:AddressMinimum + Translation_Offset, end:AddressMaximum + Translation_Offset, flags:IORESOURCE_MMIO
}
Arch code need to create the second struct resource_win object and actually setup the MMIO mapping.

Quoted 4 lines

But there's really another bug need to get fixed, funciton acpi_dev_ioresource_flags() assumes bus local IO port address space is size of 64K, which is wrong for IA64 and ARM64.

So what would be the Translation_Offset meaning for two cases DWordIo (....,TypeTranslation) vs DWordIo (....,TypeStatic)? And why we did not use TypeTranslation for IA64 so far?

I am worried that TypeTranslation fall into the IA64 category but ACPI tables were already written incorrectly.

Tomasz

Signature · 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/