[PATCH v9 01/19] cxl/mbox: Flag support for Dynamic Capacity Devices (DCD)

Ira Weiny posted 19 patches 8 months ago
[PATCH v9 01/19] cxl/mbox: Flag support for Dynamic Capacity Devices (DCD)
Posted by Ira Weiny 8 months ago
Per the CXL 3.1 specification software must check the Command Effects
Log (CEL) for dynamic capacity command support.

Detect support for the DCD commands while reading the CEL, including:

	Get DC Config
	Get DC Extent List
	Add DC Response
	Release DC

Based on an original patch by Navneet Singh.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes:
[iweiny: rebased]
[iweiny: remove tags]
[djbw: remove dcd_cmds bitmask from mds]
---
 drivers/cxl/core/mbox.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h    | 15 +++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index d72764056ce6..58d378400a4b 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -165,6 +165,43 @@ static void cxl_set_security_cmd_enabled(struct cxl_security_state *security,
 	}
 }
 
+static bool cxl_is_dcd_command(u16 opcode)
+{
+#define CXL_MBOX_OP_DCD_CMDS 0x48
+
+	return (opcode >> 8) == CXL_MBOX_OP_DCD_CMDS;
+}
+
+static void cxl_set_dcd_cmd_enabled(struct cxl_memdev_state *mds, u16 opcode,
+				    unsigned long *cmd_mask)
+{
+	switch (opcode) {
+	case CXL_MBOX_OP_GET_DC_CONFIG:
+		set_bit(CXL_DCD_ENABLED_GET_CONFIG, cmd_mask);
+		break;
+	case CXL_MBOX_OP_GET_DC_EXTENT_LIST:
+		set_bit(CXL_DCD_ENABLED_GET_EXTENT_LIST, cmd_mask);
+		break;
+	case CXL_MBOX_OP_ADD_DC_RESPONSE:
+		set_bit(CXL_DCD_ENABLED_ADD_RESPONSE, cmd_mask);
+		break;
+	case CXL_MBOX_OP_RELEASE_DC:
+		set_bit(CXL_DCD_ENABLED_RELEASE, cmd_mask);
+		break;
+	default:
+		break;
+	}
+}
+
+static bool cxl_verify_dcd_cmds(struct cxl_memdev_state *mds, unsigned long *cmds_seen)
+{
+	DECLARE_BITMAP(all_cmds, CXL_DCD_ENABLED_MAX);
+	DECLARE_BITMAP(dst, CXL_DCD_ENABLED_MAX);
+
+	bitmap_fill(all_cmds, CXL_DCD_ENABLED_MAX);
+	return bitmap_and(dst, cmds_seen, all_cmds, CXL_DCD_ENABLED_MAX);
+}
+
 static bool cxl_is_poison_command(u16 opcode)
 {
 #define CXL_MBOX_OP_POISON_CMDS 0x43
@@ -750,6 +787,7 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
 	struct cxl_cel_entry *cel_entry;
 	const int cel_entries = size / sizeof(*cel_entry);
+	DECLARE_BITMAP(dcd_cmds, CXL_DCD_ENABLED_MAX);
 	struct device *dev = mds->cxlds.dev;
 	int i, ro_cmds = 0, wr_cmds = 0;
 
@@ -778,11 +816,17 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 			enabled++;
 		}
 
+		if (cxl_is_dcd_command(opcode)) {
+			cxl_set_dcd_cmd_enabled(mds, opcode, dcd_cmds);
+			enabled++;
+		}
+
 		dev_dbg(dev, "Opcode 0x%04x %s\n", opcode,
 			enabled ? "enabled" : "unsupported by driver");
 	}
 
 	set_features_cap(cxl_mbox, ro_cmds, wr_cmds);
+	mds->dcd_supported = cxl_verify_dcd_cmds(mds, dcd_cmds);
 }
 
 static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_memdev_state *mds)
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 3ec6b906371b..394a776954f4 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -216,6 +216,15 @@ struct cxl_event_state {
 	struct mutex log_lock;
 };
 
+/* Device enabled DCD commands */
+enum dcd_cmd_enabled_bits {
+	CXL_DCD_ENABLED_GET_CONFIG,
+	CXL_DCD_ENABLED_GET_EXTENT_LIST,
+	CXL_DCD_ENABLED_ADD_RESPONSE,
+	CXL_DCD_ENABLED_RELEASE,
+	CXL_DCD_ENABLED_MAX
+};
+
 /* Device enabled poison commands */
 enum poison_cmd_enabled_bits {
 	CXL_POISON_ENABLED_LIST,
@@ -472,6 +481,7 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox)
  * @partition_align_bytes: alignment size for partition-able capacity
  * @active_volatile_bytes: sum of hard + soft volatile
  * @active_persistent_bytes: sum of hard + soft persistent
