Re: [PATCH v8 01/21] cxl/mbox: Flag support for Dynamic Capacity Devices (DCD)

[Ira Weiny]

Dan Williams wrote:
> Ira Weiny 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.
> > 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Reviewed-by: Fan Ni <fan.ni@xxxxxxxxxxx>
> > Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > Reviewed-by: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> > Reviewed-by: Li Ming <ming.li@xxxxxxxxxxxx>
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>

[snip]

> >  /* Device enabled poison commands */
> >  enum poison_cmd_enabled_bits {
> >  	CXL_POISON_ENABLED_LIST,
> > @@ -461,6 +470,7 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox)
> >   * @lsa_size: Size of Label Storage Area
> >   *                (CXL 2.0 8.2.9.5.1.1 Identify Memory Device)
> >   * @firmware_version: Firmware version for the memory device.
> > + * @dcd_cmds: List of DCD commands implemented by memory device
> >   * @enabled_cmds: Hardware commands found enabled in CEL.
> >   * @exclusive_cmds: Commands that are kernel-internal only
> >   * @total_bytes: sum of all possible capacities
> > @@ -485,6 +495,7 @@ struct cxl_memdev_state {
> >  	struct cxl_dev_state cxlds;
> >  	size_t lsa_size;
> >  	char firmware_version[0x10];
> > +	DECLARE_BITMAP(dcd_cmds, CXL_DCD_ENABLED_MAX);
> 
> Can you clarify why cxl_memdev_state needs this bitmap?

Nope.  I think you are right that there is no need for partial support.

> In the case of
> 'security' and 'poison' functionality there is a subset of functionality
> that can be enabled if some of the commands are missing. Like poison
> listing is still possible even if poison injection is missing. In the
> case of DCD it is all or nothing.
> 
> In short, I do not think the cxl_memdev_state object needs to track
> anything more than a single "DCD capable" flag, and cxl_walk_cel() can
> check for all commands locally without carrying that bitmap around
> indefinitely.
> 
> Something simple like:
> 
> cxl_walk_cel()
>     for (...) {
>       if (cxl_is_dcd_command()
>         set_bit(opcode & 0xf, &dcd_commands);
>     }
>     if (dcd_commands == 0xf)
>         mds->dcd_enabled = true;
>     else if (dcd_commands)
>         dev_dbg(...)
> 
> ...otherwise it begs the question why the driver would care about
> anything other than "all" dcd commands?

Yea this could be done.

Ira