[PATCH 0/4] PCI SMC conduit, now with DT support

[PATCH 0/4] PCI SMC conduit, now with DT support

[Jeremy Linton]

This is a rebase of the later revisions of [1], but refactored
slightly to add a DT method as well. It has all the same advantages of
the ACPI method (putting HW quirks in the firmware rather than the
kernel) but now applied to a 'pci-host-smc-generic' compatible
property which extends the pci-host-generic logic to handle cases
where the PCI Config region isn't ECAM compliant. With this in place,
and firmware managed clock/phy/etc its possible to run the generic
driver on hardware that isn't what one would consider standards
compliant PCI root ports.

The DT code was tested on the RPi4, where the ACPI/SMC is upstream in
TF-A and EDK2. On that platform the PCIe works as expected utilizing
the generic host driver rather than the pcie-brcmstb driver.

[1] https://lkml.org/lkml/2021/1/4/1255

Jeremy Linton (4):
  arm64: smccc: Add PCI SMCCCs
  arm64: PCI: Enable SMC conduit
  PCI: host-generic: Add firmware managed config ops
  dt-bindings: PCI: Note the use of pci-host-smc-generic

 .../bindings/pci/host-generic-pci.yaml        |  24 +++-
 arch/arm64/kernel/pci.c                       | 109 ++++++++++++++++++
 drivers/pci/controller/pci-host-common.c      |  34 ++++--
 drivers/pci/controller/pci-host-generic.c     |   6 +
 include/linux/arm-smccc.h                     |  29 +++++
 5 files changed, 186 insertions(+), 16 deletions(-)

-- 
2.37.1

[PATCH 1/4] arm64: smccc: Add PCI SMCCCs

[Jeremy Linton]

Arm defined a set of SMC calls for accessing PCIe config
space in The Arm PCI Configuration Space Access Firmware
Interface:
https://developer.arm.com/documentation/den0115/latest

Add the definitions.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 include/linux/arm-smccc.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 220c8c60e021..1071a997ba98 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -182,6 +182,35 @@
 			   ARM_SMCCC_OWNER_STANDARD,		\
 			   0x53)
 
