Re: [PATCH] arm64: PCI: Enable SMC conduit

From: Jeremy Linton
Date: Thu Jan 07 2021 - 14:46:37 EST


Hi,

On 1/7/21 11:36 AM, Rob Herring wrote:
On Thu, Jan 7, 2021 at 9:24 AM Jeremy Linton <jeremy.linton@xxxxxxx> 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@xxxxxxx> 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?