[RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Christopher Covington]

From: Tomasz Nowicki <tn@semihalf.com>

Some platforms may not be fully compliant with the generic PCI config
operations. For these cases we implement a way to use custom map and
accessor functions. The algorithm traverses the available quirk list,
matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
oem_revision come from the MCFG table standard header. All quirks can be
defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
contained. Example:

/* Custom PCI config ops */
static struct pci_generic_ecam_ops foo_pci_ops = {
	.bus_shift	= 24,
	.pci_ops = {
		.map_bus = pci_ecam_map_bus,
		.read = foo_ecam_config_read,
		.write = foo_ecam_config_write,
	}
};

DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
			<domain_nr>, <bus_nr>);

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
Signed-off-by: Christopher Covington <cov@codeaurora.org>

---

Changes from v2 to v3:
* Match against all three of oem_id, oem_table_id, and oem_revision.
* Perform substring match, so padding oem_id and oem_table_id isn't
  required. (Using min_t() thanks to Duc Dang's suggestion.)
* Print when quirk matched.
* Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
  when OEM and table IDs contain characters such as dash "-".
* Move extern declaration to header and fix spelling and formatting to
  satisfy checkpatch/coding style.
---
 drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
 include/asm-generic/vmlinux.lds.h |  7 ++++++
 include/linux/pci-acpi.h          | 24 +++++++++++++++++++
 3 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index b5b376e..2a5d3dd 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -22,6 +22,10 @@
 #include <linux/kernel.h>
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
+
+/* Root pointer to the mapped MCFG table */
+static struct acpi_table_mcfg *mcfg_table;
 
 /* Structure to hold entries from the MCFG table */
 struct mcfg_entry {
@@ -35,6 +39,46 @@ struct mcfg_entry {
 /* List to save MCFG entries */
 static LIST_HEAD(pci_mcfg_list);
 
+static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
+				 struct acpi_table_header *h)
+{
+	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
+	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
+
+	return (!strncmp(f->oem_id, h->oem_id, olen) &&
+		!strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
+		f->oem_revision == h->oem_revision);
+}
+
+struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
+{
+	int bus_num = root->secondary.start;
+	int domain = root->segment;
+	struct pci_cfg_fixup *f;
+
+	if (!mcfg_table)
+		return &pci_generic_ecam_ops;
+
+	/*
+	 * Match against platform specific quirks and return corresponding CAM
+	 * ops.
+	 *
+	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
+	 * table ID, and OEM revision from MCFG table standard header.
+	 */
+	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
+		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
+		    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
+		    pci_mcfg_fixup_match(f, &mcfg_table->header)) {
+			pr_info("Handling %s %s r%d PCI MCFG quirks\n",
+				f->oem_id, f->oem_table_id, f->oem_revision);
+			return f->ops;
+		}
+	}
+	/* No quirks, use ECAM */
+	return &pci_generic_ecam_ops;
+}
+
 phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
 {
 	struct mcfg_entry *e;
@@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
 
 static __init int pci_mcfg_parse(struct acpi_table_header *header)
 {
-	struct acpi_table_mcfg *mcfg;
 	struct acpi_mcfg_allocation *mptr;
 	struct mcfg_entry *e, *arr;
 	int i, n;
@@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
 
 	n = (header->length - sizeof(struct acpi_table_mcfg)) /
 					sizeof(struct acpi_mcfg_allocation);
-	mcfg = (struct acpi_table_mcfg *)header;
-	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
+	mcfg_table = (struct acpi_table_mcfg *)header;
+	mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
 
 	arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
 	if (!arr)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 6a67ab9..43604fc 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -300,6 +300,13 @@
 		VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;	\
 	}								\
 									\
+	/* ACPI MCFG quirks */						\
+	.acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {	\
+		VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;		\
+		*(.acpi_fixup_mcfg)					\
+		VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;		\
+	}								\
+									\
 	/* Built-in firmware blobs */					\
 	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 7d63a66..dac851b 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
 extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
 
 extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
+extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
 
 static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
 {
@@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
 	int (*prepare_resources)(struct acpi_pci_root_info *info);
 };
 
+struct pci_cfg_fixup {
+	struct pci_ecam_ops *ops;
+	char *oem_id;
+	char *oem_table_id;
+	u32 oem_revision;
+	int domain;
+	int bus_num;
+};
+
+extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
+extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
+
+#define PCI_MCFG_DOMAIN_ANY	-1
+#define PCI_MCFG_BUS_ANY	-1
+
+/* Designate a routine to fix up buggy MCFG */
+#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)	\
+	static const struct pci_cfg_fixup			\
+	__UNIQUE_ID(mcfg_fixup_)				\
+	__used	__attribute__((__section__(".acpi_fixup_mcfg"),	\
+				aligned((sizeof(void *))))) =	\
+	{ ops, oem, table, rev, dom, bus }
+
 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,
-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Christopher Covington]

From: Tomasz Nowicki <tn@semihalf.com>

Some platforms may not be fully compliant with the generic PCI config
operations. For these cases we implement a way to use custom map and
accessor functions. The algorithm traverses the available quirk list,
matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
oem_revision come from the MCFG table standard header. All quirks can be
defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
contained. Example:

/* Custom PCI config ops */
static struct pci_generic_ecam_ops foo_pci_ops = {
	.bus_shift	= 24,
	.pci_ops = {
		.map_bus = pci_ecam_map_bus,
		.read = foo_ecam_config_read,
		.write = foo_ecam_config_write,
	}
};

DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
			<domain_nr>, <bus_nr>);

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
Signed-off-by: Christopher Covington <cov@codeaurora.org>

---

Changes from v2 to v3:
* Match against all three of oem_id, oem_table_id, and oem_revision.
* Perform substring match, so padding oem_id and oem_table_id isn't
  required. (Using min_t() thanks to Duc Dang's suggestion.)
* Print when quirk matched.
* Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
  when OEM and table IDs contain characters such as dash "-".
* Move extern declaration to header and fix spelling and formatting to
  satisfy checkpatch/coding style.
---
 drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
 include/asm-generic/vmlinux.lds.h |  7 ++++++
 include/linux/pci-acpi.h          | 24 +++++++++++++++++++
 3 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index b5b376e..2a5d3dd 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -22,6 +22,10 @@
 #include <linux/kernel.h>
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
+
+/* Root pointer to the mapped MCFG table */
+static struct acpi_table_mcfg *mcfg_table;
 
 /* Structure to hold entries from the MCFG table */
 struct mcfg_entry {
@@ -35,6 +39,46 @@ struct mcfg_entry {
 /* List to save MCFG entries */
 static LIST_HEAD(pci_mcfg_list);
 
+static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
+				 struct acpi_table_header *h)
+{
+	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
+	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
+
+	return (!strncmp(f->oem_id, h->oem_id, olen) &&
+		!strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
+		f->oem_revision == h->oem_revision);
+}
+
+struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
+{
+	int bus_num = root->secondary.start;
+	int domain = root->segment;
+	struct pci_cfg_fixup *f;
+
+	if (!mcfg_table)
+		return &pci_generic_ecam_ops;
+
+	/*
+	 * Match against platform specific quirks and return corresponding CAM
+	 * ops.
+	 *
+	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
+	 * table ID, and OEM revision from MCFG table standard header.
+	 */
+	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
+		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
+		    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
+		    pci_mcfg_fixup_match(f, &mcfg_table->header)) {
+			pr_info("Handling %s %s r%d PCI MCFG quirks\n",
+				f->oem_id, f->oem_table_id, f->oem_revision);
+			return f->ops;
+		}
+	}
+	/* No quirks, use ECAM */
+	return &pci_generic_ecam_ops;
+}
+
 phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
 {
 	struct mcfg_entry *e;
@@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
 
 static __init int pci_mcfg_parse(struct acpi_table_header *header)
 {
-	struct acpi_table_mcfg *mcfg;
 	struct acpi_mcfg_allocation *mptr;
 	struct mcfg_entry *e, *arr;
 	int i, n;
@@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
 
 	n = (header->length - sizeof(struct acpi_table_mcfg)) /
 					sizeof(struct acpi_mcfg_allocation);
-	mcfg = (struct acpi_table_mcfg *)header;
-	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
+	mcfg_table = (struct acpi_table_mcfg *)header;
+	mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
 
 	arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
 	if (!arr)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 6a67ab9..43604fc 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -300,6 +300,13 @@
 		VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;	\
 	}								\
 									\
+	/* ACPI MCFG quirks */						\
+	.acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {	\
+		VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;		\
+		*(.acpi_fixup_mcfg)					\
+		VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;		\
+	}								\
+									\
 	/* Built-in firmware blobs */					\
 	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 7d63a66..dac851b 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
 extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
 
 extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
+extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
 
 static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
 {
@@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
 	int (*prepare_resources)(struct acpi_pci_root_info *info);
 };
 
+struct pci_cfg_fixup {
+	struct pci_ecam_ops *ops;
+	char *oem_id;
+	char *oem_table_id;
+	u32 oem_revision;
+	int domain;
+	int bus_num;
+};
+
+extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
+extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
+
+#define PCI_MCFG_DOMAIN_ANY	-1
+#define PCI_MCFG_BUS_ANY	-1
+
+/* Designate a routine to fix up buggy MCFG */
+#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)	\
+	static const struct pci_cfg_fixup			\
+	__UNIQUE_ID(mcfg_fixup_)				\
+	__used	__attribute__((__section__(".acpi_fixup_mcfg"),	\
+				aligned((sizeof(void *))))) =	\
+	{ ops, oem, table, rev, dom, bus }
+
 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,
-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Christopher Covington]

From: Tomasz Nowicki <tn@semihalf.com>

pci_generic_ecam_ops is used by default. Since there are platforms
which have non-compliant ECAM space we need to overwrite these
accessors prior to PCI buses enumeration. In order to do that
we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
we can use proper PCI config space accessors and bus_shift.

pci_generic_ecam_ops is still used for platforms free from quirks.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 arch/arm64/kernel/pci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 94cd43c..a891bda 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
 	struct pci_config_window *cfg;
 	struct resource cfgres;
 	unsigned int bsz;
+	struct pci_ecam_ops *ops;
 
 	/* Use address from _CBA if present, otherwise lookup MCFG */
 	if (!root->mcfg_addr)
@@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
 		return NULL;
 	}
 
-	bsz = 1 << pci_generic_ecam_ops.bus_shift;
+	ops = pci_mcfg_get_ops(root);
+	bsz = 1 << ops->bus_shift;
 	cfgres.start = root->mcfg_addr + bus_res->start * bsz;
 	cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
 	cfgres.flags = IORESOURCE_MEM;
-	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
-			      &pci_generic_ecam_ops);
+	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);
 	if (IS_ERR(cfg)) {
 		dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
 			seg, bus_res, PTR_ERR(cfg));
-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Christopher Covington]

From: Tomasz Nowicki <tn@semihalf.com>

pci_generic_ecam_ops is used by default. Since there are platforms
which have non-compliant ECAM space we need to overwrite these
accessors prior to PCI buses enumeration. In order to do that
we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
we can use proper PCI config space accessors and bus_shift.

pci_generic_ecam_ops is still used for platforms free from quirks.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 arch/arm64/kernel/pci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 94cd43c..a891bda 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
 	struct pci_config_window *cfg;
 	struct resource cfgres;
 	unsigned int bsz;
+	struct pci_ecam_ops *ops;
 
 	/* Use address from _CBA if present, otherwise lookup MCFG */
 	if (!root->mcfg_addr)
@@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
 		return NULL;
 	}
 
-	bsz = 1 << pci_generic_ecam_ops.bus_shift;
+	ops = pci_mcfg_get_ops(root);
+	bsz = 1 << ops->bus_shift;
 	cfgres.start = root->mcfg_addr + bus_res->start * bsz;
 	cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
 	cfgres.flags = IORESOURCE_MEM;
-	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
-			      &pci_generic_ecam_ops);
+	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);
 	if (IS_ERR(cfg)) {
 		dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
 			seg, bus_res, PTR_ERR(cfg));
-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Lorenzo Pieralisi]

