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

[Rob Herring <robh@kernel.org>]

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