Re: [PATCH] cxl/pci: Set the device timestamp

[Dan Williams <dan.j.williams@intel.com>]

Ira Weiny wrote:
> Dan Williams wrote:
> > Jonathan Cameron wrote:
> > > > > >  /*
> > > > > > @@ -857,6 +859,29 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds)
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_NS_GPL(cxl_mem_create_range_info, CXL);
> > > > > >  
> > > > > > +int cxl_set_timestamp(struct cxl_dev_state *cxlds, u64 ts)
> > > > > > +{
> > > > > > +	struct cxl_mbox_cmd mbox_cmd;
> > > > > > +	struct cxl_mbox_set_timestamp_in pi;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Command is optional and functionality should not be affected if
> > > > > > +	 * the command is not available.
> > > > > > +	 */
> > > > > > +	if (!test_bit(CXL_MEM_COMMAND_ID_SET_TIMESTAMP, cxlds->enabled_cmds))
> > > > > > +		return 0;
> > > 
> > > One side effect of dropping the userspace handling is we loose
> > > the presence in enabled_cmds (based on the CEL).  I've replaced
> > > this with specific handling of the Not Supported mailbox return code
> > > and suitable comments on why I'm not considering that an error.
> > > 
> > > Hopefully that compromise makes sense.
> > 
> > That does make sense.
> > 
> > That also has implications for the debug messages that will say that the "set
> > timestamp" opcode is unsupported.
> > 
> > What do you think of the following to try to clarify the distinction of
> > supported opcodes that are used internally by the driver and the ones
> > that are wrapped by a user command?
> > 
> > -- >8 --
> > From d36b9ce647e615be8241d7fbf86ce555c72cc2a4 Mon Sep 17 00:00:00 2001
> > From: Dan Williams <dan.j.williams@intel.com>
> > Date: Fri, 27 Jan 2023 11:00:52 -0800
> > Subject: [PATCH] cxl/mbox: Indicate internal vs user enabled for command debug
> > 
> > At initial enumeration provide an indication of whether the kernel will
> > use an opcode internally, and / or make the functionality available via
> > user ioctl command.
> > 
> > Given that 'enum cxl_opcode' needs to be updated before the kernel can
> > use a CXL opcode internally, use that list to populate known_opcode().
> > 
> > For the QEMU CXL emulation this results in the following debug messages
> > at startup:
> > 
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x100 Enabled: 0 User: none
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x101 Enabled: 0 User: none
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x102 Enabled: 0 User: none
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x103 Enabled: 0 User: none
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x200 Enabled: 1 User: Get FW Info
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x300 Enabled: 0 User: none
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x301 Enabled: 0 User: none
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x400 Enabled: 1 User: Get Supported Logs
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x401 Enabled: 1 User: Get Log
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x4000 Enabled: 1 User: Identify Command
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x4100 Enabled: 1 User: Get Partition Information
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x4102 Enabled: 1 User: Get Label Storage Area
> >  cxl_walk_cel: cxl_pci 0000:35:00.0: Opcode 0x4103 Enabled: 1 User: Set Label Storage Area
> 
> I think this is nice debug output.
> 
> This kind of threw me though.  It looks like all enabled commands have
> user commands.

Only because I ran it on my branch without 'Set Timestamp' or 'Get Event
Records'. I would also expect that since the Security Commands are
internal-only that cxl_test output would show those commands enabled,
but without an associated user command. Turns out that's a quirk of
cxl_test where it fails to publish the opcodes in the CEL, but still
attempts to call them nonetheless.

I think that needs fixing. I think cxl_walk_cel() needs to cache the
availablility of the event commands and if not present exit out of
cxl_event_config() without failing. As it stands those will fail the
driver load on any device that does not publish those optional commands.

> Would it be better to s/Enabled/Kernel.  Or perhaps driver?

Yeah, "Kernel" makes it clearer.

> Should we also check anything that is exclusive to the kernel and report
> that?

Exlcusivity can be ephemeral, like the label storage commands that are
ok through ioctl as long as nvdimm is not attached. The truly
kernel-internal exclusive commands will be "Kernel: 1 User: none".

> FWIW I've got a patch warming which updates the query command to factor in
> the kernel exclusive and hardware support, in case anyone is looking at
> this debug output as a way to determine that.