On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki <tn@semihalf.com>
> 
> Some platforms may not be fully compliant with the generic PCI config
> operations. For these cases we implement a way to use custom map and
> accessor functions. The algorithm traverses the available quirk list,
> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
> oem_revision come from the MCFG table standard header. All quirks can be
> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
> contained. Example:
> 
> /* Custom PCI config ops */
> static struct pci_generic_ecam_ops foo_pci_ops = {
> 	.bus_shift	= 24,
> 	.pci_ops = {
> 		.map_bus = pci_ecam_map_bus,
> 		.read = foo_ecam_config_read,
> 		.write = foo_ecam_config_write,
> 	}
> };
> 
> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
> 			<domain_nr>, <bus_nr>);
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> 
> ---
> 
> Changes from v2 to v3:
> * Match against all three of oem_id, oem_table_id, and oem_revision.
> * Perform substring match, so padding oem_id and oem_table_id isn't
>   required. (Using min_t() thanks to Duc Dang's suggestion.)
> * Print when quirk matched.
> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>   when OEM and table IDs contain characters such as dash "-".
> * Move extern declaration to header and fix spelling and formatting to
>   satisfy checkpatch/coding style.
> ---
>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>  3 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index b5b376e..2a5d3dd 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -22,6 +22,10 @@
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +
> +/* Root pointer to the mapped MCFG table */
> +static struct acpi_table_mcfg *mcfg_table;

Tomasz had to add an MCFG cache in its final v9 since it is not
safe to stash a pointer here, which probably means you will have
to stash oem_id and oem_table_id to carry out the fixup match
instead of relying on the header retrieved through the stashed
pointer.

Lorenzo

>  /* Structure to hold entries from the MCFG table */
>  struct mcfg_entry {
> @@ -35,6 +39,46 @@ struct mcfg_entry {
>  /* List to save MCFG entries */
>  static LIST_HEAD(pci_mcfg_list);
>  
> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
> +				 struct acpi_table_header *h)
> +{
> +	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
> +	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
> +
> +	return (!strncmp(f->oem_id, h->oem_id, olen) &&
> +		!strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
> +		f->oem_revision == h->oem_revision);
> +}
> +
> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> +{
> +	int bus_num = root->secondary.start;
> +	int domain = root->segment;
> +	struct pci_cfg_fixup *f;
> +
> +	if (!mcfg_table)
> +		return &pci_generic_ecam_ops;
> +
> +	/*
> +	 * Match against platform specific quirks and return corresponding CAM
> +	 * ops.
> +	 *
> +	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
> +	 * table ID, and OEM revision from MCFG table standard header.
> +	 */
> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> +		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> +		    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> +		    pci_mcfg_fixup_match(f, &mcfg_table->header)) {
> +			pr_info("Handling %s %s r%d PCI MCFG quirks\n",
> +				f->oem_id, f->oem_table_id, f->oem_revision);
> +			return f->ops;
> +		}
> +	}
> +	/* No quirks, use ECAM */
> +	return &pci_generic_ecam_ops;
> +}
> +
>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  {
>  	struct mcfg_entry *e;
> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  
>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  {
> -	struct acpi_table_mcfg *mcfg;
>  	struct acpi_mcfg_allocation *mptr;
>  	struct mcfg_entry *e, *arr;
>  	int i, n;
> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  
>  	n = (header->length - sizeof(struct acpi_table_mcfg)) /
>  					sizeof(struct acpi_mcfg_allocation);
> -	mcfg = (struct acpi_table_mcfg *)header;
> -	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
> +	mcfg_table = (struct acpi_table_mcfg *)header;
> +	mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>  
>  	arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>  	if (!arr)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 6a67ab9..43604fc 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -300,6 +300,13 @@
>  		VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;	\
>  	}								\
>  									\
> +	/* ACPI MCFG quirks */						\
> +	.acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {	\
> +		VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;		\
> +		*(.acpi_fixup_mcfg)					\
> +		VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;		\
> +	}								\
> +									\
>  	/* Built-in firmware blobs */					\
>  	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
>  		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 7d63a66..dac851b 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>  
>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>  
>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>  {
> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>  	int (*prepare_resources)(struct acpi_pci_root_info *info);
>  };
>  
> +struct pci_cfg_fixup {
> +	struct pci_ecam_ops *ops;
> +	char *oem_id;
> +	char *oem_table_id;
> +	u32 oem_revision;
> +	int domain;
> +	int bus_num;
> +};
> +
> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
> +
> +#define PCI_MCFG_DOMAIN_ANY	-1
> +#define PCI_MCFG_BUS_ANY	-1
> +
> +/* Designate a routine to fix up buggy MCFG */
> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)	\
> +	static const struct pci_cfg_fixup			\
> +	__UNIQUE_ID(mcfg_fixup_)				\
> +	__used	__attribute__((__section__(".acpi_fixup_mcfg"),	\
> +				aligned((sizeof(void *))))) =	\
> +	{ ops, oem, table, rev, dom, bus }
> +
>  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,
> -- 
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Lorenzo Pieralisi]

On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki <tn@semihalf.com>
> 
> Some platforms may not be fully compliant with the generic PCI config
> operations. For these cases we implement a way to use custom map and
> accessor functions. The algorithm traverses the available quirk list,
> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
> oem_revision come from the MCFG table standard header. All quirks can be
> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
> contained. Example:
> 
> /* Custom PCI config ops */
> static struct pci_generic_ecam_ops foo_pci_ops = {
> 	.bus_shift	= 24,
> 	.pci_ops = {
> 		.map_bus = pci_ecam_map_bus,
> 		.read = foo_ecam_config_read,
> 		.write = foo_ecam_config_write,
> 	}
> };
> 
> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
> 			<domain_nr>, <bus_nr>);
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> 
> ---
> 
> Changes from v2 to v3:
> * Match against all three of oem_id, oem_table_id, and oem_revision.
> * Perform substring match, so padding oem_id and oem_table_id isn't
>   required. (Using min_t() thanks to Duc Dang's suggestion.)
> * Print when quirk matched.
> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>   when OEM and table IDs contain characters such as dash "-".
> * Move extern declaration to header and fix spelling and formatting to
>   satisfy checkpatch/coding style.
> ---
>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>  3 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index b5b376e..2a5d3dd 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -22,6 +22,10 @@
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +
> +/* Root pointer to the mapped MCFG table */
> +static struct acpi_table_mcfg *mcfg_table;

Tomasz had to add an MCFG cache in its final v9 since it is not
safe to stash a pointer here, which probably means you will have
to stash oem_id and oem_table_id to carry out the fixup match
instead of relying on the header retrieved through the stashed
pointer.

Lorenzo

>  /* Structure to hold entries from the MCFG table */
>  struct mcfg_entry {
> @@ -35,6 +39,46 @@ struct mcfg_entry {
>  /* List to save MCFG entries */
>  static LIST_HEAD(pci_mcfg_list);
>  
> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
> +				 struct acpi_table_header *h)
> +{
> +	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
> +	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
> +
> +	return (!strncmp(f->oem_id, h->oem_id, olen) &&
> +		!strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
> +		f->oem_revision == h->oem_revision);
> +}
> +
> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> +{
> +	int bus_num = root->secondary.start;
> +	int domain = root->segment;
> +	struct pci_cfg_fixup *f;
> +
> +	if (!mcfg_table)
> +		return &pci_generic_ecam_ops;
> +
> +	/*
> +	 * Match against platform specific quirks and return corresponding CAM
> +	 * ops.
> +	 *
> +	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
> +	 * table ID, and OEM revision from MCFG table standard header.
> +	 */
> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> +		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> +		    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> +		    pci_mcfg_fixup_match(f, &mcfg_table->header)) {
> +			pr_info("Handling %s %s r%d PCI MCFG quirks\n",
> +				f->oem_id, f->oem_table_id, f->oem_revision);
> +			return f->ops;
> +		}
> +	}
> +	/* No quirks, use ECAM */
> +	return &pci_generic_ecam_ops;
> +}
> +
>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  {
>  	struct mcfg_entry *e;
> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  
>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  {
> -	struct acpi_table_mcfg *mcfg;
>  	struct acpi_mcfg_allocation *mptr;
>  	struct mcfg_entry *e, *arr;
>  	int i, n;
> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  
>  	n = (header->length - sizeof(struct acpi_table_mcfg)) /
>  					sizeof(struct acpi_mcfg_allocation);
> -	mcfg = (struct acpi_table_mcfg *)header;
> -	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
> +	mcfg_table = (struct acpi_table_mcfg *)header;
> +	mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>  
>  	arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>  	if (!arr)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 6a67ab9..43604fc 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -300,6 +300,13 @@
>  		VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;	\
>  	}								\
>  									\
> +	/* ACPI MCFG quirks */						\
> +	.acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {	\
> +		VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;		\
> +		*(.acpi_fixup_mcfg)					\
> +		VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;		\
> +	}								\
> +									\
>  	/* Built-in firmware blobs */					\
>  	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
>  		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 7d63a66..dac851b 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>  
>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>  
>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>  {
> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>  	int (*prepare_resources)(struct acpi_pci_root_info *info);
>  };
>  
> +struct pci_cfg_fixup {
> +	struct pci_ecam_ops *ops;
> +	char *oem_id;
> +	char *oem_table_id;
> +	u32 oem_revision;
> +	int domain;
> +	int bus_num;
> +};
> +
> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
> +
> +#define PCI_MCFG_DOMAIN_ANY	-1
> +#define PCI_MCFG_BUS_ANY	-1
> +
> +/* Designate a routine to fix up buggy MCFG */
> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)	\
> +	static const struct pci_cfg_fixup			\
> +	__UNIQUE_ID(mcfg_fixup_)				\
> +	__used	__attribute__((__section__(".acpi_fixup_mcfg"),	\
> +				aligned((sizeof(void *))))) =	\
> +	{ ops, oem, table, rev, dom, bus }
> +
>  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,
> -- 
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Lorenzo Pieralisi]

On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki <tn@semihalf.com>
> 
> pci_generic_ecam_ops is used by default. Since there are platforms
> which have non-compliant ECAM space we need to overwrite these
> accessors prior to PCI buses enumeration. In order to do that
> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> we can use proper PCI config space accessors and bus_shift.
> 
> pci_generic_ecam_ops is still used for platforms free from quirks.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  arch/arm64/kernel/pci.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 94cd43c..a891bda 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>  	struct pci_config_window *cfg;
>  	struct resource cfgres;
>  	unsigned int bsz;
> +	struct pci_ecam_ops *ops;
>  
>  	/* Use address from _CBA if present, otherwise lookup MCFG */
>  	if (!root->mcfg_addr)
> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>  		return NULL;
>  	}
>  
> -	bsz = 1 << pci_generic_ecam_ops.bus_shift;
> +	ops = pci_mcfg_get_ops(root);
> +	bsz = 1 << ops->bus_shift;
>  	cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>  	cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>  	cfgres.flags = IORESOURCE_MEM;
> -	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
> -			      &pci_generic_ecam_ops);
> +	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);

Arnd pointed this out already, I think that's the only pending question
here.

pci_ecam_create() maps ECAM space for config regions retrieved from
the MCFG, which are *supposed* to be ECAM compliant.

Do we think that's *always* correct/safe regardless of the kind
of quirk we are currently fixing up ?

Or we do think that configuration space regions should come from
a different resource declared in the ACPI namespace if the regions
are not MCFG/ECAM compliant (ie config space is not defined through
MCFG at all - possibly through a _CRS method for a vendor specific
_HID under the PNP0A03 node ?)

It might even be a choice we do not have anymore, but I think it
is important to make a decision and proceed accordingly.

Comments appreciated.

Thanks,
Lorenzo

>  	if (IS_ERR(cfg)) {
>  		dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
>  			seg, bus_res, PTR_ERR(cfg));
> -- 
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Lorenzo Pieralisi]

On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki <tn@semihalf.com>
> 
> pci_generic_ecam_ops is used by default. Since there are platforms
> which have non-compliant ECAM space we need to overwrite these
> accessors prior to PCI buses enumeration. In order to do that
> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> we can use proper PCI config space accessors and bus_shift.
> 
> pci_generic_ecam_ops is still used for platforms free from quirks.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  arch/arm64/kernel/pci.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 94cd43c..a891bda 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>  	struct pci_config_window *cfg;
>  	struct resource cfgres;
>  	unsigned int bsz;
> +	struct pci_ecam_ops *ops;
>  
>  	/* Use address from _CBA if present, otherwise lookup MCFG */
>  	if (!root->mcfg_addr)
> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>  		return NULL;
>  	}
>  
> -	bsz = 1 << pci_generic_ecam_ops.bus_shift;
> +	ops = pci_mcfg_get_ops(root);
> +	bsz = 1 << ops->bus_shift;
>  	cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>  	cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>  	cfgres.flags = IORESOURCE_MEM;
> -	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
> -			      &pci_generic_ecam_ops);
> +	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);

Arnd pointed this out already, I think that's the only pending question
here.

pci_ecam_create() maps ECAM space for config regions retrieved from
the MCFG, which are *supposed* to be ECAM compliant.

Do we think that's *always* correct/safe regardless of the kind
of quirk we are currently fixing up ?

Or we do think that configuration space regions should come from
a different resource declared in the ACPI namespace if the regions
are not MCFG/ECAM compliant (ie config space is not defined through
MCFG at all - possibly through a _CRS method for a vendor specific
_HID under the PNP0A03 node ?)

It might even be a choice we do not have anymore, but I think it
is important to make a decision and proceed accordingly.

Comments appreciated.

Thanks,
Lorenzo

>  	if (IS_ERR(cfg)) {
>  		dev_err(&root->device->dev, "%04x:%pR error %ld mapping ECAM\n",
>  			seg, bus_res, PTR_ERR(cfg));
> -- 
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

RE: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Gabriele Paoloni]

