Re: [PATCH v11 08/23] PCI/CXL: Introduce pcie_is_cxl()

Alejandro Lucero Palau
2025-08-28T08:19:30Z

Hi Terry,

On 8/27/25 02:35, Terry Bowman wrote:
> CXL and AER drivers need the ability to identify CXL devices.
>
> Introduce set_pcie_cxl() with logic checking for CXL.mem or CXL.cache
> status in the CXL Flexbus DVSEC status register. The CXL Flexbus DVSEC
> presence is used because it is required for all the CXL PCIe devices.[1]
>
> Add boolean 'struct pci_dev::is_cxl' with the purpose to cache the CXL
> CXL.cache and CXl.mem status.
>
> In the case the device is an EP or USP, call set_pcie_cxl() on behalf of
> the parent downstream device. This will make certain the correct state
> is cached.
>
> Add function pcie_is_cxl() to return 'struct pci_dev::is_cxl'.
>
> [1] CXL 3.1 Spec, 8.1.1 PCIe Designated Vendor-Specific Extended
> Capability (DVSEC) ID Assignment, Table 8-2
>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

With the changes for checking flexbus state:

Reviewed-by: Alejandro Lucero <alucerop@amd.com>

Just a minor thing below, something I do not fully understand but I
guess it was discussed/explained previously.

> ---
> Changes in v10->v11:
> - Amended set_pcie_cxl() to check for Upstream Port's and EP's parent
> downstream port by calling set_pcie_cxl(). (Dan)
> - Retitle patch: 'Add' -> 'Introduce'
> - Add check for CXL.mem and CXL.cache (Alejandro, Dan)
> ---
> drivers/pci/probe.c | 25 +++++++++++++++++++++++++
> include/linux/pci.h | 6 ++++++
> include/uapi/linux/pci_regs.h | 3 +++
> 3 files changed, 34 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4b8693ec9e4c..b08cd0346136 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1691,6 +1691,29 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> dev->is_thunderbolt = 1;
> }
>
> +static void set_pcie_cxl(struct pci_dev *dev)
> +{
> + struct pci_dev *parent;
> + u16 dvsec = pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> + PCI_DVSEC_CXL_FLEXBUS_PORT);
> + if (dvsec) {
> + u16 cap;
> +
> + pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_FLEXBUS_STATUS_OFFSET, &cap);
> +
> + dev->is_cxl = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_STATUS_CACHE_MASK, cap) ||
> + FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_STATUS_MEM_MASK, cap);
> + }
> +
> + if (!pci_is_pcie(dev) ||
> + !(pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT ||
> + pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM))
> + return;
> +
> + parent = pci_upstream_bridge(dev);
> + set_pcie_cxl(parent);

This recursion is confusing to me.

Is it not the parent already having this set from its own pci setup? Or
maybe do we expect that to change after a reset and this is a sanity check?

> +}
> +
> static void set_pcie_untrusted(struct pci_dev *dev)
> {
> struct pci_dev *parent = pci_upstream_bridge(dev);
> @@ -2021,6 +2044,8 @@ int pci_setup_device(struct pci_dev *dev)
> /* Need to have dev->cfg_size ready */
> set_pcie_thunderbolt(dev);
>
> + set_pcie_cxl(dev);
> +
> set_pcie_untrusted(dev);
>
> if (pci_is_pcie(dev))
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 05e68f35f392..79878243b681 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -453,6 +453,7 @@ struct pci_dev {
> unsigned int is_hotplug_bridge:1;
> unsigned int shpc_managed:1; /* SHPC owned by shpchp */
> unsigned int is_thunderbolt:1; /* Thunderbolt controller */
> + unsigned int is_cxl:1; /* Compute Express Link (CXL) */
> /*
> * Devices marked being untrusted are the ones that can potentially
> * execute DMA attacks and similar. They are typically connected
> @@ -744,6 +745,11 @@ static inline bool pci_is_vga(struct pci_dev *pdev)
> return false;
> }
>
> +static inline bool pcie_is_cxl(struct pci_dev *pci_dev)
> +{
> + return pci_dev->is_cxl;
> +}
> +
> #define for_each_pci_bridge(dev, bus) \
> list_for_each_entry(dev, &bus->devices, bus_list) \
> if (!pci_is_bridge(dev)) {} else
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index b03244d55aea..252c06402b13 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1274,6 +1274,9 @@
>
> /* CXL 3.2 8.1.8: PCIe DVSEC for Flex Bus Port */
> #define PCI_DVSEC_CXL_FLEXBUS_PORT 7
> +#define PCI_DVSEC_CXL_FLEXBUS_STATUS_OFFSET 0xE
> +#define PCI_DVSEC_CXL_FLEXBUS_STATUS_CACHE_MASK BIT(0)
> +#define PCI_DVSEC_CXL_FLEXBUS_STATUS_MEM_MASK BIT(2)
>
> /* CXL 3.2 8.1.9: Register Locator DVSEC */
> #define PCI_DVSEC_CXL_REG_LOCATOR 8