+/* PCI ECAM conduit (defined by ARM DEN0115A) */
+#define SMCCC_PCI_VERSION						\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   ARM_SMCCC_OWNER_STANDARD, 0x0130)
+
+#define SMCCC_PCI_FEATURES						\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   ARM_SMCCC_OWNER_STANDARD, 0x0131)
+
+#define SMCCC_PCI_READ							\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   ARM_SMCCC_OWNER_STANDARD, 0x0132)
+
+#define SMCCC_PCI_WRITE							\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   ARM_SMCCC_OWNER_STANDARD, 0x0133)
+
+#define SMCCC_PCI_SEG_INFO						\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   ARM_SMCCC_OWNER_STANDARD, 0x0134)
+
+#define SMCCC_PCI_SEG_INFO_START_BUS  GENMASK(7, 0)
+#define SMCCC_PCI_SEG_INFO_END_BUS    GENMASK(15, 8)
+
 /*
  * Return codes defined in ARM DEN 0070A
  * ARM DEN 0070A is now merged/consolidated into ARM DEN 0028 C
-- 
2.37.1

[PATCH 2/4] arm64: PCI: Enable SMC conduit

[Jeremy Linton]

Given that most arm64 platforms' PCI implementations need quirks
to deal with problematic config accesses, this is a good place
to apply a firmware abstraction. The ARM PCI Configuration Space
Access Firmware Interface specification details a standard SMC
conduit designed to provide a simple PCI config accessor. This
specification enhances the existing ACPI/PCI abstraction and
expects power, config, etc., is handled by the platform. It also
is very explicit that the resulting config space registers must
behave as is specified by the PCI specification.

Hook the ACPI/PCI config path, and when missing MCFG data is
detected, attempt to probe the SMC conduit. If the conduit
exists and responds to the requested segment,  provided by the
ACPI namespace, attach a custom pci_ecam_ops which redirects
all config read/write requests to the firmware.

The Arm PCI Configuration Space Access Firmware Interface:
https://developer.arm.com/documentation/den0115/latest

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/pci.c | 109 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 2276689b5411..beb4289a7471 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/arm-smccc.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -122,6 +123,112 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
 	return status;
 }
 
+int pcie_has_fw_conduit(void)
+{
+	struct arm_smccc_res res;
+
+	if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
+		return -EOPNOTSUPP;
+
+	arm_smccc_1_1_invoke(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
+	if ((int)res.a0 < 0)
+		return -EOPNOTSUPP;
+
+	arm_smccc_1_1_invoke(SMCCC_PCI_FEATURES,
+			     SMCCC_PCI_WRITE, 0, 0, 0, 0, 0, 0, &res);
+	if ((int)res.a0 < 0)
+		return -EOPNOTSUPP;
+
+	arm_smccc_1_1_invoke(SMCCC_PCI_FEATURES,
+			     SMCCC_PCI_READ, 0, 0, 0, 0, 0, 0, &res);
+	if ((int)res.a0 < 0)
+		return -EOPNOTSUPP;
+
+	arm_smccc_1_1_invoke(SMCCC_PCI_FEATURES,
+			     SMCCC_PCI_SEG_INFO, 0, 0, 0, 0, 0, 0, &res);
+	if ((int)res.a0 < 0)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+static int smccc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 *val)
+{
+	struct arm_smccc_res res;
+
+	devfn |= bus->number << 8;
+	devfn |= bus->domain_nr << 16;
+
+	arm_smccc_1_1_invoke(SMCCC_PCI_READ, devfn, where, size,
+			     0, 0, 0, 0, &res);
+	if (res.a0) {
+		*val = ~0;
+		return -PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+
+	*val = res.a1;
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int smccc_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
+				   int where, int size, u32 val)
+{
+	struct arm_smccc_res res;
+
+	devfn |= bus->number << 8;
+	devfn |= bus->domain_nr << 16;
+
+	arm_smccc_1_1_invoke(SMCCC_PCI_WRITE, devfn, where, size, val,
+			     0, 0, 0, &res);
+	if (res.a0)
+		return -PCIBIOS_BAD_REGISTER_NUMBER;
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static const struct pci_ecam_ops smccc_pcie_ops = {
+	.pci_ops	= {
+		.read		= smccc_pcie_config_read,
+		.write		= smccc_pcie_config_write,
+	}
+};
+
+
+struct pci_config_window *
+pci_setup_fw_mapping(struct device *dev, u16 seg, struct resource *bus_res)
+{
+	struct arm_smccc_res res;
+	struct pci_config_window *cfg;
+
+	arm_smccc_1_1_invoke(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, &res);
+	if ((int)res.a0 < 0) {
+		pr_warn("PCI: SMC segment %d doesn't exist\n", seg);
+		return NULL;
+	}
+
+	if (FIELD_GET(SMCCC_PCI_SEG_INFO_START_BUS, res.a1) != bus_res->start ||
+	    FIELD_GET(SMCCC_PCI_SEG_INFO_END_BUS, res.a1) != bus_res->end) {
+		pr_warn("PCI: SMC segment %d doesn't match ACPI description\n", seg);
+		return NULL;
+	}
+
+	cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+	if (!cfg)
+		return NULL;
+
+	cfg->parent = dev;
+	cfg->ops = &smccc_pcie_ops;
+	cfg->busr.start = FIELD_GET(SMCCC_PCI_SEG_INFO_START_BUS, res.a1);
+	cfg->busr.end = FIELD_GET(SMCCC_PCI_SEG_INFO_END_BUS, res.a1);
+	cfg->busr.flags = IORESOURCE_BUS;
+	cfg->res.name = "PCI SMCCC";
+
+	pr_info("PCI: SMC conduit attached to segment %d\n", seg);
+
+	return cfg;
+}
+
 /*
  * Lookup the bus range for the domain in MCFG, and set up config space
  * mapping.
@@ -140,6 +247,8 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
 
 	ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
 	if (ret) {
+		if (!pcie_has_fw_conduit())
+			return pci_setup_fw_mapping(dev, seg, bus_res);
 		dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res);
 		return NULL;
 	}
-- 
2.37.1

[PATCH 3/4] PCI: host-generic: Add firmware managed config ops

[Jeremy Linton]

The generic PCI host driver leaves the configuration and mgmt of the
clock/phy/etc on PCI root ports to the firmware and PCIe defined
mechanisms as is done on UEFI/ACPI systems. If the PCIe config
operations were abstracted as well then a number of the resulting
Linux PCie drivers would no longer be needed.

Given that Arm has standardized a generic SMC conduit for reading and
writing the PCIe config space. Using it on DT based systems seems a
natural way to reduce some of the driver diversity on DT based systems
as well.

This patch adds a compatible type "pci-host-smc-generic" which expects
that the 'reg' property of the root port is missing, and the PCI SMCCC
exists. When that happens it binds the SMC to the pci_ecam_ops.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/pci/controller/pci-host-common.c  | 34 ++++++++++++++++-------
 drivers/pci/controller/pci-host-generic.c |  6 ++++
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index d3924a44db02..2673fd81d3e0 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -20,22 +20,41 @@ static void gen_pci_unmap_cfg(void *ptr)
 	pci_ecam_free((struct pci_config_window *)ptr);
 }
 
+__weak int pcie_has_fw_conduit(void)
+{
+	return -EOPNOTSUPP;
+}
+
+__weak struct pci_config_window *pci_setup_fw_mapping(struct device *dev,
+		u16 seg, struct resource *bus_res)
+{
+	return NULL;
+}
+
 static struct pci_config_window *gen_pci_init(struct device *dev,
-		struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops)
+		struct pci_host_bridge *bridge)
 {
 	int err;
 	struct resource cfgres;
 	struct resource_entry *bus;
 	struct pci_config_window *cfg;
+	const struct pci_ecam_ops *ops;
+
+	bus = resource_list_first_type(&bridge->windows, IORESOURCE_BUS);
+	if (!bus)
+		return ERR_PTR(-ENODEV);
 
 	err = of_address_to_resource(dev->of_node, 0, &cfgres);
 	if (err) {
+		if (!pcie_has_fw_conduit())
+			return pci_setup_fw_mapping(dev, bridge->busnr, bus->res);
+
 		dev_err(dev, "missing \"reg\" property\n");
 		return ERR_PTR(err);
 	}
 
-	bus = resource_list_first_type(&bridge->windows, IORESOURCE_BUS);
-	if (!bus)
+	ops = of_device_get_match_data(dev);
+	if (!ops)
 		return ERR_PTR(-ENODEV);
 
 	cfg = pci_ecam_create(dev, &cfgres, bus->res, ops);
@@ -54,11 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct pci_host_bridge *bridge;
 	struct pci_config_window *cfg;
-	const struct pci_ecam_ops *ops;
-
-	ops = of_device_get_match_data(&pdev->dev);
-	if (!ops)
-		return -ENODEV;
 
 	bridge = devm_pci_alloc_host_bridge(dev, 0);
 	if (!bridge)
@@ -69,7 +83,7 @@ int pci_host_common_probe(struct platform_device *pdev)
 	of_pci_check_probe_only();
 
 	/* Parse and map our Configuration Space windows */
