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

[Jeremy Linton]

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?