[PATCH] cxl/hdm: Account for a firmware-armed Lock On Commit

[PATCH] cxl/hdm: Account for a firmware-armed Lock On Commit

[Davidlohr Bueso]

A corner case (yet valid) where firmware sets the the locked bit
without committing the decoder. setup_hw_decoder() carries the bit
through.

A later region teardown then writes a reset the hardware ignores and
clears CXL_DECODER_F_ENABLE anyway, so the flags claim a decoder is
free that still decodes: cxl_dpa_free() releases its DPA, and a new
region committed on it appears to succeed, cxld_await_commit()
returning as soon as Committed reads 1, while the old mapping stays
in effect.

Read the control register back after the commit so the decoder is
treated like one found locked at probe.

Tested on qemu modified to enforce locked semantics, setting the bit
behind the driver on an uncommitted endpoint decoder, committing and
deleting a 256M region on it, then creating a 512M one. Before, the
decoder sysfs reads locked=0, its DPA is freed and the second region
reports committed while the hardware still decodes the first; after,
locked=1, the DPA release fails with -EBUSY and the second region
cannot allocate DPA.

Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
Found while battle-testing BI locked decoders.

 drivers/cxl/core/hdm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index fdcef20e01ba..d23076aabb89 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -856,6 +856,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
 	int id = cxld->id, rc;
+	u32 ctrl;
 
 	if (cxld->flags & CXL_DECODER_F_ENABLE)
 		return 0;
@@ -899,6 +900,14 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 	port->commit_end++;
 	cxld->flags |= CXL_DECODER_F_ENABLE;
 
+	/*
+	 * Lock On Commit armed by firmware engages on this commit and
+	 * makes the decoder read-only, like one found locked at probe.
+	 */
+	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
+	if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
+		cxld->flags |= CXL_DECODER_F_LOCK;
+
 	return 0;
 }
 
-- 
2.39.5

Re: [PATCH] cxl/hdm: Account for a firmware-armed Lock On Commit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] cxl/hdm: missing CXL_REGION_F_LOCK propagation in cxl_decoder_commit()
--

commit 38da830927e8d8e6e1d5d4154ac43502a9344546
Author: Davidlohr Bueso <dave@stgolabs.net>

cxl/hdm: Account for a firmware-armed Lock On Commit

This patch reads back the HDM decoder control register after committing a
decoder in cxl_decoder_commit() and sets CXL_DECODER_F_LOCK on cxld->flags if
CXL_HDM_DECODER0_CTRL_LOCK is set, so that an uncommitted decoder with Lock
On Commit armed by firmware is treated as locked once committed.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index fdcef20e01baa..d23076aabb89b 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -856,6 +856,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>  	int id = cxld->id, rc;
> +	u32 ctrl;
>  
>  	if (cxld->flags & CXL_DECODER_F_ENABLE)
>  		return 0;
> @@ -899,6 +900,14 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	port->commit_end++;
>  	cxld->flags |= CXL_DECODER_F_ENABLE;
>  
> +	/*
> +	 * Lock On Commit armed by firmware engages on this commit and
> +	 * makes the decoder read-only, like one found locked at probe.
> +	 */
> +	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +	if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> +		cxld->flags |= CXL_DECODER_F_LOCK;

[Severity: High]
Does setting CXL_DECODER_F_LOCK here in cxl_decoder_commit() leave the owning
cxl_region unlocked because CXL_REGION_F_LOCK is never propagated to
cxlr->flags?

For a decoder found committed and locked at probe time, init_hdm_decoder()
sets CXL_DECODER_F_LOCK before targets are attached. When targets are
subsequently attached via the attach_target() -> __attach_target() ->
cxl_region_attach() -> cxl_port_attach_region() call chain,
cxl_region_setup_flags() propagates the lock to the region:

drivers/cxl/core/region.c:cxl_region_setup_flags() {
	if (cxld->flags & CXL_DECODER_F_LOCK) {
		set_bit(CXL_REGION_F_LOCK, &cxlr->flags);
		clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
	}
}

However, when firmware arms Lock On Commit on an uncommitted decoder,
CXL_DECODER_F_LOCK is not set during cxl_port_attach_region(). It is only set
later when commit_store() -> __commit() -> cxl_region_decode_commit() ->
commit_decoder() -> cxl_decoder_commit() runs:

