linux-kernel - Re: [PATCH v2] PCI: keystone: add a pci quirk to limit mrrs

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]
Date:	Fri, 5 Sep 2014 14:40:43 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Murali Karicheri <m-karicheri2@...com>
Cc:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	linux-arm <linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>
Subject: Re: [PATCH v2] PCI: keystone: add a pci quirk to limit mrrs

On Fri, Sep 5, 2014 at 2:30 PM, Murali Karicheri <m-karicheri2@...com> wrote:
> On 09/05/2014 03:23 PM, Bjorn Helgaas wrote:
>>
>> On Fri, Aug 08, 2014 at 03:45:32PM -0400, Murali Karicheri wrote:
>>>
>>> Keystone PCI controller has a limitation that memory read request
>>> size must not exceed 256 bytes. This is a hardware limitation and
>>> add a quirk to force this limit on all downstream devices by
>>> updating mrrs.
>>>
>>> Signed-off-by: Murali Karicheri<m-karicheri2@...com>
>>> ---
>>>   -v2: made the quirk happens after tuning
>>>   -v1: changed printk to indicate PCI bdf
>>>   This applies on top of the Keystone PCI controller patch series
>>>   at http://thread.gmane.org/gmane.linux.kernel.pci/33523
>>>   drivers/pci/host/pci-keystone.c |   40
>>> +++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 40 insertions(+)
>>>
>>> diff --git a/drivers/pci/host/pci-keystone.c
>>> b/drivers/pci/host/pci-keystone.c
>>> index c1cfaef..a132622 100644
>>> --- a/drivers/pci/host/pci-keystone.c
>>> +++ b/drivers/pci/host/pci-keystone.c
>>> @@ -42,8 +42,48 @@
>>>   /* DEV_STAT_CTRL */
>>>   #define PCIE_CAP_BASE         0x70
>>>
>>> +/* PCIE controller device IDs */
>>> +#define PCIE_RC_K2HK           0xb008
>>> +#define PCIE_RC_K2E            0xb009
>>> +#define PCIE_RC_K2L            0xb00a
>>
>>
>> A Root Complex doesn't appear in config space as a PCI device, so I think
>> these are actually Root Port devices, aren't they?
>
>
> Yes.
>
>>
>> I suppose the spec would allow Root Complex Integrated Endpoints, on which
>> MRRS could also be set, but I think your quirk ignores them because they
>> would appear on the root bus.  Is ignoring them OK, or would you want to
>> set MRRS for those as well?
>
>
> How does the Root Complex integrated endpoints are designed? Our PCI can act
> as a RC or EP and not both at the same time. So I guess this is not
> applicable.

My guess is that you probably don't have any integrated endpoints and
hence don't need to worry about MRRS for them.  The PCIe spec (r3.0,
sec 1.3.2.3) says "A Root Complex Integrated Endpoint is implemented
on internal logic of Root Complexes that contains [sic] the Root
Ports."

For example, my laptop has:

  00:03.0 Audio device: Intel Corporation Haswell-ULT HD Audio Controller
  00:1c.0 PCI bridge: Intel Corporation Lynx Point-LP PCI Express Root Port 1
  00:1c.3 PCI bridge: Intel Corporation Lynx Point-LP PCI Express Root Port 4

the 00:03.0 Audio device is a PCIe device but it is a sibling of the
Root Ports rather than being downstream from them, i.e., it's an
Integrated Endpoint that is part of the Root Complex itself.

>>> +
>>>   #define to_keystone_pcie(x)   container_of(x, struct keystone_pcie, pp)
>>>
>>> +static void quirk_limit_mrrs(struct pci_dev *dev)
>>> +{
>>> +       struct pci_bus *bus = dev->bus;
>>> +       struct pci_dev *bridge = bus->self;
>>> +
>>> +       if (pci_is_root_bus(bus))
>>> +               return;
>>> +
>>> +       /* look for the host bridge */
>>> +       while (!pci_is_root_bus(bus)) {
>>> +               bridge = bus->self;
>>> +               bus = bus->parent;
>>> +       }
>>> +
>>> +       if (bridge) {
>>> +               u16 id;
>>> +
>>> +               /*
>>> +                * Keystone PCI controller has a h/w limitation of
>>> +                * 256 bytes maximum read request size. It can't handle
>>> +                * anything higher than this. So force this limit on
>>> +                * all downstream devices
>>> +                */
>>> +               pci_read_config_word(bridge, PCI_DEVICE_ID,&id);
>>
>>
>> I think you should check the Vendor ID here as well.  Otherwise, there's a
>> possibility of applying this quirk to devices under a bridge with the same
>> Device ID but different Vendor ID.
>>
>> And I think you should be able to use the bridge->vendor and
>> bridge->device
>> fields instead of reading them from config space.  Actually, this might be
>> a good place to use pci_match_id().
>
>
> Ok.
>
>
>>
>>> +               if ((id == PCIE_RC_K2HK) || (id == PCIE_RC_K2E) ||
>>> +                   (id == PCIE_RC_K2L)) {
>>> +                       if (pcie_get_readrq(dev)>  256) {
>>> +                               dev_info(&dev->dev, "limiting mrrs to
>>> 256\n");
>>> +                               pcie_set_readrq(dev, 256);
>>> +                       }
>>> +               }
>>> +       }
>>> +}
>>> +DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_mrrs);
>>> +
>>>   static int ks_pcie_establish_link(struct keystone_pcie *ks_pcie)
>>>   {
>>>         struct pcie_port *pp =&ks_pcie->pp;
>>> --
>>> 1.7.9.5
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/