[PATCH 0/2] hw/cxl: CXL Event processing fixups

[PATCH 0/2] hw/cxl: CXL Event processing fixups

[Ira Weiny]

During review of the CXL Event processing series[1] these minor fixes were caught
but I did not have time to respin before Jonathan picked them up.

Make the fixes now.

These are based on Jonathan's latest branch:

        https://gitlab.com/jic23/qemu/-/tree/cxl-2023-01-20

[1] https://lore.kernel.org/all/20221221-ira-cxl-events-2022-11-17-v2-0-2ce2ecc06219@intel.com/

To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Michael Tsirkin <mst@redhat.com>
Cc: Ben Widawsky <bwidawsk@kernel.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: <qemu-devel@nongnu.org>
Cc: <linux-cxl@vger.kernel.org>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Ira Weiny (2):
      hw/cxl: Fix event log time stamp fields
      hw/cxl: Remove check for g_new0() failure

 hw/cxl/cxl-device-utils.c   | 15 +++++++++++++++
 hw/cxl/cxl-events.c         | 10 +++-------
 hw/cxl/cxl-mailbox-utils.c  | 11 +----------
 hw/mem/cxl_type3.c          |  1 -
 include/hw/cxl/cxl_device.h |  2 ++
 5 files changed, 21 insertions(+), 18 deletions(-)
---
base-commit: bb3f9b2853f9723c11a38c6b7bca7368677f2b43
change-id: 20230125-ira-cxl-events-fixups-2023-01-11-337953e87f5d

Best regards,
-- 
Ira Weiny <ira.weiny@intel.com>

[PATCH 1/2] hw/cxl: Fix event log time stamp fields

[Ira Weiny]

CXL 3.0 8.2.9.4.2 Set Timestamp and 8.2.9.4.1 Get Timestamp define the
way for software to set and get the time stamp of a device.  Events
should use a time stamp consistent with the Get Timestamp mailbox
command.

In addition avoid setting the time stamp twice.

Fixes: fb64c5661d5f ("hw/cxl/events: Wire up get/clear event mailbox commands")
Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 hw/cxl/cxl-device-utils.c   | 15 +++++++++++++++
 hw/cxl/cxl-events.c         |  4 +++-
 hw/cxl/cxl-mailbox-utils.c  | 11 +----------
 hw/mem/cxl_type3.c          |  1 -
 include/hw/cxl/cxl_device.h |  2 ++
 5 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index 7f29d40be04a..5876a3703e85 100644
--- a/hw/cxl/cxl-device-utils.c
+++ b/hw/cxl/cxl-device-utils.c
@@ -325,3 +325,18 @@ void cxl_device_register_init_swcci(CXLDeviceState *cxl_dstate)
 
     cxl_initialize_mailbox(cxl_dstate, true);
 }