-	cfg = gen_pci_init(dev, bridge, ops);
+	cfg = gen_pci_init(dev, bridge);
 	if (IS_ERR(cfg))
 		return PTR_ERR(cfg);
 
@@ -78,7 +92,7 @@ int pci_host_common_probe(struct platform_device *pdev)
 		pci_add_flags(PCI_REASSIGN_ALL_BUS);
 
 	bridge->sysdata = cfg;
-	bridge->ops = (struct pci_ops *)&ops->pci_ops;
+	bridge->ops = (struct pci_ops *)&cfg->ops->pci_ops;
 	bridge->msi_domain = true;
 
 	return pci_host_probe(bridge);
diff --git a/drivers/pci/controller/pci-host-generic.c b/drivers/pci/controller/pci-host-generic.c
index 63865aeb636b..d20d33b3c689 100644
--- a/drivers/pci/controller/pci-host-generic.c
+++ b/drivers/pci/controller/pci-host-generic.c
@@ -56,6 +56,9 @@ static const struct pci_ecam_ops pci_dw_ecam_bus_ops = {
 	}
 };
 
+static const struct pci_ecam_ops pci_generic_smc_ops = {
+};
+
 static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-cam-generic",
 	  .data = &gen_pci_cfg_cam_bus_ops },
@@ -63,6 +66,9 @@ static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-ecam-generic",
 	  .data = &pci_generic_ecam_ops },
 
+	{ .compatible = "pci-host-smc-generic",
+	  .data = &pci_generic_smc_ops },
+
 	{ .compatible = "marvell,armada8k-pcie-ecam",
 	  .data = &pci_dw_ecam_bus_ops },
 
-- 
2.37.1

