Re: [RFC PATCH 03/13] pci: pcsc: infer cacheability of PCI capabilities

[Jonathan Cameron]

On Fri, 3 Oct 2025 09:00:39 +0000
Evangelos Petrongonas <epetron@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Stanislav Spassov <stanspas@xxxxxxxxx>

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);
> +	}
> +}