| 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: <20251009133802.0000486f@huawei.com>
Date: Thu, 9 Oct 2025 13:38:02 +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 01/13] pci: pcsc: Add plumbing for the PCI
Configuration Space Cache (PCSC)
On Fri, 3 Oct 2025 09:00:37 +0000
Evangelos Petrongonas <epetron@...zon.de> wrote:
> Introduce the basic infrastructure for the PCI Configuration Space Cache
> (PCSC), a mechanism to cache PCI configuration space accesses to reduce
> latency and bus traffic.
>
> The PCSC implements a transparent interception layer for PCI config
> space operations by dynamically injecting its own ops into the PCI bus
> hierarchy. The design preserves existing PCI ops while allowing PCSC to
> intercept and cache accesses:
>
> The` struct pci_bus` is extended to hold the original `pci_ops`, while
> the cache ones are injected via `pcsc_inject_bus_ops()`. The cache ops
> are injected when new buses are added via registering it to a bus
> notifier and integrating it at:
> * `pci_register_host_bridge()` - for root buses
> * `pci_alloc_child_bus()` - for child buses
> * `pci_bus_set_ops()` - when ops are dynamically changed
>
> The implementation includes weak pcsc_hw_config_read/write functions
> that handle calling the original op, when access to the actual HW is
> required.
>
> This approach ensures complete transparency - existing drivers and
> subsystems continue to use standard PCI config access functions while
> PCSC can intercept and cache accesses as needed. The weak functions also
> allow architecture-specific implementations to override the default
> behavior.
>
> The `core` initcall level is chosen so the cache is initialised before
> the PCI driver, ensuring that all config space access go through the
> cache.
>
> Kconfig options are added for both PCSC and PCIe PCSC support, with the
> latter extending the cache to handle 4KB PCIe configuration space.
>
> In this initial patch, the cache simply passes through all accesses to
> the hardware via the original ops - actual caching functionality will be
> added in subsequent patches.
>
> There is one caveat in this patch. The map_bus operations can
> potentially alter the cache, without invalidating / updating the cache.
> This is not an issue for the current upstream usages, as it is only
> being used in Root complexes and the
> `pci_generic_config_{read,write}{,32}`
>
> Signed-off-by: Evangelos Petrongonas <epetron@...zon.de>
> Signed-off-by: Stanislav Spassov <stanspas@...zon.de>
Hi,
This is an interesting idea. I'll probably give a bunch of trivial
feedback whilst I do a first read through. I appreciate this is only
an RFC at this point though!
> ---
> drivers/pci/Kconfig | 10 ++
> drivers/pci/Makefile | 1 +
> drivers/pci/access.c | 81 ++++++++++++++-
> drivers/pci/pcie/Kconfig | 9 ++
> drivers/pci/pcsc.c | 208 +++++++++++++++++++++++++++++++++++++++
> drivers/pci/probe.c | 24 ++++-
> include/linux/pci.h | 3 +
> include/linux/pcsc.h | 86 ++++++++++++++++
> 8 files changed, 419 insertions(+), 3 deletions(-)
> create mode 100644 drivers/pci/pcsc.c
> create mode 100644 include/linux/pcsc.h
>
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 9a249c65aedc..c26162b58365 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -40,6 +40,16 @@ config PCI_DOMAINS_GENERIC
> config PCI_SYSCALL
> bool
>
> +config PCSC
> + bool "PCI Configuration Space Cache"
> + depends on PCI
> + default n
Not needed as default if you don't have this line is n.
> + help
> + This option enables support for the PCI Configuration Space Cache
> + (PCSC). PCSC is a transparent caching layer that
> + intercepts configuration space operations and maintains cached
> + copies of register values
Odd wrapping. Rewrap to consistent 80 chars.
> +
> source "drivers/pci/pcie/Kconfig"
>
> config PCI_MSI
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 67647f1880fb..012561b97e32 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PCI_DOE) += doe.o
> obj-$(CONFIG_PCI_DYNAMIC_OF_NODES) += of_property.o
> obj-$(CONFIG_PCI_NPEM) += npem.o
> obj-$(CONFIG_PCIE_TPH) += tph.o
> +obj-$(CONFIG_PCSC) += pcsc.o
>
> # Endpoint library must be initialized before its users
> obj-$(CONFIG_PCI_ENDPOINT) += endpoint/
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index b123da16b63b..b89e9210d330 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> #include <linux/pci.h>
> +#include <linux/pcsc.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/ioport.h>
> @@ -189,15 +190,93 @@ EXPORT_SYMBOL_GPL(pci_generic_config_write32);
> * @ops: new raw operations
> *
> * Return previous raw operations
> + *
> + * When PCSC is enabled, this function maintains transparency by:
> + * - Returning the original non-PCSC ops to the caller
> + * - Properly handling the case where PCSC ops are already injected
> + * - Re-injecting PCSC ops after setting new ops when appropriate
> */
> struct pci_ops *pci_bus_set_ops(struct pci_bus *bus, struct pci_ops *ops)
> {
> struct pci_ops *old_ops;
> unsigned long flags;
> +#ifdef CONFIG_PCSC
> + bool pcsc_was_injected = false;
> + struct pci_ops *pcsc_ops_ptr = NULL;
> +#endif
With change suggested below the compiler will happily remove these
without needing the ifdef fun.
>
> raw_spin_lock_irqsave(&pci_lock, flags);
I'd precede this set with a patch using guard() for this
as then you can avoid the goto complexity in your new code
and potentially just return directly in error cases which will
I think simpify flow a little more.
> - old_ops = bus->ops;
> +
> +#ifdef CONFIG_PCSC
Is it possible to use
if (IS_ENABLED(CONFIG_PCSC))
to expose the code to the compiler /static analysis etc but allow
the compiler to cleanly do dead code removal so we get same resulting
binary as here?
> + /*
> + * Check if PCSC ops are currently injected. If so, we need to:
> + * 1. Return the original (non-PCSC) ops to maintain transparency
> + * 2. Update orig_ops to point to the new ops
> + * 3. Re-inject PCSC ops if the new ops are different from PCSC ops
> + */
> + if (bus->orig_ops) {
> + pcsc_was_injected = true;
> + pcsc_ops_ptr = bus->ops; /* Save current PCSC ops */
> + old_ops = bus->orig_ops; /* Return the real original ops */
> +
> + /*
> + * If the caller is trying to restore the PCSC ops themselves,
> + * just keep the current setup and return the original ops
> + */
> + if (ops == pcsc_ops_ptr)
> + goto out_unlock;
> +
> + /* Clear orig_ops temporarily to allow re-injection */
> + bus->orig_ops = NULL;
> + } else
> +#endif
> + {
> + old_ops = bus->ops;
> + }
> +
> bus->ops = ops;
> +
> +#ifdef CONFIG_PCSC
Similar: if (IS_ENABLED())
> + /*
> + * Re-inject PCSC ops if they were previously injected and the new ops
> + * are not the PCSC ops themselves. This maintains caching transparency.
> + */
> + if (pcsc_was_injected && ops != pcsc_ops_ptr) {
> + /*
> + * IMPORTANT: Dynamic ops changes after PCSC injection can lead to
> + * cache consistency issues if operations were performed that should
> + * have invalidated the cache. We re-inject PCSC ops here, but the
> + * caller is responsible for ensuring cache consistency if needed.
> + * This will be fixed in a future commit, when PCSC resets are
> + * introduced.
> + */
> +
> + pr_warn("PCSC: Dynamic ops change detected on bus %04x:%02x, resetting cache\n",
> + pci_domain_nr(bus), bus->number);
> +
> + if (pcsc_inject_bus_ops(bus)) {
> + pr_err("PCSC: Failed to re-inject ops after ops change on bus %04x:%02x\n",
> + pci_domain_nr(bus), bus->number);
> + /*
> + * If re-injection fails, we've lost caching but at least
> + * the caller's requested ops are in place. Log it
> + */
> + pr_warn("PCSC: Cache disabled for bus %04x:%02x after ops change\n",
> + pci_domain_nr(bus), bus->number);
> + } else {
> + pr_debug("PCSC: Successfully re-injected ops after ops change on bus %04x:%02x\n",
> + pci_domain_nr(bus), bus->number);
> + }
> + } else if (!pcsc_was_injected) {
> + /* First-time injection for this bus */
> + if (pcsc_inject_bus_ops(bus)) {
> + pr_err("PCSC: Failed to inject ops on bus %04x:%02x\n",
> + pci_domain_nr(bus), bus->number);
> + }
> + }
> +
> +out_unlock:
> +#endif
> raw_spin_unlock_irqrestore(&pci_lock, flags);
> return old_ops;
> }
> diff --git a/drivers/pci/pcsc.c b/drivers/pci/pcsc.c
> new file mode 100644
> index 000000000000..dec7c51b5cfd
> --- /dev/null
> +++ b/drivers/pci/pcsc.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + * Author: Evangelos Petrongonas <epetron@...zon.de>
> + *
> + * Implementation of the PCI Configuration Space Cache (PCSC)
> + * PCSC is a module which caches the PCI Configuration Space Accesses
> + * It implements a write-invalidate policy, meaning that writes are
> + * propagated to the device and invalidating the cache. The registers that
> + * we are caching are based on the values that are safe to cache and we
> + * are not expecting them to change without OS actions.
> + *
Trivial but this trailing blank line doesn't add much.
> + */
> +/* Weak references to allow architecture-specific overrides */
> +int __weak pcsc_hw_config_read(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 *val)
> +{
> + /*
> + * This function is only called from pcsc_cached_config_read,
pcsc_cached_config_read()
> + * which means PCSC ops have already been injected and orig_ops
> + * should be valid.
> + */
> + if (bus->orig_ops && bus->orig_ops->read)
> + return bus->orig_ops->read(bus, devfn, where, size, val);
> +
> + *val = 0xffffffff;
I'd use GENMASK(31, 0);
as no one likes counting fs.
> + return PCIBIOS_FUNC_NOT_SUPPORTED;
> +}
> +EXPORT_SYMBOL_GPL(pcsc_hw_config_read);
> +
> +int __weak pcsc_hw_config_write(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 val)
> +{
> + /*
> + * This function is only called from pcsc_cached_config_write,
() same for other references to functions in comments.
> + * which means PCSC ops have already been injected and orig_ops
> + * should be valid.
> + */
> + if (bus->orig_ops && bus->orig_ops->write)
> + return bus->orig_ops->write(bus, devfn, where, size, val);
> +
> + return PCIBIOS_FUNC_NOT_SUPPORTED;
> +}
> +EXPORT_SYMBOL_GPL(pcsc_hw_config_write);
> +static int __init pcsc_init(void)
> +{
> + bus_register_notifier(&pci_bus_type, &pcsc_bus_nb);
> +
> + pcsc_initialised = true;
> + pr_info("initialised\n");
Bit noisy but I guess there is no other way to tell this is running.
> +
> + return 0;
> +}
> +
> +core_initcall(pcsc_init);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 37f5bd476f39..33a186e4bf1e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -8,6 +8,7 @@
> #include <linux/delay.h>
> #include <linux/init.h>
> #include <linux/pci.h>
> +#include <linux/pcsc.h>
> #include <linux/msi.h>
> #include <linux/of_pci.h>
> #include <linux/of_platform.h>
> @@ -1039,6 +1040,11 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> }
> #endif
>
> +#ifdef CONFIG_PCSC
if (IS_ENABLED())
and a stub probably to get rid of the ifdef mess.
> + if (pcsc_inject_bus_ops(bus))
> + pci_err(bus, "PCSC: Failed to inject ops\n");
> +#endif
> +
> b = pci_find_bus(pci_domain_nr(bus), bridge->busnr);
> if (b) {
> /* Ignore it if we already got here via a different bridge */
> @@ -1236,10 +1242,24 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> child->bus_flags = parent->bus_flags;
>
> host = pci_find_host_bridge(parent);
> - if (host->child_ops)
> + if (host->child_ops) {
> child->ops = host->child_ops;
> - else
> +#ifdef CONFIG_PCSC
> + child->orig_ops = host->child_ops;
I'd see if anyone really minds the unused orig_ops vs
the complexity of lots of local code. Otherwise wrap
setting and reading it up in access functions and stub
them out on !IS_ENABLED(CONFIG_PCSC)
> +#endif
> + } else {
> child->ops = parent->ops;
> +#ifdef CONFIG_PCSC
> + child->orig_ops = parent->orig_ops;
> +#endif
> + }
> +
> +#ifdef CONFIG_PCSC
Similar to above.
> + if (child->ops) {
> + if (pcsc_inject_bus_ops(child))
> + pci_err(child, "PCSC: Failed to inject ops\n");
> + }
> +#endif
>
> diff --git a/include/linux/pcsc.h b/include/linux/pcsc.h
> new file mode 100644
> index 000000000000..45816eb2b2c8
> --- /dev/null
> +++ b/include/linux/pcsc.h
> @@ -0,0 +1,86 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2025 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + * Author: Evangelos Petrongonas <epetron@...zon.de>
> + *
Trivial but drop this line as it adds nothing to readability and makes me scroll
a tiny little bit more ;)
> + */
> +/**
> + * pcsc_inject_bus_ops Inject the pcsc ops into bus pci_ops
> + * @bus: the bus in which to inject the ops
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +int pcsc_inject_bus_ops(struct pci_bus *bus);
It's a bit awkward given the weak functions, but mostly PCI seems to
put documentation alongside implementation rather then in the header.
> +#endif /* _LINUX_PCSC_H */
Powered by blists - more mailing lists