| Message ID | 20230130151327.32415-1-Jonathan.Cameron@huawei.com |
|---|---|
| State | Accepted |
| Commit | fa8843451bec55f900b8673d9ddc0be02a61528a |
| Headers | show |
| Series | [v2] cxl/pci: Set the device timestamp | expand |
Jonathan Cameron wrote: > CXL r3.0 section 8.2.9.4.2 "Set Timestamp" recommends that the host sets > the timestamp after every Conventional or CXL Reset to ensure accurate > timestamps. This should include on initial boot up. The time base that > is being set is used by a device for the poison list overflow timestamp > and all event timestamps. Note that the command is optional and if > not supported and the device cannot return accurate timestamps it will > fill the fields in with an appropriate marker (see the specification > description of each timestamp). > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > v2: Thanks to DavidLohr, Alison and Dan for quick reviews. > - Drop exposure to userspace. > - Drop prevention of raw command based access. > - Check for a Not Supported return code. If that happens eat the error. > - Fix missing endian conversion. > - Switch to ktime.h include rather than timekeeping.h as per comments in > the headers. > > Based on cxl/pending as of today. > > Open question: Should we only do this if Linux has control of the > error handling? In theory it should be safe anyway given the > specification is clear that the timestamp base should always be the > same - so subject to small errors we shouldn't cause any firmware first > handling to get confused. I think it's safe for the reasons you give, but I will ask our platform firmware team what they think / give them a heads up that Linux is planning to do this unconditionally.
On Mon, Jan 30, 2023 at 03:13:27PM +0000, Jonathan Cameron wrote: > CXL r3.0 section 8.2.9.4.2 "Set Timestamp" recommends that the host sets > the timestamp after every Conventional or CXL Reset to ensure accurate > timestamps. This should include on initial boot up. The time base that > is being set is used by a device for the poison list overflow timestamp > and all event timestamps. Note that the command is optional and if > not supported and the device cannot return accurate timestamps it will > fill the fields in with an appropriate marker (see the specification > description of each timestamp). > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Hi Jonathan, Could you please educate me on the reason for not supporting a "Get Timestamp" mailbox command at this time? A use-case that I can think of for having "Get Timestamp" in the UAPI is to enable userspace to better correlate the timestamp in the device logs with the wall time by querying the current timestamp on the device. This could be useful if there's some inaccuracy or skew in the device timestamp. Would that be a valid use-case or am I misunderstanding something here? I can see why "Set Timestamp" is a kernel-internal command but fail to see why "Get Timestamp" cannot be in the UAPI. Please clarify. Thanks, Nabeel
On Wed, 22 Feb 2023 09:34:14 -0600 Nabeel M Mohamed <nabeelmd.linux@gmail.com> wrote: > On Mon, Jan 30, 2023 at 03:13:27PM +0000, Jonathan Cameron wrote: > > CXL r3.0 section 8.2.9.4.2 "Set Timestamp" recommends that the host sets > > the timestamp after every Conventional or CXL Reset to ensure accurate > > timestamps. This should include on initial boot up. The time base that > > is being set is used by a device for the poison list overflow timestamp > > and all event timestamps. Note that the command is optional and if > > not supported and the device cannot return accurate timestamps it will > > fill the fields in with an appropriate marker (see the specification > > description of each timestamp). > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Hi Jonathan, > > Could you please educate me on the reason for not supporting a > "Get Timestamp" mailbox command at this time? > > A use-case that I can think of for having "Get Timestamp" in the UAPI > is to enable userspace to better correlate the timestamp in the device > logs with the wall time by querying the current timestamp on the device. > This could be useful if there's some inaccuracy or skew in the device > timestamp. > > Would that be a valid use-case or am I misunderstanding something here? > > I can see why "Set Timestamp" is a kernel-internal command but fail to > see why "Get Timestamp" cannot be in the UAPI. Hi Nabeel, I'd be fine with a patch adding the support, it just didn't solve my immediate problem so I didn't want to make this patch more complex. Jonathan > > Please clarify. > > Thanks, > Nabeel
On Wed, Feb 22, 2023 at 04:27:00PM +0000, Jonathan Cameron wrote: > On Wed, 22 Feb 2023 09:34:14 -0600 > Nabeel M Mohamed <nabeelmd.linux@gmail.com> wrote: > > > On Mon, Jan 30, 2023 at 03:13:27PM +0000, Jonathan Cameron wrote: > > > CXL r3.0 section 8.2.9.4.2 "Set Timestamp" recommends that the host sets > > > the timestamp after every Conventional or CXL Reset to ensure accurate > > > timestamps. This should include on initial boot up. The time base that > > > is being set is used by a device for the poison list overflow timestamp > > > and all event timestamps. Note that the command is optional and if > > > not supported and the device cannot return accurate timestamps it will > > > fill the fields in with an appropriate marker (see the specification > > > description of each timestamp). > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > Hi Jonathan, > > > > Could you please educate me on the reason for not supporting a > > "Get Timestamp" mailbox command at this time? > > > > A use-case that I can think of for having "Get Timestamp" in the UAPI > > is to enable userspace to better correlate the timestamp in the device > > logs with the wall time by querying the current timestamp on the device. > > This could be useful if there's some inaccuracy or skew in the device > > timestamp. > > > > Would that be a valid use-case or am I misunderstanding something here? > > > > I can see why "Set Timestamp" is a kernel-internal command but fail to > > see why "Get Timestamp" cannot be in the UAPI. > > Hi Nabeel, > > I'd be fine with a patch adding the support, it just didn't solve my > immediate problem so I didn't want to make this patch more complex. > > Jonathan Great, thanks for clarifying Jonathan! I'll be happy to post a patch which adds support for "Get Timestamp". Btw, this will be my first patch to the CXL driver. So, I'm wondering if there's any checklist listing the prerequisites for submitting a patch to the CXL driver - for instance, requirements on adding new tests for coverage, any existing CXL regression suites that the patch must pass, etc. Any pointers on the above would be greatly appreciated! Thanks, Nabeel
On Wed, 22 Feb 2023 21:55:09 -0600 Nabeel M Mohamed <nabeelmd.linux@gmail.com> wrote: > On Wed, Feb 22, 2023 at 04:27:00PM +0000, Jonathan Cameron wrote: > > On Wed, 22 Feb 2023 09:34:14 -0600 > > Nabeel M Mohamed <nabeelmd.linux@gmail.com> wrote: > > > > > On Mon, Jan 30, 2023 at 03:13:27PM +0000, Jonathan Cameron wrote: > > > > CXL r3.0 section 8.2.9.4.2 "Set Timestamp" recommends that the host sets > > > > the timestamp after every Conventional or CXL Reset to ensure accurate > > > > timestamps. This should include on initial boot up. The time base that > > > > is being set is used by a device for the poison list overflow timestamp > > > > and all event timestamps. Note that the command is optional and if > > > > not supported and the device cannot return accurate timestamps it will > > > > fill the fields in with an appropriate marker (see the specification > > > > description of each timestamp). > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > > > Hi Jonathan, > > > > > > Could you please educate me on the reason for not supporting a > > > "Get Timestamp" mailbox command at this time? > > > > > > A use-case that I can think of for having "Get Timestamp" in the UAPI > > > is to enable userspace to better correlate the timestamp in the device > > > logs with the wall time by querying the current timestamp on the device. > > > This could be useful if there's some inaccuracy or skew in the device > > > timestamp. > > > > > > Would that be a valid use-case or am I misunderstanding something here? > > > > > > I can see why "Set Timestamp" is a kernel-internal command but fail to > > > see why "Get Timestamp" cannot be in the UAPI. > > > > > Hi Nabeel, > > > > I'd be fine with a patch adding the support, it just didn't solve my > > immediate problem so I didn't want to make this patch more complex. > > > > Jonathan > > Great, thanks for clarifying Jonathan! > > I'll be happy to post a patch which adds support for "Get Timestamp". > > Btw, this will be my first patch to the CXL driver. > So, I'm wondering if there's any checklist listing the prerequisites > for submitting a patch to the CXL driver - for instance, requirements > on adding new tests for coverage, any existing CXL regression suites > that the patch must pass, etc. > > Any pointers on the above would be greatly appreciated! As we are talking about a userspace exposed feature, support on the ndctl /cxl tools should probably follow soon after the kernel side of things. Jonathan > > Thanks, > Nabeel
On Thu, Feb 23, 2023 at 03:05:24PM +0000, Jonathan Cameron wrote: > On Wed, 22 Feb 2023 21:55:09 -0600 > Nabeel M Mohamed <nabeelmd.linux@gmail.com> wrote: > > > On Wed, Feb 22, 2023 at 04:27:00PM +0000, Jonathan Cameron wrote: > > > On Wed, 22 Feb 2023 09:34:14 -0600 > > > Nabeel M Mohamed <nabeelmd.linux@gmail.com> wrote: > > > > > > > On Mon, Jan 30, 2023 at 03:13:27PM +0000, Jonathan Cameron wrote: > > > > > CXL r3.0 section 8.2.9.4.2 "Set Timestamp" recommends that the host sets > > > > > the timestamp after every Conventional or CXL Reset to ensure accurate > > > > > timestamps. This should include on initial boot up. The time base that > > > > > is being set is used by a device for the poison list overflow timestamp > > > > > and all event timestamps. Note that the command is optional and if > > > > > not supported and the device cannot return accurate timestamps it will > > > > > fill the fields in with an appropriate marker (see the specification > > > > > description of each timestamp). > > > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > > > > > > Hi Jonathan, > > > > > > > > Could you please educate me on the reason for not supporting a > > > > "Get Timestamp" mailbox command at this time? > > > > > > > > A use-case that I can think of for having "Get Timestamp" in the UAPI > > > > is to enable userspace to better correlate the timestamp in the device > > > > logs with the wall time by querying the current timestamp on the device. > > > > This could be useful if there's some inaccuracy or skew in the device > > > > timestamp. > > > > > > > > Would that be a valid use-case or am I misunderstanding something here? > > > > > > > > I can see why "Set Timestamp" is a kernel-internal command but fail to > > > > see why "Get Timestamp" cannot be in the UAPI. > > > > > > > > Hi Nabeel, > > > > > > I'd be fine with a patch adding the support, it just didn't solve my > > > immediate problem so I didn't want to make this patch more complex. > > > > > > Jonathan > > > > Great, thanks for clarifying Jonathan! > > > > I'll be happy to post a patch which adds support for "Get Timestamp". > > > > Btw, this will be my first patch to the CXL driver. > > So, I'm wondering if there's any checklist listing the prerequisites > > for submitting a patch to the CXL driver - for instance, requirements > > on adding new tests for coverage, any existing CXL regression suites > > that the patch must pass, etc. > > > > Any pointers on the above would be greatly appreciated! > > As we are talking about a userspace exposed feature, support on the ndctl > /cxl tools should probably follow soon after the kernel side of things. > > Jonathan Noted, thanks!
On Wed, 22 Feb 2023, Nabeel M Mohamed wrote: >So, I'm wondering if there's any checklist listing the prerequisites >for submitting a patch to the CXL driver - for instance, requirements >on adding new tests for coverage, any existing CXL regression suites >that the patch must pass, etc. Btw we might want to support timestamps in the mock device, no? I am aware that qemu has both get and set. --<8----- [PATCH] cxl/test: Add mock test for set_timestamp Support the command testing in a unit-test fashion. Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- tools/testing/cxl/test/mem.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c index 9263b04d35f7..09511f2fbe54 100644 --- a/tools/testing/cxl/test/mem.c +++ b/tools/testing/cxl/test/mem.c @@ -98,6 +98,7 @@ struct cxl_mockmem_data { int master_limit; struct mock_event_store mes; u8 event_buf[SZ_4K]; + u64 timestamp; }; static struct mock_event_log *event_find_log(struct device *dev, int log_type) @@ -361,6 +362,21 @@ struct cxl_event_mem_module mem_module = { } }; +static int mock_set_timestamp(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd) +{ + struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev); + struct cxl_mbox_set_timestamp_in *ts = cmd->payload_in; + + if (cmd->size_in != sizeof(*ts)) + return -EINVAL; + + if (cmd->size_out != 0) + return -EINVAL; + + mdata->timestamp = le64_to_cpu(ts->timestamp); + return 0; +} + static void cxl_mock_add_event_logs(struct mock_event_store *mes) { put_unaligned_le16(CXL_GMER_VALID_CHANNEL | CXL_GMER_VALID_RANK, @@ -894,6 +910,9 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd * int rc = -EIO; switch (cmd->opcode) { + case CXL_MBOX_OP_SET_TIMESTAMP: + rc = mock_set_timestamp(cxlds, cmd); + break; case CXL_MBOX_OP_GET_SUPPORTED_LOGS: rc = mock_gsl(cmd); break; @@ -1010,6 +1029,10 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) if (rc) return rc; + rc = cxl_set_timestamp(cxlds); + if (rc) + return rc; + rc = cxl_dev_state_identify(cxlds); if (rc) return rc;
Davidlohr Bueso wrote: > On Wed, 22 Feb 2023, Nabeel M Mohamed wrote: > > >So, I'm wondering if there's any checklist listing the prerequisites > >for submitting a patch to the CXL driver - for instance, requirements > >on adding new tests for coverage, any existing CXL regression suites > >that the patch must pass, etc. > > Btw we might want to support timestamps in the mock device, no? I am > aware that qemu has both get and set. > > --<8----- > [PATCH] cxl/test: Add mock test for set_timestamp > > Support the command testing in a unit-test fashion. > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > --- > tools/testing/cxl/test/mem.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c > index 9263b04d35f7..09511f2fbe54 100644 > --- a/tools/testing/cxl/test/mem.c > +++ b/tools/testing/cxl/test/mem.c > @@ -98,6 +98,7 @@ struct cxl_mockmem_data { > int master_limit; > struct mock_event_store mes; > u8 event_buf[SZ_4K]; > + u64 timestamp; > }; > > static struct mock_event_log *event_find_log(struct device *dev, int log_type) > @@ -361,6 +362,21 @@ struct cxl_event_mem_module mem_module = { > } > }; > > +static int mock_set_timestamp(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd) > +{ > + struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev); > + struct cxl_mbox_set_timestamp_in *ts = cmd->payload_in; > + > + if (cmd->size_in != sizeof(*ts)) > + return -EINVAL; > + > + if (cmd->size_out != 0) > + return -EINVAL; > + > + mdata->timestamp = le64_to_cpu(ts->timestamp); > + return 0; > +} > + > static void cxl_mock_add_event_logs(struct mock_event_store *mes) > { > put_unaligned_le16(CXL_GMER_VALID_CHANNEL | CXL_GMER_VALID_RANK, > @@ -894,6 +910,9 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd * > int rc = -EIO; > > switch (cmd->opcode) { > + case CXL_MBOX_OP_SET_TIMESTAMP: > + rc = mock_set_timestamp(cxlds, cmd); > + break; > case CXL_MBOX_OP_GET_SUPPORTED_LOGS: > rc = mock_gsl(cmd); > break; > @@ -1010,6 +1029,10 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) > if (rc) > return rc; > > + rc = cxl_set_timestamp(cxlds); > + if (rc) > + return rc; > + > rc = cxl_dev_state_identify(cxlds); > if (rc) > return rc; > -- > 2.39.0 >
Davidlohr Bueso wrote: > On Wed, 22 Feb 2023, Nabeel M Mohamed wrote: > > >So, I'm wondering if there's any checklist listing the prerequisites > >for submitting a patch to the CXL driver - for instance, requirements > >on adding new tests for coverage, any existing CXL regression suites > >that the patch must pass, etc. > > Btw we might want to support timestamps in the mock device, no? I am > aware that qemu has both get and set. > > --<8----- > [PATCH] cxl/test: Add mock test for set_timestamp Mind resending this in its own thread? b4 can't grab this as it's a reply to another patch, and the above is not a valid scissors line. I tried manually fixing that up, but git am is still not happy: $ git am -c patch -s warning: Patch sent with format=flowed; space at the end of lines might be lost. Applying: cxl/test: Add mock test for set_timestamp error: corrupt patch at line 16 Patch failed at 0001 cxl/test: Add mock test for set_timestamp
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index b7c8d7608a33..a0bd2170af48 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -3,6 +3,7 @@ #include <linux/io-64-nonatomic-lo-hi.h> #include <linux/security.h> #include <linux/debugfs.h> +#include <linux/ktime.h> #include <linux/mutex.h> #include <cxlmem.h> #include <cxl.h> @@ -1074,6 +1075,32 @@ 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) +{ + struct cxl_mbox_cmd mbox_cmd; + struct cxl_mbox_set_timestamp_in pi; + int rc; + + pi.timestamp = cpu_to_le64(ktime_get_real_ns()); + mbox_cmd = (struct cxl_mbox_cmd) { + .opcode = CXL_MBOX_OP_SET_TIMESTAMP, + .size_in = sizeof(pi), + .payload_in = &pi, + }; + + rc = cxl_internal_send_cmd(cxlds, &mbox_cmd); + /* + * Command is optional. Devices may have another way of providing + * a timestamp, or may return all 0s in timestamp fields. + * Don't report an error if this command isn't supported + */ + if (rc && (mbox_cmd.return_code != CXL_MBOX_CMD_RC_UNSUPPORTED)) + return rc; + + return 0; +} +EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL); + struct cxl_dev_state *cxl_dev_state_create(struct device *dev) { struct cxl_dev_state *cxlds; diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index f21692ccb7a2..802b5b396daf 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -309,6 +309,7 @@ enum cxl_opcode { CXL_MBOX_OP_SET_EVT_INT_POLICY = 0x0103, CXL_MBOX_OP_GET_FW_INFO = 0x0200, CXL_MBOX_OP_ACTIVATE_FW = 0x0202, + CXL_MBOX_OP_SET_TIMESTAMP = 0x0301, CXL_MBOX_OP_GET_SUPPORTED_LOGS = 0x0400, CXL_MBOX_OP_GET_LOG = 0x0401, CXL_MBOX_OP_IDENTIFY = 0x4000, @@ -537,6 +538,12 @@ struct cxl_mbox_set_partition_info { #define CXL_SET_PARTITION_IMMEDIATE_FLAG BIT(0) +/* Set Timestamp CXL 3.0 Spec 8.2.9.4.2 */ +struct cxl_mbox_set_timestamp_in { + __le64 timestamp; + +} __packed; + /** * struct cxl_mem_command - Driver representation of a memory device command * @info: Command information as it exists for the UAPI @@ -607,6 +614,8 @@ struct cxl_dev_state *cxl_dev_state_create(struct device *dev); void set_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds); void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cmds); void cxl_mem_get_event_records(struct cxl_dev_state *cxlds, u32 status); +int cxl_set_timestamp(struct cxl_dev_state *cxlds); + #ifdef CONFIG_CXL_SUSPEND void cxl_mem_active_inc(void); void cxl_mem_active_dec(void); diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index d47005223c4f..ad2ebe7bfaeb 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -708,6 +708,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (rc) return rc; + rc = cxl_set_timestamp(cxlds); + if (rc) + return rc; + rc = cxl_dev_state_identify(cxlds); if (rc) return rc;
CXL r3.0 section 8.2.9.4.2 "Set Timestamp" recommends that the host sets the timestamp after every Conventional or CXL Reset to ensure accurate timestamps. This should include on initial boot up. The time base that is being set is used by a device for the poison list overflow timestamp and all event timestamps. Note that the command is optional and if not supported and the device cannot return accurate timestamps it will fill the fields in with an appropriate marker (see the specification description of each timestamp). Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- v2: Thanks to DavidLohr, Alison and Dan for quick reviews. - Drop exposure to userspace. - Drop prevention of raw command based access. - Check for a Not Supported return code. If that happens eat the error. - Fix missing endian conversion. - Switch to ktime.h include rather than timekeeping.h as per comments in the headers. Based on cxl/pending as of today. Open question: Should we only do this if Linux has control of the error handling? In theory it should be safe anyway given the specification is clear that the timestamp base should always be the same - so subject to small errors we shouldn't cause any firmware first handling to get confused. --- drivers/cxl/core/mbox.c | 27 +++++++++++++++++++++++++++ drivers/cxl/cxlmem.h | 9 +++++++++ drivers/cxl/pci.c | 4 ++++ 3 files changed, 40 insertions(+) base-commit: ace4b76d4ab70335cffb5a1c8450d3b34c16771a