Hi Lorenzo and All

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
> Sent: 16 June 2016 18:49
> To: Christopher Covington
> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
> kernel@lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
> Ganapatrao Kulkarni; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
> for ACPI based PCI host controller
> 
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> > From: Tomasz Nowicki <tn@semihalf.com>
> >
> > pci_generic_ecam_ops is used by default. Since there are platforms
> > which have non-compliant ECAM space we need to overwrite these
> > accessors prior to PCI buses enumeration. In order to do that
> > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> > we can use proper PCI config space accessors and bus_shift.
> >
> > pci_generic_ecam_ops is still used for platforms free from quirks.
> >
> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> > ---
> >  arch/arm64/kernel/pci.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index 94cd43c..a891bda 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root
> *root)
> >  	struct pci_config_window *cfg;
> >  	struct resource cfgres;
> >  	unsigned int bsz;
> > +	struct pci_ecam_ops *ops;
> >
> >  	/* Use address from _CBA if present, otherwise lookup MCFG */
> >  	if (!root->mcfg_addr)
> > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct
> acpi_pci_root *root)
> >  		return NULL;
> >  	}
> >
> > -	bsz = 1 << pci_generic_ecam_ops.bus_shift;
> > +	ops = pci_mcfg_get_ops(root);
> > +	bsz = 1 << ops->bus_shift;
> >  	cfgres.start = root->mcfg_addr + bus_res->start * bsz;
> >  	cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
> >  	cfgres.flags = IORESOURCE_MEM;
> > -	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
> > -			      &pci_generic_ecam_ops);
> > +	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);
> 
> Arnd pointed this out already, I think that's the only pending question
> here.
> 
> pci_ecam_create() maps ECAM space for config regions retrieved from
> the MCFG, which are *supposed* to be ECAM compliant.
> 
> Do we think that's *always* correct/safe regardless of the kind
> of quirk we are currently fixing up ?

I didn't dig into the other vendors' quirk mechanism but I will 
quickly explain what we (would like to) do in HiSilicon Hip05/Hip06
SoCs.

>From our perspective we use ECAM access mechanism for all the MCFG
buses except the root ports.

For the root ports we declare additional ACPI devices marked as
"pnp0c02" (motherboard reserved resource) i.e.:

Scope(_SB) 
{
	// PCIe Root bus 
	Device (PCI1) 
	{ 
		Name (_HID, "HISI0080") // PCI Express Root Bridge 
		Name (_CID, "PNP0A03") // Compatible PCI Root Bridge 
		Name(_SEG, 1) // Segment of this Root complex 
		Name(_BBN, 64) // Base Bus Number 
		Name(_CCA, 1)
		Method (_CRS, 0, Serialized) { // Root complex resources 
			Name (RBUF, ResourceTemplate () { 
		
		[...]

				)
			}) // Name(RBUF)
			Return (RBUF)
		} // Method(_CRS)

		Device (RES1)
		{
			Name (_HID, "HISI0081") // HiSi PCIe RC config base address
			Name (_CID, "PNP0C02")  // Motherboard reserved resource
			Name (_CRS, ResourceTemplate (){
				Memory32Fixed (ReadWrite, 0xb0080000 , 0x10000)
			})
		}

	     [...]

	} // Device(PCI1)
	


Therefore we declare a perfectly ECAM compliant MCFG and we "waste"
the root ports addresses as in practice for the root ports we replace
the MCFG address with the one retrieved from the ACPI device above.

In terms of "safety" I think it should be ok as in practice we are
reserving some addresses in MCFG that we do not use and the ones
that are used are reserved as well in the ACPI namespace.

>From a general perspective it seems to me that pci_mcfg_lookup
can only parse mcfg entries that are declared according to
the standard (i.e. you cannot declare a hacky mcfg table).

Obviously the quirks allow the vendors to declare their own 
cfg_rd/wr function and therefore to do anything they want with
the MCFG addresses.

>From my perspective I think the easiest solution is to keep this
quirk mechanism in place and then review vendor by vendor solution
as they are pushed to the mailing list; if some vendors are abusing
of some addresses/resources then they can be rejected...

What do you think?

Thanks

Gab

> 
> Or we do think that configuration space regions should come from
> a different resource declared in the ACPI namespace if the regions
> are not MCFG/ECAM compliant (ie config space is not defined through
> MCFG at all - possibly through a _CRS method for a vendor specific
> _HID under the PNP0A03 node ?)
> 
> It might even be a choice we do not have anymore, but I think it
> is important to make a decision and proceed accordingly.
> 
> Comments appreciated.
> 
> Thanks,
> Lorenzo
> 
> >  	if (IS_ERR(cfg)) {
> >  		dev_err(&root->device->dev, "%04x:%pR error %ld mapping
> ECAM\n",
> >  			seg, bus_res, PTR_ERR(cfg));
> > --
> > Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> >

[RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Gabriele Paoloni]

Hi Lorenzo and All

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: 16 June 2016 18:49
> To: Christopher Covington
> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
> kernel at lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
> Ganapatrao Kulkarni; linux-kernel at vger.kernel.org
> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
> for ACPI based PCI host controller
> 
> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
> > From: Tomasz Nowicki <tn@semihalf.com>
> >
> > pci_generic_ecam_ops is used by default. Since there are platforms
> > which have non-compliant ECAM space we need to overwrite these
> > accessors prior to PCI buses enumeration. In order to do that
> > we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
> > we can use proper PCI config space accessors and bus_shift.
> >
> > pci_generic_ecam_ops is still used for platforms free from quirks.
> >
> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> > ---
> >  arch/arm64/kernel/pci.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index 94cd43c..a891bda 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root
> *root)
> >  	struct pci_config_window *cfg;
> >  	struct resource cfgres;
> >  	unsigned int bsz;
> > +	struct pci_ecam_ops *ops;
> >
> >  	/* Use address from _CBA if present, otherwise lookup MCFG */
> >  	if (!root->mcfg_addr)
> > @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct
> acpi_pci_root *root)
> >  		return NULL;
> >  	}
> >
> > -	bsz = 1 << pci_generic_ecam_ops.bus_shift;
> > +	ops = pci_mcfg_get_ops(root);
> > +	bsz = 1 << ops->bus_shift;
> >  	cfgres.start = root->mcfg_addr + bus_res->start * bsz;
> >  	cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
> >  	cfgres.flags = IORESOURCE_MEM;
> > -	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
> > -			      &pci_generic_ecam_ops);
> > +	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);
> 
> Arnd pointed this out already, I think that's the only pending question
> here.
> 
> pci_ecam_create() maps ECAM space for config regions retrieved from
> the MCFG, which are *supposed* to be ECAM compliant.
> 
> Do we think that's *always* correct/safe regardless of the kind
> of quirk we are currently fixing up ?

I didn't dig into the other vendors' quirk mechanism but I will 
quickly explain what we (would like to) do in HiSilicon Hip05/Hip06
SoCs.

>From our perspective we use ECAM access mechanism for all the MCFG
buses except the root ports.

For the root ports we declare additional ACPI devices marked as
"pnp0c02" (motherboard reserved resource) i.e.:

Scope(_SB) 
{
	// PCIe Root bus 
	Device (PCI1) 
	{ 
		Name (_HID, "HISI0080") // PCI Express Root Bridge 
		Name (_CID, "PNP0A03") // Compatible PCI Root Bridge 
		Name(_SEG, 1) // Segment of this Root complex 
		Name(_BBN, 64) // Base Bus Number 
		Name(_CCA, 1)
		Method (_CRS, 0, Serialized) { // Root complex resources 
			Name (RBUF, ResourceTemplate () { 
		
		[...]

				)
			}) // Name(RBUF)
			Return (RBUF)
		} // Method(_CRS)

		Device (RES1)
		{
			Name (_HID, "HISI0081") // HiSi PCIe RC config base address
			Name (_CID, "PNP0C02")  // Motherboard reserved resource
			Name (_CRS, ResourceTemplate (){
				Memory32Fixed (ReadWrite, 0xb0080000 , 0x10000)
			})
		}

	     [...]

	} // Device(PCI1)
	


Therefore we declare a perfectly ECAM compliant MCFG and we "waste"
the root ports addresses as in practice for the root ports we replace
the MCFG address with the one retrieved from the ACPI device above.

In terms of "safety" I think it should be ok as in practice we are
reserving some addresses in MCFG that we do not use and the ones
that are used are reserved as well in the ACPI namespace.

>From a general perspective it seems to me that pci_mcfg_lookup
can only parse mcfg entries that are declared according to
the standard (i.e. you cannot declare a hacky mcfg table).

Obviously the quirks allow the vendors to declare their own 
cfg_rd/wr function and therefore to do anything they want with
the MCFG addresses.

>From my perspective I think the easiest solution is to keep this
quirk mechanism in place and then review vendor by vendor solution
as they are pushed to the mailing list; if some vendors are abusing
of some addresses/resources then they can be rejected...

What do you think?

Thanks

Gab

> 
> Or we do think that configuration space regions should come from
> a different resource declared in the ACPI namespace if the regions
> are not MCFG/ECAM compliant (ie config space is not defined through
> MCFG at all - possibly through a _CRS method for a vendor specific
> _HID under the PNP0A03 node ?)
> 
> It might even be a choice we do not have anymore, but I think it
> is important to make a decision and proceed accordingly.
> 
> Comments appreciated.
> 
> Thanks,
> Lorenzo
> 
> >  	if (IS_ERR(cfg)) {
> >  		dev_err(&root->device->dev, "%04x:%pR error %ld mapping
> ECAM\n",
> >  			seg, bus_res, PTR_ERR(cfg));
> > --
> > Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> >

Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Christopher Covington]

On 06/17/2016 04:01 AM, Gabriele Paoloni wrote:
> Hi Lorenzo and All
> 
>> -----Original Message-----
>> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]
>> Sent: 16 June 2016 18:49
>> To: Christopher Covington
>> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
>> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
>> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
>> kernel@lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
>> Ganapatrao Kulkarni; linux-kernel@vger.kernel.org
>> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
>> for ACPI based PCI host controller
>>
>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>> From: Tomasz Nowicki <tn@semihalf.com>
>>>
>>> pci_generic_ecam_ops is used by default. Since there are platforms
>>> which have non-compliant ECAM space we need to overwrite these
>>> accessors prior to PCI buses enumeration. In order to do that
>>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>>> we can use proper PCI config space accessors and bus_shift.
>>>
>>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>>
>>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>>> ---
>>>  arch/arm64/kernel/pci.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>>> index 94cd43c..a891bda 100644
>>> --- a/arch/arm64/kernel/pci.c
>>> +++ b/arch/arm64/kernel/pci.c
>>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root
>> *root)
>>>  	struct pci_config_window *cfg;
>>>  	struct resource cfgres;
>>>  	unsigned int bsz;
>>> +	struct pci_ecam_ops *ops;
>>>
>>>  	/* Use address from _CBA if present, otherwise lookup MCFG */
>>>  	if (!root->mcfg_addr)
>>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct
>> acpi_pci_root *root)
>>>  		return NULL;
>>>  	}
>>>
>>> -	bsz = 1 << pci_generic_ecam_ops.bus_shift;
>>> +	ops = pci_mcfg_get_ops(root);
>>> +	bsz = 1 << ops->bus_shift;
>>>  	cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>>  	cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>>  	cfgres.flags = IORESOURCE_MEM;
>>> -	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
>>> -			      &pci_generic_ecam_ops);
>>> +	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);
>>
>> Arnd pointed this out already, I think that's the only pending question
>> here.
>>
>> pci_ecam_create() maps ECAM space for config regions retrieved from
>> the MCFG, which are *supposed* to be ECAM compliant.
>>
>> Do we think that's *always* correct/safe regardless of the kind
>> of quirk we are currently fixing up ?

> From my perspective I think the easiest solution is to keep this
> quirk mechanism in place and then review vendor by vendor solution
> as they are pushed to the mailing list; if some vendors are abusing
> of some addresses/resources then they can be rejected...

Using pci_ecam_create() works well for QDF2432:

https://us.codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc3-testing&id=ca7439a54ddd8612b435c797ffc54f4e19f03e2b

Cheers,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Christopher Covington]

