[PATCH] cxl: Add handling of locked CXL decoder

[PATCH] cxl: Add handling of locked CXL decoder

[Dave Jiang]

When a decoder is locked, it means that its configuration cannot be
changed. CXL spec r3.2 8.2.4.20.13 discusses the details regarding
locked decoders. Locking happens when bit 8 of the decoder control
register is set and then the decoder is committed afterwards (CXL
spec r3.2 8.2.4.20.7).

Given that the driver creates a virtual decoder for each CFMWS, the
Fixed Device Configuration (bit 4) of the Window Restriction field is
considered as locking for the virtual decoder by the driver.

The current driver code disregards the locked status and a region can
be destroyed regardless of the locking state.

Add a region flag to indicate the region is in a locked configuration.
The driver will considered a region locked if the CFMWS or any decoder
is configured as locked. The consideration is all or nothing regarding
the locked state. It is reasonable to determine the region "locked"
status while the region is being assembled based on the decoders.

Add a check in region commit_store() to intercept when a 0 is written
to the commit sysfs attribute in order to prevent the destruction of a
region when in locked state. This should be the only entry point from user
space to destroy a region.

Add a check is added to cxl_decoder_reset() to prevent resetting a locked
decoder within the kernel driver.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/hdm.c    |  3 +++
 drivers/cxl/core/region.c | 16 ++++++++++++++++
 drivers/cxl/cxl.h         |  8 ++++++++
 3 files changed, 27 insertions(+)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index d3a094ca01ad..1c5d2022c87a 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -905,6 +905,9 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
 	if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
 		return;
 
+	if (test_bit(CXL_DECODER_F_LOCK, &cxld->flags))
+		return;
+
 	if (port->commit_end == id)
 		cxl_port_commit_reap(cxld);
 	else
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index b06fee1978ba..8647eff4fb78 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -419,6 +419,9 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
 		return len;
 	}
 
+	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
+		return -EPERM;
+
 	rc = queue_reset(cxlr);
 	if (rc)
 		return rc;
@@ -1059,6 +1062,16 @@ static int cxl_rr_assign_decoder(struct cxl_port *port, struct cxl_region *cxlr,
 	return 0;
 }
 