+ * @dcd_supported: all DCD commands are supported
  * @event: event log driver state
  * @poison: poison driver state info
  * @security: security driver state info
@@ -491,6 +501,7 @@ struct cxl_memdev_state {
 	u64 partition_align_bytes;
 	u64 active_volatile_bytes;
 	u64 active_persistent_bytes;
+	bool dcd_supported;
 
 	struct cxl_event_state event;
 	struct cxl_poison_state poison;
@@ -551,6 +562,10 @@ enum cxl_opcode {
 	CXL_MBOX_OP_UNLOCK		= 0x4503,
 	CXL_MBOX_OP_FREEZE_SECURITY	= 0x4504,
 	CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE	= 0x4505,
+	CXL_MBOX_OP_GET_DC_CONFIG	= 0x4800,
+	CXL_MBOX_OP_GET_DC_EXTENT_LIST	= 0x4801,
+	CXL_MBOX_OP_ADD_DC_RESPONSE	= 0x4802,
+	CXL_MBOX_OP_RELEASE_DC		= 0x4803,
 	CXL_MBOX_OP_MAX			= 0x10000
 };
 

-- 
2.49.0
Re: [PATCH v9 01/19] cxl/mbox: Flag support for Dynamic Capacity Devices (DCD)
Posted by Jonathan Cameron 8 months ago
On Sun, 13 Apr 2025 17:52:09 -0500
Ira Weiny <ira.weiny@intel.com> wrote:

> Per the CXL 3.1 specification software must check the Command Effects
> Log (CEL) for dynamic capacity command support.
> 
> Detect support for the DCD commands while reading the CEL, including:
> 
> 	Get DC Config
> 	Get DC Extent List
> 	Add DC Response
> 	Release DC
> 
> Based on an original patch by Navneet Singh.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>


> +
> +static bool cxl_verify_dcd_cmds(struct cxl_memdev_state *mds, unsigned long *cmds_seen)

It's not immediately obvious to me what the right behavior
from something called cxl_verify_dcd_cmds() is.  A comment might help with that.

I think all it does right now is check if any bits are set. In my head
it was going to check that all bits needed for a useful implementation were
set. I did have to go check what a 'logical and' of a bitmap was defined as
because that bit of the bitmap_and() return value wasn't obvious to me either!


> +{
> +	DECLARE_BITMAP(all_cmds, CXL_DCD_ENABLED_MAX);
> +	DECLARE_BITMAP(dst, CXL_DCD_ENABLED_MAX);
> +
> +	bitmap_fill(all_cmds, CXL_DCD_ENABLED_MAX);
> +	return bitmap_and(dst, cmds_seen, all_cmds, CXL_DCD_ENABLED_MAX);
> +}
> +
Re: [PATCH v9 01/19] cxl/mbox: Flag support for Dynamic Capacity Devices (DCD)
Posted by Fan Ni 7 months, 1 week ago
On Mon, Apr 14, 2025 at 03:19:50PM +0100, Jonathan Cameron wrote:
> On Sun, 13 Apr 2025 17:52:09 -0500
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > Per the CXL 3.1 specification software must check the Command Effects
> > Log (CEL) for dynamic capacity command support.
> > 
> > Detect support for the DCD commands while reading the CEL, including:
> > 
> > 	Get DC Config
> > 	Get DC Extent List
> > 	Add DC Response
> > 	Release DC
> > 
> > Based on an original patch by Navneet Singh.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> 
> > +
> > +static bool cxl_verify_dcd_cmds(struct cxl_memdev_state *mds, unsigned long *cmds_seen)
> 
> It's not immediately obvious to me what the right behavior
> from something called cxl_verify_dcd_cmds() is.  A comment might help with that.
> 
> I think all it does right now is check if any bits are set. In my head
> it was going to check that all bits needed for a useful implementation were
> set. I did have to go check what a 'logical and' of a bitmap was defined as
> because that bit of the bitmap_and() return value wasn't obvious to me either!

The code only checks if any DCD command (48xx) is supported, if any is
set, it will set "dcd_supported".
As you mentioned, it seems we should check all the related commands are
supported, otherwise it is not valid implementation.

Fan
> 
> 
> > +{
> > +	DECLARE_BITMAP(all_cmds, CXL_DCD_ENABLED_MAX);
> > +	DECLARE_BITMAP(dst, CXL_DCD_ENABLED_MAX);
> > +
> > +	bitmap_fill(all_cmds, CXL_DCD_ENABLED_MAX);
> > +	return bitmap_and(dst, cmds_seen, all_cmds, CXL_DCD_ENABLED_MAX);
> > +}
> > +
> 
> 

-- 
Fan Ni
Re: [PATCH v9 01/19] cxl/mbox: Flag support for Dynamic Capacity Devices (DCD)
Posted by Ira Weiny 7 months, 1 week ago
Fan Ni wrote:
> On Mon, Apr 14, 2025 at 03:19:50PM +0100, Jonathan Cameron wrote:
> > On Sun, 13 Apr 2025 17:52:09 -0500
> > Ira Weiny <ira.weiny@intel.com> wrote:

[snip]

> > 
> > > +
> > > +static bool cxl_verify_dcd_cmds(struct cxl_memdev_state *mds, unsigned long *cmds_seen)
> > 
> > It's not immediately obvious to me what the right behavior
> > from something called cxl_verify_dcd_cmds() is.  A comment might help with that.
> > 
> > I think all it does right now is check if any bits are set. In my head
> > it was going to check that all bits needed for a useful implementation were
> > set. I did have to go check what a 'logical and' of a bitmap was defined as
> > because that bit of the bitmap_and() return value wasn't obvious to me either!
> 
> The code only checks if any DCD command (48xx) is supported, if any is
> set, it will set "dcd_supported".
> As you mentioned, it seems we should check all the related commands are
> supported, otherwise it is not valid implementation.
> 
> Fan
> > 
> > 
> > > +{
> > > +	DECLARE_BITMAP(all_cmds, CXL_DCD_ENABLED_MAX);
> > > +	DECLARE_BITMAP(dst, CXL_DCD_ENABLED_MAX);
> > > +
> > > +	bitmap_fill(all_cmds, CXL_DCD_ENABLED_MAX);
> > > +	return bitmap_and(dst, cmds_seen, all_cmds, CXL_DCD_ENABLED_MAX);

Yea... so this should read:

...
	bitmap_and(dst, cmds_seen, all_cmds, CXL_DCD_ENABLED_MAX);
	return bitmap_equal(dst, all_cmds, CXL_DCD_ENABLED_MAX);
...

Of course if a device has set any of these commands true it better have
set them all.  Otherwise the device is broken and it will fail in bad
ways.

But I agree with both of you that this is much better and explicit that
something went wrong.  A dev_dbg() might be in order to debug such an
issue.

Ira

[snip]
Re: [PATCH v9 01/19] cxl/mbox: Flag support for Dynamic Capacity Devices (DCD)
Posted by Fan Ni 7 months, 1 week ago
On Tue, May 06, 2025 at 11:09:09AM -0500, Ira Weiny wrote:
> Fan Ni wrote:
> > On Mon, Apr 14, 2025 at 03:19:50PM +0100, Jonathan Cameron wrote:
> > > On Sun, 13 Apr 2025 17:52:09 -0500
> > > Ira Weiny <ira.weiny@intel.com> wrote:
> 
> [snip]
> 
> > > 
> > > > +
> > > > +static bool cxl_verify_dcd_cmds(struct cxl_memdev_state *mds, unsigned long *cmds_seen)
> > > 
> > > It's not immediately obvious to me what the right behavior
> > > from something called cxl_verify_dcd_cmds() is.  A comment might help with that.
> > > 
> > > I think all it does right now is check if any bits are set. In my head
> > > it was going to check that all bits needed for a useful implementation were
> > > set. I did have to go check what a 'logical and' of a bitmap was defined as
> > > because that bit of the bitmap_and() return value wasn't obvious to me either!
> > 
> > The code only checks if any DCD command (48xx) is supported, if any is
> > set, it will set "dcd_supported".
> > As you mentioned, it seems we should check all the related commands are
> > supported, otherwise it is not valid implementation.
> > 
> > Fan
> > > 
> > > 
> > > > +{
> > > > +	DECLARE_BITMAP(all_cmds, CXL_DCD_ENABLED_MAX);
> > > > +	DECLARE_BITMAP(dst, CXL_DCD_ENABLED_MAX);
> > > > +
> > > > +	bitmap_fill(all_cmds, CXL_DCD_ENABLED_MAX);
> > > > +	return bitmap_and(dst, cmds_seen, all_cmds, CXL_DCD_ENABLED_MAX);
> 
> Yea... so this should read:
> 
> ...
> 	bitmap_and(dst, cmds_seen, all_cmds, CXL_DCD_ENABLED_MAX);
> 	return bitmap_equal(dst, all_cmds, CXL_DCD_ENABLED_MAX);
Maybe only 
    return bitmap_equal(cmds_seen, all_cmds, CXL_DCD_ENABLED_MAX)?

Fan
> ...
> 
> Of course if a device has set any of these commands true it better have
> set them all.  Otherwise the device is broken and it will fail in bad
> ways.
> 
> But I agree with both of you that this is much better and explicit that
> something went wrong.  A dev_dbg() might be in order to debug such an
> issue.
> 
> Ira
> 
> [snip]

-- 
Fan Ni