| 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
| ||
|
Message-ID: <330f9f0d-761a-706b-2a58-2ae15f2e0105@broadcom.com>
Date: Tue, 12 Jun 2018 09:56:46 -0700
From: Ray Jui <ray.jui@...adcom.com>
To: poza@...eaurora.org
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Bjorn Helgaas <helgaas@...nel.org>,
linux-kernel@...r.kernel.org,
bcm-kernel-feedback-list@...adcom.com, linux-pci@...r.kernel.org,
Ray Jui <rjui@...adcom.com>, linux-pci-owner@...r.kernel.org
Subject: Re: [PATCH v2 2/5] PCI: iproc: Fix up corrupted PAXC root complex
config registers
On 6/12/2018 5:23 AM, poza@...eaurora.org wrote:
> On 2018-06-12 13:57, poza@...eaurora.org wrote:
>> On 2018-06-12 05:51, Ray Jui wrote:
>>> On certain versions of Broadcom PAXC based root complexes, certain
>>> regions of the configuration space are corrupted. As a result, it
>>> prevents the Linux PCIe stack from traversing the linked list of the
>>> capability registers completely and therefore the root complex is
>>> not advertised as "PCIe capable". This prevents the correct PCIe RID
>>> from being parsed in the kernel PCIe stack. A correct RID is required
>>> for mapping to a stream ID from the SMMU or the device ID from the
>>> GICv3 ITS
>>>
>>> This patch fixes up the issue by manually populating the related
>>> PCIe capabilities
>>>
>>> Signed-off-by: Ray Jui <rjui@...adcom.com>
>>> ---
>>> drivers/pci/host/pcie-iproc.c | 65
>>> +++++++++++++++++++++++++++++++++++++++----
>>> drivers/pci/host/pcie-iproc.h | 3 ++
>>> 2 files changed, 62 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-iproc.c
>>> b/drivers/pci/host/pcie-iproc.c
>>> index 3c76c5f..680f6b1 100644
>>> --- a/drivers/pci/host/pcie-iproc.c
>>> +++ b/drivers/pci/host/pcie-iproc.c
>>> @@ -85,6 +85,8 @@
>>> #define IMAP_VALID_SHIFT 0
>>> #define IMAP_VALID BIT(IMAP_VALID_SHIFT)
>>>
>>> +#define IPROC_PCI_PM_CAP 0x48
>>> +#define IPROC_PCI_PM_CAP_MASK 0xffff
>>> #define IPROC_PCI_EXP_CAP 0xac
>>>
>>> #define IPROC_PCIE_REG_INVALID 0xffff
>>> @@ -375,6 +377,17 @@ static const u16 iproc_pcie_reg_paxc_v2[] = {
>>> [IPROC_PCIE_CFG_DATA] = 0x1fc,
>>> };
>>>
>>> +/*
>>> + * List of device IDs of controllers that have corrupted capability
>>> list that
>>> + * require SW fixup
>>> + */
>>> +static const u16 iproc_pcie_corrupt_cap_did[] = {
>>> + 0x16cd,
>>> + 0x16f0,
>>> + 0xd802,
>>> + 0xd804
>>> +};
>>> +
>>> static inline struct iproc_pcie *iproc_data(struct pci_bus *bus)
>>> {
>>> struct iproc_pcie *pcie = bus->sysdata;
>>> @@ -495,6 +508,49 @@ static unsigned int iproc_pcie_cfg_retry(void
>>> __iomem *cfg_data_p)
>>> return data;
>>> }
>>>
>>> +static void iproc_pcie_fix_cap(struct iproc_pcie *pcie, int where,
>>> u32 *val)
>>> +{
>>> + u32 i, dev_id;
>>> +
>>> + switch (where & ~0x3) {
>>> + case PCI_VENDOR_ID:
>>> + dev_id = *val >> 16;
>>> +
>>> + /*
>>> + * Activate fixup for those controllers that have corrupted
>>> + * capability list registers
>>> + */
>>> + for (i = 0; i < ARRAY_SIZE(iproc_pcie_corrupt_cap_did); i++)
>>> + if (dev_id == iproc_pcie_corrupt_cap_did[i])
>>> + pcie->fix_paxc_cap = true;
>>
>> and I think this code will try to fix up every time config space is read.
>> Does this get corrupted often, randomly ?
>> Can it not be solved by using one time Quirk ?
>> and if not Quirk, you dont want to be setting pcie->fix_paxc_cap =
>> false somewhere
>>
>> besides, pcie->fix_paxc_cap = true; is set if PCI_VENDOR_ID is read
>> first.
>> and rest cases stay with the assumption that PCI_VENDOR_ID will be
>> read first.
>> which is infact read first during enumeration
>> (that is the assumption code is making), but that is safe assumption
>> to make I think.
>>
>
> ok I see that Bjorn has suggested to fix it this way instead of Quirks.
> will just mark
Right, and there are benefits with this approach like Bjorn has
explained. We now have consistent lspci and config dump output using
this approach.
>
> Reviewed-by: Oza Pawandeep <poza@...eaurora.org>
>
>>> + break;
>>> +
>>> + case IPROC_PCI_PM_CAP:
>>> + if (pcie->fix_paxc_cap) {
>>> + /* advertise PM, force next capability to PCIe */
>>> + *val &= ~IPROC_PCI_PM_CAP_MASK;
>>> + *val |= IPROC_PCI_EXP_CAP << 8 | PCI_CAP_ID_PM;
>>> + }
>>> + break;
>>> +
>>> + case IPROC_PCI_EXP_CAP:
>>> + if (pcie->fix_paxc_cap) {
>>> + /* advertise root port, version 2, terminate here */
>>> + *val = (PCI_EXP_TYPE_ROOT_PORT << 4 | 2) << 16 |
>>> + PCI_CAP_ID_EXP;
>>> + }
>>> + break;
>>> +
>>> + case IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL:
>>> + /* Don't advertise CRS SV support */
>>> + *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
>>> + break;
>>> +
>>> + default:
>>> + break;
>>> + }
>>> +}
>>> +
>>> static int iproc_pcie_config_read(struct pci_bus *bus, unsigned int
>>> devfn,
>>> int where, int size, u32 *val)
>>> {
>>> @@ -509,13 +565,10 @@ static int iproc_pcie_config_read(struct pci_bus
>>> *bus, unsigned int devfn,
>>> /* root complex access */
>>> if (busno == 0) {
>>> ret = pci_generic_config_read32(bus, devfn, where, size, val);
>>> - if (ret != PCIBIOS_SUCCESSFUL)
>>> - return ret;
>>> + if (ret == PCIBIOS_SUCCESSFUL)
>>> + iproc_pcie_fix_cap(pcie, where, val);
>>>
>>> - /* Don't advertise CRS SV support */
>>> - if ((where & ~0x3) == IPROC_PCI_EXP_CAP + PCI_EXP_RTCTL)
>>> - *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
>>> - return PCIBIOS_SUCCESSFUL;
>>> + return ret;
>>> }
>>>
>>> cfg_data_p = iproc_pcie_map_ep_cfg_reg(pcie, busno, slot, fn,
>>> where);
>>> diff --git a/drivers/pci/host/pcie-iproc.h
>>> b/drivers/pci/host/pcie-iproc.h
>>> index 814b600..9d5cfee 100644
>>> --- a/drivers/pci/host/pcie-iproc.h
>>> +++ b/drivers/pci/host/pcie-iproc.h
>>> @@ -60,6 +60,8 @@ struct iproc_msi;
>>> * @ep_is_internal: indicates an internal emulated endpoint device
>>> is connected
>>> * @has_apb_err_disable: indicates the controller can be configured
>>> to prevent
>>> * unsupported request from being forwarded as an APB bus error
>>> + * @fix_paxc_cap: indicates the controller has corrupted capability
>>> list in its
>>> + * config space registers and requires SW based fixup
>>> *
>>> * @need_ob_cfg: indicates SW needs to configure the outbound
>>> mapping window
>>> * @ob: outbound mapping related parameters
>>> @@ -85,6 +87,7 @@ struct iproc_pcie {
>>> int (*map_irq)(const struct pci_dev *, u8, u8);
>>> bool ep_is_internal;
>>> bool has_apb_err_disable;
>>> + bool fix_paxc_cap;
>>>
>>> bool need_ob_cfg;
>>> struct iproc_pcie_ob ob;
Powered by blists - more mailing lists