| 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: <20251009151736.00002f45@huawei.com>
Date: Thu, 9 Oct 2025 15:17:36 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Evangelos Petrongonas <epetron@...zon.de>
CC: Bjorn Helgaas <bhelgaas@...gle.com>, Alex Williamson
<alex.williamson@...hat.com>, "Rafael J . Wysocki" <rafael@...nel.org>, Len
Brown <lenb@...nel.org>, Pasha Tatashin <pasha.tatashin@...een.com>, David
Matlack <dmatlack@...gle.com>, Vipin Sharma <vipinsh@...gle.com>, Chris Li
<chrisl@...nel.org>, Jason Miu <jasonmiu@...gle.com>, "Pratyush Yadav"
<pratyush@...nel.org>, Stanislav Spassov <stanspas@...zon.de>,
<linux-pci@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <nh-open-source@...zon.com>
Subject: Re: [RFC PATCH 03/13] pci: pcsc: infer cacheability of PCI
capabilities
On Fri, 3 Oct 2025 09:00:39 +0000
Evangelos Petrongonas <epetron@...zon.de> wrote:
> Implement cacheability inference for PCI capabilities to
> determine which configuration space registers can be safely cached.
>
> The first 64 bytes of PCI configuration space follow a standardized
> format, allowing straightforward cacheability determination. For
> capability-specific registers, the implementation traverses the PCI
> capability list to identify supported capabilities.
>
> Cacheable registers are identified for the following capabilities:
> - Power Management (PM)
> - Message Signaled Interrupts (MSI)
> - Message Signaled Interrupts Extensions (MSI-X)
> - PCI Express
> - PCI Advanced Features (AF)
> - Enhanced Allocation (EA)
> - Vital Product Data (VPD)
> - Vendor Specific
>
> The implementation pre-populates the cache with known values including
> device/vendor IDs and header type to avoid unnecessary configuration
> space reads during initialization.
>
> We are currently not caching the Command/Status registers.
>
> The cacheability of all capabilities apart from MSI, are straightforward
> and can be deduced from the spec. Regarding MSI the MSI flags are read
> and based on this, the cacheability is inferred.
>
> Signed-off-by: Evangelos Petrongonas <epetron@...zon.de>
> Signed-off-by: Stanislav Spassov <stanspas@...zon.de>
A few minor things below.
> +
> +static void infer_capabilities_pointers(struct pci_dev *dev)
> +{
> + u8 pos, cap_id, next_cap;
> + u32 val;
> + int i;
> +
> + if (pcsc_hw_config_read(dev->bus, dev->devfn, PCI_CAPABILITY_LIST, 1,
> + &val) != PCIBIOS_SUCCESSFUL)
> + return;
> +
> + pos = (val & 0xFF) & ~0x3;
Given single byte read, shouldn't need the 0xFF masking.
Might be worth setting val = 0 at declaration though so that the compiler
can see it is assigned if we reach here.
> +
> + while (pos) {
> + if (pos < 0x40 || pos > 0xFE)
> + break;
> +
> + pos &= ~0x3;
I couldn't immediately find a spec statement of the bottom two bits being 0 for
the next capability pointers. (There is one for the PCI_CAPABILITY_LIST)
> + if (pcsc_hw_config_read(dev->bus, dev->devfn, pos, 2, &val) !=
> + PCIBIOS_SUCCESSFUL)
> + break;
> +
> + cap_id = val & 0xFF; /* PCI_CAP_LIST_ID */
> + next_cap = (val >> 8) & 0xFF; /* PCI_CAP_LIST_NEXT */
> +
> + bitmap_set(dev->pcsc->cachable_bitmask, pos, 2);
> + pcsc_update_byte(dev, pos, cap_id); /* PCI_CAP_LIST_ID */
> + pcsc_update_byte(dev, pos + 1,
> + next_cap); /* PCI_CAP_LIST_NEXT */
Could you do something like moving the bitmap_set before the pcsc_hw_config_read() and
cal pcsc_cached_config_read() to fill the cache directly during the read?
> +
> + pci_dbg(dev, "Capability ID %#x found at %#x\n", cap_id, pos);
> +
> + /* Check if this is a supported capability and infer cacheability */
> + for (i = 0; i < ARRAY_SIZE(PCSC_SUPPORTED_CAPABILITIES); i++) {
> + if (cap_id == PCSC_SUPPORTED_CAPABILITIES[i]) {
> + infer_capability_cacheability(dev, pos, cap_id);
> + break;
> + }
> + }
> +
> + /* Move to next capability */
> + pos = next_cap;
> + }
> +}
> +
> +static void infer_cacheability(struct pci_dev *dev)
> +{
> + if (WARN_ON(!dev || !dev->pcsc || !dev->pcsc->cfg_space))
> + return;
> +
> + bitmap_zero(dev->pcsc->cachable_bitmask, PCSC_CFG_SPC_SIZE);
> +
> + /* Type 0 Configuration Space Header */
> + if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
Unless you plan to add type 1 very soon I'd flip the logic to reduce
indent of the code that follows.
> + /*
> + * Mark cacheable registers in the PCI configuration space header.
> + * We cache read-only and rarely changing registers:
> + * - PCI_VENDOR_ID, PCI_DEVICE_ID (0x00-0x03)
> + * - PCI_CLASS_REVISION through PCI_CAPABILITY_LIST (0x08-0x34)
> + * Includes: CLASS_REVISION, CACHE_LINE_SIZE, LATENCY_TIMER,
> + * HEADER_TYPE, BIST, BASE_ADDRESS_0-5, CARDBUS_CIS,
> + * SUBSYSTEM_VENDOR_ID, SUBSYSTEM_ID, ROM_ADDRESS, CAPABILITY_LIST
> + * - PCI_INTERRUPT_LINE through PCI_MAX_LAT (0x3c-0x3f)
> + * Includes: INTERRUPT_LINE, INTERRUPT_PIN, MIN_GNT, MAX_LAT
> + */
> + bitmap_set(dev->pcsc->cachable_bitmask, PCI_VENDOR_ID, 4);
> + bitmap_set(dev->pcsc->cachable_bitmask, PCI_CLASS_REVISION, 45);
For this large range can you derive that 45 as something like
PCI_CAPABILITY_LIST - PCI_CLASS_REVISION + 1
Same applies for the other multifield bitmap sets
> + bitmap_set(dev->pcsc->cachable_bitmask, PCI_INTERRUPT_LINE, 4);
> +
> + /* Pre populate the cache with the values that we already know */
I'm curious - do you have perf numbers to show it's worth writing these 5 bytes
directly into the cache? Feels like complexity that maybe doesn't belong there
in initial patch but should come along later in a patch with numbers to support
the small amount of extra complexity.
> + pcsc_update_byte(dev, PCI_HEADER_TYPE,
> + dev->hdr_type |
> + (dev->multifunction ? 0x80 : 0));
> +
> + /*
> + * SR-IOV VFs must return 0xFFFF (PCI_ANY_ID) for vendor/device ID
> + * registers per PCIe spec.
> + */
> + if (dev->is_virtfn) {
> + pcsc_update_byte(dev, PCI_VENDOR_ID, 0xFF);
> + pcsc_update_byte(dev, PCI_VENDOR_ID + 1, 0xFF);
> + pcsc_update_byte(dev, PCI_DEVICE_ID, 0xFF);
> + pcsc_update_byte(dev, PCI_DEVICE_ID + 1, 0xFF);
> + } else {
> + if (dev->vendor != PCI_ANY_ID) {
> + pcsc_update_byte(dev, PCI_VENDOR_ID,
> + dev->vendor & 0xFF);
> + pcsc_update_byte(dev, PCI_VENDOR_ID + 1,
> + (dev->vendor >> 8) & 0xFF);
> + }
> + if (dev->device != PCI_ANY_ID) {
> + pcsc_update_byte(dev, PCI_DEVICE_ID,
> + dev->device & 0xFF);
> + pcsc_update_byte(dev, PCI_DEVICE_ID + 1,
> + (dev->device >> 8) & 0xFF);
> + }
> + }
> +
> + infer_capabilities_pointers(dev);
> + }
> +}
Powered by blists - more mailing lists