+static void cxl_region_set_lock(struct cxl_region *cxlr,
+				struct cxl_decoder *cxld)
+{
+	if (!test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
+		return;
+
+	set_bit(CXL_REGION_F_LOCK, &cxlr->flags);
+	clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
+}
+
 /**
  * cxl_port_attach_region() - track a region's interest in a port by endpoint
  * @port: port to add a new region reference 'struct cxl_region_ref'
@@ -1170,6 +1183,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
 		}
 	}
 
+	cxl_region_set_lock(cxlr, cxld);
+
 	rc = cxl_rr_ep_add(cxl_rr, cxled);
 	if (rc) {
 		dev_dbg(&cxlr->dev,
@@ -2439,6 +2454,7 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
 	dev->bus = &cxl_bus_type;
 	dev->type = &cxl_region_type;
 	cxlr->id = id;
+	cxl_region_set_lock(cxlr, &cxlrd->cxlsd.cxld);
 
 	return cxlr;
 }
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 231ddccf8977..6382f1983865 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -517,6 +517,14 @@ enum cxl_partition_mode {
  */
 #define CXL_REGION_F_NEEDS_RESET 1
 
+/*
+ * Indicate whether this region is locked due to 1 or more decoders that have
+ * been locked. The approach of all or nothing is taken with regard to the
+ * locked attribute. CXL_REGION_F_NEEDS_RESET should not be set if this flag is
+ * set.
+ */
+#define CXL_REGION_F_LOCK 2
+
 /**
  * struct cxl_region - CXL region
  * @dev: This region's device

base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
-- 
2.51.0

Re: [PATCH] cxl: Add handling of locked CXL decoder

[Jonathan Cameron]

On Tue, 21 Oct 2025 13:50:55 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> When a decoder is locked, it means that its configuration cannot be
> changed. CXL spec r3.2 8.2.4.20.13 discusses the details regarding
> locked decoders. Locking happens when bit 8 of the decoder control
> register is set and then the decoder is committed afterwards (CXL
> spec r3.2 8.2.4.20.7).
> 
> Given that the driver creates a virtual decoder for each CFMWS, the
> Fixed Device Configuration (bit 4) of the Window Restriction field is
> considered as locking for the virtual decoder by the driver.
> 
> The current driver code disregards the locked status and a region can
> be destroyed regardless of the locking state.
> 
> Add a region flag to indicate the region is in a locked configuration.
> The driver will considered a region locked if the CFMWS or any decoder
> is configured as locked. The consideration is all or nothing regarding
> the locked state. It is reasonable to determine the region "locked"
> status while the region is being assembled based on the decoders.
> 
> Add a check in region commit_store() to intercept when a 0 is written
> to the commit sysfs attribute in order to prevent the destruction of a
> region when in locked state. This should be the only entry point from user
> space to destroy a region.
> 
> Add a check is added to cxl_decoder_reset() to prevent resetting a locked
> decoder within the kernel driver.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Fully agree with what should be happening here, but a question below
on the implementation.

> ---
>  drivers/cxl/core/hdm.c    |  3 +++
>  drivers/cxl/core/region.c | 16 ++++++++++++++++
>  drivers/cxl/cxl.h         |  8 ++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index d3a094ca01ad..1c5d2022c87a 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -905,6 +905,9 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>  	if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
>  		return;
>  
> +	if (test_bit(CXL_DECODER_F_LOCK, &cxld->flags))
> +		return;
> +
>  	if (port->commit_end == id)
>  		cxl_port_commit_reap(cxld);
>  	else
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b06fee1978ba..8647eff4fb78 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -419,6 +419,9 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
>  		return len;
>  	}
>  
> +	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
> +		return -EPERM;
> +
>  	rc = queue_reset(cxlr);
>  	if (rc)
>  		return rc;
> @@ -1059,6 +1062,16 @@ static int cxl_rr_assign_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>  	return 0;
>  }
>  
> +static void cxl_region_set_lock(struct cxl_region *cxlr,
> +				struct cxl_decoder *cxld)
> +{
> +	if (!test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
> +		return;
> +
> +	set_bit(CXL_REGION_F_LOCK, &cxlr->flags);

The sequence above here looks odd.  If the bit is not set, then don't 
set it? If already set then set it again? Was one of these meant to be
related to the decoder that was passed in? 

Maybe I need more coffee...


> +	clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
> +}
> +
>  /**
>   * cxl_port_attach_region() - track a region's interest in a port by endpoint
>   * @port: port to add a new region reference 'struct cxl_region_ref'
> @@ -1170,6 +1183,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  		}
>  	}
>  
> +	cxl_region_set_lock(cxlr, cxld);
> +
>  	rc = cxl_rr_ep_add(cxl_rr, cxled);
>  	if (rc) {
>  		dev_dbg(&cxlr->dev,
> @@ -2439,6 +2454,7 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
>  	dev->bus = &cxl_bus_type;
>  	dev->type = &cxl_region_type;
>  	cxlr->id = id;
> +	cxl_region_set_lock(cxlr, &cxlrd->cxlsd.cxld);
>  
>  	return cxlr;
>  }
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 231ddccf8977..6382f1983865 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -517,6 +517,14 @@ enum cxl_partition_mode {
>   */
>  #define CXL_REGION_F_NEEDS_RESET 1
>  
> +/*
> + * Indicate whether this region is locked due to 1 or more decoders that have
> + * been locked. The approach of all or nothing is taken with regard to the
> + * locked attribute. CXL_REGION_F_NEEDS_RESET should not be set if this flag is
> + * set.
> + */
> +#define CXL_REGION_F_LOCK 2
> +
>  /**
>   * struct cxl_region - CXL region
>   * @dev: This region's device
> 
> base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada

Re: [PATCH] cxl: Add handling of locked CXL decoder

[Dave Jiang]

On 10/28/25 7:39 AM, Jonathan Cameron wrote:
> On Tue, 21 Oct 2025 13:50:55 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> When a decoder is locked, it means that its configuration cannot be
>> changed. CXL spec r3.2 8.2.4.20.13 discusses the details regarding
>> locked decoders. Locking happens when bit 8 of the decoder control
>> register is set and then the decoder is committed afterwards (CXL
>> spec r3.2 8.2.4.20.7).
>>
>> Given that the driver creates a virtual decoder for each CFMWS, the
>> Fixed Device Configuration (bit 4) of the Window Restriction field is
>> considered as locking for the virtual decoder by the driver.
>>
>> The current driver code disregards the locked status and a region can
>> be destroyed regardless of the locking state.
>>
>> Add a region flag to indicate the region is in a locked configuration.
>> The driver will considered a region locked if the CFMWS or any decoder
>> is configured as locked. The consideration is all or nothing regarding
>> the locked state. It is reasonable to determine the region "locked"
>> status while the region is being assembled based on the decoders.
>>
>> Add a check in region commit_store() to intercept when a 0 is written
>> to the commit sysfs attribute in order to prevent the destruction of a
>> region when in locked state. This should be the only entry point from user
>> space to destroy a region.
>>
>> Add a check is added to cxl_decoder_reset() to prevent resetting a locked
>> decoder within the kernel driver.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> Fully agree with what should be happening here, but a question below
> on the implementation.
> 
>> ---
>>  drivers/cxl/core/hdm.c    |  3 +++
>>  drivers/cxl/core/region.c | 16 ++++++++++++++++
>>  drivers/cxl/cxl.h         |  8 ++++++++
>>  3 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index d3a094ca01ad..1c5d2022c87a 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -905,6 +905,9 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>>  	if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
>>  		return;
>>  
>> +	if (test_bit(CXL_DECODER_F_LOCK, &cxld->flags))
>> +		return;
>> +
>>  	if (port->commit_end == id)
>>  		cxl_port_commit_reap(cxld);
>>  	else
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index b06fee1978ba..8647eff4fb78 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -419,6 +419,9 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
>>  		return len;
>>  	}
>>  
>> +	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
>> +		return -EPERM;
>> +
>>  	rc = queue_reset(cxlr);
>>  	if (rc)
>>  		return rc;
>> @@ -1059,6 +1062,16 @@ static int cxl_rr_assign_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>>  	return 0;
>>  }
>>  
>> +static void cxl_region_set_lock(struct cxl_region *cxlr,
>> +				struct cxl_decoder *cxld)
>> +{
>> +	if (!test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
>> +		return;
>> +
>> +	set_bit(CXL_REGION_F_LOCK, &cxlr->flags);
> 
> The sequence above here looks odd.  If the bit is not set, then don't 
> set it? If already set then set it again? Was one of these meant to be
> related to the decoder that was passed in? 
> 
> Maybe I need more coffee...

No. That's my mistake. It should be checking the cxld for locked bit and not the region.

DJ

> 
> 
>> +	clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
>> +}
>> +
>>  /**
>>   * cxl_port_attach_region() - track a region's interest in a port by endpoint
>>   * @port: port to add a new region reference 'struct cxl_region_ref'
>> @@ -1170,6 +1183,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
>>  		}
>>  	}
>>  
>> +	cxl_region_set_lock(cxlr, cxld);
>> +
>>  	rc = cxl_rr_ep_add(cxl_rr, cxled);
>>  	if (rc) {
>>  		dev_dbg(&cxlr->dev,
>> @@ -2439,6 +2454,7 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
>>  	dev->bus = &cxl_bus_type;
>>  	dev->type = &cxl_region_type;
>>  	cxlr->id = id;
>> +	cxl_region_set_lock(cxlr, &cxlrd->cxlsd.cxld);
>>  
>>  	return cxlr;
>>  }
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 231ddccf8977..6382f1983865 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -517,6 +517,14 @@ enum cxl_partition_mode {
>>   */
>>  #define CXL_REGION_F_NEEDS_RESET 1
>>  
>> +/*
>> + * Indicate whether this region is locked due to 1 or more decoders that have
>> + * been locked. The approach of all or nothing is taken with regard to the
>> + * locked attribute. CXL_REGION_F_NEEDS_RESET should not be set if this flag is
>> + * set.
>> + */
>> +#define CXL_REGION_F_LOCK 2
>> +
>>  /**
>>   * struct cxl_region - CXL region
>>   * @dev: This region's device
>>
>> base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
> 

Re: [PATCH] cxl: Add handling of locked CXL decoder

[Alejandro Lucero Palau]

On 10/21/25 21:50, Dave Jiang wrote:
> When a decoder is locked, it means that its configuration cannot be
> changed. CXL spec r3.2 8.2.4.20.13 discusses the details regarding
> locked decoders. Locking happens when bit 8 of the decoder control
> register is set and then the decoder is committed afterwards (CXL
> spec r3.2 8.2.4.20.7).
>
> Given that the driver creates a virtual decoder for each CFMWS, the
> Fixed Device Configuration (bit 4) of the Window Restriction field is
> considered as locking for the virtual decoder by the driver.
>
> The current driver code disregards the locked status and a region can
> be destroyed regardless of the locking state.
>
> Add a region flag to indicate the region is in a locked configuration.
> The driver will considered a region locked if the CFMWS or any decoder
> is configured as locked. The consideration is all or nothing regarding
> the locked state. It is reasonable to determine the region "locked"
> status while the region is being assembled based on the decoders.
>
> Add a check in region commit_store() to intercept when a 0 is written
> to the commit sysfs attribute in order to prevent the destruction of a
> region when in locked state. This should be the only entry point from user
> space to destroy a region.
>
> Add a check is added to cxl_decoder_reset() to prevent resetting a locked
> decoder within the kernel driver.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/cxl/core/hdm.c    |  3 +++
>   drivers/cxl/core/region.c | 16 ++++++++++++++++
>   drivers/cxl/cxl.h         |  8 ++++++++
>   3 files changed, 27 insertions(+)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index d3a094ca01ad..1c5d2022c87a 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -905,6 +905,9 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>   	if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
>   		return;
>   
> +	if (test_bit(CXL_DECODER_F_LOCK, &cxld->flags))
> +		return;
> +


This is correct, but is it enough?


Would not the region teardown imply also the reset of those decoders in 
the path from the root port? Can we assume those will also be locked or 
should we add some sanity checking here?


>   	if (port->commit_end == id)
>   		cxl_port_commit_reap(cxld);
>   	else
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b06fee1978ba..8647eff4fb78 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -419,6 +419,9 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
>   		return len;
>   	}
>   
> +	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
> +		return -EPERM;
> +
>   	rc = queue_reset(cxlr);
>   	if (rc)
>   		return rc;
> @@ -1059,6 +1062,16 @@ static int cxl_rr_assign_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>   	return 0;
>   }
>   
> +static void cxl_region_set_lock(struct cxl_region *cxlr,
> +				struct cxl_decoder *cxld)
> +{
> +	if (!test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
> +		return;
> +
> +	set_bit(CXL_REGION_F_LOCK, &cxlr->flags);
> +	clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
> +}
> +
>   /**
>    * cxl_port_attach_region() - track a region's interest in a port by endpoint
>    * @port: port to add a new region reference 'struct cxl_region_ref'
> @@ -1170,6 +1183,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
>   		}
>   	}
>   
> +	cxl_region_set_lock(cxlr, cxld);
> +
>   	rc = cxl_rr_ep_add(cxl_rr, cxled);
>   	if (rc) {
>   		dev_dbg(&cxlr->dev,
> @@ -2439,6 +2454,7 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
>   	dev->bus = &cxl_bus_type;
>   	dev->type = &cxl_region_type;
>   	cxlr->id = id;
> +	cxl_region_set_lock(cxlr, &cxlrd->cxlsd.cxld);
>   
>   	return cxlr;
>   }
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 231ddccf8977..6382f1983865 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -517,6 +517,14 @@ enum cxl_partition_mode {
>    */
>   #define CXL_REGION_F_NEEDS_RESET 1
>   
> +/*
> + * Indicate whether this region is locked due to 1 or more decoders that have
> + * been locked. The approach of all or nothing is taken with regard to the
> + * locked attribute. CXL_REGION_F_NEEDS_RESET should not be set if this flag is
> + * set.
> + */
> +#define CXL_REGION_F_LOCK 2
> +
>   /**
>    * struct cxl_region - CXL region
>    * @dev: This region's device
>
> base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada

Re: [PATCH] cxl: Add handling of locked CXL decoder

[Dave Jiang]

On 10/29/25 9:17 AM, Alejandro Lucero Palau wrote:
> 
> On 10/21/25 21:50, Dave Jiang wrote:
>> When a decoder is locked, it means that its configuration cannot be
>> changed. CXL spec r3.2 8.2.4.20.13 discusses the details regarding
>> locked decoders. Locking happens when bit 8 of the decoder control
>> register is set and then the decoder is committed afterwards (CXL
>> spec r3.2 8.2.4.20.7).
>>
>> Given that the driver creates a virtual decoder for each CFMWS, the
>> Fixed Device Configuration (bit 4) of the Window Restriction field is
>> considered as locking for the virtual decoder by the driver.
>>
>> The current driver code disregards the locked status and a region can
>> be destroyed regardless of the locking state.
>>
>> Add a region flag to indicate the region is in a locked configuration.
>> The driver will considered a region locked if the CFMWS or any decoder
>> is configured as locked. The consideration is all or nothing regarding
>> the locked state. It is reasonable to determine the region "locked"
>> status while the region is being assembled based on the decoders.
>>
>> Add a check in region commit_store() to intercept when a 0 is written
>> to the commit sysfs attribute in order to prevent the destruction of a
>> region when in locked state. This should be the only entry point from user
>> space to destroy a region.
>>
>> Add a check is added to cxl_decoder_reset() to prevent resetting a locked
>> decoder within the kernel driver.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/hdm.c    |  3 +++
>>   drivers/cxl/core/region.c | 16 ++++++++++++++++
>>   drivers/cxl/cxl.h         |  8 ++++++++
>>   3 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index d3a094ca01ad..1c5d2022c87a 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -905,6 +905,9 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>>       if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
>>           return;
>>   +    if (test_bit(CXL_DECODER_F_LOCK, &cxld->flags))
>> +        return;
>> +
> 
> 
> This is correct, but is it enough?
> 
> 
> Would not the region teardown imply also the reset of those decoders in the path from the root port? Can we assume those will also be locked or should we add some sanity checking here?

Would checking the region the decoder belongs to for lock flag be sufficient?

DJ

> 
> 
>>       if (port->commit_end == id)
>>           cxl_port_commit_reap(cxld);
>>       else
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index b06fee1978ba..8647eff4fb78 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -419,6 +419,9 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
>>           return len;
>>       }
>>   +    if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
>> +        return -EPERM;
>> +
>>       rc = queue_reset(cxlr);
>>       if (rc)
>>           return rc;
>> @@ -1059,6 +1062,16 @@ static int cxl_rr_assign_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>>       return 0;
>>   }
>>   +static void cxl_region_set_lock(struct cxl_region *cxlr,
>> +                struct cxl_decoder *cxld)
>> +{
>> +    if (!test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
>> +        return;
>> +
>> +    set_bit(CXL_REGION_F_LOCK, &cxlr->flags);
>> +    clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
>> +}
>> +
>>   /**
>>    * cxl_port_attach_region() - track a region's interest in a port by endpoint
>>    * @port: port to add a new region reference 'struct cxl_region_ref'
>> @@ -1170,6 +1183,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
>>           }
>>       }
>>   +    cxl_region_set_lock(cxlr, cxld);
>> +
>>       rc = cxl_rr_ep_add(cxl_rr, cxled);
>>       if (rc) {
>>           dev_dbg(&cxlr->dev,
>> @@ -2439,6 +2454,7 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
>>       dev->bus = &cxl_bus_type;
>>       dev->type = &cxl_region_type;
>>       cxlr->id = id;
>> +    cxl_region_set_lock(cxlr, &cxlrd->cxlsd.cxld);
>>         return cxlr;
>>   }
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 231ddccf8977..6382f1983865 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -517,6 +517,14 @@ enum cxl_partition_mode {
>>    */
>>   #define CXL_REGION_F_NEEDS_RESET 1
>>   +/*
>> + * Indicate whether this region is locked due to 1 or more decoders that have
>> + * been locked. The approach of all or nothing is taken with regard to the
>> + * locked attribute. CXL_REGION_F_NEEDS_RESET should not be set if this flag is
>> + * set.
>> + */
>> +#define CXL_REGION_F_LOCK 2
>> +
>>   /**
>>    * struct cxl_region - CXL region
>>    * @dev: This region's device
>>
>> base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada

Re: [PATCH] cxl: Add handling of locked CXL decoder

[Alejandro Lucero Palau]

On 10/31/25 23:16, Dave Jiang wrote:
>
> On 10/29/25 9:17 AM, Alejandro Lucero Palau wrote:
>> On 10/21/25 21:50, Dave Jiang wrote:
>>> When a decoder is locked, it means that its configuration cannot be
>>> changed. CXL spec r3.2 8.2.4.20.13 discusses the details regarding
>>> locked decoders. Locking happens when bit 8 of the decoder control
>>> register is set and then the decoder is committed afterwards (CXL
>>> spec r3.2 8.2.4.20.7).
>>>
>>> Given that the driver creates a virtual decoder for each CFMWS, the
>>> Fixed Device Configuration (bit 4) of the Window Restriction field is
>>> considered as locking for the virtual decoder by the driver.
>>>
>>> The current driver code disregards the locked status and a region can
>>> be destroyed regardless of the locking state.
>>>
>>> Add a region flag to indicate the region is in a locked configuration.
>>> The driver will considered a region locked if the CFMWS or any decoder
>>> is configured as locked. The consideration is all or nothing regarding
>>> the locked state. It is reasonable to determine the region "locked"
>>> status while the region is being assembled based on the decoders.
>>>
>>> Add a check in region commit_store() to intercept when a 0 is written
>>> to the commit sysfs attribute in order to prevent the destruction of a
>>> region when in locked state. This should be the only entry point from user
>>> space to destroy a region.
>>>
>>> Add a check is added to cxl_decoder_reset() to prevent resetting a locked
>>> decoder within the kernel driver.
>>>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>> ---
>>>    drivers/cxl/core/hdm.c    |  3 +++
>>>    drivers/cxl/core/region.c | 16 ++++++++++++++++
>>>    drivers/cxl/cxl.h         |  8 ++++++++
>>>    3 files changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>> index d3a094ca01ad..1c5d2022c87a 100644
>>> --- a/drivers/cxl/core/hdm.c
>>> +++ b/drivers/cxl/core/hdm.c
>>> @@ -905,6 +905,9 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>>>        if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
>>>            return;
>>>    +    if (test_bit(CXL_DECODER_F_LOCK, &cxld->flags))
>>> +        return;
>>> +
>>
>> This is correct, but is it enough?
>>
>>
>> Would not the region teardown imply also the reset of those decoders in the path from the root port? Can we assume those will also be locked or should we add some sanity checking here?
> Would checking the region the decoder belongs to for lock flag be sufficient?


Yes, so any trying for removing the region failing, but maybe I'm 
confused with this check inside cxl_decoder_reset which implies to me 
such a region is being removed. I would expect a check in 
cxl_region_decode_reset which invokes this other function.


But my point is if we should check, maybe just at region creation time, 
if all the involved decoders have the lock bit set for coherency, 
assuming here the specs refer to such "locking path". Does locking an 
endpoint HDM decoder, by firmware or driver, implies the locking of the 
related decoders creating the path?


> DJ
>
>>
>>>        if (port->commit_end == id)
>>>            cxl_port_commit_reap(cxld);
>>>        else
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index b06fee1978ba..8647eff4fb78 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -419,6 +419,9 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
>>>            return len;
>>>        }
>>>    +    if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
>>> +        return -EPERM;
>>> +
>>>        rc = queue_reset(cxlr);
>>>        if (rc)
>>>            return rc;
>>> @@ -1059,6 +1062,16 @@ static int cxl_rr_assign_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>>>        return 0;
>>>    }
>>>    +static void cxl_region_set_lock(struct cxl_region *cxlr,
>>> +                struct cxl_decoder *cxld)
>>> +{
>>> +    if (!test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
>>> +        return;
>>> +
>>> +    set_bit(CXL_REGION_F_LOCK, &cxlr->flags);
>>> +    clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
>>> +}
>>> +
>>>    /**
>>>     * cxl_port_attach_region() - track a region's interest in a port by endpoint
>>>     * @port: port to add a new region reference 'struct cxl_region_ref'
>>> @@ -1170,6 +1183,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
>>>            }
>>>        }
>>>    +    cxl_region_set_lock(cxlr, cxld);
>>> +
>>>        rc = cxl_rr_ep_add(cxl_rr, cxled);
>>>        if (rc) {
>>>            dev_dbg(&cxlr->dev,
>>> @@ -2439,6 +2454,7 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
>>>        dev->bus = &cxl_bus_type;
>>>        dev->type = &cxl_region_type;
>>>        cxlr->id = id;
>>> +    cxl_region_set_lock(cxlr, &cxlrd->cxlsd.cxld);
>>>          return cxlr;
>>>    }
>>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>>> index 231ddccf8977..6382f1983865 100644
>>> --- a/drivers/cxl/cxl.h
>>> +++ b/drivers/cxl/cxl.h
>>> @@ -517,6 +517,14 @@ enum cxl_partition_mode {
>>>     */
>>>    #define CXL_REGION_F_NEEDS_RESET 1
>>>    +/*
>>> + * Indicate whether this region is locked due to 1 or more decoders that have
>>> + * been locked. The approach of all or nothing is taken with regard to the
>>> + * locked attribute. CXL_REGION_F_NEEDS_RESET should not be set if this flag is
>>> + * set.
>>> + */
>>> +#define CXL_REGION_F_LOCK 2
>>> +
>>>    /**
>>>     * struct cxl_region - CXL region
>>>     * @dev: This region's device
>>>
>>> base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada

Re: [PATCH] cxl: Add handling of locked CXL decoder

[Dave Jiang]

On 11/3/25 4:25 AM, Alejandro Lucero Palau wrote:
> 
> On 10/31/25 23:16, Dave Jiang wrote:
>>
>> On 10/29/25 9:17 AM, Alejandro Lucero Palau wrote:
>>> On 10/21/25 21:50, Dave Jiang wrote:
>>>> When a decoder is locked, it means that its configuration cannot be
>>>> changed. CXL spec r3.2 8.2.4.20.13 discusses the details regarding
>>>> locked decoders. Locking happens when bit 8 of the decoder control
>>>> register is set and then the decoder is committed afterwards (CXL
>>>> spec r3.2 8.2.4.20.7).
>>>>
>>>> Given that the driver creates a virtual decoder for each CFMWS, the
>>>> Fixed Device Configuration (bit 4) of the Window Restriction field is
>>>> considered as locking for the virtual decoder by the driver.
>>>>
>>>> The current driver code disregards the locked status and a region can
>>>> be destroyed regardless of the locking state.
>>>>
>>>> Add a region flag to indicate the region is in a locked configuration.
>>>> The driver will considered a region locked if the CFMWS or any decoder
>>>> is configured as locked. The consideration is all or nothing regarding
>>>> the locked state. It is reasonable to determine the region "locked"
>>>> status while the region is being assembled based on the decoders.
>>>>
>>>> Add a check in region commit_store() to intercept when a 0 is written
>>>> to the commit sysfs attribute in order to prevent the destruction of a
>>>> region when in locked state. This should be the only entry point from user
>>>> space to destroy a region.
>>>>
>>>> Add a check is added to cxl_decoder_reset() to prevent resetting a locked
>>>> decoder within the kernel driver.
>>>>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>> ---
>>>>    drivers/cxl/core/hdm.c    |  3 +++
>>>>    drivers/cxl/core/region.c | 16 ++++++++++++++++
>>>>    drivers/cxl/cxl.h         |  8 ++++++++
>>>>    3 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>>> index d3a094ca01ad..1c5d2022c87a 100644
>>>> --- a/drivers/cxl/core/hdm.c
>>>> +++ b/drivers/cxl/core/hdm.c
>>>> @@ -905,6 +905,9 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>>>>        if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
>>>>            return;
>>>>    +    if (test_bit(CXL_DECODER_F_LOCK, &cxld->flags))
>>>> +        return;
>>>> +
>>>
>>> This is correct, but is it enough?
>>>
>>>
>>> Would not the region teardown imply also the reset of those decoders in the path from the root port? Can we assume those will also be locked or should we add some sanity checking here?
>> Would checking the region the decoder belongs to for lock flag be sufficient?
> 
> 
> Yes, so any trying for removing the region failing, but maybe I'm confused with this check inside cxl_decoder_reset which implies to me such a region is being removed. I would expect a check in cxl_region_decode_reset which invokes this other function.

Yeah you are right, we should do the region lock check at cxl_region_decode_reset() and just leave the simple decoder lock check to cxl_decoder_reset(). > 
> 
> But my point is if we should check, maybe just at region creation time, if all the involved decoders have the lock bit set for coherency, assuming here the specs refer to such "locking path". Does locking an endpoint HDM decoder, by firmware or driver, implies the locking of the related decoders creating the path?

I'm not sure if the spec implies that. At least currently I'm setting that as a Linux policy to make things simple. We can consider altering that in the future if there's some specific case to consider that goes opposing to that.

DJ

> 
> 
>> DJ
>>
>>>
>>>>        if (port->commit_end == id)
>>>>            cxl_port_commit_reap(cxld);
>>>>        else
>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>> index b06fee1978ba..8647eff4fb78 100644
>>>> --- a/drivers/cxl/core/region.c
>>>> +++ b/drivers/cxl/core/region.c
>>>> @@ -419,6 +419,9 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
>>>>            return len;
>>>>        }
>>>>    +    if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
>>>> +        return -EPERM;
>>>> +
>>>>        rc = queue_reset(cxlr);
>>>>        if (rc)
>>>>            return rc;
>>>> @@ -1059,6 +1062,16 @@ static int cxl_rr_assign_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>>>>        return 0;
>>>>    }
>>>>    +static void cxl_region_set_lock(struct cxl_region *cxlr,
>>>> +                struct cxl_decoder *cxld)
>>>> +{
>>>> +    if (!test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
>>>> +        return;
>>>> +
>>>> +    set_bit(CXL_REGION_F_LOCK, &cxlr->flags);
>>>> +    clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
>>>> +}
>>>> +
>>>>    /**
>>>>     * cxl_port_attach_region() - track a region's interest in a port by endpoint
>>>>     * @port: port to add a new region reference 'struct cxl_region_ref'
>>>> @@ -1170,6 +1183,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
>>>>            }
>>>>        }
>>>>    +    cxl_region_set_lock(cxlr, cxld);
>>>> +
>>>>        rc = cxl_rr_ep_add(cxl_rr, cxled);
>>>>        if (rc) {
>>>>            dev_dbg(&cxlr->dev,
>>>> @@ -2439,6 +2454,7 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
>>>>        dev->bus = &cxl_bus_type;
>>>>        dev->type = &cxl_region_type;
>>>>        cxlr->id = id;
>>>> +    cxl_region_set_lock(cxlr, &cxlrd->cxlsd.cxld);
>>>>          return cxlr;
>>>>    }
>>>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>>>> index 231ddccf8977..6382f1983865 100644
>>>> --- a/drivers/cxl/cxl.h
>>>> +++ b/drivers/cxl/cxl.h
>>>> @@ -517,6 +517,14 @@ enum cxl_partition_mode {
>>>>     */
>>>>    #define CXL_REGION_F_NEEDS_RESET 1
>>>>    +/*
>>>> + * Indicate whether this region is locked due to 1 or more decoders that have
>>>> + * been locked. The approach of all or nothing is taken with regard to the
>>>> + * locked attribute. CXL_REGION_F_NEEDS_RESET should not be set if this flag is
>>>> + * set.
>>>> + */
>>>> +#define CXL_REGION_F_LOCK 2
>>>> +
>>>>    /**
>>>>     * struct cxl_region - CXL region
>>>>     * @dev: This region's device
>>>>
>>>> base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada