| 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: <02EA37E5-87CB-401E-B5CA-BDE3A0D92517@intel.com>
Date: Fri, 25 Sep 2020 14:49:27 -0700
From: "Sean V Kelley" <sean.v.kelley@...el.com>
To: "Bjorn Helgaas" <helgaas@...nel.org>
Cc: "Sean V Kelley" <seanvk.dev@...gontracks.org>, bhelgaas@...gle.com,
Jonathan.Cameron@...wei.com, rafael.j.wysocki@...el.com,
ashok.raj@...el.com, tony.luck@...el.com,
sathyanarayanan.kuppuswamy@...el.com, qiuxu.zhuo@...el.com,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 03/10] PCI/RCEC: Cache RCEC capabilities in
pci_init_capabilities()
On 25 Sep 2020, at 13:13, Bjorn Helgaas wrote:
> On Tue, Sep 22, 2020 at 02:38:52PM -0700, Sean V Kelley wrote:
>> From: Sean V Kelley <sean.v.kelley@...el.com>
>>
>> Extend support for Root Complex Event Collectors by decoding and
>> caching the RCEC Endpoint Association Extended Capabilities when
>> enumerating. Use that cached information for later error source
>> reporting. See PCI Express Base Specification, version 5.0-1,
>> section 7.9.10.
>>
>> Suggested-by: Bjorn Helgaas <bhelgaas@...gle.com>
>> Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@...el.com>
>> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@...el.com>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@...el.com>
>> ---
>> drivers/pci/pci.h | 18 ++++++++++++++
>> drivers/pci/pcie/Makefile | 2 +-
>> drivers/pci/pcie/rcec.c | 52
>> +++++++++++++++++++++++++++++++++++++++
>> drivers/pci/probe.c | 3 ++-
>> include/linux/pci.h | 4 +++
>> 5 files changed, 77 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/pci/pcie/rcec.c
>>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index fa12f7cbc1a0..83670a6425d8 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -449,6 +449,16 @@ int aer_get_device_error_info(struct pci_dev
>> *dev, struct aer_err_info *info);
>> void aer_print_error(struct pci_dev *dev, struct aer_err_info
>> *info);
>> #endif /* CONFIG_PCIEAER */
>>
>> +#ifdef CONFIG_PCIEPORTBUS
>> +/* Cached RCEC Associated Endpoint Extended Capabilities */
>> +struct rcec_ext {
>> + u8 ver;
>> + u8 nextbusn;
>> + u8 lastbusn;
>> + u32 bitmap;
>> +};
>> +#endif
>> +
>> #ifdef CONFIG_PCIE_DPC
>> void pci_save_dpc_state(struct pci_dev *dev);
>> void pci_restore_dpc_state(struct pci_dev *dev);
>> @@ -461,6 +471,14 @@ static inline void pci_restore_dpc_state(struct
>> pci_dev *dev) {}
>> static inline void pci_dpc_init(struct pci_dev *pdev) {}
>> #endif
>>
>> +#ifdef CONFIG_PCIEPORTBUS
>> +void pci_rcec_init(struct pci_dev *dev);
>> +void pci_rcec_exit(struct pci_dev *dev);
>> +#else
>> +static inline void pci_rcec_init(struct pci_dev *dev) {}
>> +static inline void pci_rcec_exit(struct pci_dev *dev) {}
>> +#endif
>> +
>> #ifdef CONFIG_PCI_ATS
>> /* Address Translation Service */
>> void pci_ats_init(struct pci_dev *dev);
>> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
>> index 68da9280ff11..d9697892fa3e 100644
>> --- a/drivers/pci/pcie/Makefile
>> +++ b/drivers/pci/pcie/Makefile
>> @@ -2,7 +2,7 @@
>> #
>> # Makefile for PCI Express features and port driver
>>
>> -pcieportdrv-y := portdrv_core.o portdrv_pci.o err.o
>> +pcieportdrv-y := portdrv_core.o portdrv_pci.o err.o rcec.o
>>
>> obj-$(CONFIG_PCIEPORTBUS) += pcieportdrv.o
>>
>> diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c
>> new file mode 100644
>> index 000000000000..519ae086ff41
>> --- /dev/null
>> +++ b/drivers/pci/pcie/rcec.c
>> @@ -0,0 +1,52 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Root Complex Event Collector Support
>> + *
>> + * Authors:
>> + * Sean V Kelley <sean.v.kelley@...el.com>
>> + * Qiuxu Zhuo <qiuxu.zhuo@...el.com>
>> + *
>> + * Copyright (C) 2020 Intel Corp.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/errno.h>
>> +#include <linux/bitops.h>
>> +#include <linux/pci.h>
>> +#include <linux/pci_regs.h>
>
> Do we really need all the above? I don't see any errno or bitops
> here.
Will remove unused.
>
>> +#include "../pci.h"
>> +
>> +void pci_rcec_init(struct pci_dev *dev)
>> +{
>> + u32 rcec, hdr, busn;
>> +
>> + /* Only for Root Complex Event Collectors */
>> + if (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)
>> + return;
>> +
>> + dev->rcec_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_RCEC);
>> + if (!dev->rcec_cap)
>> + return;
>> +
>> + dev->rcec_ext = kzalloc(sizeof(*dev->rcec_ext), GFP_KERNEL);
>> +
>> + rcec = dev->rcec_cap;
>> + pci_read_config_dword(dev, rcec + PCI_RCEC_RCIEP_BITMAP,
>> &dev->rcec_ext->bitmap);
>> +
>> + /* Check whether RCEC BUSN register is present */
>> + pci_read_config_dword(dev, rcec, &hdr);
>> + dev->rcec_ext->ver = PCI_EXT_CAP_VER(hdr);
>> + if (dev->rcec_ext->ver < PCI_RCEC_BUSN_REG_VER)
>> + return;
>> +
>> + pci_read_config_dword(dev, rcec + PCI_RCEC_BUSN, &busn);
>> + dev->rcec_ext->nextbusn = PCI_RCEC_BUSN_NEXT(busn);
>> + dev->rcec_ext->lastbusn = PCI_RCEC_BUSN_LAST(busn);
>> +}
>> +
>> +void pci_rcec_exit(struct pci_dev *dev)
>> +{
>> + kfree(dev->rcec_ext);
>> + dev->rcec_ext = NULL;
>> +}
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 03d37128a24f..16bc651fecb7 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2201,6 +2201,7 @@ static void pci_configure_device(struct pci_dev
>> *dev)
>> static void pci_release_capabilities(struct pci_dev *dev)
>> {
>> pci_aer_exit(dev);
>> + pci_rcec_exit(dev);
>> pci_vpd_release(dev);
>> pci_iov_release(dev);
>> pci_free_cap_save_buffers(dev);
>> @@ -2400,7 +2401,7 @@ static void pci_init_capabilities(struct
>> pci_dev *dev)
>> pci_ptm_init(dev); /* Precision Time Measurement */
>> pci_aer_init(dev); /* Advanced Error Reporting */
>> pci_dpc_init(dev); /* Downstream Port Containment */
>> -
>> + pci_rcec_init(dev); /* Root Complex Event Collector */
>> pcie_report_downtraining(dev);
>>
>> if (pci_probe_reset_function(dev) == 0)
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 835530605c0d..5c5c4eb642b6 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -326,6 +326,10 @@ struct pci_dev {
>> #ifdef CONFIG_PCIEAER
>> u16 aer_cap; /* AER capability offset */
>> struct aer_stats *aer_stats; /* AER stats for this device */
>> +#endif
>> +#ifdef CONFIG_PCIEPORTBUS
>> + u16 rcec_cap; /* RCEC capability offset */
>
> Looking through the whole series, I think rcec_cap is used (a) in
> pci_rcec_init(), where we actually read the RCEC EA capability, and
> (b) in walk_rcec() and pcie_link_rcec(), where we only use it to test
> whether the device has an RCEC EA capability.
>
> Couldn't we accomplish (b) just by testing "dev->rcec_ext"? Then we
> wouldn't need to save rcec_cap at all.
Yes, rcec_ext really implies that the cap is there so the explicit cap
would not be necessary. Will remove and make use of the renamed rcec_ea
(per below).
>
>> + struct rcec_ext *rcec_ext; /* RCEC cached assoc. endpoint extended
>> capabilities */
>
> Maybe "rcec_ea"? The important part is that this is the Endpoint
> Association information. The fact that it happens to be an "extended"
> capability isn't very interesting.
Right, better to be more specific here. Will change.
Thanks,
Sean
>
>> #endif
>> u8 pcie_cap; /* PCIe capability offset */
>> u8 msi_cap; /* MSI capability offset */
>> --
>> 2.28.0
>>
Powered by blists - more mailing lists