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

[Jeremy Linton <jeremy.linton@arm.com>]

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?