linux-kernel - Re: [PATCH] arm64: PCI: Enable SMC conduit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5d4f85a4-248b-b62e-f976-63c6214bf588@arm.com>
Date:   Thu, 7 Jan 2021 13:45:45 -0600
From:   Jeremy Linton <jeremy.linton@....com>
To:     Rob Herring <robh@...nel.org>
Cc:     linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        PCI <linux-pci@...r.kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Mark Rutland <mark.rutland@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] arm64: PCI: Enable SMC conduit

Hi,

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