[PATCH v5] cxl/pci: Skip reset detection for DVSEC emulated decoders

[PATCH v5] cxl/pci: Skip reset detection for DVSEC emulated decoders

[Guixin Liu]

HDM decoders are emulated from the DVSEC range registers in two cases:
(a) the component registers expose no HDM decoder capability, or (b) the
capability is present but the DVSEC ranges were the ones in use at driver
load.

After an FLR or SBR, __cxl_endpoint_decoder_reset_detected() reads the HDM
decoder Committed bit for every decoder marked enabled, emulated ones
included. In case (a) regs.hdm_decoder is NULL and the read oopses. In case
(b) the Committed bit was never set, so a reset gets reported that never
happened.

Use the absence of cxld->commit to elide the check for emulated decoders.

Case (b) tested under QEMU: a reset on an endpoint driven down the DVSEC
emulation path no longer reports a reset or strips the decoder flags.

Fixes: 934edcd436dc ("cxl: Add post-reset warning if reset results in loss of previously committed HDM decoders")
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
---
This was patch 4/8 of the "cxl: Assorted fixes" series [1], resent
individually per review feedback.

The two synchronisation concerns the review bot raised on v2 - the missing
exclusion against a cxl_port unbind freeing the devm allocated cxl_hdm, and
the plain read-modify-write of cxld->flags in
cxl_endpoint_decoder_clear_reset_flags() while the region paths update the
same word under cxl_rwsem.region - are still untouched here. Both are
about the reset handler's locking rather than about which registers it
reads, and neither fix is local; happy to follow up with separate patches.

Testing

Reaching case (b) needs an endpoint whose DVSEC ranges are live while the
global HDM decoder enable is clear. Unbinding cxl_pci runs the
disable_hdm() and clear_mem_enable() devm actions, so both bits start
clean. setpci then programs DVSEC Range 1 Base to 0x890000000, which sits
inside a RAM capable CFMWS, and sets Mem_Enable. Rebinding takes the
emulation path:

  cxl_port endpoint3: Fallback map 1 range register
  cxl_pci 0000:35:00.0: DVSEC Range0 allowed by platform

The endpoint ends up with a single decoder, enabled and locked, with no
Committed bit behind it:

  decoder3.0: locked=1 start=0x890000000 size=0x100000000 mode=ram

The device's only available reset method is cxl_bus, which unmasks SBR
through the port DVSEC. Before this change:

  cxl_pci 0000:35:00.0: resetting
  cxl_pci 0000:35:00.0: SBR happened without memory regions removal.
  cxl_pci 0000:35:00.0: System may be unstable if regions hosted system memory.

/proc/sys/kernel/tainted picked up TAINT_USER (8192 -> 8256) and locked
dropped from 1 to 0. After this change the same reset logs only
"resetting", locked stays 1, and no taint is added. The decoder is
decoder6.0 in the second run because reloading cxl_core renumbered the
memdevs.

QEMU does not clear the DVSEC range registers on SBR, so the emulated
decode is still fully programmed by the time the reset completes. The old
report was spurious here too.

Not covered: case (a) needs an endpoint with no component register block,
which QEMU's type3 always provides, and the FLR variant needs FLR support
that QEMU's type3 does not advertise (FLReset-). I also did not run a
positive control confirming that a normally committed HDM decoder is still
detected after this change.

v1->v2:
- rebase onto cxl/next
- rewrite the commit message to describe the behaviour rather than narrate
  the code change (Alison Schofield)

v2->v3:
- test cxld->commit instead of cxlhdm->regs.hdm_decoder, so that decoders
  emulated from the DVSEC ranges are also skipped when the HDM decoder
  registers exist but are globally disabled (Richard Cheng)
- update the subject and the commit message for the widened scope

v3->v4:
- shorten the commit message (Dave Jiang)

v4->v5:
- condense the commit message further, wording taken from Jonathan
  Cameron's suggestion on v4
- note the case (b) test in the commit message

[1] https://lore.kernel.org/linux-cxl/20260811113608.2815625-1-kanie@linux.alibaba.com/

 drivers/cxl/core/pci.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 9d807c1a002c..d8b07f86bab0 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -683,6 +683,13 @@ static int __cxl_endpoint_decoder_reset_detected(struct device *dev, void *data)
 	if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
 		return 0;
 
