diff mbox series

arm64: PCI: Enable SMC conduit

Message ID 20210105045735.1709825-1-jeremy.linton@arm.com
State New
Headers show
Series arm64: PCI: Enable SMC conduit | expand

Commit Message

Jeremy Linton Jan. 5, 2021, 4:57 a.m. UTC
Given that most arm64 platform's PCI implementations needs quirks
to deal with problematic config accesses, this is a good place to
apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.

Lets hook the normal ACPI/PCI config path, and when we detect
missing MADT data, attempt to probe the SMC conduit. If the conduit
exists and responds for the requested segment number (provided by the
ACPI namespace) attach a custom pci_ecam_ops which redirects
all config read/write requests to the firmware.

This patch is based on the Arm PCI Config space access document @
https://developer.arm.com/documentation/den0115/latest

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/pci.c   | 87 +++++++++++++++++++++++++++++++++++++++
 include/linux/arm-smccc.h | 26 ++++++++++++
 2 files changed, 113 insertions(+)

Comments

Rob Herring Jan. 7, 2021, 3:28 p.m. UTC | #1
On Mon, Jan 4, 2021 at 9:57 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Given that most arm64 platform's PCI implementations needs quirks
> to deal with problematic config accesses, this is a good place to
> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.

If we put it in a document, they must behave.

> Lets hook the normal ACPI/PCI config path, and when we detect
> missing MADT data, attempt to probe the SMC conduit. If the conduit
> exists and responds for the requested segment number (provided by the
> ACPI namespace) attach a custom pci_ecam_ops which redirects
> all config read/write requests to the firmware.
>
> This patch is based on the Arm PCI Config space access document @
> https://developer.arm.com/documentation/den0115/latest

From the spec:
"On some platforms, the SoC may only support 32-bit PCI configuration
space writes. On such platforms, calls to this function with access
size of 1 or 2 bytes may require the implementation of this function
to perform a PCI configuration read, following by the write. This
could result in inadvertently corrupting adjacent RW1C fields. It is
the implementation responsibility to be aware of these situations and
guard against them if possible."

First, this contradicts the above statement about being compliant with
the PCI spec. Second, Linux is left to just guess whether this is the
case or not? I guess it would be pointless for firmware to advertise
this because it could just lie.

I would like to know how to 'guard against them' so I can implement
that in the kernel accessors.

> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  arch/arm64/kernel/pci.c   | 87 +++++++++++++++++++++++++++++++++++++++
>  include/linux/arm-smccc.h | 26 ++++++++++++
>  2 files changed, 113 insertions(+)
>
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 1006ed2d7c60..56d3773aaa25 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>
> @@ -107,6 +108,90 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>         return status;
>  }
>
> +static int smccc_pcie_check_conduit(u16 seg)

check what? Based on how you use this, perhaps _has_conduit() instead.