On 06/17/2016 04:01 AM, Gabriele Paoloni wrote:
> Hi Lorenzo and All
> 
>> -----Original Message-----
>> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
>> Sent: 16 June 2016 18:49
>> To: Christopher Covington
>> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
>> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
>> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
>> kernel at lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas;
>> Ganapatrao Kulkarni; linux-kernel at vger.kernel.org
>> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
>> for ACPI based PCI host controller
>>
>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>> From: Tomasz Nowicki <tn@semihalf.com>
>>>
>>> pci_generic_ecam_ops is used by default. Since there are platforms
>>> which have non-compliant ECAM space we need to overwrite these
>>> accessors prior to PCI buses enumeration. In order to do that
>>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>>> we can use proper PCI config space accessors and bus_shift.
>>>
>>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>>
>>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>>> ---
>>>  arch/arm64/kernel/pci.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>>> index 94cd43c..a891bda 100644
>>> --- a/arch/arm64/kernel/pci.c
>>> +++ b/arch/arm64/kernel/pci.c
>>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root
>> *root)
>>>  	struct pci_config_window *cfg;
>>>  	struct resource cfgres;
>>>  	unsigned int bsz;
>>> +	struct pci_ecam_ops *ops;
>>>
>>>  	/* Use address from _CBA if present, otherwise lookup MCFG */
>>>  	if (!root->mcfg_addr)
>>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct
>> acpi_pci_root *root)
>>>  		return NULL;
>>>  	}
>>>
>>> -	bsz = 1 << pci_generic_ecam_ops.bus_shift;
>>> +	ops = pci_mcfg_get_ops(root);
>>> +	bsz = 1 << ops->bus_shift;
>>>  	cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>>  	cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>>  	cfgres.flags = IORESOURCE_MEM;
>>> -	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
>>> -			      &pci_generic_ecam_ops);
>>> +	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);
>>
>> Arnd pointed this out already, I think that's the only pending question
>> here.
>>
>> pci_ecam_create() maps ECAM space for config regions retrieved from
>> the MCFG, which are *supposed* to be ECAM compliant.
>>
>> Do we think that's *always* correct/safe regardless of the kind
>> of quirk we are currently fixing up ?

> From my perspective I think the easiest solution is to keep this
> quirk mechanism in place and then review vendor by vendor solution
> as they are pushed to the mailing list; if some vendors are abusing
> of some addresses/resources then they can be rejected...

Using pci_ecam_create() works well for QDF2432:

https://us.codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc3-testing&id=ca7439a54ddd8612b435c797ffc54f4e19f03e2b

Cheers,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Lorenzo Pieralisi]

[+ Ard, Arnd]

On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki <tn@semihalf.com>
> 
> Some platforms may not be fully compliant with the generic PCI config
> operations. For these cases we implement a way to use custom map and
> accessor functions. The algorithm traverses the available quirk list,
> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
> oem_revision come from the MCFG table standard header. All quirks can be
> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
> contained. Example:
> 
> /* Custom PCI config ops */
> static struct pci_generic_ecam_ops foo_pci_ops = {
> 	.bus_shift	= 24,
> 	.pci_ops = {
> 		.map_bus = pci_ecam_map_bus,
> 		.read = foo_ecam_config_read,
> 		.write = foo_ecam_config_write,
> 	}
> };
> 
> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
> 			<domain_nr>, <bus_nr>);
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> 
> ---
> 
> Changes from v2 to v3:
> * Match against all three of oem_id, oem_table_id, and oem_revision.
> * Perform substring match, so padding oem_id and oem_table_id isn't
>   required. (Using min_t() thanks to Duc Dang's suggestion.)
> * Print when quirk matched.
> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>   when OEM and table IDs contain characters such as dash "-".
> * Move extern declaration to header and fix spelling and formatting to
>   satisfy checkpatch/coding style.
> ---
>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>  3 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index b5b376e..2a5d3dd 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -22,6 +22,10 @@
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +
> +/* Root pointer to the mapped MCFG table */
> +static struct acpi_table_mcfg *mcfg_table;
>  
>  /* Structure to hold entries from the MCFG table */
>  struct mcfg_entry {
> @@ -35,6 +39,46 @@ struct mcfg_entry {
>  /* List to save MCFG entries */
>  static LIST_HEAD(pci_mcfg_list);
>  
> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
> +				 struct acpi_table_header *h)
> +{
> +	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
> +	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
> +
> +	return (!strncmp(f->oem_id, h->oem_id, olen) &&
> +		!strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
> +		f->oem_revision == h->oem_revision);
> +}
> +
> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> +{
> +	int bus_num = root->secondary.start;
> +	int domain = root->segment;
> +	struct pci_cfg_fixup *f;
> +
> +	if (!mcfg_table)
> +		return &pci_generic_ecam_ops;
> +
> +	/*
> +	 * Match against platform specific quirks and return corresponding CAM
> +	 * ops.
> +	 *
> +	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
> +	 * table ID, and OEM revision from MCFG table standard header.
> +	 */
> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> +		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> +		    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> +		    pci_mcfg_fixup_match(f, &mcfg_table->header)) {
> +			pr_info("Handling %s %s r%d PCI MCFG quirks\n",
> +				f->oem_id, f->oem_table_id, f->oem_revision);
> +			return f->ops;
> +		}
> +	}
> +	/* No quirks, use ECAM */
> +	return &pci_generic_ecam_ops;
> +}
> +
>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  {
>  	struct mcfg_entry *e;
> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  
>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  {
> -	struct acpi_table_mcfg *mcfg;
>  	struct acpi_mcfg_allocation *mptr;
>  	struct mcfg_entry *e, *arr;
>  	int i, n;
> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  
>  	n = (header->length - sizeof(struct acpi_table_mcfg)) /
>  					sizeof(struct acpi_mcfg_allocation);
> -	mcfg = (struct acpi_table_mcfg *)header;
> -	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
> +	mcfg_table = (struct acpi_table_mcfg *)header;
> +	mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>  
>  	arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>  	if (!arr)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 6a67ab9..43604fc 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -300,6 +300,13 @@
>  		VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;	\
>  	}								\
>  									\
> +	/* ACPI MCFG quirks */						\
> +	.acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {	\
> +		VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;		\
> +		*(.acpi_fixup_mcfg)					\
> +		VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;		\
> +	}								\
> +									\
>  	/* Built-in firmware blobs */					\
>  	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
>  		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 7d63a66..dac851b 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>  
>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>  
>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>  {
> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>  	int (*prepare_resources)(struct acpi_pci_root_info *info);
>  };
>  
> +struct pci_cfg_fixup {
> +	struct pci_ecam_ops *ops;
> +	char *oem_id;
> +	char *oem_table_id;
> +	u32 oem_revision;
> +	int domain;
> +	int bus_num;
> +};
> +
> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
> +
> +#define PCI_MCFG_DOMAIN_ANY	-1
> +#define PCI_MCFG_BUS_ANY	-1
> +
> +/* Designate a routine to fix up buggy MCFG */
> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)	\
> +	static const struct pci_cfg_fixup			\
> +	__UNIQUE_ID(mcfg_fixup_)				\
> +	__used	__attribute__((__section__(".acpi_fixup_mcfg"),	\
> +				aligned((sizeof(void *))))) =	\
> +	{ ops, oem, table, rev, dom, bus }
> +

Ok, a couple of comments on this quirks handling mechanism.

Given that it is supposed to be just a temporary workaround for
1st generation non-ECAM-compliant platforms, the same mechanism
can be implemented through a static array of hooks, that, through
the same MCFG matching mechanism, initialize the pci_ops for
the given platform.

Furthermore, I suspect we do not even need a way to pass the
non-ECAM compliant config space resources to the OS (ie we can't
change FW anymore anyway in some platforms) so the quirks hooks
are likely to hardcode the required config space addresses for
the respective MCFG match.

It should be easier to implement (provided we find a place where
to add this static array of hooks matching MCFG, I suspect it is
going to be a file in drivers/pci/host but Tomasz and I need
input on that) and prevent abuse (since it is a static array of
hooks in a single place, it is easier to manage than section
entries).

Either that or we keep the section entry linker script approach
this series implements, which means that the quirks handling hook
will be added to a specific section instead of a static array,
I see no other option.

Last but not least, the MCFG table must not contain any region
that is not ECAM compliant, I understand we need quirks but
we must not abuse a standard table to provide the OS resources
that are really not ECAM compliant when they are supposed to be.

Tomasz will post a follow-up patch to this series implementing
the above, if you have any comments/concerns/questions or you see
anything wrong with what I say above please do chime in.

Thanks !
Lorenzo

Re: [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Lorenzo Pieralisi]

[+ Ard, Arnd]

On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki <tn@semihalf.com>
> 
> Some platforms may not be fully compliant with the generic PCI config
> operations. For these cases we implement a way to use custom map and
> accessor functions. The algorithm traverses the available quirk list,
> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
> oem_revision come from the MCFG table standard header. All quirks can be
> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
> contained. Example:
> 
> /* Custom PCI config ops */
> static struct pci_generic_ecam_ops foo_pci_ops = {
> 	.bus_shift	= 24,
> 	.pci_ops = {
> 		.map_bus = pci_ecam_map_bus,
> 		.read = foo_ecam_config_read,
> 		.write = foo_ecam_config_write,
> 	}
> };
> 
> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
> 			<domain_nr>, <bus_nr>);
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> 
> ---
> 
> Changes from v2 to v3:
> * Match against all three of oem_id, oem_table_id, and oem_revision.
> * Perform substring match, so padding oem_id and oem_table_id isn't
>   required. (Using min_t() thanks to Duc Dang's suggestion.)
> * Print when quirk matched.
> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>   when OEM and table IDs contain characters such as dash "-".
> * Move extern declaration to header and fix spelling and formatting to
>   satisfy checkpatch/coding style.
> ---
>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>  3 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index b5b376e..2a5d3dd 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -22,6 +22,10 @@
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +
> +/* Root pointer to the mapped MCFG table */
> +static struct acpi_table_mcfg *mcfg_table;
>  
>  /* Structure to hold entries from the MCFG table */
>  struct mcfg_entry {
> @@ -35,6 +39,46 @@ struct mcfg_entry {
>  /* List to save MCFG entries */
>  static LIST_HEAD(pci_mcfg_list);
>  
> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
> +				 struct acpi_table_header *h)
> +{
> +	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
> +	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
> +
> +	return (!strncmp(f->oem_id, h->oem_id, olen) &&
> +		!strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
> +		f->oem_revision == h->oem_revision);
> +}
> +
> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> +{
> +	int bus_num = root->secondary.start;
> +	int domain = root->segment;
> +	struct pci_cfg_fixup *f;
> +
> +	if (!mcfg_table)
> +		return &pci_generic_ecam_ops;
> +
> +	/*
> +	 * Match against platform specific quirks and return corresponding CAM
> +	 * ops.
> +	 *
> +	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
> +	 * table ID, and OEM revision from MCFG table standard header.
> +	 */
> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> +		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> +		    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> +		    pci_mcfg_fixup_match(f, &mcfg_table->header)) {
> +			pr_info("Handling %s %s r%d PCI MCFG quirks\n",
> +				f->oem_id, f->oem_table_id, f->oem_revision);
> +			return f->ops;
> +		}
> +	}
> +	/* No quirks, use ECAM */
> +	return &pci_generic_ecam_ops;
> +}
> +
>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  {
>  	struct mcfg_entry *e;
> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  
>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  {
> -	struct acpi_table_mcfg *mcfg;
>  	struct acpi_mcfg_allocation *mptr;
>  	struct mcfg_entry *e, *arr;
>  	int i, n;
> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  
>  	n = (header->length - sizeof(struct acpi_table_mcfg)) /
>  					sizeof(struct acpi_mcfg_allocation);
> -	mcfg = (struct acpi_table_mcfg *)header;
> -	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
> +	mcfg_table = (struct acpi_table_mcfg *)header;
> +	mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>  
>  	arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>  	if (!arr)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 6a67ab9..43604fc 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -300,6 +300,13 @@
>  		VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;	\
>  	}								\
>  									\
> +	/* ACPI MCFG quirks */						\
> +	.acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {	\
> +		VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;		\
> +		*(.acpi_fixup_mcfg)					\
> +		VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;		\
> +	}								\
> +									\
>  	/* Built-in firmware blobs */					\
>  	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
>  		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 7d63a66..dac851b 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>  
>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>  
>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>  {
> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>  	int (*prepare_resources)(struct acpi_pci_root_info *info);
>  };
>  
> +struct pci_cfg_fixup {
> +	struct pci_ecam_ops *ops;
> +	char *oem_id;
> +	char *oem_table_id;
> +	u32 oem_revision;
> +	int domain;
> +	int bus_num;
> +};
> +
> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
> +
> +#define PCI_MCFG_DOMAIN_ANY	-1
> +#define PCI_MCFG_BUS_ANY	-1
> +
> +/* Designate a routine to fix up buggy MCFG */
> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)	\
> +	static const struct pci_cfg_fixup			\
> +	__UNIQUE_ID(mcfg_fixup_)				\
> +	__used	__attribute__((__section__(".acpi_fixup_mcfg"),	\
> +				aligned((sizeof(void *))))) =	\
> +	{ ops, oem, table, rev, dom, bus }
> +

Ok, a couple of comments on this quirks handling mechanism.

Given that it is supposed to be just a temporary workaround for
1st generation non-ECAM-compliant platforms, the same mechanism
can be implemented through a static array of hooks, that, through
the same MCFG matching mechanism, initialize the pci_ops for
the given platform.

Furthermore, I suspect we do not even need a way to pass the
non-ECAM compliant config space resources to the OS (ie we can't
change FW anymore anyway in some platforms) so the quirks hooks
are likely to hardcode the required config space addresses for
the respective MCFG match.

It should be easier to implement (provided we find a place where
to add this static array of hooks matching MCFG, I suspect it is
going to be a file in drivers/pci/host but Tomasz and I need
input on that) and prevent abuse (since it is a static array of
hooks in a single place, it is easier to manage than section
entries).