[PATCH 4/4] dt-bindings: PCI: Note the use of pci-host-smc-generic

[Jeremy Linton]

Note the addition of pci-host-smc-generic for
firmware based config assistance, and the relaxation
of the 'reg' property for such machines.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 .../bindings/pci/host-generic-pci.yaml        | 24 ++++++++++++++-----
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
index 6bcaa8f2c3cf..b4471617fa46 100644
--- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
+++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
@@ -18,10 +18,11 @@ description: |
   presenting a set of fixed windows describing a subset of IO, Memory and
   Configuration Spaces.
 
-  Configuration Space is assumed to be memory-mapped (as opposed to being
-  accessed via an ioport) and laid out with a direct correspondence to the
-  geography of a PCI bus address by concatenating the various components to
-  form an offset.
+  Configuration Space is assumed to be laid out with a direct correspondence
+  to the geography of a PCI bus address by concatenating the various components
+  to form an offset. The CAM and ECAM mechanisms require a memory mapped
+  interface, while the SMC traps to the firmware to perform config space
+  reads and writes decoded similarly to the ECAM mapping.
 
   For CAM, this 24-bit offset is:
 
@@ -86,6 +87,7 @@ properties:
         enum:
           - pci-host-cam-generic
           - pci-host-ecam-generic
+          - pci-host-smc-generic
 
   reg:
     description:
@@ -93,7 +95,8 @@ properties:
       bus. The base address corresponds to the first bus in the "bus-range"
       property. If no "bus-range" is specified, this will be bus 0 (the
       default). Some host controllers have a 2nd non-compliant address range,
-      so 2 entries are allowed.
+      so 2 entries are allowed. Machines which implement the Arm PCI SMCCC spec
+      must not provide this property.
     minItems: 1
     maxItems: 2
 
@@ -109,7 +112,6 @@ properties:
 
 required:
   - compatible
-  - reg
   - ranges
 
 allOf:
@@ -123,6 +125,16 @@ allOf:
       required:
         - dma-coherent
 
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              const: pci-host-smc-generic
+    then:
+      required:
+        - reg
+
   - if:
       properties:
         compatible:
-- 
2.37.1

Re: [PATCH 0/4] PCI SMC conduit, now with DT support

[Will Deacon]

On Mon, Jul 25, 2022 at 11:39:01AM -0500, Jeremy Linton wrote:
> This is a rebase of the later revisions of [1], but refactored
> slightly to add a DT method as well. It has all the same advantages of
> the ACPI method (putting HW quirks in the firmware rather than the
> kernel) but now applied to a 'pci-host-smc-generic' compatible
> property which extends the pci-host-generic logic to handle cases
> where the PCI Config region isn't ECAM compliant. With this in place,
> and firmware managed clock/phy/etc its possible to run the generic
> driver on hardware that isn't what one would consider standards
> compliant PCI root ports.

I still think that hiding the code in firmware because the hardware is
broken is absolutely the wrong way to tackle this problem and I thought
the general idea from last time was that we were going to teach Linux
about the broken hardware instead [1]. I'd rather have the junk where we
can see it, reason about it and modify it.

What's changed?

In my mind, the main thing that's happened since we last discussed this
is that Apple shipped arm64 client hardware with working ECAM. *Apple*
for goodness sake: a company with basically no incentive to follow
standards for their vertically integrated devices! Perhaps others need
to raise their game instead of wasting everybody's time on firmware
hacks; getting the hardware right obviously isn't as difficult as folks
would lead us to believe.

Will

[1] https://lore.kernel.org/r/20210325131231.GA18590@e121166-lin.cambridge.arm.com

Re: [PATCH 0/4] PCI SMC conduit, now with DT support

[Jeremy Linton]

Hi,

On 7/26/22 06:40, Will Deacon wrote:
> On Mon, Jul 25, 2022 at 11:39:01AM -0500, Jeremy Linton wrote:
>> This is a rebase of the later revisions of [1], but refactored
>> slightly to add a DT method as well. It has all the same advantages of
>> the ACPI method (putting HW quirks in the firmware rather than the
>> kernel) but now applied to a 'pci-host-smc-generic' compatible
>> property which extends the pci-host-generic logic to handle cases
>> where the PCI Config region isn't ECAM compliant. With this in place,
>> and firmware managed clock/phy/etc its possible to run the generic
>> driver on hardware that isn't what one would consider standards
>> compliant PCI root ports.
> 
> I still think that hiding the code in firmware because the hardware is
> broken is absolutely the wrong way to tackle this problem and I thought
> the general idea from last time was that we were going to teach Linux
> about the broken hardware instead [1]. I'd rather have the junk where we
> can see it, reason about it and modify it.