+
+uint64_t cxl_device_get_timestamp(CXLDeviceState *cxl_dstate)
+{
+    uint64_t time, delta;
+    uint64_t final_time = 0;
+
+    if (cxl_dstate->timestamp.set) {
+        /* First find the delta from the last time the host set the time. */
+        time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        delta = time - cxl_dstate->timestamp.last_set;
+        final_time = cxl_dstate->timestamp.host_set + delta;
+    }
+
+    return final_time;
+}
diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
index 08fd52b66188..2536aafc55fb 100644
--- a/hw/cxl/cxl-events.c
+++ b/hw/cxl/cxl-events.c
@@ -100,7 +100,7 @@ bool cxl_event_insert(CXLDeviceState *cxlds,
                       enum cxl_event_log_type log_type,
                       struct cxl_event_record_raw *event)
 {
-    uint64_t time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    uint64_t time;
     struct cxl_event_log *log;
     CXLEvent *entry;
 
@@ -108,6 +108,8 @@ bool cxl_event_insert(CXLDeviceState *cxlds,
         return false;
     }
 
+    time = cxl_device_get_timestamp(cxlds);
+
     log = &cxlds->event_logs[log_type];
 
     QEMU_LOCK_GUARD(&log->lock);
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 75703023434b..0e64873c2395 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -394,17 +394,8 @@ static CXLRetCode cmd_timestamp_get(struct cxl_cmd *cmd,
                                     CXLDeviceState *cxl_dstate,
                                     uint16_t *len)
 {
-    uint64_t time, delta;
-    uint64_t final_time = 0;
-
-    if (cxl_dstate->timestamp.set) {
-        /* First find the delta from the last time the host set the time. */
-        time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        delta = time - cxl_dstate->timestamp.last_set;
-        final_time = cxl_dstate->timestamp.host_set + delta;
-    }
+    uint64_t final_time = cxl_device_get_timestamp(cxl_dstate);
 
-    /* Then adjust the actual time */
     stq_le_p(cmd->payload, final_time);
     *len = 8;
 
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index a7b587780af2..42e291dd9f76 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1291,7 +1291,6 @@ static void cxl_assign_event_header(struct cxl_event_record_hdr *hdr,
     hdr->flags[0] = flags;
     hdr->length = length;
     memcpy(&hdr->id, uuid, sizeof(hdr->id));
-    hdr->timestamp = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 }
 
 static const QemuUUID gen_media_uuid = {
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index cbb37c541c44..31579af342f1 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -426,4 +426,6 @@ CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
 
 void cxl_event_irq_assert(CXLType3Dev *ct3d);
 
+uint64_t cxl_device_get_timestamp(CXLDeviceState *cxlds);
+
 #endif

-- 
2.39.1

[PATCH 2/2] hw/cxl: Remove check for g_new0() failure

[Ira Weiny]

g_new0() will terminate the application if it fails.  Remove the check.

Fixes: fb64c5661d5f ("hw/cxl/events: Wire up get/clear event mailbox commands")
Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 hw/cxl/cxl-events.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
index 2536aafc55fb..4cbc838e8ff4 100644
--- a/hw/cxl/cxl-events.c
+++ b/hw/cxl/cxl-events.c
@@ -124,13 +124,7 @@ bool cxl_event_insert(CXLDeviceState *cxlds,
     }
 
     entry = g_new0(CXLEvent, 1);
-    if (!entry) {
-        error_report("Failed to allocate memory for event log entry");
-        return false;
-    }
-
     memcpy(&entry->data, event, sizeof(*event));
-
     entry->data.hdr.handle = cpu_to_le16(log->next_handle);
     log->next_handle++;
     /* 0 handle is never valid */

-- 
2.39.1

Re: [PATCH 1/2] hw/cxl: Fix event log time stamp fields

[Jonathan Cameron]

On Wed, 25 Jan 2023 21:37:27 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> CXL 3.0 8.2.9.4.2 Set Timestamp and 8.2.9.4.1 Get Timestamp define the
> way for software to set and get the time stamp of a device.  Events
> should use a time stamp consistent with the Get Timestamp mailbox
> command.
> 
> In addition avoid setting the time stamp twice.
> 
> Fixes: fb64c5661d5f ("hw/cxl/events: Wire up get/clear event mailbox commands")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Hi Ira,

I'm going to split this patch as I am carrying a very similar
utility function for an updated version of the poison list code
and I'm not sure what order everything will go upstream in.

So I'll split this into:
1) Patch that adds cxl_device_get_timestamp() - adding the
use of this in the GET_TIMESTAMP mailbox command.

2) Changes pushed down into the patch you mention above.

Given all the code is yours, just split up, I'll keep the SOB.
Shout if you mind me doing that.

Thanks,

Jonathan

> ---
>  hw/cxl/cxl-device-utils.c   | 15 +++++++++++++++
>  hw/cxl/cxl-events.c         |  4 +++-
>  hw/cxl/cxl-mailbox-utils.c  | 11 +----------
>  hw/mem/cxl_type3.c          |  1 -
>  include/hw/cxl/cxl_device.h |  2 ++
>  5 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index 7f29d40be04a..5876a3703e85 100644
> --- a/hw/cxl/cxl-device-utils.c
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -325,3 +325,18 @@ void cxl_device_register_init_swcci(CXLDeviceState *cxl_dstate)
>  
>      cxl_initialize_mailbox(cxl_dstate, true);
>  }
> +
> +uint64_t cxl_device_get_timestamp(CXLDeviceState *cxl_dstate)
> +{
> +    uint64_t time, delta;
> +    uint64_t final_time = 0;
> +
> +    if (cxl_dstate->timestamp.set) {
> +        /* First find the delta from the last time the host set the time. */
> +        time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        delta = time - cxl_dstate->timestamp.last_set;
> +        final_time = cxl_dstate->timestamp.host_set + delta;
> +    }
> +
> +    return final_time;
> +}
> diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> index 08fd52b66188..2536aafc55fb 100644
> --- a/hw/cxl/cxl-events.c
> +++ b/hw/cxl/cxl-events.c
> @@ -100,7 +100,7 @@ bool cxl_event_insert(CXLDeviceState *cxlds,
>                        enum cxl_event_log_type log_type,
>                        struct cxl_event_record_raw *event)
>  {
> -    uint64_t time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    uint64_t time;
>      struct cxl_event_log *log;
>      CXLEvent *entry;
>  
> @@ -108,6 +108,8 @@ bool cxl_event_insert(CXLDeviceState *cxlds,
>          return false;
>      }
>  
> +    time = cxl_device_get_timestamp(cxlds);
> +
>      log = &cxlds->event_logs[log_type];
>  
>      QEMU_LOCK_GUARD(&log->lock);
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 75703023434b..0e64873c2395 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -394,17 +394,8 @@ static CXLRetCode cmd_timestamp_get(struct cxl_cmd *cmd,
>                                      CXLDeviceState *cxl_dstate,
>                                      uint16_t *len)
>  {
> -    uint64_t time, delta;
> -    uint64_t final_time = 0;
> -
> -    if (cxl_dstate->timestamp.set) {
> -        /* First find the delta from the last time the host set the time. */
> -        time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -        delta = time - cxl_dstate->timestamp.last_set;
> -        final_time = cxl_dstate->timestamp.host_set + delta;
> -    }
> +    uint64_t final_time = cxl_device_get_timestamp(cxl_dstate);
>  
> -    /* Then adjust the actual time */
>      stq_le_p(cmd->payload, final_time);
>      *len = 8;
>  
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index a7b587780af2..42e291dd9f76 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1291,7 +1291,6 @@ static void cxl_assign_event_header(struct cxl_event_record_hdr *hdr,
>      hdr->flags[0] = flags;
>      hdr->length = length;
>      memcpy(&hdr->id, uuid, sizeof(hdr->id));
> -    hdr->timestamp = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>  }
>  
>  static const QemuUUID gen_media_uuid = {
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index cbb37c541c44..31579af342f1 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -426,4 +426,6 @@ CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
>  
>  void cxl_event_irq_assert(CXLType3Dev *ct3d);
>  
> +uint64_t cxl_device_get_timestamp(CXLDeviceState *cxlds);
> +
>  #endif
> 

Re: [PATCH 1/2] hw/cxl: Fix event log time stamp fields

[Jonathan Cameron via]

On Wed, 25 Jan 2023 21:37:27 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> CXL 3.0 8.2.9.4.2 Set Timestamp and 8.2.9.4.1 Get Timestamp define the
> way for software to set and get the time stamp of a device.  Events
> should use a time stamp consistent with the Get Timestamp mailbox
> command.
> 
> In addition avoid setting the time stamp twice.
> 
> Fixes: fb64c5661d5f ("hw/cxl/events: Wire up get/clear event mailbox commands")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Hi Ira,

I'm going to split this patch as I am carrying a very similar
utility function for an updated version of the poison list code
and I'm not sure what order everything will go upstream in.

So I'll split this into:
1) Patch that adds cxl_device_get_timestamp() - adding the
use of this in the GET_TIMESTAMP mailbox command.

2) Changes pushed down into the patch you mention above.

Given all the code is yours, just split up, I'll keep the SOB.
Shout if you mind me doing that.

Thanks,

Jonathan

> ---
>  hw/cxl/cxl-device-utils.c   | 15 +++++++++++++++
>  hw/cxl/cxl-events.c         |  4 +++-
>  hw/cxl/cxl-mailbox-utils.c  | 11 +----------
>  hw/mem/cxl_type3.c          |  1 -
>  include/hw/cxl/cxl_device.h |  2 ++
>  5 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index 7f29d40be04a..5876a3703e85 100644
> --- a/hw/cxl/cxl-device-utils.c
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -325,3 +325,18 @@ void cxl_device_register_init_swcci(CXLDeviceState *cxl_dstate)
>  
>      cxl_initialize_mailbox(cxl_dstate, true);
>  }
> +
> +uint64_t cxl_device_get_timestamp(CXLDeviceState *cxl_dstate)
> +{
> +    uint64_t time, delta;
> +    uint64_t final_time = 0;
> +
> +    if (cxl_dstate->timestamp.set) {
> +        /* First find the delta from the last time the host set the time. */
> +        time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        delta = time - cxl_dstate->timestamp.last_set;
> +        final_time = cxl_dstate->timestamp.host_set + delta;
> +    }
> +
> +    return final_time;
> +}
> diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> index 08fd52b66188..2536aafc55fb 100644
> --- a/hw/cxl/cxl-events.c
> +++ b/hw/cxl/cxl-events.c
> @@ -100,7 +100,7 @@ bool cxl_event_insert(CXLDeviceState *cxlds,
>                        enum cxl_event_log_type log_type,
>                        struct cxl_event_record_raw *event)
>  {
> -    uint64_t time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    uint64_t time;
>      struct cxl_event_log *log;
>      CXLEvent *entry;
>  
> @@ -108,6 +108,8 @@ bool cxl_event_insert(CXLDeviceState *cxlds,
>          return false;
>      }
>  
> +    time = cxl_device_get_timestamp(cxlds);
> +
>      log = &cxlds->event_logs[log_type];
>  
>      QEMU_LOCK_GUARD(&log->lock);
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 75703023434b..0e64873c2395 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -394,17 +394,8 @@ static CXLRetCode cmd_timestamp_get(struct cxl_cmd *cmd,
>                                      CXLDeviceState *cxl_dstate,
>                                      uint16_t *len)
>  {
> -    uint64_t time, delta;
> -    uint64_t final_time = 0;
> -
> -    if (cxl_dstate->timestamp.set) {
> -        /* First find the delta from the last time the host set the time. */
> -        time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -        delta = time - cxl_dstate->timestamp.last_set;
> -        final_time = cxl_dstate->timestamp.host_set + delta;
> -    }
> +    uint64_t final_time = cxl_device_get_timestamp(cxl_dstate);
>  
> -    /* Then adjust the actual time */
>      stq_le_p(cmd->payload, final_time);
>      *len = 8;
>  
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index a7b587780af2..42e291dd9f76 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1291,7 +1291,6 @@ static void cxl_assign_event_header(struct cxl_event_record_hdr *hdr,
>      hdr->flags[0] = flags;
>      hdr->length = length;
>      memcpy(&hdr->id, uuid, sizeof(hdr->id));
> -    hdr->timestamp = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>  }
>  
>  static const QemuUUID gen_media_uuid = {
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index cbb37c541c44..31579af342f1 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -426,4 +426,6 @@ CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
>  
>  void cxl_event_irq_assert(CXLType3Dev *ct3d);
>  
> +uint64_t cxl_device_get_timestamp(CXLDeviceState *cxlds);
> +
>  #endif
> 

Re: [PATCH 2/2] hw/cxl: Remove check for g_new0() failure

[Jonathan Cameron]

On Wed, 25 Jan 2023 21:37:28 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> g_new0() will terminate the application if it fails.  Remove the check.
> 
> Fixes: fb64c5661d5f ("hw/cxl/events: Wire up get/clear event mailbox commands")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Squashed into original patch.  Thanks,

Jonathan

> ---
>  hw/cxl/cxl-events.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> index 2536aafc55fb..4cbc838e8ff4 100644
> --- a/hw/cxl/cxl-events.c
> +++ b/hw/cxl/cxl-events.c
> @@ -124,13 +124,7 @@ bool cxl_event_insert(CXLDeviceState *cxlds,
>      }
>  
>      entry = g_new0(CXLEvent, 1);
> -    if (!entry) {
> -        error_report("Failed to allocate memory for event log entry");
> -        return false;
> -    }
> -
>      memcpy(&entry->data, event, sizeof(*event));
> -
>      entry->data.hdr.handle = cpu_to_le16(log->next_handle);
>      log->next_handle++;
>      /* 0 handle is never valid */
> 

Re: [PATCH 2/2] hw/cxl: Remove check for g_new0() failure

[Jonathan Cameron via]

On Wed, 25 Jan 2023 21:37:28 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> g_new0() will terminate the application if it fails.  Remove the check.
> 
> Fixes: fb64c5661d5f ("hw/cxl/events: Wire up get/clear event mailbox commands")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Squashed into original patch.  Thanks,

Jonathan

> ---
>  hw/cxl/cxl-events.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> index 2536aafc55fb..4cbc838e8ff4 100644
> --- a/hw/cxl/cxl-events.c
> +++ b/hw/cxl/cxl-events.c
> @@ -124,13 +124,7 @@ bool cxl_event_insert(CXLDeviceState *cxlds,
>      }
>  
>      entry = g_new0(CXLEvent, 1);
> -    if (!entry) {
> -        error_report("Failed to allocate memory for event log entry");
> -        return false;
> -    }
> -
>      memcpy(&entry->data, event, sizeof(*event));
> -
>      entry->data.hdr.handle = cpu_to_le16(log->next_handle);
>      log->next_handle++;
>      /* 0 handle is never valid */
> 

Re: [PATCH 1/2] hw/cxl: Fix event log time stamp fields

[Ira Weiny]

Jonathan Cameron wrote:
> On Wed, 25 Jan 2023 21:37:27 -0800
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > CXL 3.0 8.2.9.4.2 Set Timestamp and 8.2.9.4.1 Get Timestamp define the
> > way for software to set and get the time stamp of a device.  Events
> > should use a time stamp consistent with the Get Timestamp mailbox
> > command.
> > 
> > In addition avoid setting the time stamp twice.
> > 
> > Fixes: fb64c5661d5f ("hw/cxl/events: Wire up get/clear event mailbox commands")
> > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Hi Ira,
> 
> I'm going to split this patch as I am carrying a very similar
> utility function for an updated version of the poison list code
> and I'm not sure what order everything will go upstream in.
> 
> So I'll split this into:
> 1) Patch that adds cxl_device_get_timestamp() - adding the
> use of this in the GET_TIMESTAMP mailbox command.
> 
> 2) Changes pushed down into the patch you mention above.
> 
> Given all the code is yours, just split up, I'll keep the SOB.
> Shout if you mind me doing that.

That sounds great.  I'm just sorry I did not get to all this sooner.

Thanks!
Ira