Cool.

> 
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/mbox.c | 48 ++++++++++++++++++++++++++++++++++-------
> >  drivers/cxl/cxlmem.h    |  1 +
> >  2 files changed, 41 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index cae43cea00cd..45206465fd4d 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -587,6 +587,40 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
> >  	return 0;
> >  }
> >  
> > +static bool known_opcode(u16 opcode)
> 
> Perhaps driver_known_opcode?

A bit on the nose, this is the driver.

> 
> > +{
> > +	switch (opcode) {
> > +	case CXL_MBOX_OP_GET_FW_INFO:
> > +	case CXL_MBOX_OP_ACTIVATE_FW:
> > +	case CXL_MBOX_OP_GET_SUPPORTED_LOGS:
> > +	case CXL_MBOX_OP_GET_LOG:
> > +	case CXL_MBOX_OP_IDENTIFY:
> > +	case CXL_MBOX_OP_GET_PARTITION_INFO:
> > +	case CXL_MBOX_OP_SET_PARTITION_INFO:
> > +	case CXL_MBOX_OP_GET_LSA:
> > +	case CXL_MBOX_OP_SET_LSA:
> > +	case CXL_MBOX_OP_GET_HEALTH_INFO:
> > +	case CXL_MBOX_OP_GET_ALERT_CONFIG:
> > +	case CXL_MBOX_OP_SET_ALERT_CONFIG:
> > +	case CXL_MBOX_OP_GET_SHUTDOWN_STATE:
> > +	case CXL_MBOX_OP_SET_SHUTDOWN_STATE:
> > +	case CXL_MBOX_OP_GET_POISON:
> > +	case CXL_MBOX_OP_INJECT_POISON:
> > +	case CXL_MBOX_OP_CLEAR_POISON:
> > +	case CXL_MBOX_OP_GET_SCAN_MEDIA_CAPS:
> > +	case CXL_MBOX_OP_SCAN_MEDIA:
> > +	case CXL_MBOX_OP_GET_SCAN_MEDIA:
> > +	case CXL_MBOX_OP_GET_SECURITY_STATE:
> > +	case CXL_MBOX_OP_SET_PASSPHRASE:
> > +	case CXL_MBOX_OP_DISABLE_PASSPHRASE:
> > +	case CXL_MBOX_OP_UNLOCK:
> > +	case CXL_MBOX_OP_FREEZE_SECURITY:
> > +	case CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE:
> > +		return true;
> > +	}
> > +	return false;
> > +}
> > +
> >  /**
> >   * cxl_walk_cel() - Walk through the Command Effects Log.
> >   * @cxlds: The device data for the operation
> > @@ -607,15 +641,13 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel)
> >  	for (i = 0; i < cel_entries; i++) {
> >  		u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> >  		struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
> > +		bool known = known_opcode(opcode);
> >  
> > -		if (!cmd) {
> > -			dev_dbg(cxlds->dev,
> > -				"Opcode 0x%04x unsupported by driver\n", opcode);
> > -			continue;
> > -		}
> > -
> > -		set_bit(cmd->info.id, cxlds->enabled_cmds);
> > -		dev_dbg(cxlds->dev, "Opcode 0x%04x enabled\n", opcode);
> > +		if (cmd)
> > +			set_bit(cmd->info.id, cxlds->enabled_cmds);
> > +		dev_dbg(cxlds->dev, "Opcode %#04x Enabled: %d User: %s\n",
> > +			opcode, !!known,
> > +			cmd ? cxl_command_names[cmd->info.id].name : "none");
> >  	}
> >  }
> >  
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index ab138004f644..21f97d631823 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -299,6 +299,7 @@ enum cxl_opcode {
> >  	CXL_MBOX_OP_FREEZE_SECURITY	= 0x4504,
> >  	CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE	= 0x4505,
> >  	CXL_MBOX_OP_MAX			= 0x10000
> > +	/* update known_opcode() when adding opcode support to this list */
> 
> I'm wishing there was a way to not have to do this.  All I can think of to
> make it easier would be to make 'driver_know_opcode()' static inline and
> in this header.  I'm sure some macro magic could be used too.  Although
> I'm not always a fan.

Yeah, a val_is_in_enum() helper seems like something someone would have
wrote...