Well, the CM4/ACPI/PCIe quirk still hasn't landed, but that's not the point.

I would like to understand why you think this patch is any different 
than the dozens of other firmware traps, quite a number merged in the 
last year, for "broken" hardware or simply as generic platform interfaces?

Without rehashing, the entire discussion in the previous thread, I'm 
going to repeat that this is an official Arm standard the same as the 
firmware traps to handle speculative execution mitigations or to 
standardize platform functionality, ex: PSCI or the recent TRNG code. It 
also has uses beyond fixing broken hardware.

But similar to those examples, I think everyone here understands the 
kernel is both a poor place for this kind of logic, while at the same 
time may not be technically feasible without supplying EL3, management 
processor code, or traps to said code.

Is it the official position of the Linux kernel maintainers that they 
will refuse to support future Arm standards in order to gate keep 
specific hardware platforms?

> 
> What's changed?

Well, the code to support this interface is upstream in both TFA, edk2, 
and various other OS's. So now Linux is trailing.

> 
> In my mind, the main thing that's happened since we last discussed this
> is that Apple shipped arm64 client hardware with working ECAM. *Apple*
> for goodness sake: a company with basically no incentive to follow
> standards for their vertically integrated devices! Perhaps others need
> to raise their game instead of wasting everybody's time on firmware
> hacks; getting the hardware right obviously isn't as difficult as folks
> would lead us to believe.

I find it interesting that you hold up the M1 as an example of good 
hardware. That hardware is one of the worse violators of both platform 
standards, as well has having a lot of "broken" hardware requiring 
changes to the kernel that previously were rejected as too far out of 
line. Never mind, as you point out it has basically zero vendor support 
and exists only due to a large reverse engineering effort.


Thanks for looking at this,

Re: [PATCH 0/4] PCI SMC conduit, now with DT support

[Catalin Marinas]

Hi Jeremy,

On Thu, Jul 28, 2022 at 12:20:55PM -0500, Jeremy Linton wrote:
> On 7/26/22 06:40, Will Deacon wrote:
> > On Mon, Jul 25, 2022 at 11:39:01AM -0500, Jeremy Linton wrote:
> > > This is a rebase of the later revisions of [1], but refactored
> > > slightly to add a DT method as well. It has all the same advantages of
> > > the ACPI method (putting HW quirks in the firmware rather than the
> > > kernel) but now applied to a 'pci-host-smc-generic' compatible
> > > property which extends the pci-host-generic logic to handle cases
> > > where the PCI Config region isn't ECAM compliant. With this in place,
> > > and firmware managed clock/phy/etc its possible to run the generic
> > > driver on hardware that isn't what one would consider standards
> > > compliant PCI root ports.
> > 
> > I still think that hiding the code in firmware because the hardware is
> > broken is absolutely the wrong way to tackle this problem and I thought
> > the general idea from last time was that we were going to teach Linux
> > about the broken hardware instead [1]. I'd rather have the junk where we
> > can see it, reason about it and modify it.
[...]
> Is it the official position of the Linux kernel maintainers that they will
> refuse to support future Arm standards in order to gate keep specific
> hardware platforms?

(just back from holiday; well, briefly, going away for a few days soon)

We shouldn't generalise what maintainers wwould accept or not. We decide
on a case by case basis. With speculative execution mitigations, for
example, we try to do as much as we can in the kernel but sometimes
that's just not possible, hence an EL3 call and we'd rather have this
standardised (e.g. custom branch loops to flush the branch predictor if
possible from the normal world, secure call if not).

You mention PSCI but that's not working around broken hardware, it was a
concious decision from the start to standardise the booting protocol and
CPU power management.

Now this PCI SMC protocol was simply created because hardware did not
comply with another PCI standard that has been around for a long time.
As with the speculative execution mitigations, we'd rather work around
broken hardware in the kernel first and, if it's not possible, we can
look at a firmware interface (and ideally standardised). Do you have an
example where we cannot work around the PCI hardware bugs in the kernel
and EL3 firmware involvement is necessary?