Either that or we keep the section entry linker script approach
this series implements, which means that the quirks handling hook
will be added to a specific section instead of a static array,
I see no other option.

Last but not least, the MCFG table must not contain any region
that is not ECAM compliant, I understand we need quirks but
we must not abuse a standard table to provide the OS resources
that are really not ECAM compliant when they are supposed to be.

Tomasz will post a follow-up patch to this series implementing
the above, if you have any comments/concerns/questions or you see
anything wrong with what I say above please do chime in.

Thanks !
Lorenzo

Re: [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Lorenzo Pieralisi]

[+ Ard, Arnd]

On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki <tn@semihalf.com>
> 
> Some platforms may not be fully compliant with the generic PCI config
> operations. For these cases we implement a way to use custom map and
> accessor functions. The algorithm traverses the available quirk list,
> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
> oem_revision come from the MCFG table standard header. All quirks can be
> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
> contained. Example:
> 
> /* Custom PCI config ops */
> static struct pci_generic_ecam_ops foo_pci_ops = {
> 	.bus_shift	= 24,
> 	.pci_ops = {
> 		.map_bus = pci_ecam_map_bus,
> 		.read = foo_ecam_config_read,
> 		.write = foo_ecam_config_write,
> 	}
> };
> 
> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
> 			<domain_nr>, <bus_nr>);
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> 
> ---
> 
> Changes from v2 to v3:
> * Match against all three of oem_id, oem_table_id, and oem_revision.
> * Perform substring match, so padding oem_id and oem_table_id isn't
>   required. (Using min_t() thanks to Duc Dang's suggestion.)
> * Print when quirk matched.
> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>   when OEM and table IDs contain characters such as dash "-".
> * Move extern declaration to header and fix spelling and formatting to
>   satisfy checkpatch/coding style.
> ---
>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>  3 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index b5b376e..2a5d3dd 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -22,6 +22,10 @@
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +
> +/* Root pointer to the mapped MCFG table */
> +static struct acpi_table_mcfg *mcfg_table;
>  
>  /* Structure to hold entries from the MCFG table */
>  struct mcfg_entry {
> @@ -35,6 +39,46 @@ struct mcfg_entry {
>  /* List to save MCFG entries */
>  static LIST_HEAD(pci_mcfg_list);
>  
> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
> +				 struct acpi_table_header *h)
> +{
> +	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
> +	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
> +
> +	return (!strncmp(f->oem_id, h->oem_id, olen) &&
> +		!strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
> +		f->oem_revision == h->oem_revision);
> +}
> +
> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> +{
> +	int bus_num = root->secondary.start;
> +	int domain = root->segment;
> +	struct pci_cfg_fixup *f;
> +
> +	if (!mcfg_table)
> +		return &pci_generic_ecam_ops;
> +
> +	/*
> +	 * Match against platform specific quirks and return corresponding CAM
> +	 * ops.
> +	 *
> +	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
> +	 * table ID, and OEM revision from MCFG table standard header.
> +	 */
> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> +		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> +		    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> +		    pci_mcfg_fixup_match(f, &mcfg_table->header)) {
> +			pr_info("Handling %s %s r%d PCI MCFG quirks\n",
> +				f->oem_id, f->oem_table_id, f->oem_revision);
> +			return f->ops;
> +		}
> +	}
> +	/* No quirks, use ECAM */
> +	return &pci_generic_ecam_ops;
> +}
> +
>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  {
>  	struct mcfg_entry *e;
> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  
>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  {
> -	struct acpi_table_mcfg *mcfg;
>  	struct acpi_mcfg_allocation *mptr;
>  	struct mcfg_entry *e, *arr;
>  	int i, n;
> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  
>  	n = (header->length - sizeof(struct acpi_table_mcfg)) /
>  					sizeof(struct acpi_mcfg_allocation);
> -	mcfg = (struct acpi_table_mcfg *)header;
> -	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
> +	mcfg_table = (struct acpi_table_mcfg *)header;
> +	mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>  
>  	arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>  	if (!arr)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 6a67ab9..43604fc 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -300,6 +300,13 @@
>  		VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;	\
>  	}								\
>  									\
> +	/* ACPI MCFG quirks */						\
> +	.acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {	\
> +		VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;		\
> +		*(.acpi_fixup_mcfg)					\
> +		VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;		\
> +	}								\
> +									\
>  	/* Built-in firmware blobs */					\
>  	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
>  		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 7d63a66..dac851b 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>  
>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>  
>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>  {
> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>  	int (*prepare_resources)(struct acpi_pci_root_info *info);
>  };
>  
> +struct pci_cfg_fixup {
> +	struct pci_ecam_ops *ops;
> +	char *oem_id;
> +	char *oem_table_id;
> +	u32 oem_revision;
> +	int domain;
> +	int bus_num;
> +};
> +
> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
> +
> +#define PCI_MCFG_DOMAIN_ANY	-1
> +#define PCI_MCFG_BUS_ANY	-1
> +
> +/* Designate a routine to fix up buggy MCFG */
> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)	\
> +	static const struct pci_cfg_fixup			\
> +	__UNIQUE_ID(mcfg_fixup_)				\
> +	__used	__attribute__((__section__(".acpi_fixup_mcfg"),	\
> +				aligned((sizeof(void *))))) =	\
> +	{ ops, oem, table, rev, dom, bus }
> +

Ok, a couple of comments on this quirks handling mechanism.

Given that it is supposed to be just a temporary workaround for
1st generation non-ECAM-compliant platforms, the same mechanism
can be implemented through a static array of hooks, that, through
the same MCFG matching mechanism, initialize the pci_ops for
the given platform.

Furthermore, I suspect we do not even need a way to pass the
non-ECAM compliant config space resources to the OS (ie we can't
change FW anymore anyway in some platforms) so the quirks hooks
are likely to hardcode the required config space addresses for
the respective MCFG match.

It should be easier to implement (provided we find a place where
to add this static array of hooks matching MCFG, I suspect it is
going to be a file in drivers/pci/host but Tomasz and I need
input on that) and prevent abuse (since it is a static array of
hooks in a single place, it is easier to manage than section
entries).

Either that or we keep the section entry linker script approach
this series implements, which means that the quirks handling hook
will be added to a specific section instead of a static array,
I see no other option.

Last but not least, the MCFG table must not contain any region
that is not ECAM compliant, I understand we need quirks but
we must not abuse a standard table to provide the OS resources
that are really not ECAM compliant when they are supposed to be.

Tomasz will post a follow-up patch to this series implementing
the above, if you have any comments/concerns/questions or you see
anything wrong with what I say above please do chime in.

Thanks !
Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Lorenzo Pieralisi]

[+ Ard, Arnd]

On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
> From: Tomasz Nowicki <tn@semihalf.com>
> 
> Some platforms may not be fully compliant with the generic PCI config
> operations. For these cases we implement a way to use custom map and
> accessor functions. The algorithm traverses the available quirk list,
> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
> oem_revision come from the MCFG table standard header. All quirks can be
> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
> contained. Example:
> 
> /* Custom PCI config ops */
> static struct pci_generic_ecam_ops foo_pci_ops = {
> 	.bus_shift	= 24,
> 	.pci_ops = {
> 		.map_bus = pci_ecam_map_bus,
> 		.read = foo_ecam_config_read,
> 		.write = foo_ecam_config_write,
> 	}
> };
> 
> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
> 			<domain_nr>, <bus_nr>);
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> 
> ---
> 
> Changes from v2 to v3:
> * Match against all three of oem_id, oem_table_id, and oem_revision.
> * Perform substring match, so padding oem_id and oem_table_id isn't
>   required. (Using min_t() thanks to Duc Dang's suggestion.)
> * Print when quirk matched.
> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>   when OEM and table IDs contain characters such as dash "-".
> * Move extern declaration to header and fix spelling and formatting to
>   satisfy checkpatch/coding style.
> ---
>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>  3 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index b5b376e..2a5d3dd 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -22,6 +22,10 @@
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +
> +/* Root pointer to the mapped MCFG table */
> +static struct acpi_table_mcfg *mcfg_table;
>  
>  /* Structure to hold entries from the MCFG table */
>  struct mcfg_entry {
> @@ -35,6 +39,46 @@ struct mcfg_entry {
>  /* List to save MCFG entries */
>  static LIST_HEAD(pci_mcfg_list);
>  
> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
> +				 struct acpi_table_header *h)
> +{
> +	int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
> +	int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
> +
> +	return (!strncmp(f->oem_id, h->oem_id, olen) &&
> +		!strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
> +		f->oem_revision == h->oem_revision);
> +}
> +
> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> +{
> +	int bus_num = root->secondary.start;
> +	int domain = root->segment;
> +	struct pci_cfg_fixup *f;
> +
> +	if (!mcfg_table)
> +		return &pci_generic_ecam_ops;
> +
> +	/*
> +	 * Match against platform specific quirks and return corresponding CAM
> +	 * ops.
> +	 *
> +	 * First match against PCI topology <domain:bus> then use OEM ID, OEM
> +	 * table ID, and OEM revision from MCFG table standard header.
> +	 */
> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> +		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> +		    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> +		    pci_mcfg_fixup_match(f, &mcfg_table->header)) {
> +			pr_info("Handling %s %s r%d PCI MCFG quirks\n",
> +				f->oem_id, f->oem_table_id, f->oem_revision);
> +			return f->ops;
> +		}
> +	}
> +	/* No quirks, use ECAM */
> +	return &pci_generic_ecam_ops;
> +}
> +
>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  {
>  	struct mcfg_entry *e;
> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>  
>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  {
> -	struct acpi_table_mcfg *mcfg;
>  	struct acpi_mcfg_allocation *mptr;
>  	struct mcfg_entry *e, *arr;
>  	int i, n;
> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>  
>  	n = (header->length - sizeof(struct acpi_table_mcfg)) /
>  					sizeof(struct acpi_mcfg_allocation);
> -	mcfg = (struct acpi_table_mcfg *)header;
> -	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
> +	mcfg_table = (struct acpi_table_mcfg *)header;
> +	mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>  
>  	arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>  	if (!arr)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 6a67ab9..43604fc 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -300,6 +300,13 @@
>  		VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;	\
>  	}								\
>  									\
> +	/* ACPI MCFG quirks */						\
> +	.acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {	\
> +		VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;		\
> +		*(.acpi_fixup_mcfg)					\
> +		VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;		\
> +	}								\
> +									\
>  	/* Built-in firmware blobs */					\
>  	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
>  		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 7d63a66..dac851b 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>  
>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>  
>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>  {
> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>  	int (*prepare_resources)(struct acpi_pci_root_info *info);
>  };
>  
> +struct pci_cfg_fixup {
> +	struct pci_ecam_ops *ops;
> +	char *oem_id;
> +	char *oem_table_id;
> +	u32 oem_revision;
> +	int domain;
> +	int bus_num;
> +};
> +
> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
> +
> +#define PCI_MCFG_DOMAIN_ANY	-1
> +#define PCI_MCFG_BUS_ANY	-1
> +
> +/* Designate a routine to fix up buggy MCFG */
> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)	\
> +	static const struct pci_cfg_fixup			\
> +	__UNIQUE_ID(mcfg_fixup_)				\
> +	__used	__attribute__((__section__(".acpi_fixup_mcfg"),	\
> +				aligned((sizeof(void *))))) =	\
> +	{ ops, oem, table, rev, dom, bus }
> +

Ok, a couple of comments on this quirks handling mechanism.

Given that it is supposed to be just a temporary workaround for
1st generation non-ECAM-compliant platforms, the same mechanism
can be implemented through a static array of hooks, that, through
the same MCFG matching mechanism, initialize the pci_ops for
the given platform.

Furthermore, I suspect we do not even need a way to pass the
non-ECAM compliant config space resources to the OS (ie we can't
change FW anymore anyway in some platforms) so the quirks hooks
are likely to hardcode the required config space addresses for
the respective MCFG match.

It should be easier to implement (provided we find a place where
to add this static array of hooks matching MCFG, I suspect it is
going to be a file in drivers/pci/host but Tomasz and I need
input on that) and prevent abuse (since it is a static array of
hooks in a single place, it is easier to manage than section
entries).

Either that or we keep the section entry linker script approach
this series implements, which means that the quirks handling hook
will be added to a specific section instead of a static array,
I see no other option.

Last but not least, the MCFG table must not contain any region
that is not ECAM compliant, I understand we need quirks but
we must not abuse a standard table to provide the OS resources
that are really not ECAM compliant when they are supposed to be.

Tomasz will post a follow-up patch to this series implementing
the above, if you have any comments/concerns/questions or you see
anything wrong with what I say above please do chime in.

Thanks !
Lorenzo

Re: [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Ard Biesheuvel]

