Re: [PATCH 03/16] PCI: PCIe portdrv: Add CXL Isolation service driver

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Wed, 30 Jul 2025 16:47:05 -0500
Ben Cheatham <Benjamin.Cheatham@amd.com> wrote:

> Add the CXL isolation service, which will provide the necessary
> information to the PCIe portdrv and CXL drivers to map, setup, and
> handle CXL isolation interrupts.
> 
> Add functions to get the CXL isolation MSI/-X interrupt vector
> from the PCIe portdrv.
> 
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>

Hmm. I feel a bit guilty I haven't gotten back to a rework of the
portdrv.  To potentially make that easier (when I or someone else
get the time), would it work for you if this only did MSI-X?
That would let us handle this in a somewhat 'pluggable' way without
having to deal with the limitations of MSI wrt to dynamic interrupt
allocation.

> ---
>  drivers/cxl/Kconfig              | 14 +++++
>  drivers/cxl/cxl.h                |  4 ++
>  drivers/pci/pcie/Makefile        |  1 +
>  drivers/pci/pcie/cxl_isolation.c | 87 ++++++++++++++++++++++++++++++++
>  drivers/pci/pcie/portdrv.c       |  1 +
>  drivers/pci/pcie/portdrv.h       | 18 ++++++-
>  6 files changed, 124 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/pcie/cxl_isolation.c
> 
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 57274de54a45..537e1e8e13da 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -247,4 +247,18 @@ config CXL_NATIVE_RAS
>  	  If unsure, or if this kernel is meant for production environments,
>  	  say Y.
>  
> +config CXL_ISOLATION
> +	bool "CXL.mem Isolation Support"
> +	depends on PCIEPORTBUS
> +	depends on CXL_BUS=PCIEPORTBUS
> +	help
> +	  Enables the CXL.mem isolation PCIe port bus service driver. This
> +	  driver, in combination with the CXL driver core, is responsible
> +	  for managing CXL-capable PCIe root ports that undergo CXL.mem
> +	  error isolation due to either a CXL.mem transaction timeout or
> +	  link down condition. Without error isolation, either of these
> +	  conditions will trigger a system reset.
> +
> +	  If unsure say 'y'
I'd drop this last line or say something more about when it makes sense to enable.


> diff --git a/drivers/pci/pcie/cxl_isolation.c b/drivers/pci/pcie/cxl_isolation.c
> new file mode 100644
> index 000000000000..550f16271d1c
> --- /dev/null
> +++ b/drivers/pci/pcie/cxl_isolation.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * The CXL Isolation PCIe port service driver provides functions to allocate
> + * and set up CXL Timeout & Isolation interrupts (CXL 3.2 12.3). This driver
> + * does no actual interrupt handling, it only provides the information for
> + * the CXL driver to set up its own handling because the CXL driver is better
> + * equipped to handle isolation interrupts.
> + *
> + * Copyright (C) 2025, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Author: Ben Cheatham <Benjamin.Cheatham@amd.com>
> + */
> +
> +#include <linux/pci.h>
> +
> +#include "../../cxl/cxlpci.h"
> +#include "portdrv.h"
> +
> +static int get_isolation_intr_vec(u32 cap)
> +{
> +	if (!FIELD_GET(CXL_ISOLATION_CAP_INTR_SUPP, cap) ||
> +	    !FIELD_GET(CXL_ISOLATION_CAP_MEM_ISO_SUPP, cap))
> +		return -ENXIO;
> +
> +	return FIELD_GET(CXL_ISOLATION_CAP_INTR_MASK, cap);
> +}
> +
> +int pcie_cxliso_get_intr_vec(struct pci_dev *dev, int *vec)
> +{
> +	struct cxl_component_regs regs;
> +	struct cxl_register_map map;
> +	u32 cap;
> +	int rc;
> +
> +	rc = cxl_find_regblock(dev, CXL_REGLOC_RBI_COMPONENT, &map);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_setup_regs(&map);
> +	if (rc)
> +		return rc;
> +
> +	if (!map.component_map.isolation.valid)
> +		return -ENXIO;
> +

Add some comments here on why we go through this dance of temporary mapping.
(I have similar for the CPMU code I never finished upstreaming).

> +	rc = cxl_map_component_regs(&map, &regs,
> +				    BIT(CXL_CM_CAP_CAP_ID_ISOLATION));
> +	if (rc)
> +		return rc;
> +
> +	cap = readl(regs.isolation + CXL_ISOLATION_CAPABILITY_OFFSET);
> +	rc = get_isolation_intr_vec(cap);
> +	if (rc < 0) {

Probably use a goto given common shared stuff to do on exit.

> +		cxl_unmap_component_regs(&map, &regs,
> +					 BIT(CXL_CM_CAP_CAP_ID_ISOLATION));
> +		return rc;
> +	}
> +
> +	if (vec)
> +		*vec = rc;
> +
> +	cxl_unmap_component_regs(&map, &regs, BIT(CXL_CM_CAP_CAP_ID_ISOLATION));
> +	return 0;
> +
> +}
> +
> +static int cxl_isolation_probe(struct pcie_device *dev)
> +{
> +	if (!pcie_is_cxl(dev->port) || pcie_cxliso_get_intr_vec(dev->port, NULL))
The second call has rich error codes so better not to eat them.

	if (!pcie_is_cxl(dev->port))
		return -ENXIO;

	rc = pcie_cxl_iso_get_intr_vec();
	if (rc)
		return rc;

> +		return -ENXIO;
> +
> +	pci_info(dev->port, "CXLISO: Signaling with IRQ %d\n", dev->irq);
I guess there is history of this for other portdrv services, but to me too noisy
for a normal boot and should be trivial to get from /proc/interrupts

> +	return 0;
> +}