> +{
> +       struct arm_smccc_res res;
> +
> +       if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
> +               return -EOPNOTSUPP;
> +
> +       arm_smccc_smc(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
> +       if ((int)res.a0 < 0)
> +               return -EOPNOTSUPP;
> +
> +       arm_smccc_smc(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, &res);
> +       if ((int)res.a0 < 0)
> +               return -EOPNOTSUPP;

Don't you need to check that read and write functions are supported?

> +
> +       pr_info("PCI: SMC conduit attached to segment %d\n", seg);
> +
> +       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_smc(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_smc(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_ecam_ops = {
> +       .bus_shift      = 8,

Drop. You don't use this and it's not ECAM. Though I'm wondering why
the smc interface is not just ECAM shifts instead of making up our
own. I would have made segment its own register rather than reg
offset.

> +       .pci_ops        = {
> +               .read           = smccc_pcie_config_read,
> +               .write          = smccc_pcie_config_write,
> +       }
> +};
> +
> +static struct pci_config_window *
> +pci_acpi_setup_smccc_mapping(struct acpi_pci_root *root)
> +{
> +       struct device *dev = &root->device->dev;
> +       struct resource *bus_res = &root->secondary;
> +       struct pci_config_window *cfg;
> +
> +       cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
> +       if (!cfg)
> +               return ERR_PTR(-ENOMEM);
> +
> +       cfg->parent = dev;
> +       cfg->ops = &smccc_pcie_ecam_ops;
> +       cfg->busr.start = bus_res->start;
> +       cfg->busr.end = bus_res->end;
> +       cfg->busr.flags = IORESOURCE_BUS;
> +
> +       cfg->res.name = "PCI SMCCC";
> +       if (cfg->ops->init)
> +               cfg->ops->init(cfg);
> +       return cfg;
> +}
> +
>  /*
>   * Lookup the bus range for the domain in MCFG, and set up config space
>   * mapping.
> @@ -125,6 +210,8 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>
>         ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
>         if (ret) {
> +               if (!smccc_pcie_check_conduit(seg))
> +                       return pci_acpi_setup_smccc_mapping(root);
>                 dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res);
>                 return NULL;
>         }
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index f860645f6512..327f52533c71 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -89,6 +89,32 @@
>
>  #define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED   1
>
> +/* PCI ECAM conduit */
> +#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)
> +
>  /* Paravirtualised time calls (defined by ARM DEN0057A) */
>  #define ARM_SMCCC_HV_PV_TIME_FEATURES                          \
>         ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                 \
> --
> 2.26.2
>
Jeremy Linton Jan. 7, 2021, 4:23 p.m. UTC | #2
Hi,


On 1/7/21 9:28 AM, Rob Herring wrote:
> On Mon, Jan 4, 2021 at 9:57 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
>>
>> Given that most arm64 platform's PCI implementations needs quirks
>> to deal with problematic config accesses, this is a good place to
>> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
> 
> If we put it in a document, they must behave.
> 
>> Lets hook the normal ACPI/PCI config path, and when we detect
>> missing MADT data, attempt to probe the SMC conduit. If the conduit
>> exists and responds for the requested segment number (provided by the
>> ACPI namespace) attach a custom pci_ecam_ops which redirects
>> all config read/write requests to the firmware.
>>
>> This patch is based on the Arm PCI Config space access document @
>> https://developer.arm.com/documentation/den0115/latest
> 
>  From the spec:
> "On some platforms, the SoC may only support 32-bit PCI configuration
> space writes. On such platforms, calls to this function with access
> size of 1 or 2 bytes may require the implementation of this function
> to perform a PCI configuration read, following by the write. This
> could result in inadvertently corrupting adjacent RW1C fields. It is
> the implementation responsibility to be aware of these situations and
> guard against them if possible."
> 
> First, this contradicts the above statement about being compliant with
> the PCI spec. Second, Linux is left to just guess whether this is the
> case or not? I guess it would be pointless for firmware to advertise
> this because it could just lie.

Thanks for taking a look at this.

Right, to clarify. The result of the SMC calls must appear to be 
compliant, but the underlying hardware of course may not be.

The RW1C discussion during the spec reviews was extensive, but as you 
can see from the language the intention is that the results appear 
compliant. But IMHO, considering that ECAM is a configuration mechanism 
not an operational one, if one looks at the results of the existing 
alignment quirks in the kernel & the DT host bridge drivers, its not 
particularly problematic. In the case that there is a problem with a 
particular adapter, its the firmware's responsibility to deal with it. 
If that isn't possible then of course the machine is neither ECAM or 
compatible with this specification, same as what happens if there is a 
fundamental issue with the MMIO mapping. Its also not unheard of for 
cards to simply be incompatible with machines due to lack of optional 
features like PIO.


> 
> I would like to know how to 'guard against them' so I can implement
> that in the kernel accessors.  >
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   arch/arm64/kernel/pci.c   | 87 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/arm-smccc.h | 26 ++++++++++++
>>   2 files changed, 113 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 1006ed2d7c60..56d3773aaa25 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>
>> @@ -107,6 +108,90 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>>          return status;
>>   }
>>
>> +static int smccc_pcie_check_conduit(u16 seg)
> 
> check what? Based on how you use this, perhaps _has_conduit() instead.

Sure.

> 
>> +{
>> +       struct arm_smccc_res res;
>> +
>> +       if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
>> +               return -EOPNOTSUPP;
>> +
>> +       arm_smccc_smc(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
>> +       if ((int)res.a0 < 0)
>> +               return -EOPNOTSUPP;
>> +
>> +       arm_smccc_smc(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, &res);
>> +       if ((int)res.a0 < 0)
>> +               return -EOPNOTSUPP;
> 
> Don't you need to check that read and write functions are supported?

In theory no, the first version of the specification makes them 
mandatory for all implementations. There isn't a partial access method, 
so nothing works if only read or write were implemented.


> 
>> +
>> +       pr_info("PCI: SMC conduit attached to segment %d\n", seg);
>> +
>> +       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_smc(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_smc(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_ecam_ops = {
>> +       .bus_shift      = 8,
> 
> Drop. You don't use this and it's not ECAM. Though I'm wondering why
> the smc interface is not just ECAM shifts instead of making up our
> own. I would have made segment its own register rather than reg
> offset.

Sure.

Thanks again.


> 
>> +       .pci_ops        = {
>> +               .read           = smccc_pcie_config_read,
>> +               .write          = smccc_pcie_config_write,
>> +       }
>> +};
>> +
>> +static struct pci_config_window *
>> +pci_acpi_setup_smccc_mapping(struct acpi_pci_root *root)
>> +{
>> +       struct device *dev = &root->device->dev;
>> +       struct resource *bus_res = &root->secondary;
>> +       struct pci_config_window *cfg;
>> +
>> +       cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
>> +       if (!cfg)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       cfg->parent = dev;
>> +       cfg->ops = &smccc_pcie_ecam_ops;
>> +       cfg->busr.start = bus_res->start;
>> +       cfg->busr.end = bus_res->end;
>> +       cfg->busr.flags = IORESOURCE_BUS;
>> +
>> +       cfg->res.name = "PCI SMCCC";
>> +       if (cfg->ops->init)
>> +               cfg->ops->init(cfg);
>> +       return cfg;
>> +}
>> +
>>   /*
>>    * Lookup the bus range for the domain in MCFG, and set up config space
>>    * mapping.
>> @@ -125,6 +210,8 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>>
>>          ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
>>          if (ret) {
>> +               if (!smccc_pcie_check_conduit(seg))
>> +                       return pci_acpi_setup_smccc_mapping(root);
>>                  dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res);
>>                  return NULL;
>>          }
>> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
>> index f860645f6512..327f52533c71 100644
>> --- a/include/linux/arm-smccc.h
>> +++ b/include/linux/arm-smccc.h
>> @@ -89,6 +89,32 @@
>>
>>   #define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED   1
>>
>> +/* PCI ECAM conduit */
>> +#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)
>> +
>>   /* Paravirtualised time calls (defined by ARM DEN0057A) */
>>   #define ARM_SMCCC_HV_PV_TIME_FEATURES                          \
>>          ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                 \
>> --
>> 2.26.2
>>
Rob Herring Jan. 7, 2021, 5:36 p.m. UTC | #3
On Thu, Jan 7, 2021 at 9:24 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
>
> On 1/7/21 9:28 AM, Rob Herring wrote:
> > On Mon, Jan 4, 2021 at 9:57 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
> >>
> >> Given that most arm64 platform's PCI implementations needs quirks
> >> to deal with problematic config accesses, this is a good place to
> >> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
> >
> > If we put it in a document, they must behave.
> >
> >> Lets hook the normal ACPI/PCI config path, and when we detect
> >> missing MADT data, attempt to probe the SMC conduit. If the conduit
> >> exists and responds for the requested segment number (provided by the
> >> ACPI namespace) attach a custom pci_ecam_ops which redirects
> >> all config read/write requests to the firmware.
> >>
> >> This patch is based on the Arm PCI Config space access document @
> >> https://developer.arm.com/documentation/den0115/latest
> >
> >  From the spec:
> > "On some platforms, the SoC may only support 32-bit PCI configuration
> > space writes. On such platforms, calls to this function with access
> > size of 1 or 2 bytes may require the implementation of this function
> > to perform a PCI configuration read, following by the write. This
> > could result in inadvertently corrupting adjacent RW1C fields. It is
> > the implementation responsibility to be aware of these situations and
> > guard against them if possible."
> >
> > First, this contradicts the above statement about being compliant with
> > the PCI spec. Second, Linux is left to just guess whether this is the
> > case or not? I guess it would be pointless for firmware to advertise
> > this because it could just lie.
>
> Thanks for taking a look at this.
>
> Right, to clarify. The result of the SMC calls must appear to be
> compliant, but the underlying hardware of course may not be.
>
> The RW1C discussion during the spec reviews was extensive, but as you
> can see from the language the intention is that the results appear
> compliant. But IMHO, considering that ECAM is a configuration mechanism
> not an operational one, if one looks at the results of the existing
> alignment quirks in the kernel & the DT host bridge drivers, its not
> particularly problematic. In the case that there is a problem with a
> particular adapter, its the firmware's responsibility to deal with it.
> If that isn't possible then of course the machine is neither ECAM or
> compatible with this specification, same as what happens if there is a
> fundamental issue with the MMIO mapping. Its also not unheard of for
> cards to simply be incompatible with machines due to lack of optional
> features like PIO.
>
>
> >
> > I would like to know how to 'guard against them' so I can implement
> > that in the kernel accessors.  >
> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >> ---
> >>   arch/arm64/kernel/pci.c   | 87 +++++++++++++++++++++++++++++++++++++++
> >>   include/linux/arm-smccc.h | 26 ++++++++++++
> >>   2 files changed, 113 insertions(+)
> >>
> >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> >> index 1006ed2d7c60..56d3773aaa25 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>
> >> @@ -107,6 +108,90 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> >>          return status;
> >>   }
> >>
> >> +static int smccc_pcie_check_conduit(u16 seg)
> >
> > check what? Based on how you use this, perhaps _has_conduit() instead.
>
> Sure.
>
> >
> >> +{
> >> +       struct arm_smccc_res res;
> >> +
> >> +       if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
> >> +               return -EOPNOTSUPP;
> >> +
> >> +       arm_smccc_smc(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
> >> +       if ((int)res.a0 < 0)
> >> +               return -EOPNOTSUPP;
> >> +
> >> +       arm_smccc_smc(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, &res);
> >> +       if ((int)res.a0 < 0)
> >> +               return -EOPNOTSUPP;
> >
> > Don't you need to check that read and write functions are supported?
>
> In theory no, the first version of the specification makes them
> mandatory for all implementations. There isn't a partial access method,
> so nothing works if only read or write were implemented.

Then the spec should change:

2.3.3 Caller responsibilities
The caller has the following responsibilities:
• The caller must ensure that this function is implemented before
issuing a call. This function is discoverable
by calling PCI_FEATURES with pci_func_id set to 0x8400_0132.


I guess knowing the version is ensuring, but the 2nd sentence makes it
seem that is how one should ensure.

Related, are there any sort of tests for the interface? I generally
don't think the kernel's job is validating firmware (a frequent topic
for DT), but we should have something. Maybe an SMC unittest module?
If nothing else, seems like we should have at least one PCI_FEATURES
call to make sure it works. We don't want to add it later only to find
that it breaks on some firmware implementations. Though we can just
add firmware quirks. ;)

Rob
Will Deacon Jan. 7, 2021, 6:14 p.m. UTC | #4
On Mon, Jan 04, 2021 at 10:57:35PM -0600, Jeremy Linton wrote:
> Given that most arm64 platform's PCI implementations needs quirks
> to deal with problematic config accesses, this is a good place to
> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
> 
> Lets hook the normal ACPI/PCI config path, and when we detect
> missing MADT data, attempt to probe the SMC conduit. If the conduit
> exists and responds for the requested segment number (provided by the
> ACPI namespace) attach a custom pci_ecam_ops which redirects
> all config read/write requests to the firmware.
> 
> This patch is based on the Arm PCI Config space access document @
> https://developer.arm.com/documentation/den0115/latest

Why does firmware need to be involved with this at all? Can't we just
quirk Linux when these broken designs show up in production? We'll need
to modify Linux _anyway_ when the firmware interface isn't implemented
correctly...

Will
Jeremy Linton Jan. 7, 2021, 7:18 p.m. UTC | #5
Hi,

On 1/7/21 12:14 PM, Will Deacon wrote:
> On Mon, Jan 04, 2021 at 10:57:35PM -0600, Jeremy Linton wrote:
>> Given that most arm64 platform's PCI implementations needs quirks
>> to deal with problematic config accesses, this is a good place to
>> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
>>
>> Lets hook the normal ACPI/PCI config path, and when we detect
>> missing MADT data, attempt to probe the SMC conduit. If the conduit
>> exists and responds for the requested segment number (provided by the
>> ACPI namespace) attach a custom pci_ecam_ops which redirects
>> all config read/write requests to the firmware.
>>
>> This patch is based on the Arm PCI Config space access document @
>> https://developer.arm.com/documentation/den0115/latest
> 
> Why does firmware need to be involved with this at all? Can't we just
> quirk Linux when these broken designs show up in production? We'll need
> to modify Linux _anyway_ when the firmware interface isn't implemented
> correctly...


IMHO, The short answer is that having the quirk in the firmware keeps it 
centralized over multiple OSs and linux distro versions and avoids a lot 
of costly kernel->distro churning to backport/maintain quirks over a 
dozen distro versions.

There is also a long-term maintenance advantage since its hard for the 
kernel community as a  whole to have a good view of how long a 
particular model of machine is actually in use. For example, today we 
could ask are any of those xgene1's still in use and remove their 
quirks, but no one really has a clear view.

As far as working around the firmware, that is of course potentially 
problematic, but I think it is easier to say "fix the firmware if you 
want/need linux support" than it is to get people to fix their ECAM 
implementations. Hypothetically, if at some point there is a broken 
version of this interface in firmware, the kernel could choose to bypass 
it entirely and talk to whatever broken config space method the hardware 
implements. At which point we aren't any worse off than the situation 
today.

The flip side of this is that a fair number of these platforms have open 
source firmware as well, so it may be trivial to fix the firmware.

Thanks for looking a this!
Jeremy Linton Jan. 7, 2021, 7:45 p.m. UTC | #6
Hi,

On 1/7/21 11:36 AM, Rob Herring wrote:
> On Thu, Jan 7, 2021 at 9:24 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
>>
>> Hi,
>>
>>
>> On 1/7/21 9:28 AM, Rob Herring wrote:
>>> On Mon, Jan 4, 2021 at 9:57 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
>>>>
>>>> Given that most arm64 platform's PCI implementations needs quirks
>>>> to deal with problematic config accesses, this is a good place to
>>>> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality is handled

(trimming)

>>>>
>>>> +static int smccc_pcie_check_conduit(u16 seg)
>>>
>>> check what? Based on how you use this, perhaps _has_conduit() instead.
>>
>> Sure.
>>
>>>
>>>> +{
>>>> +       struct arm_smccc_res res;
>>>> +
>>>> +       if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
>>>> +               return -EOPNOTSUPP;
>>>> +
>>>> +       arm_smccc_smc(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
>>>> +       if ((int)res.a0 < 0)
>>>> +               return -EOPNOTSUPP;
>>>> +
>>>> +       arm_smccc_smc(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, &res);
>>>> +       if ((int)res.a0 < 0)
>>>> +               return -EOPNOTSUPP;
>>>
>>> Don't you need to check that read and write functions are supported?
>>
>> In theory no, the first version of the specification makes them
>> mandatory for all implementations. There isn't a partial access method,
>> so nothing works if only read or write were implemented.
> 
> Then the spec should change:
> 
> 2.3.3 Caller responsibilities
> The caller has the following responsibilities:
> • The caller must ensure that this function is implemented before
> issuing a call. This function is discoverable
> by calling PCI_FEATURES with pci_func_id set to 0x8400_0132.
> 
> 
> I guess knowing the version is ensuring, but the 2nd sentence makes it
> seem that is how one should ensure.

Ok, yes i understand, I will add the check.

> 
> Related, are there any sort of tests for the interface? I generally
> don't think the kernel's job is validating firmware (a frequent topic
> for DT), but we should have something. Maybe an SMC unittest module?
> If nothing else, seems like we should have at least one PCI_FEATURES
> call to make sure it works. We don't want to add it later only to find
> that it breaks on some firmware implementations. Though we can just
> add firmware quirks. ;)

I'm not aware of any unit tests at the moment. My testing so far has 
been against these patches: 
https://review.trustedfirmware.org/q/topic:"Arm_PCI_Config_Space_Interface"

But given the next version does the PCI_FEATURES calls, that will 
satisfy your concern, right?
Rob Herring Jan. 7, 2021, 8:35 p.m. UTC | #7
On Thu, Jan 7, 2021 at 12:45 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
> On 1/7/21 11:36 AM, Rob Herring wrote:
> > On Thu, Jan 7, 2021 at 9:24 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
> >>
> >> Hi,
> >>
> >>
> >> On 1/7/21 9:28 AM, Rob Herring wrote:
> >>> On Mon, Jan 4, 2021 at 9:57 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
> >>>>
> >>>> Given that most arm64 platform's PCI implementations needs quirks
> >>>> to deal with problematic config accesses, this is a good place to
> >>>> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality is handled
>
> (trimming)
>
> >>>>
> >>>> +static int smccc_pcie_check_conduit(u16 seg)
> >>>
> >>> check what? Based on how you use this, perhaps _has_conduit() instead.
> >>
> >> Sure.
> >>
> >>>
> >>>> +{
> >>>> +       struct arm_smccc_res res;
> >>>> +
> >>>> +       if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
> >>>> +               return -EOPNOTSUPP;
> >>>> +
> >>>> +       arm_smccc_smc(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
> >>>> +       if ((int)res.a0 < 0)
> >>>> +               return -EOPNOTSUPP;
> >>>> +
> >>>> +       arm_smccc_smc(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, &res);
> >>>> +       if ((int)res.a0 < 0)
> >>>> +               return -EOPNOTSUPP;
> >>>
> >>> Don't you need to check that read and write functions are supported?
> >>
> >> In theory no, the first version of the specification makes them
> >> mandatory for all implementations. There isn't a partial access method,
> >> so nothing works if only read or write were implemented.
> >
> > Then the spec should change:
> >
> > 2.3.3 Caller responsibilities
> > The caller has the following responsibilities:
> > • The caller must ensure that this function is implemented before
> > issuing a call. This function is discoverable
> > by calling PCI_FEATURES with pci_func_id set to 0x8400_0132.
> >
> >
> > I guess knowing the version is ensuring, but the 2nd sentence makes it
> > seem that is how one should ensure.
>
> Ok, yes i understand, I will add the check.
>
> >
> > Related, are there any sort of tests for the interface? I generally
> > don't think the kernel's job is validating firmware (a frequent topic
> > for DT), but we should have something. Maybe an SMC unittest module?
> > If nothing else, seems like we should have at least one PCI_FEATURES
> > call to make sure it works. We don't want to add it later only to find
> > that it breaks on some firmware implementations. Though we can just
> > add firmware quirks. ;)
>
> I'm not aware of any unit tests at the moment. My testing so far has
> been against these patches:
> https://review.trustedfirmware.org/q/topic:"Arm_PCI_Config_Space_Interface"
>
> But given the next version does the PCI_FEATURES calls, that will
> satisfy your concern, right?

Somewhat, but that doesn't replace the need for unittests. We have
PSCI checker, maybe the same thing should be required here. Otherwise,
implementations will only be as good as what Linux currently expects.

Rob
Jon Masters Jan. 7, 2021, 9:05 p.m. UTC | #8
Hi will, everyone,

On 1/7/21 1:14 PM, Will Deacon wrote:

> On Mon, Jan 04, 2021 at 10:57:35PM -0600, Jeremy Linton wrote:
>> Given that most arm64 platform's PCI implementations needs quirks
>> to deal with problematic config accesses, this is a good place to
>> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
>>
>> Lets hook the normal ACPI/PCI config path, and when we detect
>> missing MADT data, attempt to probe the SMC conduit. If the conduit
>> exists and responds for the requested segment number (provided by the
>> ACPI namespace) attach a custom pci_ecam_ops which redirects
>> all config read/write requests to the firmware.
>>
>> This patch is based on the Arm PCI Config space access document @
>> https://developer.arm.com/documentation/den0115/latest
> 
> Why does firmware need to be involved with this at all? Can't we just
> quirk Linux when these broken designs show up in production? We'll need
> to modify Linux _anyway_ when the firmware interface isn't implemented
> correctly...

I agree with Will on this. I think we want to find a way to address some 
of the non-compliance concerns through quirks in Linux. However...

Several folks here (particularly Lorenzo) have diligently worked hard 
over the past few years - and pushed their patience - to accommodate 
hardware vendors with early "not quite compliant" systems. They've taken 
lots of quirks that frankly shouldn't continue to be necessary were it 
even remotely a priority in the vendor ecosystem to get a handle on 
addressing PCIe compliance once and for all. But, again frankly, it 
hasn't been enough of a priority to get this fixed. The third party IP 
vendors *need* to address this, and their customers *need* to push back.

We can't keep having a situation in which kinda-sorta compliant stuff 
comes to market that would work out of the box but for whatever the 
quirk is this time around. There have been multiple OS releases for the 
past quite a few years on which this stuff could be tested prior to ever 
taping out a chip, and so it ought not to be possible to come to market 
now with an excuse that it wasn't tested. And yet here we still are. All 
these years and still the message isn't quite being received properly. I 
do know it takes time to make hardware, and some of it was designed 
years before and is still trickling down into these threads. But I also 
think there are cases where much more could have been done earlier.

None of these vendors can possibly want this deep down. Their engineers 
almost certainly realize that just having compliant ECAM would mean that 
the hardware was infinitely more valuable being able to run out of the 
box software that much more easily. And it's not just ECAM. Inevitably, 
that is just the observable syndrome for worse issues, often with the 
ITS and forcing quirked systems to have lousy legacy interrupts, etc. 
Alas, this level of nuance is likely lost by the time it reaches upper 
management, where "Linux" is all the same to them. I would hope that can 
change. I would also remind them that if they want to run non-Linux 
OSes, they will also want to be actually compliant. The willingness of 
kind folks like Lorenzo and others here to entertain quirks is not 
necessarily something you will find in every part of the industry.

But that all said, we have a situation in which there are still 
platforms out there that aren't fully compliant and something has to be 
done to support them because otherwise it's going to be even more ugly 
with vendor trees, distro hacks, and other stuff.

Some of you in recent weeks have asked what I and others can do to help 
from the distro and standardization side of things. To do my part, I'm 
going to commit to reach out to assorted vendors and have a heart to 
heart with them about really, truly fully addressing their compliance 
issues. That includes Cadence, Synopsys, and others who need to stop 
shipping IP that requires quirks, as well as SoC vendors who need to do 
more to test their silicon with stock kernels prior to taping out. And I 
would like to involve the good folks here who are trying to navigate.

I would also politely suggest that we collectively consider how much 
wiggle room there can be to use quirks for what we are stuck with rather 
than an SMC-based solution. We all know that quirks can't be a free ride 
forever. Those who need them should offer something strong in return. A 
firm commitment that they will never come asking for the same stuff in 
the future. Is there a way we can do something like that?

Jon.
Rob Herring Jan. 7, 2021, 9:49 p.m. UTC | #9
On Thu, Jan 7, 2021 at 2:06 PM Jon Masters <jcm@jonmasters.org> wrote:
>
> Hi will, everyone,
>
> On 1/7/21 1:14 PM, Will Deacon wrote:
>
> > On Mon, Jan 04, 2021 at 10:57:35PM -0600, Jeremy Linton wrote:
> >> Given that most arm64 platform's PCI implementations needs quirks
> >> to deal with problematic config accesses, this is a good place to
> >> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
> >>
> >> Lets hook the normal ACPI/PCI config path, and when we detect
> >> missing MADT data, attempt to probe the SMC conduit. If the conduit
> >> exists and responds for the requested segment number (provided by the
> >> ACPI namespace) attach a custom pci_ecam_ops which redirects
> >> all config read/write requests to the firmware.
> >>
> >> This patch is based on the Arm PCI Config space access document @
> >> https://developer.arm.com/documentation/den0115/latest
> >
> > Why does firmware need to be involved with this at all? Can't we just
> > quirk Linux when these broken designs show up in production? We'll need
> > to modify Linux _anyway_ when the firmware interface isn't implemented
> > correctly...
>
> I agree with Will on this. I think we want to find a way to address some
> of the non-compliance concerns through quirks in Linux. However...
>
> Several folks here (particularly Lorenzo) have diligently worked hard
> over the past few years - and pushed their patience - to accommodate
> hardware vendors with early "not quite compliant" systems. They've taken
> lots of quirks that frankly shouldn't continue to be necessary were it
> even remotely a priority in the vendor ecosystem to get a handle on
> addressing PCIe compliance once and for all. But, again frankly, it
> hasn't been enough of a priority to get this fixed. The third party IP
> vendors *need* to address this, and their customers *need* to push back.
>
> We can't keep having a situation in which kinda-sorta compliant stuff
> comes to market that would work out of the box but for whatever the
> quirk is this time around. There have been multiple OS releases for the
> past quite a few years on which this stuff could be tested prior to ever
> taping out a chip, and so it ought not to be possible to come to market
> now with an excuse that it wasn't tested. And yet here we still are. All
> these years and still the message isn't quite being received properly. I
> do know it takes time to make hardware, and some of it was designed
> years before and is still trickling down into these threads. But I also
> think there are cases where much more could have been done earlier.
>
> None of these vendors can possibly want this deep down. Their engineers
> almost certainly realize that just having compliant ECAM would mean that
> the hardware was infinitely more valuable being able to run out of the
> box software that much more easily. And it's not just ECAM. Inevitably,
> that is just the observable syndrome for worse issues, often with the
> ITS and forcing quirked systems to have lousy legacy interrupts, etc.
> Alas, this level of nuance is likely lost by the time it reaches upper
> management, where "Linux" is all the same to them. I would hope that can
> change. I would also remind them that if they want to run non-Linux
> OSes, they will also want to be actually compliant. The willingness of
> kind folks like Lorenzo and others here to entertain quirks is not
> necessarily something you will find in every part of the industry.
>
> But that all said, we have a situation in which there are still
> platforms out there that aren't fully compliant and something has to be
> done to support them because otherwise it's going to be even more ugly
> with vendor trees, distro hacks, and other stuff.
>
> Some of you in recent weeks have asked what I and others can do to help
> from the distro and standardization side of things. To do my part, I'm
> going to commit to reach out to assorted vendors and have a heart to
> heart with them about really, truly fully addressing their compliance
> issues. That includes Cadence, Synopsys, and others who need to stop
> shipping IP that requires quirks, as well as SoC vendors who need to do
> more to test their silicon with stock kernels prior to taping out. And I
> would like to involve the good folks here who are trying to navigate.

I agree with almost all this, but one issue on testing with stock
kernels. I've been on the other side of this though it's been a while
now. I've never seen much more than 'boot Linux' for pre Si testing.
I'd guess every platform did that (Calxeda's 64-bit chip did :)).
Maybe deeper pockets do more. Given the increased firmware that runs
before Linux nowadays that's probably only gotten harder. So while
testing with Linux is great, we still need to be specific about what's
compliant and not compliant. For example, stock linux can support
32-bit only accesses, but that's not what we'd consider passing. Maybe
Linux quirks need to be louder. Customers tend to not like seeing
error messages.

Rob
Lorenzo Pieralisi Jan. 8, 2021, 10:32 a.m. UTC | #10
On Thu, Jan 07, 2021 at 04:05:48PM -0500, Jon Masters wrote:
> Hi will, everyone,
> 
> On 1/7/21 1:14 PM, Will Deacon wrote:
> 
> > On Mon, Jan 04, 2021 at 10:57:35PM -0600, Jeremy Linton wrote:
> > > Given that most arm64 platform's PCI implementations needs quirks
> > > to deal with problematic config accesses, this is a good place to
> > > apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
> > > 
> > > Lets hook the normal ACPI/PCI config path, and when we detect
> > > missing MADT data, attempt to probe the SMC conduit. If the conduit
> > > exists and responds for the requested segment number (provided by the
> > > ACPI namespace) attach a custom pci_ecam_ops which redirects
> > > all config read/write requests to the firmware.
> > > 
> > > This patch is based on the Arm PCI Config space access document @
> > > https://developer.arm.com/documentation/den0115/latest
> > 
> > Why does firmware need to be involved with this at all? Can't we just
> > quirk Linux when these broken designs show up in production? We'll need
> > to modify Linux _anyway_ when the firmware interface isn't implemented
> > correctly...
> 
> I agree with Will on this. I think we want to find a way to address some
> of the non-compliance concerns through quirks in Linux. However...

I understand the concern and if you are asking me if this can be fixed
in Linux it obviously can. The point is, at what cost for SW and
maintenance - in Linux and other OSes, I think Jeremy summed it up
pretty well:

https://lore.kernel.org/linux-pci/61558f73-9ac8-69fe-34c1-2074dec5f18a@arm.com

The issue here is that what we are asked to support on ARM64 ACPI is a
moving target and the target carries PCI with it.

This potentially means that all drivers in:

drivers/pci/controller

may require an MCFG quirk and to implement it we may have to:

- Define new ACPI bindings (that may need AML and that's already a
  showstopper for some OSes)
- Require to manage clocks in the kernel (see link-up checks)
- Handle PCI config space faults in the kernel

Do we really want to do that ? I don't think so. Therefore we need
to have a policy to define what constitutes a "reasonable" quirk and
that's not objective I am afraid, however we slice it (there is no
such a thing as eg 90% ECAM).

The SMC is an olive branch and just to make sure it is crystal clear
there won't be room for adding quirks if the implementation turns out
to be broken, if a line in the sand is what we want here it is.

The question is: is there a reason we must not implement SMC support
given the above ?

As I said and you could imagine, I am the first who has concerns over
deviating from ECAM but if we do want ACPI support for platforms that
are not ECAM compliant something has to be done (HW may take years to
comply).

> Several folks here (particularly Lorenzo) have diligently worked hard
> over the past few years - and pushed their patience - to accommodate
> hardware vendors with early "not quite compliant" systems. They've taken
> lots of quirks that frankly shouldn't continue to be necessary were it
> even remotely a priority in the vendor ecosystem to get a handle on
> addressing PCIe compliance once and for all. But, again frankly, it
> hasn't been enough of a priority to get this fixed. The third party IP
> vendors *need* to address this, and their customers *need* to push back.
> 
> We can't keep having a situation in which kinda-sorta compliant stuff
> comes to market that would work out of the box but for whatever the quirk
> is this time around. There have been multiple OS releases for the past
> quite a few years on which this stuff could be tested prior to ever
> taping out a chip, and so it ought not to be possible to come to market
> now with an excuse that it wasn't tested. And yet here we still are. All
> these years and still the message isn't quite being received properly. I
> do know it takes time to make hardware, and some of it was designed years
> before and is still trickling down into these threads. But I also think
> there are cases where much more could have been done earlier.
> 
> None of these vendors can possibly want this deep down. Their engineers
> almost certainly realize that just having compliant ECAM would mean that
> the hardware was infinitely more valuable being able to run out of the
> box software that much more easily. And it's not just ECAM. Inevitably,
> that is just the observable syndrome for worse issues, often with the ITS
> and forcing quirked systems to have lousy legacy interrupts, etc. Alas,
> this level of nuance is likely lost by the time it reaches upper
> management, where "Linux" is all the same to them. I would hope that can
> change. I would also remind them that if they want to run non-Linux OSes,
> they will also want to be actually compliant. The willingness of kind
> folks like Lorenzo and others here to entertain quirks is not necessarily
> something you will find in every part of the industry.
> 
> But that all said, we have a situation in which there are still platforms
> out there that aren't fully compliant and something has to be done to
> support them because otherwise it's going to be even more ugly with
> vendor trees, distro hacks, and other stuff.
> 
> Some of you in recent weeks have asked what I and others can do to help
> from the distro and standardization side of things. To do my part, I'm
> going to commit to reach out to assorted vendors and have a heart to
> heart with them about really, truly fully addressing their compliance
> issues. That includes Cadence, Synopsys, and others who need to stop
> shipping IP that requires quirks, as well as SoC vendors who need to do
> more to test their silicon with stock kernels prior to taping out. And I
> would like to involve the good folks here who are trying to navigate.
> 
> I would also politely suggest that we collectively consider how much
> wiggle room there can be to use quirks for what we are stuck with rather
> than an SMC-based solution. We all know that quirks can't be a free ride
> forever. Those who need them should offer something strong in return. A
> firm commitment that they will never come asking for the same stuff in
> the future. Is there a way we can do something like that?

It depends on what we are asked to support and consequently what we
are willing to accept (and to be honest it is more Bjorn's patience
than mine that was exercised over the last few years on this topic).

Thanks,
Lorenzo
Vidya Sagar Jan. 12, 2021, 4:16 p.m. UTC | #11
On 1/5/2021 10:27 AM, Jeremy Linton wrote:
> External email: Use caution opening links or attachments
> 
> 
> Given that most arm64 platform's PCI implementations needs quirks
> to deal with problematic config accesses, this is a good place to
> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
> 
> Lets hook the normal ACPI/PCI config path, and when we detect
> missing MADT data, attempt to probe the SMC conduit. If the conduit
> exists and responds for the requested segment number (provided by the
> ACPI namespace) attach a custom pci_ecam_ops which redirects
> all config read/write requests to the firmware.
> 
> This patch is based on the Arm PCI Config space access document @
> https://developer.arm.com/documentation/den0115/latest
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>   arch/arm64/kernel/pci.c   | 87 +++++++++++++++++++++++++++++++++++++++
>   include/linux/arm-smccc.h | 26 ++++++++++++
>   2 files changed, 113 insertions(+)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 1006ed2d7c60..56d3773aaa25 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>
> @@ -107,6 +108,90 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>          return status;
>   }
> 
> +static int smccc_pcie_check_conduit(u16 seg)
> +{
> +       struct arm_smccc_res res;
> +
> +       if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
> +               return -EOPNOTSUPP;
> +
> +       arm_smccc_smc(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
> +       if ((int)res.a0 < 0)
> +               return -EOPNOTSUPP;
> +
> +       arm_smccc_smc(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, &res);
> +       if ((int)res.a0 < 0)
> +               return -EOPNOTSUPP;
> +
> +       pr_info("PCI: SMC conduit attached to segment %d\n", seg);
Shouldn't this print be moved towards the end of 
pci_acpi_setup_smccc_mapping() API?

> +
> +       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_smc(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_smc(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_ecam_ops = {
> +       .bus_shift      = 8,
> +       .pci_ops        = {
> +               .read           = smccc_pcie_config_read,
> +               .write          = smccc_pcie_config_write,
> +       }
> +};
> +
> +static struct pci_config_window *
> +pci_acpi_setup_smccc_mapping(struct acpi_pci_root *root)
> +{
> +       struct device *dev = &root->device->dev;
> +       struct resource *bus_res = &root->secondary;
> +       struct pci_config_window *cfg;
> +
> +       cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
> +       if (!cfg)
> +               return ERR_PTR(-ENOMEM);
> +
> +       cfg->parent = dev;
> +       cfg->ops = &smccc_pcie_ecam_ops;
> +       cfg->busr.start = bus_res->start;
> +       cfg->busr.end = bus_res->end;
> +       cfg->busr.flags = IORESOURCE_BUS;
> +
> +       cfg->res.name = "PCI SMCCC";
> +       if (cfg->ops->init)
Since there is no init implemented, what is the purpose of having this?

> +               cfg->ops->init(cfg);
> +       return cfg;
> +}
> +
>   /*
>    * Lookup the bus range for the domain in MCFG, and set up config space
>    * mapping.
> @@ -125,6 +210,8 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
> 
>          ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
>          if (ret) {
> +               if (!smccc_pcie_check_conduit(seg))
> +                       return pci_acpi_setup_smccc_mapping(root);
>                  dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res);
>                  return NULL;
>          }
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index f860645f6512..327f52533c71 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -89,6 +89,32 @@
> 
>   #define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED   1
> 
> +/* PCI ECAM conduit */
> +#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)
> +
>   /* Paravirtualised time calls (defined by ARM DEN0057A) */
>   #define ARM_SMCCC_HV_PV_TIME_FEATURES                          \
>          ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                 \
> --
> 2.26.2
>
Jeremy Linton Jan. 12, 2021, 4:57 p.m. UTC | #12
Hi,

On 1/12/21 10:16 AM, Vidya Sagar wrote:
> 
> 
> On 1/5/2021 10:27 AM, Jeremy Linton wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Given that most arm64 platform's PCI implementations needs quirks
>> to deal with problematic config accesses, this is a good place to
>> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
>>
>> Lets hook the normal ACPI/PCI config path, and when we detect
>> missing MADT data, attempt to probe the SMC conduit. If the conduit
>> exists and responds for the requested segment number (provided by the
>> ACPI namespace) attach a custom pci_ecam_ops which redirects
>> all config read/write requests to the firmware.
>>
>> This patch is based on the Arm PCI Config space access document @
>> https://developer.arm.com/documentation/den0115/latest
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   arch/arm64/kernel/pci.c   | 87 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/arm-smccc.h | 26 ++++++++++++
>>   2 files changed, 113 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 1006ed2d7c60..56d3773aaa25 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>
>> @@ -107,6 +108,90 @@ static int pci_acpi_root_prepare_resources(struct 
>> acpi_pci_root_info *ci)
>>          return status;
>>   }
>>
>> +static int smccc_pcie_check_conduit(u16 seg)
>> +{
>> +       struct arm_smccc_res res;
>> +
>> +       if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
>> +               return -EOPNOTSUPP;
>> +
>> +       arm_smccc_smc(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
>> +       if ((int)res.a0 < 0)
>> +               return -EOPNOTSUPP;
>> +
>> +       arm_smccc_smc(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, &res);
>> +       if ((int)res.a0 < 0)
>> +               return -EOPNOTSUPP;
>> +
>> +       pr_info("PCI: SMC conduit attached to segment %d\n", seg);
> Shouldn't this print be moved towards the end of 
> pci_acpi_setup_smccc_mapping() API?

Thanks for looking at this.

It probably should be, the assumption was that it would attach at this 
point, but its possible the message is inaccurate if something fails a 
bit later. I left it there because the segment number is easily 
available. I've been playing with this a bit for the V2 where I added 
the additional function checks.



> 
>> +
>> +       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_smc(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_smc(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_ecam_ops = {
>> +       .bus_shift      = 8,
>> +       .pci_ops        = {
>> +               .read           = smccc_pcie_config_read,
>> +               .write          = smccc_pcie_config_write,
>> +       }
>> +};
>> +
>> +static struct pci_config_window *
>> +pci_acpi_setup_smccc_mapping(struct acpi_pci_root *root)
>> +{
>> +       struct device *dev = &root->device->dev;
>> +       struct resource *bus_res = &root->secondary;
>> +       struct pci_config_window *cfg;
>> +
>> +       cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
>> +       if (!cfg)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       cfg->parent = dev;
>> +       cfg->ops = &smccc_pcie_ecam_ops;
>> +       cfg->busr.start = bus_res->start;
>> +       cfg->busr.end = bus_res->end;
>> +       cfg->busr.flags = IORESOURCE_BUS;
>> +
>> +       cfg->res.name = "PCI SMCCC";
>> +       if (cfg->ops->init)
> Since there is no init implemented, what is the purpose of having this?

Its basically dead.


> 
>> +               cfg->ops->init(cfg);
>> +       return cfg;
>> +}
>> +
>>   /*
>>    * Lookup the bus range for the domain in MCFG, and set up config space
>>    * mapping.
>> @@ -125,6 +210,8 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root 
>> *root)
>>
>>          ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
>>          if (ret) {
>> +               if (!smccc_pcie_check_conduit(seg))
>> +                       return pci_acpi_setup_smccc_mapping(root);
>>                  dev_err(dev, "%04x:%pR ECAM region not found\n", seg, 
>> bus_res);
>>                  return NULL;
>>          }
>> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
>> index f860645f6512..327f52533c71 100644
>> --- a/include/linux/arm-smccc.h
>> +++ b/include/linux/arm-smccc.h
>> @@ -89,6 +89,32 @@
>>
>>   #define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED   1
>>
>> +/* PCI ECAM conduit */
>> +#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)
>> +
>>   /* Paravirtualised time calls (defined by ARM DEN0057A) */
>>   #define ARM_SMCCC_HV_PV_TIME_FEATURES                          \
>>          ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                 \
>> -- 
>> 2.26.2
>>
Will Deacon Jan. 22, 2021, 7:48 p.m. UTC | #13
Hi Lorenzo,

On Fri, Jan 08, 2021 at 10:32:16AM +0000, Lorenzo Pieralisi wrote:
> On Thu, Jan 07, 2021 at 04:05:48PM -0500, Jon Masters wrote:
> > On 1/7/21 1:14 PM, Will Deacon wrote:
> > 
> > > On Mon, Jan 04, 2021 at 10:57:35PM -0600, Jeremy Linton wrote:
> > > > Given that most arm64 platform's PCI implementations needs quirks
> > > > to deal with problematic config accesses, this is a good place to
> > > > apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
> > > > 
> > > > Lets hook the normal ACPI/PCI config path, and when we detect
> > > > missing MADT data, attempt to probe the SMC conduit. If the conduit
> > > > exists and responds for the requested segment number (provided by the
> > > > ACPI namespace) attach a custom pci_ecam_ops which redirects
> > > > all config read/write requests to the firmware.
> > > > 
> > > > This patch is based on the Arm PCI Config space access document @
> > > > https://developer.arm.com/documentation/den0115/latest
> > > 
> > > Why does firmware need to be involved with this at all? Can't we just
> > > quirk Linux when these broken designs show up in production? We'll need
> > > to modify Linux _anyway_ when the firmware interface isn't implemented
> > > correctly...
> > 
> > I agree with Will on this. I think we want to find a way to address some
> > of the non-compliance concerns through quirks in Linux. However...
> 
> I understand the concern and if you are asking me if this can be fixed
> in Linux it obviously can. The point is, at what cost for SW and
> maintenance - in Linux and other OSes, I think Jeremy summed it up
> pretty well:
> 
> https://lore.kernel.org/linux-pci/61558f73-9ac8-69fe-34c1-2074dec5f18a@arm.com
> 
> The issue here is that what we are asked to support on ARM64 ACPI is a
> moving target and the target carries PCI with it.
> 
> This potentially means that all drivers in:
> 
> drivers/pci/controller
> 
> may require an MCFG quirk and to implement it we may have to:
> 
> - Define new ACPI bindings (that may need AML and that's already a
>   showstopper for some OSes)
> - Require to manage clocks in the kernel (see link-up checks)
> - Handle PCI config space faults in the kernel
> 
> Do we really want to do that ? I don't think so. Therefore we need
> to have a policy to define what constitutes a "reasonable" quirk and
> that's not objective I am afraid, however we slice it (there is no
> such a thing as eg 90% ECAM).

Without a doubt, I would much prefer to see these quirks and workarounds
in Linux than hidden behind a firmware interface. Every single time.

This isn't like the usual fragmentation problems, where firmware swoops in
to save the day; CPU onlining, spectre mitigations, early entropy etc. All
of these problems exist because there isn't a standard method to implement
them outside of firmware, and so adding a layer of abstraction there makes
sense.

But PCIe is already a standard!

We shouldn't paper over hardware designers' inability to follow a ~20 year
old standard by hiding it behind another standard that is hot off the press.
Seriously.

There is not a scrap of evidence to suggest that the firmware
implementations will be any better, but they will certainly be harder to
debug and maintain.  I have significant reservations about Arm's interest in
maintaining the spec as both more errata appear and the PCIe spec evolves
(after all, this is outside of SBSA, no?). The whole thing stinks of "if all
you have is a hammer, then everything looks like a nail". But this isn't the
sort of problem that is solved with yet another spec -- instead, how about
encouraging vendors to read the specs that already exist?

> The SMC is an olive branch and just to make sure it is crystal clear
> there won't be room for adding quirks if the implementation turns out
> to be broken, if a line in the sand is what we want here it is.

I appreciate the sentiment, but you're not solving the problem here. You're
moving it somewhere else. Somewhere where you don't have to deal with it
(and I honestly can't blame you for that), but also somewhere where you
_can't_ necessarily deal with it. The inevitable outcome is an endless
succession of crappy, non-compliant machines which only appear to operate
correctly with particularly kernel/firmware combinations. Imagine trying to
use something like that?

The approach championed here actively discourages vendors from building
spec-compliant hardware and reduces our ability to work around problems
on such hardware at the same time.

So I won't be applying these patches, sorry.

Will
Jeremy Linton Jan. 26, 2021, 4:46 p.m. UTC | #14
Hi,

On 1/22/21 1:48 PM, Will Deacon wrote:
> Hi Lorenzo,
> 
> On Fri, Jan 08, 2021 at 10:32:16AM +0000, Lorenzo Pieralisi wrote:
>> On Thu, Jan 07, 2021 at 04:05:48PM -0500, Jon Masters wrote:
>>> On 1/7/21 1:14 PM, Will Deacon wrote:
>>>
>>>> On Mon, Jan 04, 2021 at 10:57:35PM -0600, Jeremy Linton wrote:
>>>>> Given that most arm64 platform's PCI implementations needs quirks
>>>>> to deal with problematic config accesses, this is a good place to
>>>>> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
>>>>>
>>>>> Lets hook the normal ACPI/PCI config path, and when we detect
>>>>> missing MADT data, attempt to probe the SMC conduit. If the conduit
>>>>> exists and responds for the requested segment number (provided by the
>>>>> ACPI namespace) attach a custom pci_ecam_ops which redirects
>>>>> all config read/write requests to the firmware.
>>>>>
>>>>> This patch is based on the Arm PCI Config space access document @
>>>>> https://developer.arm.com/documentation/den0115/latest
>>>>
>>>> Why does firmware need to be involved with this at all? Can't we just
>>>> quirk Linux when these broken designs show up in production? We'll need
>>>> to modify Linux _anyway_ when the firmware interface isn't implemented
>>>> correctly...
>>>
>>> I agree with Will on this. I think we want to find a way to address some
>>> of the non-compliance concerns through quirks in Linux. However...
>>
>> I understand the concern and if you are asking me if this can be fixed
>> in Linux it obviously can. The point is, at what cost for SW and
>> maintenance - in Linux and other OSes, I think Jeremy summed it up
>> pretty well:
>>
>> https://lore.kernel.org/linux-pci/61558f73-9ac8-69fe-34c1-2074dec5f18a@arm.com
>>
>> The issue here is that what we are asked to support on ARM64 ACPI is a
>> moving target and the target carries PCI with it.
>>
>> This potentially means that all drivers in:
>>
>> drivers/pci/controller
>>
>> may require an MCFG quirk and to implement it we may have to:
>>
>> - Define new ACPI bindings (that may need AML and that's already a
>>    showstopper for some OSes)
>> - Require to manage clocks in the kernel (see link-up checks)
>> - Handle PCI config space faults in the kernel
>>
>> Do we really want to do that ? I don't think so. Therefore we need
>> to have a policy to define what constitutes a "reasonable" quirk and
>> that's not objective I am afraid, however we slice it (there is no
>> such a thing as eg 90% ECAM).
> 
> Without a doubt, I would much prefer to see these quirks and workarounds
> in Linux than hidden behind a firmware interface. Every single time.
> 
> This isn't like the usual fragmentation problems, where firmware swoops in
> to save the day; CPU onlining, spectre mitigations, early entropy etc. All
> of these problems exist because there isn't a standard method to implement
> them outside of firmware, and so adding a layer of abstraction there makes
> sense.

There are a lot of parallels with PSCI here because there were existing 
standards for cpu online.

> 
> But PCIe is already a standard!

And it says that ECAM is optional, particularly if there are 
firmware/platform standardized ways of accessing the config space.

> 
> We shouldn't paper over hardware designers' inability to follow a ~20 year
> old standard by hiding it behind another standard that is hot off the press.
> Seriously.

No disagreement, but its been more than half a decade and there are some 
high (millions!) volume parts, that still don't have kernel support.

> 
> There is not a scrap of evidence to suggest that the firmware
> implementations will be any better, but they will certainly be harder to
> debug and maintain.  I have significant reservations about Arm's interest in
> maintaining the spec as both more errata appear and the PCIe spec evolves
> (after all, this is outside of SBSA, no?). The whole thing stinks of "if all
> you have is a hammer, then everything looks like a nail". But this isn't the
> sort of problem that is solved with yet another spec -- instead, how about
> encouraging vendors to read the specs that already exist?

PSCI, isn't a good example of a firmware interface that works?

> 
>> The SMC is an olive branch and just to make sure it is crystal clear
>> there won't be room for adding quirks if the implementation turns out
>> to be broken, if a line in the sand is what we want here it is.
> 
> I appreciate the sentiment, but you're not solving the problem here. You're
> moving it somewhere else. Somewhere where you don't have to deal with it
> (and I honestly can't blame you for that), but also somewhere where you
> _can't_ necessarily deal with it. The inevitable outcome is an endless
> succession of crappy, non-compliant machines which only appear to operate
> correctly with particularly kernel/firmware combinations. Imagine trying to
> use something like that?
> 
> The approach championed here actively discourages vendors from building
> spec-compliant hardware and reduces our ability to work around problems
> on such hardware at the same time.
> 
> So I won't be applying these patches, sorry.

Does that mean its open season for ECAM quirks, and we can expect them 
to start being merged now?

Thanks.
Vikram Sethi Jan. 26, 2021, 5:08 p.m. UTC | #15
Hi Will, Lorenzo, Bjorn,

On 1/22/2021 1:48 PM, Will Deacon wrote:
> Hi Lorenzo,
>
> On Fri, Jan 08, 2021 at 10:32:16AM +0000, Lorenzo Pieralisi wrote:
>> On Thu, Jan 07, 2021 at 04:05:48PM -0500, Jon Masters wrote:
>>> On 1/7/21 1:14 PM, Will Deacon wrote:
>>>
>>>> On Mon, Jan 04, 2021 at 10:57:35PM -0600, Jeremy Linton wrote:
>>>>> Given that most arm64 platform's PCI implementations needs quirks
>>>>> to deal with problematic config accesses, this is a good place to
>>>>> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
>>>>>
>>>>> Lets hook the normal ACPI/PCI config path, and when we detect
>>>>> missing MADT data, attempt to probe the SMC conduit. If the conduit
>>>>> exists and responds for the requested segment number (provided by the
>>>>> ACPI namespace) attach a custom pci_ecam_ops which redirects
>>>>> all config read/write requests to the firmware.
>>>>>
>>>>> This patch is based on the Arm PCI Config space access document @
>>>>> https://developer.arm.com/documentation/den0115/latest
>>>> Why does firmware need to be involved with this at all? Can't we just
>>>> quirk Linux when these broken designs show up in production? We'll need
>>>> to modify Linux _anyway_ when the firmware interface isn't implemented
>>>> correctly...
>>> I agree with Will on this. I think we want to find a way to address some
>>> of the non-compliance concerns through quirks in Linux. However...
>> I understand the concern and if you are asking me if this can be fixed
>> in Linux it obviously can. The point is, at what cost for SW and
>> maintenance - in Linux and other OSes, I think Jeremy summed it up
>> pretty well:
>>
>> https://lore.kernel.org/linux-pci/61558f73-9ac8-69fe-34c1-2074dec5f18a@arm.com
>>
>> The issue here is that what we are asked to support on ARM64 ACPI is a
>> moving target and the target carries PCI with it.
>>
>> This potentially means that all drivers in:
>>
>> drivers/pci/controller
>>
>> may require an MCFG quirk and to implement it we may have to:
>>
>> - Define new ACPI bindings (that may need AML and that's already a
>>   showstopper for some OSes)
>> - Require to manage clocks in the kernel (see link-up checks)
>> - Handle PCI config space faults in the kernel
>>
>> Do we really want to do that ? I don't think so. Therefore we need
>> to have a policy to define what constitutes a "reasonable" quirk and
>> that's not objective I am afraid, however we slice it (there is no
>> such a thing as eg 90% ECAM).
> Without a doubt, I would much prefer to see these quirks and workarounds
> in Linux than hidden behind a firmware interface. Every single time.

In that case, can you please comment on/apply Tegra194 ECAM quirk that was rejected

a year ago, and was the reason we worked with Samer/ARM to define this common

mechanism?

https://lkml.org/lkml/2020/1/3/395

The T194 ECAM is from widely used Root Port IP from a IP vendor. That is one reason so many

*existing* SOCs have ECAM quirks. ARM is only now working with the Root port IP vendors

to test ECAM, MSI etc, but the reality is there were deficiencies in industry IP that is widely

used. If this common quirk is not the way to go, then please apply the T194 specific quirk which was

NAK'd a year ago, or suggest how to improve that quirk.

The ECAM issue has been fixed on future Tegra chips and is validated preSilicon with BSA

tests, so it is not going to be a recurrent issue for us.

>
> This isn't like the usual fragmentation problems, where firmware swoops in
> to save the day; CPU onlining, spectre mitigations, early entropy etc. All
> of these problems exist because there isn't a standard method to implement
> them outside of firmware, and so adding a layer of abstraction there makes
> sense.
>
> But PCIe is already a standard!
>
> We shouldn't paper over hardware designers' inability to follow a ~20 year
> old standard by hiding it behind another standard that is hot off the press.
> Seriously.
>
> There is not a scrap of evidence to suggest that the firmware
> implementations will be any better, but they will certainly be harder to
> debug and maintain.  I have significant reservations about Arm's interest in
> maintaining the spec as both more errata appear and the PCIe spec evolves
> (after all, this is outside of SBSA, no?). The whole thing stinks of "if all
> you have is a hammer, then everything looks like a nail". But this isn't the
> sort of problem that is solved with yet another spec -- instead, how about
> encouraging vendors to read the specs that already exist?
>
>> The SMC is an olive branch and just to make sure it is crystal clear
>> there won't be room for adding quirks if the implementation turns out
>> to be broken, if a line in the sand is what we want here it is.
> I appreciate the sentiment, but you're not solving the problem here. You're
> moving it somewhere else. Somewhere where you don't have to deal with it
> (and I honestly can't blame you for that), but also somewhere where you
> _can't_ necessarily deal with it. The inevitable outcome is an endless
> succession of crappy, non-compliant machines which only appear to operate
> correctly with particularly kernel/firmware combinations. Imagine trying to
> use something like that?
>
> The approach championed here actively discourages vendors from building
> spec-compliant hardware and reduces our ability to work around problems
> on such hardware at the same time.
>
> So I won't be applying these patches, sorry.
>
> Will
>
Will Deacon Jan. 26, 2021, 10:53 p.m. UTC | #16
On Tue, Jan 26, 2021 at 11:08:31AM -0600, Vikram Sethi wrote:
> On 1/22/2021 1:48 PM, Will Deacon wrote:
> > On Fri, Jan 08, 2021 at 10:32:16AM +0000, Lorenzo Pieralisi wrote:
> >> On Thu, Jan 07, 2021 at 04:05:48PM -0500, Jon Masters wrote:
> >>> On 1/7/21 1:14 PM, Will Deacon wrote:
> >>>> On Mon, Jan 04, 2021 at 10:57:35PM -0600, Jeremy Linton wrote:
> >>>>> Given that most arm64 platform's PCI implementations needs quirks
> >>>>> to deal with problematic config accesses, this is a good place to
> >>>>> apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
> >>>>>
> >>>>> Lets hook the normal ACPI/PCI config path, and when we detect
> >>>>> missing MADT data, attempt to probe the SMC conduit. If the conduit
> >>>>> exists and responds for the requested segment number (provided by the
> >>>>> ACPI namespace) attach a custom pci_ecam_ops which redirects
> >>>>> all config read/write requests to the firmware.
> >>>>>
> >>>>> This patch is based on the Arm PCI Config space access document @
> >>>>> https://developer.arm.com/documentation/den0115/latest
> >>>> Why does firmware need to be involved with this at all? Can't we just
> >>>> quirk Linux when these broken designs show up in production? We'll need
> >>>> to modify Linux _anyway_ when the firmware interface isn't implemented
> >>>> correctly...
> >>> I agree with Will on this. I think we want to find a way to address some
> >>> of the non-compliance concerns through quirks in Linux. However...
> >> I understand the concern and if you are asking me if this can be fixed
> >> in Linux it obviously can. The point is, at what cost for SW and
> >> maintenance - in Linux and other OSes, I think Jeremy summed it up
> >> pretty well:
> >>
> >> https://lore.kernel.org/linux-pci/61558f73-9ac8-69fe-34c1-2074dec5f18a@arm.com
> >>
> >> The issue here is that what we are asked to support on ARM64 ACPI is a
> >> moving target and the target carries PCI with it.
> >>
> >> This potentially means that all drivers in:
> >>
> >> drivers/pci/controller
> >>
> >> may require an MCFG quirk and to implement it we may have to:
> >>
> >> - Define new ACPI bindings (that may need AML and that's already a
> >>   showstopper for some OSes)
> >> - Require to manage clocks in the kernel (see link-up checks)
> >> - Handle PCI config space faults in the kernel
> >>
> >> Do we really want to do that ? I don't think so. Therefore we need
> >> to have a policy to define what constitutes a "reasonable" quirk and
> >> that's not objective I am afraid, however we slice it (there is no
> >> such a thing as eg 90% ECAM).
> > Without a doubt, I would much prefer to see these quirks and workarounds
> > in Linux than hidden behind a firmware interface. Every single time.
> 
> In that case, can you please comment on/apply Tegra194 ECAM quirk that was rejected
> 
> a year ago, and was the reason we worked with Samer/ARM to define this common
> 
> mechanism?
> 
> https://lkml.org/lkml/2020/1/3/395
> 
> The T194 ECAM is from widely used Root Port IP from a IP vendor. That is one reason so many
> 
> *existing* SOCs have ECAM quirks. ARM is only now working with the Root port IP vendors
> 
> to test ECAM, MSI etc, but the reality is there were deficiencies in industry IP that is widely
> 
> used. If this common quirk is not the way to go, then please apply the T194 specific quirk which was
> 
> NAK'd a year ago, or suggest how to improve that quirk.
> 
> The ECAM issue has been fixed on future Tegra chips and is validated preSilicon with BSA
> 
> tests, so it is not going to be a recurrent issue for us.

(aside: please fix your mail client not to add all these blank lines)

Personally, if a hundred lines of self-contained quirk code is all that is
needed to get your legacy IP running, then I would say we should merge it.
But I don't maintain the PCI subsystem, and I trust Bjorn and Lorenzo's
judgement as to what is the right thing to do when it concerns that code.
After all, they're the ones who end up having to look after this stuff long
after the hardware companies have stopped caring.

Will
Will Deacon Jan. 26, 2021, 10:54 p.m. UTC | #17
On Tue, Jan 26, 2021 at 10:46:04AM -0600, Jeremy Linton wrote:
> On 1/22/21 1:48 PM, Will Deacon wrote:
> > This isn't like the usual fragmentation problems, where firmware swoops in
> > to save the day; CPU onlining, spectre mitigations, early entropy etc. All
> > of these problems exist because there isn't a standard method to implement
> > them outside of firmware, and so adding a layer of abstraction there makes
> > sense.
> 
> There are a lot of parallels with PSCI here because there were existing
> standards for cpu online.

I don't recall anything that I would consider a standard at the time.

> > But PCIe is already a standard!
> 
> And it says that ECAM is optional, particularly if there are
> firmware/platform standardized ways of accessing the config space.

Nice loophole; I haven't checked.

> > We shouldn't paper over hardware designers' inability to follow a ~20 year
> > old standard by hiding it behind another standard that is hot off the press.
> > Seriously.
> 
> No disagreement, but its been more than half a decade and there are some
> high (millions!) volume parts, that still don't have kernel support.

Ok.

> > There is not a scrap of evidence to suggest that the firmware
> > implementations will be any better, but they will certainly be harder to
> > debug and maintain.  I have significant reservations about Arm's interest in
> > maintaining the spec as both more errata appear and the PCIe spec evolves
> > (after all, this is outside of SBSA, no?). The whole thing stinks of "if all
> > you have is a hammer, then everything looks like a nail". But this isn't the
> > sort of problem that is solved with yet another spec -- instead, how about
> > encouraging vendors to read the specs that already exist?
> 
> PSCI, isn't a good example of a firmware interface that works?

Not sure what you're getting at here.

> > > The SMC is an olive branch and just to make sure it is crystal clear
> > > there won't be room for adding quirks if the implementation turns out
> > > to be broken, if a line in the sand is what we want here it is.
> > 
> > I appreciate the sentiment, but you're not solving the problem here. You're
> > moving it somewhere else. Somewhere where you don't have to deal with it
> > (and I honestly can't blame you for that), but also somewhere where you
> > _can't_ necessarily deal with it. The inevitable outcome is an endless
> > succession of crappy, non-compliant machines which only appear to operate
> > correctly with particularly kernel/firmware combinations. Imagine trying to
> > use something like that?
> > 
> > The approach championed here actively discourages vendors from building
> > spec-compliant hardware and reduces our ability to work around problems
> > on such hardware at the same time.
> > 
> > So I won't be applying these patches, sorry.
> 
> Does that mean its open season for ECAM quirks, and we can expect them to
> start being merged now?

That's not for me to say.

Will
Jeremy Linton Jan. 28, 2021, 6:50 p.m. UTC | #18
Hi,

On 1/26/21 4:54 PM, Will Deacon wrote:
> On Tue, Jan 26, 2021 at 10:46:04AM -0600, Jeremy Linton wrote:
>> On 1/22/21 1:48 PM, Will Deacon wrote:
>>> This isn't like the usual fragmentation problems, where firmware swoops in
>>> to save the day; CPU onlining, spectre mitigations, early entropy etc. All
>>> of these problems exist because there isn't a standard method to implement
>>> them outside of firmware, and so adding a layer of abstraction there makes
>>> sense.
>>
>> There are a lot of parallels with PSCI here because there were existing
>> standards for cpu online.
> 
> I don't recall anything that I would consider a standard at the time.
> 
>>> But PCIe is already a standard!
>>
>> And it says that ECAM is optional, particularly if there are
>> firmware/platform standardized ways of accessing the config space.
> 
> Nice loophole; I haven't checked.
> 
>>> We shouldn't paper over hardware designers' inability to follow a ~20 year
>>> old standard by hiding it behind another standard that is hot off the press.
>>> Seriously.
>>
>> No disagreement, but its been more than half a decade and there are some
>> high (millions!) volume parts, that still don't have kernel support.
> 
> Ok.
> 
>>> There is not a scrap of evidence to suggest that the firmware
>>> implementations will be any better, but they will certainly be harder to
>>> debug and maintain.  I have significant reservations about Arm's interest in
>>> maintaining the spec as both more errata appear and the PCIe spec evolves
>>> (after all, this is outside of SBSA, no?). The whole thing stinks of "if all
>>> you have is a hammer, then everything looks like a nail". But this isn't the
>>> sort of problem that is solved with yet another spec -- instead, how about
>>> encouraging vendors to read the specs that already exist?
>>
>> PSCI, isn't a good example of a firmware interface that works?
> 
> Not sure what you're getting at here.

Simply, that PSCI is working fairly well today, and that hopefully this
specification will similarly mature.

Right now, there are at least three prongs for assuring compliance. The 
TF-A core service is being written to assure parameter validation and 
error checking is common across all the platforms. That code is 
attempting to cover every case called out by the SMC/PCI spec. The 
kernel is now utilizing the entire API service and cross validating the 
ACPI/SMC bus values and function existence. Lastly, but most important, 
there is the ACS PCI compliance suite*. That suite attempts to validate, 
among other things, config space behavior. When run on top of the SMC 
conduit, it provides assurance that root port registers/etc behave as 
the PCIe standard dictates.

* https://github.com/ARM-software/sbsa-acs/tree/master/test_pool/pcie

Thanks,
> 
>>>> The SMC is an olive branch and just to make sure it is crystal clear
>>>> there won't be room for adding quirks if the implementation turns out
>>>> to be broken, if a line in the sand is what we want here it is.
>>>
>>> I appreciate the sentiment, but you're not solving the problem here. You're
>>> moving it somewhere else. Somewhere where you don't have to deal with it
>>> (and I honestly can't blame you for that), but also somewhere where you
>>> _can't_ necessarily deal with it. The inevitable outcome is an endless
>>> succession of crappy, non-compliant machines which only appear to operate
>>> correctly with particularly kernel/firmware combinations. Imagine trying to
>>> use something like that?
>>>
>>> The approach championed here actively discourages vendors from building
>>> spec-compliant hardware and reduces our ability to work around problems
>>> on such hardware at the same time.
>>>
>>> So I won't be applying these patches, sorry.
>>
>> Does that mean its open season for ECAM quirks, and we can expect them to
>> start being merged now?
> 
> That's not for me to say.
> 
> Will
>
Bjorn Helgaas Jan. 28, 2021, 11:31 p.m. UTC | #19
On Tue, Jan 26, 2021 at 10:46:04AM -0600, Jeremy Linton wrote:
> On 1/22/21 1:48 PM, Will Deacon wrote:
> > On Fri, Jan 08, 2021 at 10:32:16AM +0000, Lorenzo Pieralisi wrote:
> > > On Thu, Jan 07, 2021 at 04:05:48PM -0500, Jon Masters wrote:
> > > > On 1/7/21 1:14 PM, Will Deacon wrote:
> > > > > On Mon, Jan 04, 2021 at 10:57:35PM -0600, Jeremy Linton wrote:
> > > > > > Given that most arm64 platform's PCI implementations needs quirks
> > > > > > to deal with problematic config accesses, this is a good place to
> > > > > > apply a firmware abstraction. The ARM PCI SMMCCC spec 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 functionality 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.
> > > > > > 
> > > > > > Lets hook the normal ACPI/PCI config path, and when we detect
> > > > > > missing MADT data, attempt to probe the SMC conduit. If the conduit
> > > > > > exists and responds for the requested segment number (provided by the
> > > > > > ACPI namespace) attach a custom pci_ecam_ops which redirects
> > > > > > all config read/write requests to the firmware.
> > > > > > 
> > > > > > This patch is based on the Arm PCI Config space access document @
> > > > > > https://developer.arm.com/documentation/den0115/latest
> > > > > 
> > > > > Why does firmware need to be involved with this at all? Can't we just
> > > > > quirk Linux when these broken designs show up in production? We'll need
> > > > > to modify Linux _anyway_ when the firmware interface isn't implemented
> > > > > correctly...
> > > > 
> > > > I agree with Will on this. I think we want to find a way to address some
> > > > of the non-compliance concerns through quirks in Linux. However...
> > > 
> > > I understand the concern and if you are asking me if this can be fixed
> > > in Linux it obviously can. The point is, at what cost for SW and
> > > maintenance - in Linux and other OSes, I think Jeremy summed it up
> > > pretty well:
> > > 
> > > https://lore.kernel.org/linux-pci/61558f73-9ac8-69fe-34c1-2074dec5f18a@arm.com
> > > 
> > > The issue here is that what we are asked to support on ARM64 ACPI is a
> > > moving target and the target carries PCI with it.
> > > 
> > > This potentially means that all drivers in:
> > > 
> > > drivers/pci/controller
> > > 
> > > may require an MCFG quirk and to implement it we may have to:
> > > 
> > > - Define new ACPI bindings (that may need AML and that's already a
> > >    showstopper for some OSes)
> > > - Require to manage clocks in the kernel (see link-up checks)
> > > - Handle PCI config space faults in the kernel
> > > 
> > > Do we really want to do that ? I don't think so. Therefore we need
> > > to have a policy to define what constitutes a "reasonable" quirk and
> > > that's not objective I am afraid, however we slice it (there is no
> > > such a thing as eg 90% ECAM).
> > 
> > Without a doubt, I would much prefer to see these quirks and workarounds
> > in Linux than hidden behind a firmware interface. Every single time.
> > 
> > This isn't like the usual fragmentation problems, where firmware swoops in
> > to save the day; CPU onlining, spectre mitigations, early entropy etc. All
> > of these problems exist because there isn't a standard method to implement
> > them outside of firmware, and so adding a layer of abstraction there makes
> > sense.

> > But PCIe is already a standard!
> 
> And it says that ECAM is optional, particularly if there are
> firmware/platform standardized ways of accessing the config space.

This is really a tangent to the main discussion, but I don't read it
quite that way.  PCIe r5.0, sec 7.2.2 says:

  For systems that are PC-compatible, or that do not implement a
  processor-architecture-specific firmware interface standard that
  allows access to the Configuration Space, the Enhanced Configuration
  Access Mechanism (ECAM) is required as defined in this section.

I read that as "ECAM is *required* unless you have a standard firmware
interface."  I think the firmware interface originally referred to
ia64 SAL, but I would say this new ARM thing also qualifies.

But the bottom line is we all want to make it so new hardware works
with old kernels.  Nobody wants to ask distros to release updates just
to boot a new platform.  That means a generic host bridge driver (like
the ACPI pci_root.c) and generic config access (like ECAM or SAL).

> > I appreciate the sentiment, but you're not solving the problem
> > here. You're moving it somewhere else. Somewhere where you don't
> > have to deal with it (and I honestly can't blame you for that),
> > but also somewhere where you _can't_ necessarily deal with it. The
> > inevitable outcome is an endless succession of crappy,
> > non-compliant machines which only appear to operate correctly with
> > particularly kernel/firmware combinations. Imagine trying to use
> > something like that?
> > 
> > The approach championed here actively discourages vendors from
> > building spec-compliant hardware and reduces our ability to work
> > around problems on such hardware at the same time.

That's a pretty good argument.  And it's true that there are
subtleties even in seemingly trivial interface like this.
Synchronization and error handling are common problem areas.

> Does that mean its open season for ECAM quirks, and we can expect
> them to start being merged now?

"Open season" makes me cringe because it suggests we have a license to
use quirks indiscriminately forever, and I hope that's not the case.

Lorenzo is closer to this issue than I am and has much better insight
into the mess this could turn into.  From my point of view, it's
shocking how much of a hassle this is compared to x86.  There just
aren't ECAM quirks, in-kernel clock management, or any of that crap.
I don't know how they do it on x86 and I don't have to care.  Whatever
they need to do, they apparently do in AML.  Eventually ARM64 has to
get there as well if vendors want distro support.

I don't want to be in the position of enforcing a draconian "no more
quirks ever" policy.  The intent -- to encourage/force vendors to
develop spec-compliant machines -- is good, but it seems like the
reward of having compliant machines "just work" vs the penalty of
having to write quirks and shepherd them upstream and into distros
will probably be more effective and not much slower.

Bjorn
Lorenzo Pieralisi Feb. 25, 2021, 9:30 a.m. UTC | #20
On Thu, Feb 18, 2021 at 12:43:30PM -0500, Jon Masters wrote:
> Hi Bjorn, all,
> 
> On Thu, Jan 28, 2021 at 6:31 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
>     On Tue, Jan 26, 2021 at 10:46:04AM -0600, Jeremy Linton wrote:
> 
>  
> 
>     > Does that mean its open season for ECAM quirks, and we can expect
>     > them to start being merged now?
> 
>     "Open season" makes me cringe because it suggests we have a license to
>     use quirks indiscriminately forever, and I hope that's not the case.
> 
>     Lorenzo is closer to this issue than I am and has much better insight
>     into the mess this could turn into.  From my point of view, it's
>     shocking how much of a hassle this is compared to x86.  There just
>     aren't ECAM quirks, in-kernel clock management, or any of that crap.
>     I don't know how they do it on x86 and I don't have to care.  Whatever
>     they need to do, they apparently do in AML.  Eventually ARM64 has to
>     get there as well if vendors want distro support.
> 
>     I don't want to be in the position of enforcing a draconian "no more
>     quirks ever" policy.  The intent -- to encourage/force vendors to
>     develop spec-compliant machines -- is good, but it seems like the
>     reward of having compliant machines "just work" vs the penalty of
>     having to write quirks and shepherd them upstream and into distros
>     will probably be more effective and not much slower.
> 
> 
> The problem is that the third party IP vendors (still) make too much junk. For
> years, there wasn't a compliance program (e.g. SystemReady with some of the
> meat behind PCI-SIG compliance) and even when there was the third party IP
> vendors building "root ports" (not even RCs) would make some junk with a hacked
> up Linux kernel booting on a model and demo that as "PCI". There wasn't the
> kind of adult supervision that was required. It is (slowly) happening now, but
> it's years and years late. It's just embarrassing to see the lack of ECAM that
> works. In many cases, it's because the IP being used was baked years ago or
> made for some "non server" (as if there is such a thing) use case, etc. But in
> others, there was a chance to do it right, and it still happens. Some of us
> have lost what hair we had over the years getting third party IP vendors to
> wake up and start caring about this.
> 
> So there's no excuse. None at all. However, this is where we are. And it /is/
> improving. But it's still too slow, and we have platforms still coming to
> market that need to boot and run. Based on this, and the need to have something
> more flexible than just solving for ECAM deficiencies (which are really just a
> symptom), I can see the allure of an SMC. I don't like it, but if that's where
> folks want to go, and if we can find a way to constrain the enthusiasm for it,
> then perhaps it is a path forward. But if we are to go down that path it needs
> to come with a giant warning from the kernel that a system was booted at is
> relying on that. Something that will cause an OS certification program to fail
> without a waiver, or will cause customers to phone up for support wondering why
> the hw is broken. It *must* not be a silent thing. It needs to be "this
> hardware is broken and non-standard, get the next version fixed".

It is a stance I agree with in many respects, it should be shared (it
was in HTML format - the lists unfortunately dropped the message) so I
am replying to it to make it public.

Thanks,
Lorenzo
Jeremy Linton Feb. 25, 2021, 10:31 p.m. UTC | #21
Hi,

On 2/25/21 3:30 AM, Lorenzo Pieralisi wrote:
> On Thu, Feb 18, 2021 at 12:43:30PM -0500, Jon Masters wrote:
>> Hi Bjorn, all,
>>
>> On Thu, Jan 28, 2021 at 6:31 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>
>>      On Tue, Jan 26, 2021 at 10:46:04AM -0600, Jeremy Linton wrote:
>>
>>   
>>
>>      > Does that mean its open season for ECAM quirks, and we can expect
>>      > them to start being merged now?
>>
>>      "Open season" makes me cringe because it suggests we have a license to
>>      use quirks indiscriminately forever, and I hope that's not the case.
>>
>>      Lorenzo is closer to this issue than I am and has much better insight
>>      into the mess this could turn into.  From my point of view, it's
>>      shocking how much of a hassle this is compared to x86.  There just
>>      aren't ECAM quirks, in-kernel clock management, or any of that crap.
>>      I don't know how they do it on x86 and I don't have to care.  Whatever
>>      they need to do, they apparently do in AML.  Eventually ARM64 has to
>>      get there as well if vendors want distro support.
>>
>>      I don't want to be in the position of enforcing a draconian "no more
>>      quirks ever" policy.  The intent -- to encourage/force vendors to
>>      develop spec-compliant machines -- is good, but it seems like the
>>      reward of having compliant machines "just work" vs the penalty of
>>      having to write quirks and shepherd them upstream and into distros
>>      will probably be more effective and not much slower.
>>
>>
>> The problem is that the third party IP vendors (still) make too much junk. For
>> years, there wasn't a compliance program (e.g. SystemReady with some of the
>> meat behind PCI-SIG compliance) and even when there was the third party IP
>> vendors building "root ports" (not even RCs) would make some junk with a hacked
>> up Linux kernel booting on a model and demo that as "PCI". There wasn't the
>> kind of adult supervision that was required. It is (slowly) happening now, but
>> it's years and years late. It's just embarrassing to see the lack of ECAM that
>> works. In many cases, it's because the IP being used was baked years ago or
>> made for some "non server" (as if there is such a thing) use case, etc. But in
>> others, there was a chance to do it right, and it still happens. Some of us
>> have lost what hair we had over the years getting third party IP vendors to
>> wake up and start caring about this.
>>
>> So there's no excuse. None at all. However, this is where we are. And it /is/
>> improving. But it's still too slow, and we have platforms still coming to
>> market that need to boot and run. Based on this, and the need to have something
>> more flexible than just solving for ECAM deficiencies (which are really just a
>> symptom), I can see the allure of an SMC. I don't like it, but if that's where
>> folks want to go, and if we can find a way to constrain the enthusiasm for it,
>> then perhaps it is a path forward. But if we are to go down that path it needs
>> to come with a giant warning from the kernel that a system was booted at is
>> relying on that. Something that will cause an OS certification program to fail
>> without a waiver, or will cause customers to phone up for support wondering why
>> the hw is broken. It *must* not be a silent thing. It needs to be "this
>> hardware is broken and non-standard, get the next version fixed".
> 
> It is a stance I agree with in many respects, it should be shared (it
> was in HTML format - the lists unfortunately dropped the message) so I
> am replying to it to make it public.


So, the V3 of this set has a pr_info of "PCI: SMC conduit attached to 
segment %d". I will respin with that at pr_warn() which seems to fulfill 
the comment above. Is that "giant" enough, or should it be higher/worded 
differently?

Thanks,
diff mbox series

Patch

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 1006ed2d7c60..56d3773aaa25 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>
@@ -107,6 +108,90 @@  static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
 	return status;
 }
 
+static int smccc_pcie_check_conduit(u16 seg)
+{
+	struct arm_smccc_res res;
+
+	if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
+		return -EOPNOTSUPP;
+
+	arm_smccc_smc(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
+	if ((int)res.a0 < 0)
+		return -EOPNOTSUPP;
+
+	arm_smccc_smc(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, &res);
+	if ((int)res.a0 < 0)
+		return -EOPNOTSUPP;
+
+	pr_info("PCI: SMC conduit attached to segment %d\n", seg);
+
+	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_smc(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_smc(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_ecam_ops = {
+	.bus_shift	= 8,
+	.pci_ops	= {
+		.read		= smccc_pcie_config_read,
+		.write		= smccc_pcie_config_write,
+	}
+};
+
+static struct pci_config_window *
+pci_acpi_setup_smccc_mapping(struct acpi_pci_root *root)
+{
+	struct device *dev = &root->device->dev;
+	struct resource *bus_res = &root->secondary;
+	struct pci_config_window *cfg;
+
+	cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+	if (!cfg)
+		return ERR_PTR(-ENOMEM);
+
+	cfg->parent = dev;
+	cfg->ops = &smccc_pcie_ecam_ops;
+	cfg->busr.start = bus_res->start;
+	cfg->busr.end = bus_res->end;
+	cfg->busr.flags = IORESOURCE_BUS;
+
+	cfg->res.name = "PCI SMCCC";
+	if (cfg->ops->init)
+		cfg->ops->init(cfg);
+	return cfg;
+}
+
 /*
  * Lookup the bus range for the domain in MCFG, and set up config space
  * mapping.
@@ -125,6 +210,8 @@  pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
 
 	ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
 	if (ret) {
+		if (!smccc_pcie_check_conduit(seg))
+			return pci_acpi_setup_smccc_mapping(root);
 		dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res);
 		return NULL;
 	}
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index f860645f6512..327f52533c71 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -89,6 +89,32 @@ 
 
 #define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED	1
 
+/* PCI ECAM conduit */
+#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)
+
 /* Paravirtualised time calls (defined by ARM DEN0057A) */
 #define ARM_SMCCC_HV_PV_TIME_FEATURES				\
 	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\