On 20 June 2016 at 19:16, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> [+ Ard, Arnd]
>
> On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki <tn@semihalf.com>
>>
>> Some platforms may not be fully compliant with the generic PCI config
>> operations. For these cases we implement a way to use custom map and
>> accessor functions. The algorithm traverses the available quirk list,
>> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
>> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
>> oem_revision come from the MCFG table standard header. All quirks can be
>> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
>> contained. Example:
>>
>> /* Custom PCI config ops */
>> static struct pci_generic_ecam_ops foo_pci_ops = {
>>       .bus_shift      = 24,
>>       .pci_ops = {
>>               .map_bus = pci_ecam_map_bus,
>>               .read = foo_ecam_config_read,
>>               .write = foo_ecam_config_write,
>>       }
>> };
>>
>> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
>>                       <domain_nr>, <bus_nr>);
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>
>> ---
>>
>> Changes from v2 to v3:
>> * Match against all three of oem_id, oem_table_id, and oem_revision.
>> * Perform substring match, so padding oem_id and oem_table_id isn't
>>   required. (Using min_t() thanks to Duc Dang's suggestion.)
>> * Print when quirk matched.
>> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>>   when OEM and table IDs contain characters such as dash "-".
>> * Move extern declaration to header and fix spelling and formatting to
>>   satisfy checkpatch/coding style.
>> ---
>>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>>  3 files changed, 77 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index b5b376e..2a5d3dd 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -22,6 +22,10 @@
>>  #include <linux/kernel.h>
>>  #include <linux/pci.h>
>>  #include <linux/pci-acpi.h>
>> +#include <linux/pci-ecam.h>
>> +
>> +/* Root pointer to the mapped MCFG table */
>> +static struct acpi_table_mcfg *mcfg_table;
>>
>>  /* Structure to hold entries from the MCFG table */
>>  struct mcfg_entry {
>> @@ -35,6 +39,46 @@ struct mcfg_entry {
>>  /* List to save MCFG entries */
>>  static LIST_HEAD(pci_mcfg_list);
>>
>> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
>> +                              struct acpi_table_header *h)
>> +{
>> +     int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
>> +     int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
>> +
>> +     return (!strncmp(f->oem_id, h->oem_id, olen) &&
>> +             !strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
>> +             f->oem_revision == h->oem_revision);
>> +}
>> +
>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>> +{
>> +     int bus_num = root->secondary.start;
>> +     int domain = root->segment;
>> +     struct pci_cfg_fixup *f;
>> +
>> +     if (!mcfg_table)
>> +             return &pci_generic_ecam_ops;
>> +
>> +     /*
>> +      * Match against platform specific quirks and return corresponding CAM
>> +      * ops.
>> +      *
>> +      * First match against PCI topology <domain:bus> then use OEM ID, OEM
>> +      * table ID, and OEM revision from MCFG table standard header.
>> +      */
>> +     for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
>> +             if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
>> +                 (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
>> +                 pci_mcfg_fixup_match(f, &mcfg_table->header)) {
>> +                     pr_info("Handling %s %s r%d PCI MCFG quirks\n",
>> +                             f->oem_id, f->oem_table_id, f->oem_revision);
>> +                     return f->ops;
>> +             }
>> +     }
>> +     /* No quirks, use ECAM */
>> +     return &pci_generic_ecam_ops;
>> +}
>> +
>>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>>  {
>>       struct mcfg_entry *e;
>> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>>
>>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>  {
>> -     struct acpi_table_mcfg *mcfg;
>>       struct acpi_mcfg_allocation *mptr;
>>       struct mcfg_entry *e, *arr;
>>       int i, n;
>> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>
>>       n = (header->length - sizeof(struct acpi_table_mcfg)) /
>>                                       sizeof(struct acpi_mcfg_allocation);
>> -     mcfg = (struct acpi_table_mcfg *)header;
>> -     mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
>> +     mcfg_table = (struct acpi_table_mcfg *)header;
>> +     mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>>
>>       arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>>       if (!arr)
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index 6a67ab9..43604fc 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -300,6 +300,13 @@
>>               VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;      \
>>       }                                                               \
>>                                                                       \
>> +     /* ACPI MCFG quirks */                                          \
>> +     .acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {      \
>> +             VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;           \
>> +             *(.acpi_fixup_mcfg)                                     \
>> +             VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;             \
>> +     }                                                               \
>> +                                                                     \
>>       /* Built-in firmware blobs */                                   \
>>       .builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {      \
>>               VMLINUX_SYMBOL(__start_builtin_fw) = .;                 \
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 7d63a66..dac851b 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>>
>>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
>> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>>
>>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>>  {
>> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>>       int (*prepare_resources)(struct acpi_pci_root_info *info);
>>  };
>>
>> +struct pci_cfg_fixup {
>> +     struct pci_ecam_ops *ops;
>> +     char *oem_id;
>> +     char *oem_table_id;
>> +     u32 oem_revision;
>> +     int domain;
>> +     int bus_num;
>> +};
>> +
>> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
>> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
>> +
>> +#define PCI_MCFG_DOMAIN_ANY  -1
>> +#define PCI_MCFG_BUS_ANY     -1
>> +
>> +/* Designate a routine to fix up buggy MCFG */
>> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)      \
>> +     static const struct pci_cfg_fixup                       \
>> +     __UNIQUE_ID(mcfg_fixup_)                                \
>> +     __used  __attribute__((__section__(".acpi_fixup_mcfg"), \
>> +                             aligned((sizeof(void *))))) =   \
>> +     { ops, oem, table, rev, dom, bus }
>> +
>
> Ok, a couple of comments on this quirks handling mechanism.
>
> Given that it is supposed to be just a temporary workaround for
> 1st generation non-ECAM-compliant platforms, the same mechanism
> can be implemented through a static array of hooks, that, through
> the same MCFG matching mechanism, initialize the pci_ops for
> the given platform.
>
> Furthermore, I suspect we do not even need a way to pass the
> non-ECAM compliant config space resources to the OS (ie we can't
> change FW anymore anyway in some platforms) so the quirks hooks
> are likely to hardcode the required config space addresses for
> the respective MCFG match.
>

As discussed off-list, I strongly second the idea to have a static
quirks array for hardware/firmware combos that are currently in the
pipeline, and for which we need special handling in software to be
able to drive them at all. This should be based on exact OEM table/rev
id matches, and contain all supplementary information required to
implement the quirk in the kernel source code. Anything that could
potentially be used to 'cheat', e.g., an OEM id triggering a substring
match on a platform that we currently don't know about, or things like
_DSD properties to parametrize the quirks implementation are out of
the question IMO

> It should be easier to implement (provided we find a place where
> to add this static array of hooks matching MCFG, I suspect it is
> going to be a file in drivers/pci/host but Tomasz and I need
> input on that) and prevent abuse (since it is a static array of
> hooks in a single place, it is easier to manage than section
> entries).
>
> Either that or we keep the section entry linker script approach
> this series implements, which means that the quirks handling hook
> will be added to a specific section instead of a static array,
> I see no other option.
>

I know __weak functions are unpopular, but in this case, having a
__weak empty function in the MCFG/ECAM core code, and overriding it in
arch/arm64/xxx/mcfg-quirks.c does not sound unreasonable to me.

> Last but not least, the MCFG table must not contain any region
> that is not ECAM compliant, I understand we need quirks but
> we must not abuse a standard table to provide the OS resources
> that are really not ECAM compliant when they are supposed to be.
>
> Tomasz will post a follow-up patch to this series implementing
> the above, if you have any comments/concerns/questions or you see
> anything wrong with what I say above please do chime in.
>
> Thanks !
> Lorenzo

Re: [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Ard Biesheuvel]

On 20 June 2016 at 19:16, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> [+ Ard, Arnd]
>
> On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki <tn@semihalf.com>
>>
>> Some platforms may not be fully compliant with the generic PCI config
>> operations. For these cases we implement a way to use custom map and
>> accessor functions. The algorithm traverses the available quirk list,
>> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
>> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
>> oem_revision come from the MCFG table standard header. All quirks can be
>> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
>> contained. Example:
>>
>> /* Custom PCI config ops */
>> static struct pci_generic_ecam_ops foo_pci_ops = {
>>       .bus_shift      = 24,
>>       .pci_ops = {
>>               .map_bus = pci_ecam_map_bus,
>>               .read = foo_ecam_config_read,
>>               .write = foo_ecam_config_write,
>>       }
>> };
>>
>> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
>>                       <domain_nr>, <bus_nr>);
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>
>> ---
>>
>> Changes from v2 to v3:
>> * Match against all three of oem_id, oem_table_id, and oem_revision.
>> * Perform substring match, so padding oem_id and oem_table_id isn't
>>   required. (Using min_t() thanks to Duc Dang's suggestion.)
>> * Print when quirk matched.
>> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>>   when OEM and table IDs contain characters such as dash "-".
>> * Move extern declaration to header and fix spelling and formatting to
>>   satisfy checkpatch/coding style.
>> ---
>>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>>  3 files changed, 77 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index b5b376e..2a5d3dd 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -22,6 +22,10 @@
>>  #include <linux/kernel.h>
>>  #include <linux/pci.h>
>>  #include <linux/pci-acpi.h>
>> +#include <linux/pci-ecam.h>
>> +
>> +/* Root pointer to the mapped MCFG table */
>> +static struct acpi_table_mcfg *mcfg_table;
>>
>>  /* Structure to hold entries from the MCFG table */
>>  struct mcfg_entry {
>> @@ -35,6 +39,46 @@ struct mcfg_entry {
>>  /* List to save MCFG entries */
>>  static LIST_HEAD(pci_mcfg_list);
>>
>> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
>> +                              struct acpi_table_header *h)
>> +{
>> +     int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
>> +     int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
>> +
>> +     return (!strncmp(f->oem_id, h->oem_id, olen) &&
>> +             !strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
>> +             f->oem_revision == h->oem_revision);
>> +}
>> +
>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>> +{
>> +     int bus_num = root->secondary.start;
>> +     int domain = root->segment;
>> +     struct pci_cfg_fixup *f;
>> +
>> +     if (!mcfg_table)
>> +             return &pci_generic_ecam_ops;
>> +
>> +     /*
>> +      * Match against platform specific quirks and return corresponding CAM
>> +      * ops.
>> +      *
>> +      * First match against PCI topology <domain:bus> then use OEM ID, OEM
>> +      * table ID, and OEM revision from MCFG table standard header.
>> +      */
>> +     for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
>> +             if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
>> +                 (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
>> +                 pci_mcfg_fixup_match(f, &mcfg_table->header)) {
>> +                     pr_info("Handling %s %s r%d PCI MCFG quirks\n",
>> +                             f->oem_id, f->oem_table_id, f->oem_revision);
>> +                     return f->ops;
>> +             }
>> +     }
>> +     /* No quirks, use ECAM */
>> +     return &pci_generic_ecam_ops;
>> +}
>> +
>>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>>  {
>>       struct mcfg_entry *e;
>> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>>
>>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>  {
>> -     struct acpi_table_mcfg *mcfg;
>>       struct acpi_mcfg_allocation *mptr;
>>       struct mcfg_entry *e, *arr;
>>       int i, n;
>> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>
>>       n = (header->length - sizeof(struct acpi_table_mcfg)) /
>>                                       sizeof(struct acpi_mcfg_allocation);
>> -     mcfg = (struct acpi_table_mcfg *)header;
>> -     mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
>> +     mcfg_table = (struct acpi_table_mcfg *)header;
>> +     mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>>
>>       arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>>       if (!arr)
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index 6a67ab9..43604fc 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -300,6 +300,13 @@
>>               VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;      \
>>       }                                                               \
>>                                                                       \
>> +     /* ACPI MCFG quirks */                                          \
>> +     .acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {      \
>> +             VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;           \
>> +             *(.acpi_fixup_mcfg)                                     \
>> +             VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;             \
>> +     }                                                               \
>> +                                                                     \
>>       /* Built-in firmware blobs */                                   \
>>       .builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {      \
>>               VMLINUX_SYMBOL(__start_builtin_fw) = .;                 \
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 7d63a66..dac851b 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>>
>>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
>> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>>
>>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>>  {
>> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>>       int (*prepare_resources)(struct acpi_pci_root_info *info);
>>  };
>>
>> +struct pci_cfg_fixup {
>> +     struct pci_ecam_ops *ops;
>> +     char *oem_id;
>> +     char *oem_table_id;
>> +     u32 oem_revision;
>> +     int domain;
>> +     int bus_num;
>> +};
>> +
>> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
>> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
>> +
>> +#define PCI_MCFG_DOMAIN_ANY  -1
>> +#define PCI_MCFG_BUS_ANY     -1
>> +
>> +/* Designate a routine to fix up buggy MCFG */
>> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)      \
>> +     static const struct pci_cfg_fixup                       \
>> +     __UNIQUE_ID(mcfg_fixup_)                                \
>> +     __used  __attribute__((__section__(".acpi_fixup_mcfg"), \
>> +                             aligned((sizeof(void *))))) =   \
>> +     { ops, oem, table, rev, dom, bus }
>> +
>
> Ok, a couple of comments on this quirks handling mechanism.
>
> Given that it is supposed to be just a temporary workaround for
> 1st generation non-ECAM-compliant platforms, the same mechanism
> can be implemented through a static array of hooks, that, through
> the same MCFG matching mechanism, initialize the pci_ops for
> the given platform.
>
> Furthermore, I suspect we do not even need a way to pass the
> non-ECAM compliant config space resources to the OS (ie we can't
> change FW anymore anyway in some platforms) so the quirks hooks
> are likely to hardcode the required config space addresses for
> the respective MCFG match.
>

As discussed off-list, I strongly second the idea to have a static
quirks array for hardware/firmware combos that are currently in the
pipeline, and for which we need special handling in software to be
able to drive them at all. This should be based on exact OEM table/rev
id matches, and contain all supplementary information required to
implement the quirk in the kernel source code. Anything that could
potentially be used to 'cheat', e.g., an OEM id triggering a substring
match on a platform that we currently don't know about, or things like
_DSD properties to parametrize the quirks implementation are out of
the question IMO

> It should be easier to implement (provided we find a place where
> to add this static array of hooks matching MCFG, I suspect it is
> going to be a file in drivers/pci/host but Tomasz and I need
> input on that) and prevent abuse (since it is a static array of
> hooks in a single place, it is easier to manage than section
> entries).
>
> Either that or we keep the section entry linker script approach
> this series implements, which means that the quirks handling hook
> will be added to a specific section instead of a static array,
> I see no other option.
>

I know __weak functions are unpopular, but in this case, having a
__weak empty function in the MCFG/ECAM core code, and overriding it in
arch/arm64/xxx/mcfg-quirks.c does not sound unreasonable to me.

> Last but not least, the MCFG table must not contain any region
> that is not ECAM compliant, I understand we need quirks but
> we must not abuse a standard table to provide the OS resources
> that are really not ECAM compliant when they are supposed to be.
>
> Tomasz will post a follow-up patch to this series implementing
> the above, if you have any comments/concerns/questions or you see
> anything wrong with what I say above please do chime in.
>
> Thanks !
> Lorenzo

Re: [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Ard Biesheuvel]

On 20 June 2016 at 19:16, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> [+ Ard, Arnd]
>
> On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki <tn@semihalf.com>
>>
>> Some platforms may not be fully compliant with the generic PCI config
>> operations. For these cases we implement a way to use custom map and
>> accessor functions. The algorithm traverses the available quirk list,
>> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
>> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
>> oem_revision come from the MCFG table standard header. All quirks can be
>> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
>> contained. Example:
>>
>> /* Custom PCI config ops */
>> static struct pci_generic_ecam_ops foo_pci_ops = {
>>       .bus_shift      = 24,
>>       .pci_ops = {
>>               .map_bus = pci_ecam_map_bus,
>>               .read = foo_ecam_config_read,
>>               .write = foo_ecam_config_write,
>>       }
>> };
>>
>> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
>>                       <domain_nr>, <bus_nr>);
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>
>> ---
>>
>> Changes from v2 to v3:
>> * Match against all three of oem_id, oem_table_id, and oem_revision.
>> * Perform substring match, so padding oem_id and oem_table_id isn't
>>   required. (Using min_t() thanks to Duc Dang's suggestion.)
>> * Print when quirk matched.
>> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>>   when OEM and table IDs contain characters such as dash "-".
>> * Move extern declaration to header and fix spelling and formatting to
>>   satisfy checkpatch/coding style.
>> ---
>>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>>  3 files changed, 77 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index b5b376e..2a5d3dd 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -22,6 +22,10 @@
>>  #include <linux/kernel.h>
>>  #include <linux/pci.h>
>>  #include <linux/pci-acpi.h>
>> +#include <linux/pci-ecam.h>
>> +
>> +/* Root pointer to the mapped MCFG table */
>> +static struct acpi_table_mcfg *mcfg_table;
>>
>>  /* Structure to hold entries from the MCFG table */
>>  struct mcfg_entry {
>> @@ -35,6 +39,46 @@ struct mcfg_entry {
>>  /* List to save MCFG entries */
>>  static LIST_HEAD(pci_mcfg_list);
>>
>> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
>> +                              struct acpi_table_header *h)
>> +{
>> +     int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
>> +     int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
>> +
>> +     return (!strncmp(f->oem_id, h->oem_id, olen) &&
>> +             !strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
>> +             f->oem_revision == h->oem_revision);
>> +}
>> +
>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>> +{
>> +     int bus_num = root->secondary.start;
>> +     int domain = root->segment;
>> +     struct pci_cfg_fixup *f;
>> +
>> +     if (!mcfg_table)
>> +             return &pci_generic_ecam_ops;
>> +
>> +     /*
>> +      * Match against platform specific quirks and return corresponding CAM
>> +      * ops.
>> +      *
>> +      * First match against PCI topology <domain:bus> then use OEM ID, OEM
>> +      * table ID, and OEM revision from MCFG table standard header.
>> +      */
>> +     for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
>> +             if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
>> +                 (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
>> +                 pci_mcfg_fixup_match(f, &mcfg_table->header)) {
>> +                     pr_info("Handling %s %s r%d PCI MCFG quirks\n",
>> +                             f->oem_id, f->oem_table_id, f->oem_revision);
>> +                     return f->ops;
>> +             }
>> +     }
>> +     /* No quirks, use ECAM */
>> +     return &pci_generic_ecam_ops;
>> +}
>> +
>>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>>  {
>>       struct mcfg_entry *e;
>> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>>
>>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>  {
>> -     struct acpi_table_mcfg *mcfg;
>>       struct acpi_mcfg_allocation *mptr;
>>       struct mcfg_entry *e, *arr;
>>       int i, n;
>> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>
>>       n = (header->length - sizeof(struct acpi_table_mcfg)) /
>>                                       sizeof(struct acpi_mcfg_allocation);
>> -     mcfg = (struct acpi_table_mcfg *)header;
>> -     mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
>> +     mcfg_table = (struct acpi_table_mcfg *)header;
>> +     mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>>
>>       arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>>       if (!arr)
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index 6a67ab9..43604fc 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -300,6 +300,13 @@
>>               VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;      \
>>       }                                                               \
>>                                                                       \
>> +     /* ACPI MCFG quirks */                                          \
>> +     .acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {      \
>> +             VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;           \
>> +             *(.acpi_fixup_mcfg)                                     \
>> +             VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;             \
>> +     }                                                               \
>> +                                                                     \
>>       /* Built-in firmware blobs */                                   \
>>       .builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {      \
>>               VMLINUX_SYMBOL(__start_builtin_fw) = .;                 \
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 7d63a66..dac851b 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>>
>>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
>> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>>
>>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>>  {
>> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>>       int (*prepare_resources)(struct acpi_pci_root_info *info);
>>  };
>>
>> +struct pci_cfg_fixup {
>> +     struct pci_ecam_ops *ops;
>> +     char *oem_id;
>> +     char *oem_table_id;
>> +     u32 oem_revision;
>> +     int domain;
>> +     int bus_num;
>> +};
>> +
>> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
>> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
>> +
>> +#define PCI_MCFG_DOMAIN_ANY  -1
>> +#define PCI_MCFG_BUS_ANY     -1
>> +
>> +/* Designate a routine to fix up buggy MCFG */
>> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)      \
>> +     static const struct pci_cfg_fixup                       \
>> +     __UNIQUE_ID(mcfg_fixup_)                                \
>> +     __used  __attribute__((__section__(".acpi_fixup_mcfg"), \
>> +                             aligned((sizeof(void *))))) =   \
>> +     { ops, oem, table, rev, dom, bus }
>> +
>
> Ok, a couple of comments on this quirks handling mechanism.
>
> Given that it is supposed to be just a temporary workaround for
> 1st generation non-ECAM-compliant platforms, the same mechanism
> can be implemented through a static array of hooks, that, through
> the same MCFG matching mechanism, initialize the pci_ops for
> the given platform.
>
> Furthermore, I suspect we do not even need a way to pass the
> non-ECAM compliant config space resources to the OS (ie we can't
> change FW anymore anyway in some platforms) so the quirks hooks
> are likely to hardcode the required config space addresses for
> the respective MCFG match.
>

As discussed off-list, I strongly second the idea to have a static
quirks array for hardware/firmware combos that are currently in the
pipeline, and for which we need special handling in software to be
able to drive them at all. This should be based on exact OEM table/rev
id matches, and contain all supplementary information required to
implement the quirk in the kernel source code. Anything that could
potentially be used to 'cheat', e.g., an OEM id triggering a substring
match on a platform that we currently don't know about, or things like
_DSD properties to parametrize the quirks implementation are out of
the question IMO

> It should be easier to implement (provided we find a place where
> to add this static array of hooks matching MCFG, I suspect it is
> going to be a file in drivers/pci/host but Tomasz and I need
> input on that) and prevent abuse (since it is a static array of
> hooks in a single place, it is easier to manage than section
> entries).
>
> Either that or we keep the section entry linker script approach
> this series implements, which means that the quirks handling hook
> will be added to a specific section instead of a static array,
> I see no other option.
>

I know __weak functions are unpopular, but in this case, having a
__weak empty function in the MCFG/ECAM core code, and overriding it in
arch/arm64/xxx/mcfg-quirks.c does not sound unreasonable to me.

> Last but not least, the MCFG table must not contain any region
> that is not ECAM compliant, I understand we need quirks but
> we must not abuse a standard table to provide the OS resources
> that are really not ECAM compliant when they are supposed to be.
>
> Tomasz will post a follow-up patch to this series implementing
> the above, if you have any comments/concerns/questions or you see
> anything wrong with what I say above please do chime in.
>
> Thanks !
> Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Ard Biesheuvel]

On 20 June 2016 at 19:16, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> [+ Ard, Arnd]
>
> On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki <tn@semihalf.com>
>>
>> Some platforms may not be fully compliant with the generic PCI config
>> operations. For these cases we implement a way to use custom map and
>> accessor functions. The algorithm traverses the available quirk list,
>> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
>> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
>> oem_revision come from the MCFG table standard header. All quirks can be
>> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
>> contained. Example:
>>
>> /* Custom PCI config ops */
>> static struct pci_generic_ecam_ops foo_pci_ops = {
>>       .bus_shift      = 24,
>>       .pci_ops = {
>>               .map_bus = pci_ecam_map_bus,
>>               .read = foo_ecam_config_read,
>>               .write = foo_ecam_config_write,
>>       }
>> };
>>
>> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
>>                       <domain_nr>, <bus_nr>);
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>
>> ---
>>
>> Changes from v2 to v3:
>> * Match against all three of oem_id, oem_table_id, and oem_revision.
>> * Perform substring match, so padding oem_id and oem_table_id isn't
>>   required. (Using min_t() thanks to Duc Dang's suggestion.)
>> * Print when quirk matched.
>> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>>   when OEM and table IDs contain characters such as dash "-".
>> * Move extern declaration to header and fix spelling and formatting to
>>   satisfy checkpatch/coding style.
>> ---
>>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>>  3 files changed, 77 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index b5b376e..2a5d3dd 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -22,6 +22,10 @@
>>  #include <linux/kernel.h>
>>  #include <linux/pci.h>
>>  #include <linux/pci-acpi.h>
>> +#include <linux/pci-ecam.h>
>> +
>> +/* Root pointer to the mapped MCFG table */
>> +static struct acpi_table_mcfg *mcfg_table;
>>
>>  /* Structure to hold entries from the MCFG table */
>>  struct mcfg_entry {
>> @@ -35,6 +39,46 @@ struct mcfg_entry {
>>  /* List to save MCFG entries */
>>  static LIST_HEAD(pci_mcfg_list);
>>
>> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
>> +                              struct acpi_table_header *h)
>> +{
>> +     int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
>> +     int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
>> +
>> +     return (!strncmp(f->oem_id, h->oem_id, olen) &&
>> +             !strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
>> +             f->oem_revision == h->oem_revision);
>> +}
>> +
>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>> +{
>> +     int bus_num = root->secondary.start;
>> +     int domain = root->segment;
>> +     struct pci_cfg_fixup *f;
>> +
>> +     if (!mcfg_table)
>> +             return &pci_generic_ecam_ops;
>> +
>> +     /*
>> +      * Match against platform specific quirks and return corresponding CAM
>> +      * ops.
>> +      *
>> +      * First match against PCI topology <domain:bus> then use OEM ID, OEM
>> +      * table ID, and OEM revision from MCFG table standard header.
>> +      */
>> +     for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
>> +             if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
>> +                 (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
>> +                 pci_mcfg_fixup_match(f, &mcfg_table->header)) {
>> +                     pr_info("Handling %s %s r%d PCI MCFG quirks\n",
>> +                             f->oem_id, f->oem_table_id, f->oem_revision);
>> +                     return f->ops;
>> +             }
>> +     }
>> +     /* No quirks, use ECAM */
>> +     return &pci_generic_ecam_ops;
>> +}
>> +
>>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>>  {
>>       struct mcfg_entry *e;
>> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>>
>>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>  {
>> -     struct acpi_table_mcfg *mcfg;
>>       struct acpi_mcfg_allocation *mptr;
>>       struct mcfg_entry *e, *arr;
>>       int i, n;
>> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>
>>       n = (header->length - sizeof(struct acpi_table_mcfg)) /
>>                                       sizeof(struct acpi_mcfg_allocation);
>> -     mcfg = (struct acpi_table_mcfg *)header;
>> -     mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
>> +     mcfg_table = (struct acpi_table_mcfg *)header;
>> +     mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>>
>>       arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>>       if (!arr)
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index 6a67ab9..43604fc 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -300,6 +300,13 @@
>>               VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;      \
>>       }                                                               \
>>                                                                       \
>> +     /* ACPI MCFG quirks */                                          \
>> +     .acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {      \
>> +             VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;           \
>> +             *(.acpi_fixup_mcfg)                                     \
>> +             VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;             \
>> +     }                                                               \
>> +                                                                     \
>>       /* Built-in firmware blobs */                                   \
>>       .builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {      \
>>               VMLINUX_SYMBOL(__start_builtin_fw) = .;                 \
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 7d63a66..dac851b 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>>
>>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
>> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>>
>>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>>  {
>> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>>       int (*prepare_resources)(struct acpi_pci_root_info *info);
>>  };
>>
>> +struct pci_cfg_fixup {
>> +     struct pci_ecam_ops *ops;
>> +     char *oem_id;
>> +     char *oem_table_id;
>> +     u32 oem_revision;
>> +     int domain;
>> +     int bus_num;
>> +};
>> +
>> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
>> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
>> +
>> +#define PCI_MCFG_DOMAIN_ANY  -1
>> +#define PCI_MCFG_BUS_ANY     -1
>> +
>> +/* Designate a routine to fix up buggy MCFG */
>> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)      \
>> +     static const struct pci_cfg_fixup                       \
>> +     __UNIQUE_ID(mcfg_fixup_)                                \
>> +     __used  __attribute__((__section__(".acpi_fixup_mcfg"), \
>> +                             aligned((sizeof(void *))))) =   \
>> +     { ops, oem, table, rev, dom, bus }
>> +
>
> Ok, a couple of comments on this quirks handling mechanism.
>
> Given that it is supposed to be just a temporary workaround for
> 1st generation non-ECAM-compliant platforms, the same mechanism
> can be implemented through a static array of hooks, that, through
> the same MCFG matching mechanism, initialize the pci_ops for
> the given platform.
>
> Furthermore, I suspect we do not even need a way to pass the
> non-ECAM compliant config space resources to the OS (ie we can't
> change FW anymore anyway in some platforms) so the quirks hooks
> are likely to hardcode the required config space addresses for
> the respective MCFG match.
>

As discussed off-list, I strongly second the idea to have a static
quirks array for hardware/firmware combos that are currently in the
pipeline, and for which we need special handling in software to be
able to drive them at all. This should be based on exact OEM table/rev
id matches, and contain all supplementary information required to
implement the quirk in the kernel source code. Anything that could
potentially be used to 'cheat', e.g., an OEM id triggering a substring
match on a platform that we currently don't know about, or things like
_DSD properties to parametrize the quirks implementation are out of
the question IMO

> It should be easier to implement (provided we find a place where
> to add this static array of hooks matching MCFG, I suspect it is
> going to be a file in drivers/pci/host but Tomasz and I need
> input on that) and prevent abuse (since it is a static array of
> hooks in a single place, it is easier to manage than section
> entries).
>
> Either that or we keep the section entry linker script approach
> this series implements, which means that the quirks handling hook
> will be added to a specific section instead of a static array,
> I see no other option.
>

I know __weak functions are unpopular, but in this case, having a
__weak empty function in the MCFG/ECAM core code, and overriding it in
arch/arm64/xxx/mcfg-quirks.c does not sound unreasonable to me.

> Last but not least, the MCFG table must not contain any region
> that is not ECAM compliant, I understand we need quirks but
> we must not abuse a standard table to provide the OS resources
> that are really not ECAM compliant when they are supposed to be.
>
> Tomasz will post a follow-up patch to this series implementing
> the above, if you have any comments/concerns/questions or you see
> anything wrong with what I say above please do chime in.
>
> Thanks !
> Lorenzo

[RFC PATCH v3 1/2] ACPI/PCI: Check platform specific ECAM quirks

[Ard Biesheuvel]

On 20 June 2016 at 19:16, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> [+ Ard, Arnd]
>
> On Wed, Jun 15, 2016 at 11:34:10AM -0400, Christopher Covington wrote:
>> From: Tomasz Nowicki <tn@semihalf.com>
>>
>> Some platforms may not be fully compliant with the generic PCI config
>> operations. For these cases we implement a way to use custom map and
>> accessor functions. The algorithm traverses the available quirk list,
>> matches against <oem_id, oem_table_id, oem_revision, domain, bus number>
>> tuples and returns corresponding PCI config ops. oem_id, oem_table_id, and
>> oem_revision come from the MCFG table standard header. All quirks can be
>> defined using the DECLARE_ACPI_MCFG_FIXUP() macro and are kept self
>> contained. Example:
>>
>> /* Custom PCI config ops */
>> static struct pci_generic_ecam_ops foo_pci_ops = {
>>       .bus_shift      = 24,
>>       .pci_ops = {
>>               .map_bus = pci_ecam_map_bus,
>>               .read = foo_ecam_config_read,
>>               .write = foo_ecam_config_write,
>>       }
>> };
>>
>> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, "OEM", "TABLE-ID", <revision>,
>>                       <domain_nr>, <bus_nr>);
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>
>> ---
>>
>> Changes from v2 to v3:
>> * Match against all three of oem_id, oem_table_id, and oem_revision.
>> * Perform substring match, so padding oem_id and oem_table_id isn't
>>   required. (Using min_t() thanks to Duc Dang's suggestion.)
>> * Print when quirk matched.
>> * Use __UNIQUE_ID() macro to generate a valid, unique symbol name even
>>   when OEM and table IDs contain characters such as dash "-".
>> * Move extern declaration to header and fix spelling and formatting to
>>   satisfy checkpatch/coding style.
>> ---
>>  drivers/acpi/pci_mcfg.c           | 49 ++++++++++++++++++++++++++++++++++++---
>>  include/asm-generic/vmlinux.lds.h |  7 ++++++
>>  include/linux/pci-acpi.h          | 24 +++++++++++++++++++
>>  3 files changed, 77 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index b5b376e..2a5d3dd 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -22,6 +22,10 @@
>>  #include <linux/kernel.h>
>>  #include <linux/pci.h>
>>  #include <linux/pci-acpi.h>
>> +#include <linux/pci-ecam.h>
>> +
>> +/* Root pointer to the mapped MCFG table */
>> +static struct acpi_table_mcfg *mcfg_table;
>>
>>  /* Structure to hold entries from the MCFG table */
>>  struct mcfg_entry {
>> @@ -35,6 +39,46 @@ struct mcfg_entry {
>>  /* List to save MCFG entries */
>>  static LIST_HEAD(pci_mcfg_list);
>>
>> +static bool pci_mcfg_fixup_match(struct pci_cfg_fixup *f,
>> +                              struct acpi_table_header *h)
>> +{
>> +     int olen = min_t(u8, strlen(f->oem_id), ACPI_OEM_ID_SIZE);
>> +     int tlen = min_t(u8, strlen(f->oem_table_id), ACPI_OEM_TABLE_ID_SIZE);
>> +
>> +     return (!strncmp(f->oem_id, h->oem_id, olen) &&
>> +             !strncmp(f->oem_table_id, h->oem_table_id, tlen) &&
>> +             f->oem_revision == h->oem_revision);
>> +}
>> +
>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
>> +{
>> +     int bus_num = root->secondary.start;
>> +     int domain = root->segment;
>> +     struct pci_cfg_fixup *f;
>> +
>> +     if (!mcfg_table)
>> +             return &pci_generic_ecam_ops;
>> +
>> +     /*
>> +      * Match against platform specific quirks and return corresponding CAM
>> +      * ops.
>> +      *
>> +      * First match against PCI topology <domain:bus> then use OEM ID, OEM
>> +      * table ID, and OEM revision from MCFG table standard header.
>> +      */
>> +     for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
>> +             if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
>> +                 (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
>> +                 pci_mcfg_fixup_match(f, &mcfg_table->header)) {
>> +                     pr_info("Handling %s %s r%d PCI MCFG quirks\n",
>> +                             f->oem_id, f->oem_table_id, f->oem_revision);
>> +                     return f->ops;
>> +             }
>> +     }
>> +     /* No quirks, use ECAM */
>> +     return &pci_generic_ecam_ops;
>> +}
>> +
>>  phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>>  {
>>       struct mcfg_entry *e;
>> @@ -54,7 +98,6 @@ phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
>>
>>  static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>  {
>> -     struct acpi_table_mcfg *mcfg;
>>       struct acpi_mcfg_allocation *mptr;
>>       struct mcfg_entry *e, *arr;
>>       int i, n;
>> @@ -64,8 +107,8 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>
>>       n = (header->length - sizeof(struct acpi_table_mcfg)) /
>>                                       sizeof(struct acpi_mcfg_allocation);
>> -     mcfg = (struct acpi_table_mcfg *)header;
>> -     mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
>> +     mcfg_table = (struct acpi_table_mcfg *)header;
>> +     mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
>>
>>       arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>>       if (!arr)
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index 6a67ab9..43604fc 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -300,6 +300,13 @@
>>               VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;      \
>>       }                                                               \
>>                                                                       \
>> +     /* ACPI MCFG quirks */                                          \
>> +     .acpi_fixup        : AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {      \
>> +             VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;           \
>> +             *(.acpi_fixup_mcfg)                                     \
>> +             VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;             \
>> +     }                                                               \
>> +                                                                     \
>>       /* Built-in firmware blobs */                                   \
>>       .builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {      \
>>               VMLINUX_SYMBOL(__start_builtin_fw) = .;                 \
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 7d63a66..dac851b 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -25,6 +25,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>>
>>  extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
>> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
>>
>>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>>  {
>> @@ -72,6 +73,29 @@ struct acpi_pci_root_ops {
>>       int (*prepare_resources)(struct acpi_pci_root_info *info);
>>  };
>>
>> +struct pci_cfg_fixup {
>> +     struct pci_ecam_ops *ops;
>> +     char *oem_id;
>> +     char *oem_table_id;
>> +     u32 oem_revision;
>> +     int domain;
>> +     int bus_num;
>> +};
>> +
>> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
>> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
>> +
>> +#define PCI_MCFG_DOMAIN_ANY  -1
>> +#define PCI_MCFG_BUS_ANY     -1
>> +
>> +/* Designate a routine to fix up buggy MCFG */
>> +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem, table, rev, dom, bus)      \
>> +     static const struct pci_cfg_fixup                       \
>> +     __UNIQUE_ID(mcfg_fixup_)                                \
>> +     __used  __attribute__((__section__(".acpi_fixup_mcfg"), \
>> +                             aligned((sizeof(void *))))) =   \
>> +     { ops, oem, table, rev, dom, bus }
>> +
>
> Ok, a couple of comments on this quirks handling mechanism.
>
> Given that it is supposed to be just a temporary workaround for
> 1st generation non-ECAM-compliant platforms, the same mechanism
> can be implemented through a static array of hooks, that, through
> the same MCFG matching mechanism, initialize the pci_ops for
> the given platform.
>
> Furthermore, I suspect we do not even need a way to pass the
> non-ECAM compliant config space resources to the OS (ie we can't
> change FW anymore anyway in some platforms) so the quirks hooks
> are likely to hardcode the required config space addresses for
> the respective MCFG match.
>

As discussed off-list, I strongly second the idea to have a static
quirks array for hardware/firmware combos that are currently in the
pipeline, and for which we need special handling in software to be
able to drive them at all. This should be based on exact OEM table/rev
id matches, and contain all supplementary information required to
implement the quirk in the kernel source code. Anything that could
potentially be used to 'cheat', e.g., an OEM id triggering a substring
match on a platform that we currently don't know about, or things like
_DSD properties to parametrize the quirks implementation are out of
the question IMO

> It should be easier to implement (provided we find a place where
> to add this static array of hooks matching MCFG, I suspect it is
> going to be a file in drivers/pci/host but Tomasz and I need
> input on that) and prevent abuse (since it is a static array of
> hooks in a single place, it is easier to manage than section
> entries).
>
> Either that or we keep the section entry linker script approach
> this series implements, which means that the quirks handling hook
> will be added to a specific section instead of a static array,
> I see no other option.
>

I know __weak functions are unpopular, but in this case, having a
__weak empty function in the MCFG/ECAM core code, and overriding it in
arch/arm64/xxx/mcfg-quirks.c does not sound unreasonable to me.

> Last but not least, the MCFG table must not contain any region
> that is not ECAM compliant, I understand we need quirks but
> we must not abuse a standard table to provide the OS resources
> that are really not ECAM compliant when they are supposed to be.
>
> Tomasz will post a follow-up patch to this series implementing
> the above, if you have any comments/concerns/questions or you see
> anything wrong with what I say above please do chime in.
>
> Thanks !
> Lorenzo