So, in summary, Arm Ltd proposing a new standard because hardware
companies can't be bothered with an existing one is not an argument for
accepting its support in the Linux kernel. This PCI SMC conduit is not
presented as a hardware bug workaround interface but rather as an
alternative to ECAM (and, yes, the kernel maintainers can choose not to
support specific "standards" in Linux).

-- 
Catalin

Re: [PATCH 0/4] PCI SMC conduit, now with DT support

[Pali Rohár]

On Tuesday 16 August 2022 08:59:05 Catalin Marinas wrote:
> Hi Jeremy,
> 
> On Thu, Jul 28, 2022 at 12:20:55PM -0500, Jeremy Linton wrote:
> > On 7/26/22 06:40, Will Deacon wrote:
> > > On Mon, Jul 25, 2022 at 11:39:01AM -0500, Jeremy Linton wrote:
> > > > This is a rebase of the later revisions of [1], but refactored
> > > > slightly to add a DT method as well. It has all the same advantages of
> > > > the ACPI method (putting HW quirks in the firmware rather than the
> > > > kernel) but now applied to a 'pci-host-smc-generic' compatible
> > > > property which extends the pci-host-generic logic to handle cases
> > > > where the PCI Config region isn't ECAM compliant. With this in place,
> > > > and firmware managed clock/phy/etc its possible to run the generic
> > > > driver on hardware that isn't what one would consider standards
> > > > compliant PCI root ports.
> > > 
> > > I still think that hiding the code in firmware because the hardware is
> > > broken is absolutely the wrong way to tackle this problem and I thought
> > > the general idea from last time was that we were going to teach Linux
> > > about the broken hardware instead [1]. I'd rather have the junk where we
> > > can see it, reason about it and modify it.
> [...]
> > Is it the official position of the Linux kernel maintainers that they will
> > refuse to support future Arm standards in order to gate keep specific
> > hardware platforms?
> 
> (just back from holiday; well, briefly, going away for a few days soon)
> 
> We shouldn't generalise what maintainers wwould accept or not. We decide
> on a case by case basis. With speculative execution mitigations, for
> example, we try to do as much as we can in the kernel but sometimes
> that's just not possible, hence an EL3 call and we'd rather have this
> standardised (e.g. custom branch loops to flush the branch predictor if
> possible from the normal world, secure call if not).
> 
> You mention PSCI but that's not working around broken hardware, it was a
> concious decision from the start to standardise the booting protocol and
> CPU power management.
> 
> Now this PCI SMC protocol was simply created because hardware did not
> comply with another PCI standard that has been around for a long time.
> As with the speculative execution mitigations, we'd rather work around
> broken hardware in the kernel first and, if it's not possible, we can
> look at a firmware interface (and ideally standardised). Do you have an
> example where we cannot work around the PCI hardware bugs in the kernel
> and EL3 firmware involvement is necessary?
> 
> So, in summary, Arm Ltd proposing a new standard because hardware
> companies can't be bothered with an existing one is not an argument for
> accepting its support in the Linux kernel. This PCI SMC conduit is not
> presented as a hardware bug workaround interface but rather as an
> alternative to ECAM (and, yes, the kernel maintainers can choose not to
> support specific "standards" in Linux).

Hello! I think that this PCI SMC could be already marked as deprecated
as Linux can use "native" drivers to access PCIe config space, without
need to use any kind of RPC mechanism, like ARM SMC.

Note that for example kernel driver phy-mvebu-a3700-comphy.c was
converted from ARM SMC API to true "native" linux driver which touch
hardware directly (and does not use RPC API). And this is the right
direction, stop using RPC APIs in kernel and configure hardware
directly without need to depends on firmware, SMC or any other SW which
is running on CPU. Depending on the firmware or its functionality which
access same HW as kernel itself, is always nightmare. x86 developers
have enough experience with BIOS and its poor implementations and there
was for a long time direction to not use x86 BIOS and rather communicate
with hardware directly. And if PCIe hardware is broken? Well, PCIe
controller drivers should be extended to handle or workaround it. I have
already sent lot of patches for Marvell PCIe controllers to workaround
HW design issues, so similarly it should be done for other (known
broken) vendor HW.

So in my opinion, instead of PCI SMC, kernel PCIe controller drivers
should be fixed to correctly access PCIe config space and completely
deprecate/remove this PCI SMC from kernel. And if PCI SMC has not landed
in kernel yet, even better, because deprecation step can be skipped.