drivers/cxl/core/region.c:cxl_region_decode_commit() {
		/* commit bottom up */
		for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
		     iter = to_cxl_port(iter->dev.parent)) {
			cxl_rr = cxl_rr_load(iter, cxlr);
			cxld = cxl_rr->decoder;
			rc = commit_decoder(cxld);
			if (rc)
				break;
		}
}

Because neither cxl_decoder_commit() nor cxl_region_decode_commit() updates
cxlr->flags after committing the decoder, CXL_REGION_F_LOCK remains clear on
the region.

Without CXL_REGION_F_LOCK set on cxlr->flags:

1. The region sysfs attribute locked_show() reports 0 even though the
   underlying decoder is locked.

2. Writing 0 to the region commit sysfs attribute in commit_store() bypasses
   the -EPERM check:

drivers/cxl/core/region.c:commit_store() {
	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
		return -EPERM;
}

3. When the region is reset or torn down via commit_store(),
   unregister_region(), or the error rollback path in
   cxl_region_decode_commit(), cxl_region_decode_reset() bypasses its early
   return:

drivers/cxl/core/region.c:cxl_region_decode_reset() {
	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
		return;
}

   As a result, cxl_region_decode_reset() resets any unlocked upstream switch,
   host-bridge, or peer endpoint decoders in the region (clearing their
   CXL_DECODER_F_ENABLE flag and allowing peer DPA to be freed) while
   cxl_decoder_reset() skips the locked decoder, leaving the hardware decode
   hierarchy partially torn down.

Should cxl_region_decode_commit() call cxl_region_setup_flags(cxlr, cxld) (or
propagate CXL_DECODER_F_LOCK to cxlr->flags) after commit_decoder() succeeds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028211836.939648-1-dave@stgolabs.net?part=1

Re: [PATCH] cxl/hdm: Account for a firmware-armed Lock On Commit

[Li Ming]

在 2026/10/29 05:18, Davidlohr Bueso 写道:
> A corner case (yet valid) where firmware sets the the locked bit
> without committing the decoder. setup_hw_decoder() carries the bit
> through.
>
> A later region teardown then writes a reset the hardware ignores and
> clears CXL_DECODER_F_ENABLE anyway, so the flags claim a decoder is
> free that still decodes: cxl_dpa_free() releases its DPA, and a new
> region committed on it appears to succeed, cxld_await_commit()
> returning as soon as Committed reads 1, while the old mapping stays
> in effect.
>
> Read the control register back after the commit so the decoder is
> treated like one found locked at probe.
>
> Tested on qemu modified to enforce locked semantics, setting the bit
> behind the driver on an uncommitted endpoint decoder, committing and
> deleting a 256M region on it, then creating a 512M one. Before, the
> decoder sysfs reads locked=0, its DPA is freed and the second region
> reports committed while the hardware still decodes the first; after,
> locked=1, the DPA release fails with -EBUSY and the second region
> cannot allocate DPA.

Hi Davidlohr,

My first feeling is that we should read this decoder locked state during 
init_hdm_decoder()(currently, init_hdm_decoder() already does it for 
committed decoders), so that driver and user have change to know the 
locked bit of the decoder has been set at the beginning.

Besides, I am not sure if ndctl can expose decoder locked state for an 
uncommitted decoder, but I think let user know that is neccessary.

Does it make more sense?


Ming

>
> Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
> Found while battle-testing BI locked decoders.
>
>   drivers/cxl/core/hdm.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index fdcef20e01ba..d23076aabb89 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -856,6 +856,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>   	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
>   	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>   	int id = cxld->id, rc;
> +	u32 ctrl;
>   
>   	if (cxld->flags & CXL_DECODER_F_ENABLE)
>   		return 0;
> @@ -899,6 +900,14 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>   	port->commit_end++;
>   	cxld->flags |= CXL_DECODER_F_ENABLE;
>   
> +	/*
> +	 * Lock On Commit armed by firmware engages on this commit and
> +	 * makes the decoder read-only, like one found locked at probe.
> +	 */
> +	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +	if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> +		cxld->flags |= CXL_DECODER_F_LOCK;
> +
>   	return 0;
>   }
>