+	/*
+	 * Decoders emulated from the DVSEC range registers have no commit
+	 * callback and no HDM decoder registers to consult.
+	 */
+	if (!cxld->commit)
+		return 0;
+
 	cxlhdm = dev_get_drvdata(&port->dev);
 	hdm = cxlhdm->regs.hdm_decoder;
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));

base-commit: 7098e9cd98a05c0c5de2fae0c2465f9d966fdd07
-- 
2.43.7

Re: [PATCH v5] cxl/pci: Skip reset detection for DVSEC emulated decoders

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Bypassing reset detection for DVSEC-emulated decoders masks hardware configuration loss on Secondary Bus Reset (SBR).

Pre-existing issues:
- [High] Concurrent read-modify-write data race on cxld->flags in cxl_endpoint_decoder_clear_reset_flags.
- [High] Missing exclusion against cxl_port unbind causes a potential use-after-free of devm-allocated cxlhdm.
--

commit 5cb99f1a9d781a36990dc77bd01012f502a6380b
Author: Guixin Liu <kanie@linux.alibaba.com>

cxl/pci: Skip reset detection for DVSEC emulated decoders

This commit changes the CXL PCIe reset detection to skip checking the
hardware reset state for HDM decoders that are emulated from DVSEC range
registers. It does this by returning early from
__cxl_endpoint_decoder_reset_detected() if cxld->commit is NULL.

Link: https://lore.kernel.org/linux-cxl/20260811113608.2815625-1-kanie@linux.alibaba.com/

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 9d807c1a002ca..d8b07f86bab09 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -683,6 +683,13 @@ static int __cxl_endpoint_decoder_reset_detected(struct device *dev, void *data)
>  	if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
>  		return 0;
>  
> +	/*
> +	 * Decoders emulated from the DVSEC range registers have no commit
> +	 * callback and no HDM decoder registers to consult.
> +	 */
> +	if (!cxld->commit)
> +		return 0;
> +

[Severity: High]
Does bypassing reset detection for DVSEC-emulated decoders in
__cxl_endpoint_decoder_reset_detected() mask hardware configuration loss
on Secondary Bus Reset (SBR)?

When a bus reset occurs on a CXL endpoint with DVSEC-emulated HDM
decoders, this early return unconditionally skips hardware reset
verification. On physical hardware, SBR wipes non-sticky PCI configuration
registers, disabling the DVSEC memory mapping. By skipping verification,
the OS will attempt to use disconnected memory, which could lead to a
system crash (Machine Check Exception) or data corruption.

>  	cxlhdm = dev_get_drvdata(&port->dev);
>  	hdm = cxlhdm->regs.hdm_decoder;

[Severity: High]
This is a pre-existing issue, but does the missing exclusion against
cxl_port unbind cause a potential use-after-free of the devm-allocated
cxlhdm?

In __cxl_endpoint_decoder_reset_detected(), a concurrent unbind of the
cxl_port driver while cxl_reset_done() iterates endpoints could allow the
driver core devres to free cxlhdm mid-execution, since the reset handler
fails to lock the port device. This use-after-free could lead to a kernel
panic, undefined behavior, or memory corruption.

>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));

[ ... ]

Additionally, there is another pre-existing issue in the reset handling
path in drivers/cxl/pci.c:

drivers/cxl/pci.c:cxl_endpoint_decoder_clear_reset_flags() {
...
	cxled = to_cxl_endpoint_decoder(dev);
	cxled->cxld.flags &= ~CXL_DECODER_F_RESET_MASK;
...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this non-atomic
read-modify-write operation access shared state concurrently modified
by region paths?

When concurrent device reset handling (calling
cxl_endpoint_decoder_clear_reset_flags) and region management paths
(like commit/teardown) modify the same cxld->flags word without holding
a shared lock (e.g., cxl_rwsem.region), concurrent flag changes can be
overwritten. This can cause state bits like CXL_DECODER_F_ENABLE or
CXL_DECODER_F_LOCK to be lost, leading to mapping issues or crashes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831110449.719086-1-kanie@linux.alibaba.com?part=1