drivers/cxl/core/hdm.c | 4 +++- drivers/cxl/core/region.c | 6 ++++-- drivers/cxl/cxl.h | 3 ++- tools/testing/cxl/test/cxl.c | 1 + 4 files changed, 10 insertions(+), 4 deletions(-)
During kernel booting, CXL drivers will attempt to construct the CXL region
according to the pre-programed(firmware provisioning) HDM decoders.
This construction process will fail for some reasons, in this case, the
userspace cli like ndctl/cxl cannot destroy nor create regions upon the
existing decoders.
Introuce a new flag CXL_DECODER_F_NEED_RESET tell the driver to reset
the decoder during `cxl destroy-region regionN`, so that region can be
create again after that.
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
Cover letter is here.
Hi all,
Previously, we encountered a CXL device with differing IG between the device
and its USP, which resulted in the driver’s failure to automatically
complete the creation of the region, and user cannot create/destroy region
after this failure.
Although this IG issue has been ignored in the new kernel [1], I realized
that if there was an error in firmware provisioning the HDM decoders, the
user might not be able to destroy the unfinished region and recreate it.
This is because certain components related to this region are in an
inconsistent state, preventing the CXL tool from operating on it.
This implies that the OS administrator cannot reconfigure the CXL device,
which is largely contrary to user expectations. I am keen to hear your
thoughts on this matter.
A modified QEMU [2] is able to limitly program the HDM decoders to inject some
wrong decoder configurations.
[1] https://lore.kernel.org/lkml/20250402232552.999634-1-gourry@gourry.net/
[2] https://github.com/zhijianli88/qemu/tree/program-decoder
---
---
drivers/cxl/core/hdm.c | 4 +++-
drivers/cxl/core/region.c | 6 ++++--
drivers/cxl/cxl.h | 3 ++-
tools/testing/cxl/test/cxl.c | 1 +
4 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 70cae4ebf8a4..afbbda780d4d 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -559,7 +559,8 @@ int cxl_dpa_free(struct cxl_endpoint_decoder *cxled)
dev_name(&cxled->cxld.region->dev));
return -EBUSY;
}
- if (cxled->cxld.flags & CXL_DECODER_F_ENABLE) {
+ if (cxled->cxld.flags & CXL_DECODER_F_ENABLE &&
+ !(cxled->cxld.flags & CXL_DECODER_F_NEED_RESET)) {
dev_dbg(dev, "decoder enabled\n");
return -EBUSY;
}
@@ -918,6 +919,7 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
up_read(&cxl_dpa_rwsem);
cxld->flags &= ~CXL_DECODER_F_ENABLE;
+ cxld->flags &= ~CXL_DECODER_F_NEED_RESET;
/* Userspace is now responsible for reconfiguring this decoder */
if (is_endpoint_decoder(&cxld->dev)) {
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index c3f4dc244df7..d025e892d07d 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2096,7 +2096,8 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
p = &cxlr->params;
get_device(&cxlr->dev);
- if (p->state > CXL_CONFIG_ACTIVE) {
+ if (p->state > CXL_CONFIG_ACTIVE ||
+ cxled->cxld.flags & CXL_DECODER_F_NEED_RESET) {
cxl_region_decode_reset(cxlr, p->interleave_ways);
p->state = CXL_CONFIG_ACTIVE;
}
@@ -3434,7 +3435,8 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
if (device_attach(&cxlr->dev) < 0)
dev_err(&cxlr->dev, "failed to enable, range: %pr\n",
p->res);
- }
+ } else
+ cxled->cxld.flags |= CXL_DECODER_F_NEED_RESET;
put_device(region_dev);
out:
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index be8a7dc77719..60fae072bbcf 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -331,7 +331,8 @@ int cxl_dport_map_rcd_linkcap(struct pci_dev *pdev, struct cxl_dport *dport);
#define CXL_DECODER_F_TYPE3 BIT(3)
#define CXL_DECODER_F_LOCK BIT(4)
#define CXL_DECODER_F_ENABLE BIT(5)
-#define CXL_DECODER_F_MASK GENMASK(5, 0)
+#define CXL_DECODER_F_NEED_RESET BIT(6)
+#define CXL_DECODER_F_MASK GENMASK(6, 0)
enum cxl_decoder_type {
CXL_DECODER_DEVMEM = 2,
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 1c3336095923..5dac454ca0e2 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -709,6 +709,7 @@ static void mock_decoder_reset(struct cxl_decoder *cxld)
"%s: out of order reset, expected decoder%d.%d\n",
dev_name(&cxld->dev), port->id, port->commit_end);
cxld->flags &= ~CXL_DECODER_F_ENABLE;
+ cxld->flags &= ~CXL_DECODER_F_NEED_RESET;
}
static void default_mock_decoder(struct cxl_decoder *cxld)
--
2.27.0
On Wed, Apr 30, 2025 at 09:29:15AM +0800, Li Zhijian wrote: > During kernel booting, CXL drivers will attempt to construct the CXL region > according to the pre-programed(firmware provisioning) HDM decoders. > > This construction process will fail for some reasons, in this case, the > userspace cli like ndctl/cxl cannot destroy nor create regions upon the > existing decoders. > > Introuce a new flag CXL_DECODER_F_NEED_RESET tell the driver to reset > the decoder during `cxl destroy-region regionN`, so that region can be > create again after that. > My best understanding of why this is disallowed is that firmware/bios programmed decoders need to be locked because there is an assumption that the platform programmed it that way *for a reason* - and that changing the programming would break it (cause MCEs for other reasons, etc). So the appropriate solution here is for the platform vendor to fix their firmware. But I am not a platform people - so I will defer to them on whether my understanding is correct. ~Gregory
On 30/04/2025 10:17, Gregory Price wrote: > On Wed, Apr 30, 2025 at 09:29:15AM +0800, Li Zhijian wrote: >> During kernel booting, CXL drivers will attempt to construct the CXL region >> according to the pre-programed(firmware provisioning) HDM decoders. >> >> This construction process will fail for some reasons, in this case, the >> userspace cli like ndctl/cxl cannot destroy nor create regions upon the >> existing decoders. >> >> Introuce a new flag CXL_DECODER_F_NEED_RESET tell the driver to reset >> the decoder during `cxl destroy-region regionN`, so that region can be >> create again after that. >> > > My best understanding of why this is disallowed is that firmware/bios > programmed decoders need to be locked because there is an assumption > that the platform programmed it that way *for a reason* - and that > changing the programming would break it (cause MCEs for other reasons, > etc). Hi Gregory, Thank you for the feedback. Based on current CXL driver behavior, user-space tools can indeed reprogram firmware-provisioned HDM decoders in practice. For example, after a successful boot, one may destroy the auto-constructed region via cxl destroy-region and create a new different region. This indicates that the kernel does not inherently lock down these decoders. As for the locking rationale you mentioned, platform vendors might enforce their policies through mechanisms like the *Lock-On-Commit* in CXL HDM Decoder n Control Register While platform vendors may have valid considerations (as you noted), from a driver and end-user perspective, depending solely on firmware updates to fix transient failures is not smooth sometimes :). > > So the appropriate solution here is for the platform vendor to fix their > firmware. > > But I am not a platform people - so I will defer to them on whether my > understanding is correct. Yeah, it's still in the RFC stage, let's hear more voices. Thanks Zhijian > > ~Gregory > >
On 4/30/25 04:24, Zhijian Li (Fujitsu) wrote: > > On 30/04/2025 10:17, Gregory Price wrote: >> On Wed, Apr 30, 2025 at 09:29:15AM +0800, Li Zhijian wrote: >>> During kernel booting, CXL drivers will attempt to construct the CXL region >>> according to the pre-programed(firmware provisioning) HDM decoders. >>> >>> This construction process will fail for some reasons, in this case, the >>> userspace cli like ndctl/cxl cannot destroy nor create regions upon the >>> existing decoders. >>> >>> Introuce a new flag CXL_DECODER_F_NEED_RESET tell the driver to reset >>> the decoder during `cxl destroy-region regionN`, so that region can be >>> create again after that. >>> >> My best understanding of why this is disallowed is that firmware/bios >> programmed decoders need to be locked because there is an assumption >> that the platform programmed it that way *for a reason* - and that >> changing the programming would break it (cause MCEs for other reasons, >> etc). > > Hi Gregory, > > Thank you for the feedback. Based on current CXL driver behavior, user-space tools > can indeed reprogram firmware-provisioned HDM decoders in practice. > > For example, after a successful boot, one may destroy the auto-constructed region > via cxl destroy-region and create a new different region. > This indicates that the kernel does not inherently lock down these decoders. > > As for the locking rationale you mentioned, platform vendors might enforce their policies > through mechanisms like the *Lock-On-Commit* in CXL HDM Decoder n Control Register > > While platform vendors may have valid considerations (as you noted), from a driver and > end-user perspective, depending solely on firmware updates to fix transient failures > is not smooth sometimes :). > Hi Zhijan, From my current effort trying to get a Type2 device properly initialized by the kernel after the BIOS/platform firmware doing whatever it needs to do, I really think we should have a wider discussion regarding this sync, and maybe to have first something from the kernel expectation of what the BIOS should and should not do. If this makes sense, I could work on a initial draft about the outline or points to discuss about this. Thank you >> So the appropriate solution here is for the platform vendor to fix their >> firmware. >> >> But I am not a platform people - so I will defer to them on whether my >> understanding is correct. > Yeah, it's still in the RFC stage, let's hear more voices. > > Thanks > Zhijian > >> ~Gregory >>
On 30/04/2025 16:42, Alejandro Lucero Palau wrote: >> >> Hi Gregory, >> >> Thank you for the feedback. Based on current CXL driver behavior, user-space tools >> can indeed reprogram firmware-provisioned HDM decoders in practice. >> >> For example, after a successful boot, one may destroy the auto-constructed region >> via cxl destroy-region and create a new different region. >> This indicates that the kernel does not inherently lock down these decoders. >> >> As for the locking rationale you mentioned, platform vendors might enforce their policies >> through mechanisms like the *Lock-On-Commit* in CXL HDM Decoder n Control Register >> >> While platform vendors may have valid considerations (as you noted), from a driver and >> end-user perspective, depending solely on firmware updates to fix transient failures >> is not smooth sometimes 🙂. >> > > Hi Zhijan, > > > From my current effort trying to get a Type2 device properly initialized by the kernel after the BIOS/platform firmware doing whatever it needs to do, I really think we should have a wider discussion regarding this sync, and maybe to have first something from the kernel expectation of what the BIOS should and should not do. > > > If this makes sense, I could work on a initial draft about the outline or points to discuss about this. > Hi Alejandro, Thanks for sharing this concrete pain point. Your experience highlights a critical gap in "defining clear handoff protocols between firmware and the kernel" for CXL device initialization. I agree that we need a community-driven effort to establish these expectations. I’m happy to see your draft or thread for deeper discussion. Best, Zhijian > > Thank you
On 5/7/25 07:42, Zhijian Li (Fujitsu) wrote: > > On 30/04/2025 16:42, Alejandro Lucero Palau wrote: >>> Hi Gregory, >>> >>> Thank you for the feedback. Based on current CXL driver behavior, user-space tools >>> can indeed reprogram firmware-provisioned HDM decoders in practice. >>> >>> For example, after a successful boot, one may destroy the auto-constructed region >>> via cxl destroy-region and create a new different region. >>> This indicates that the kernel does not inherently lock down these decoders. >>> >>> As for the locking rationale you mentioned, platform vendors might enforce their policies >>> through mechanisms like the *Lock-On-Commit* in CXL HDM Decoder n Control Register >>> >>> While platform vendors may have valid considerations (as you noted), from a driver and >>> end-user perspective, depending solely on firmware updates to fix transient failures >>> is not smooth sometimes 🙂. >>> >> Hi Zhijan, >> >> >> From my current effort trying to get a Type2 device properly initialized by the kernel after the BIOS/platform firmware doing whatever it needs to do, I really think we should have a wider discussion regarding this sync, and maybe to have first something from the kernel expectation of what the BIOS should and should not do. >> >> >> If this makes sense, I could work on a initial draft about the outline or points to discuss about this. >> > Hi Alejandro, > > Thanks for sharing this concrete pain point. Your experience highlights a critical gap in > "defining clear handoff protocols between firmware and the kernel" for CXL device initialization. > I agree that we need a community-driven effort to establish these expectations. > > > I’m happy to see your draft or thread for deeper discussion. Hi Zhijan, Great. I'll work on this and hopefully something to share in a couple of weeks. Thanks > > Best, > Zhijian > > >> Thank you
© 2016 - 2026 Red Hat, Inc.