qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support
@ 2023-04-23 16:51 Jonathan Cameron via
  2023-04-23 16:51 ` [PATCH v5 1/7] hw/cxl/events: Add event status register Jonathan Cameron via
                   ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-04-23 16:51 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin, Fan Ni
  Cc: linux-cxl, linuxarm, Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
	Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
	Marc-André Lureau, Thomas Huth

v5: Rebase including version number updates to 8.1

Depends on 
[PATCH 0/2] hw/cxl: CDAT file handling fixes.
[PATCH v2 0/3] hw/cxl: Fix decoder commit and uncommit handling
[PATCH 0/3] docs/cxl: Gathering of fixes for 8.0 CXL docs.
[PATCH v5 0/3] hw/mem: CXL Type-3 Volatile Memory Support
[PATCH v5 0/6] hw/cxl: Poison get, inject, clear

Based on: Message-ID: 20230421132020.7408-1-Jonathan.Cameron@huawei.com
Based on: Message-ID: 20230421135906.3515-1-Jonathan.Cameron@huawei.com
Based on: Message-ID: 20230421134507.26842-1-Jonathan.Cameron@huawei.com
Based on: Message-ID: 20230421160827.2227-1-Jonathan.Cameron@huawei.com
Based on: Message-ID: 20230423162013.4535-1-Jonathan.Cameron@huawei.com

Cover letter from earlier version

One challenge here is striking the right balance between lots of constraints
in the injection code to enforce particular reserved bits etc by breaking
out all the flags as individual parameters vs having a reasonably concise
API.  I think this set strikes the right balance but others may well
disagree :)   Note that Ira raised the question of whether we should be
automatically establishing the volatile flag based on the Device Physical
Address of the injected error. My proposal is to not do so for now, but
to possibly revisit tightening the checking of injected errors in future.
Whilst the volatile flag is straight forwards, some of the other flags that
could be automatically set (or perhaps checked for validiaty) are much more
complex. Adding verification at this stage would greatly increase the
complexity of the patch + we are missing other elements that would interact
with this.  I'm not concerned about potential breaking of backwards compatibility
if it only related to the injection of errors that make no sense for a real
device.

CXL Event records inform the OS of various CXL device events.  Thus far CXL
memory devices are emulated and therefore don't naturally generate events.

Add an event infrastructure and mock event injection.  Previous versions
included a bulk insertion of lots of events.  However, this series focuses on
providing the ability to inject individual events through QMP.  Only the
General Media Event is included in this series as an example.  Other events can
be added pretty easily once the infrastructure is acceptable.

In addition, this version updates the code to be in line with the
specification based on discussions around the kernel patches.

Injection examples;

{ "execute": "cxl-inject-gen-media-event",
    "arguments": {
        "path": "/machine/peripheral/cxl-mem0",
        "log": "informational",
        "flags": 1,
        "physaddr": 1000,
        "descriptor": 3,
        "type": 3,
        "transaction-type": 192,
        "channel": 3,
        "device": 5,
        "component-id": "iras mem"
    }}


{ "execute": "cxl-inject-dram-event",
    "arguments": {
        "path": "/machine/peripheral/cxl-mem0",
        "log": "informational",
        "flags": 1,
        "physaddr": 1000,
        "descriptor": 3,
        "type": 3,
        "transaction-type": 192,
        "channel": 3,
        "rank": 17,
        "nibble-mask": 37421234,
        "bank-group": 7,
        "bank": 11,
        "row": 2,
        "column": 77,
        "correction-mask": [33, 44, 55, 66]
    }}

{ "execute": "cxl-inject-memory-module-event",
  "arguments": {
    "path": "/machine/peripheral/cxl-mem0",
    "log": "informational",
    "flags": 1,
    "type": 3,
    "health-status": 3,
    "media-status": 7,
    "additional-status": 33,
    "life-used": 30,
    "temperature": -15,
    "dirty-shutdown-count": 4,
    "corrected-volatile-error-count": 3233,
    "corrected-persistent-error-count": 1300
  }}

Ira Weiny (4):
  hw/cxl/events: Add event status register
  hw/cxl/events: Wire up get/clear event mailbox commands
  hw/cxl/events: Add event interrupt support
  hw/cxl/events: Add injection of General Media Events

Jonathan Cameron (3):
  hw/cxl: Move CXLRetCode definition to cxl_device.h
  hw/cxl/events: Add injection of DRAM events
  hw/cxl/events: Add injection of Memory Module Events

 hw/cxl/cxl-device-utils.c   |  43 +++++-
 hw/cxl/cxl-events.c         | 248 ++++++++++++++++++++++++++++++
 hw/cxl/cxl-mailbox-utils.c  | 166 ++++++++++++++------
 hw/cxl/meson.build          |   1 +
 hw/mem/cxl_type3.c          | 292 +++++++++++++++++++++++++++++++++++-
 hw/mem/cxl_type3_stubs.c    |  35 +++++
 include/hw/cxl/cxl_device.h |  80 +++++++++-
 include/hw/cxl/cxl_events.h | 168 +++++++++++++++++++++
 qapi/cxl.json               | 120 +++++++++++++++
 9 files changed, 1097 insertions(+), 56 deletions(-)
 create mode 100644 hw/cxl/cxl-events.c
 create mode 100644 include/hw/cxl/cxl_events.h

-- 
2.37.2



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v5 1/7] hw/cxl/events: Add event status register
  2023-04-23 16:51 [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
@ 2023-04-23 16:51 ` Jonathan Cameron via
  2023-04-23 16:51 ` [PATCH v5 2/7] hw/cxl: Move CXLRetCode definition to cxl_device.h Jonathan Cameron via
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-04-23 16:51 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin, Fan Ni
  Cc: linux-cxl, linuxarm, Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
	Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
	Marc-André Lureau, Thomas Huth

From: Ira Weiny <ira.weiny@intel.com>

The device status register block was defined.  However, there were no
individual registers nor any data wired up.

Define the event status register [CXL 3.0; 8.2.8.3.1] as part of the
device status register block.  Wire up the register and initialize the
event status for each log.

To support CXL 3.0 the version of the device status register block needs
to be 2.  Change the macro to allow for setting the version.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v5: Rebase.
---
 hw/cxl/cxl-device-utils.c   | 43 ++++++++++++++++++++++++++++++++-----
 include/hw/cxl/cxl_device.h | 23 +++++++++++++++++---
 include/hw/cxl/cxl_events.h | 28 ++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 8 deletions(-)

diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index 86e1cea8ce..517f06d869 100644
--- a/hw/cxl/cxl-device-utils.c
+++ b/hw/cxl/cxl-device-utils.c
@@ -41,7 +41,20 @@ static uint64_t caps_reg_read(void *opaque, hwaddr offset, unsigned size)
 
 static uint64_t dev_reg_read(void *opaque, hwaddr offset, unsigned size)
 {
-    return 0;
+    CXLDeviceState *cxl_dstate = opaque;
+
+    switch (size) {
+    case 1:
+        return cxl_dstate->dev_reg_state[offset];
+    case 2:
+        return cxl_dstate->dev_reg_state16[offset / size];
+    case 4:
+        return cxl_dstate->dev_reg_state32[offset / size];
+    case 8:
+        return cxl_dstate->dev_reg_state64[offset / size];
+    default:
+        g_assert_not_reached();
+    }
 }
 
 static uint64_t mailbox_reg_read(void *opaque, hwaddr offset, unsigned size)
@@ -236,7 +249,27 @@ void cxl_device_register_block_init(Object *obj, CXLDeviceState *cxl_dstate)
                                 &cxl_dstate->memory_device);
 }
 
-static void device_reg_init_common(CXLDeviceState *cxl_dstate) { }
+void cxl_event_set_status(CXLDeviceState *cxl_dstate, CXLEventLogType log_type,
+                          bool available)
+{
+    if (available) {
+        cxl_dstate->event_status |= (1 << log_type);
+    } else {
+        cxl_dstate->event_status &= ~(1 << log_type);
+    }
+
+    ARRAY_FIELD_DP64(cxl_dstate->dev_reg_state64, CXL_DEV_EVENT_STATUS,
+                     EVENT_STATUS, cxl_dstate->event_status);
+}
+
+static void device_reg_init_common(CXLDeviceState *cxl_dstate)
+{
+    CXLEventLogType log;
+
+    for (log = 0; log < CXL_EVENT_TYPE_MAX; log++) {
+        cxl_event_set_status(cxl_dstate, log, false);
+    }
+}
 
 static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
 {
@@ -258,13 +291,13 @@ void cxl_device_register_init_common(CXLDeviceState *cxl_dstate)
     ARRAY_FIELD_DP64(cap_hdrs, CXL_DEV_CAP_ARRAY, CAP_VERSION, 1);
     ARRAY_FIELD_DP64(cap_hdrs, CXL_DEV_CAP_ARRAY, CAP_COUNT, cap_count);
 
-    cxl_device_cap_init(cxl_dstate, DEVICE_STATUS, 1);
+    cxl_device_cap_init(cxl_dstate, DEVICE_STATUS, 1, 2);
     device_reg_init_common(cxl_dstate);
 
-    cxl_device_cap_init(cxl_dstate, MAILBOX, 2);
+    cxl_device_cap_init(cxl_dstate, MAILBOX, 2, 1);
     mailbox_reg_init_common(cxl_dstate);
 
-    cxl_device_cap_init(cxl_dstate, MEMORY_DEVICE, 0x4000);
+    cxl_device_cap_init(cxl_dstate, MEMORY_DEVICE, 0x4000, 1);
     memdev_reg_init_common(cxl_dstate);
 
     cxl_initialize_mailbox(cxl_dstate);
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 73328a52cf..16993f7098 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -13,6 +13,7 @@
 #include "hw/cxl/cxl_component.h"
 #include "hw/pci/pci_device.h"
 #include "hw/register.h"
+#include "hw/cxl/cxl_events.h"
 
 /*
  * The following is how a CXL device's Memory Device registers are laid out.
@@ -86,7 +87,16 @@ typedef struct cxl_device_state {
     MemoryRegion device_registers;
 
     /* mmio for device capabilities array - 8.2.8.2 */
-    MemoryRegion device;
+    struct {
+        MemoryRegion device;
+        union {
+            uint8_t dev_reg_state[CXL_DEVICE_STATUS_REGISTERS_LENGTH];
+            uint16_t dev_reg_state16[CXL_DEVICE_STATUS_REGISTERS_LENGTH / 2];
+            uint32_t dev_reg_state32[CXL_DEVICE_STATUS_REGISTERS_LENGTH / 4];
+            uint64_t dev_reg_state64[CXL_DEVICE_STATUS_REGISTERS_LENGTH / 8];
+        };
+        uint64_t event_status;
+    };
     MemoryRegion memory_device;
     struct {
         MemoryRegion caps;
@@ -141,6 +151,9 @@ REG64(CXL_DEV_CAP_ARRAY, 0) /* Documented as 128 bit register but 64 byte access
     FIELD(CXL_DEV_CAP_ARRAY, CAP_VERSION, 16, 8)
     FIELD(CXL_DEV_CAP_ARRAY, CAP_COUNT, 32, 16)
 
+void cxl_event_set_status(CXLDeviceState *cxl_dstate, CXLEventLogType log_type,
+                          bool available);
+
 /*
  * Helper macro to initialize capability headers for CXL devices.
  *
@@ -175,7 +188,7 @@ CXL_DEVICE_CAPABILITY_HEADER_REGISTER(MEMORY_DEVICE,
 void cxl_initialize_mailbox(CXLDeviceState *cxl_dstate);
 void cxl_process_mailbox(CXLDeviceState *cxl_dstate);
 
-#define cxl_device_cap_init(dstate, reg, cap_id)                           \
+#define cxl_device_cap_init(dstate, reg, cap_id, ver)                      \
     do {                                                                   \
         uint32_t *cap_hdrs = dstate->caps_reg_state32;                     \
         int which = R_CXL_DEV_##reg##_CAP_HDR0;                            \
@@ -183,7 +196,7 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate);
             FIELD_DP32(cap_hdrs[which], CXL_DEV_##reg##_CAP_HDR0,          \
                        CAP_ID, cap_id);                                    \
         cap_hdrs[which] = FIELD_DP32(                                      \
-            cap_hdrs[which], CXL_DEV_##reg##_CAP_HDR0, CAP_VERSION, 1);    \
+            cap_hdrs[which], CXL_DEV_##reg##_CAP_HDR0, CAP_VERSION, ver);  \
         cap_hdrs[which + 1] =                                              \
             FIELD_DP32(cap_hdrs[which + 1], CXL_DEV_##reg##_CAP_HDR1,      \
                        CAP_OFFSET, CXL_##reg##_REGISTERS_OFFSET);          \
@@ -192,6 +205,10 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate);
                        CAP_LENGTH, CXL_##reg##_REGISTERS_LENGTH);          \
     } while (0)
 
+/* CXL 3.0 8.2.8.3.1 Event Status Register */
+REG64(CXL_DEV_EVENT_STATUS, 0)
+    FIELD(CXL_DEV_EVENT_STATUS, EVENT_STATUS, 0, 32)
+
 /* CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register */
 REG32(CXL_DEV_MAILBOX_CAP, 0)
     FIELD(CXL_DEV_MAILBOX_CAP, PAYLOAD_SIZE, 0, 5)
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
new file mode 100644
index 0000000000..aeb3b0590e
--- /dev/null
+++ b/include/hw/cxl/cxl_events.h
@@ -0,0 +1,28 @@
+/*
+ * QEMU CXL Events
+ *
+ * Copyright (c) 2022 Intel
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See the
+ * COPYING file in the top-level directory.
+ */
+
+#ifndef CXL_EVENTS_H
+#define CXL_EVENTS_H
+
+/*
+ * CXL rev 3.0 section 8.2.9.2.2; Table 8-49
+ *
+ * Define these as the bit position for the event status register for ease of
+ * setting the status.
+ */
+typedef enum CXLEventLogType {
+    CXL_EVENT_TYPE_INFO          = 0,
+    CXL_EVENT_TYPE_WARN          = 1,
+    CXL_EVENT_TYPE_FAIL          = 2,
+    CXL_EVENT_TYPE_FATAL         = 3,
+    CXL_EVENT_TYPE_DYNAMIC_CAP   = 4,
+    CXL_EVENT_TYPE_MAX
+} CXLEventLogType;
+
+#endif /* CXL_EVENTS_H */
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v5 2/7] hw/cxl: Move CXLRetCode definition to cxl_device.h
  2023-04-23 16:51 [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
  2023-04-23 16:51 ` [PATCH v5 1/7] hw/cxl/events: Add event status register Jonathan Cameron via
@ 2023-04-23 16:51 ` Jonathan Cameron via
  2023-04-23 16:51 ` [PATCH v5 3/7] hw/cxl/events: Wire up get/clear event mailbox commands Jonathan Cameron via
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-04-23 16:51 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin, Fan Ni
  Cc: linux-cxl, linuxarm, Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
	Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
	Marc-André Lureau, Thomas Huth

Following patches will need access to the mailbox return code
type so move it to the header.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v5: Rebase
---
 hw/cxl/cxl-mailbox-utils.c  | 28 ----------------------------
 include/hw/cxl/cxl_device.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index e3401b6be8..d7e114aaae 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -68,34 +68,6 @@ enum {
         #define CLEAR_POISON           0x2
 };
 
-/* 8.2.8.4.5.1 Command Return Codes */
-typedef enum {
-    CXL_MBOX_SUCCESS = 0x0,
-    CXL_MBOX_BG_STARTED = 0x1,
-    CXL_MBOX_INVALID_INPUT = 0x2,
-    CXL_MBOX_UNSUPPORTED = 0x3,
-    CXL_MBOX_INTERNAL_ERROR = 0x4,
-    CXL_MBOX_RETRY_REQUIRED = 0x5,
-    CXL_MBOX_BUSY = 0x6,
-    CXL_MBOX_MEDIA_DISABLED = 0x7,
-    CXL_MBOX_FW_XFER_IN_PROGRESS = 0x8,
-    CXL_MBOX_FW_XFER_OUT_OF_ORDER = 0x9,
-    CXL_MBOX_FW_AUTH_FAILED = 0xa,
-    CXL_MBOX_FW_INVALID_SLOT = 0xb,
-    CXL_MBOX_FW_ROLLEDBACK = 0xc,
-    CXL_MBOX_FW_REST_REQD = 0xd,
-    CXL_MBOX_INVALID_HANDLE = 0xe,
-    CXL_MBOX_INVALID_PA = 0xf,
-    CXL_MBOX_INJECT_POISON_LIMIT = 0x10,
-    CXL_MBOX_PERMANENT_MEDIA_FAILURE = 0x11,
-    CXL_MBOX_ABORTED = 0x12,
-    CXL_MBOX_INVALID_SECURITY_STATE = 0x13,
-    CXL_MBOX_INCORRECT_PASSPHRASE = 0x14,
-    CXL_MBOX_UNSUPPORTED_MAILBOX = 0x15,
-    CXL_MBOX_INVALID_PAYLOAD_LENGTH = 0x16,
-    CXL_MBOX_MAX = 0x17
-} CXLRetCode;
-
 struct cxl_cmd;
 typedef CXLRetCode (*opcode_handler)(struct cxl_cmd *cmd,
                                    CXLDeviceState *cxl_dstate, uint16_t *len);
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 16993f7098..9f8ee85f8a 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -83,6 +83,34 @@
     (CXL_DEVICE_CAP_REG_SIZE + CXL_DEVICE_STATUS_REGISTERS_LENGTH +     \
      CXL_MAILBOX_REGISTERS_LENGTH + CXL_MEMORY_DEVICE_REGISTERS_LENGTH)
 
+/* 8.2.8.4.5.1 Command Return Codes */
+typedef enum {
+    CXL_MBOX_SUCCESS = 0x0,
+    CXL_MBOX_BG_STARTED = 0x1,
+    CXL_MBOX_INVALID_INPUT = 0x2,
+    CXL_MBOX_UNSUPPORTED = 0x3,
+    CXL_MBOX_INTERNAL_ERROR = 0x4,
+    CXL_MBOX_RETRY_REQUIRED = 0x5,
+    CXL_MBOX_BUSY = 0x6,
+    CXL_MBOX_MEDIA_DISABLED = 0x7,
+    CXL_MBOX_FW_XFER_IN_PROGRESS = 0x8,
+    CXL_MBOX_FW_XFER_OUT_OF_ORDER = 0x9,
+    CXL_MBOX_FW_AUTH_FAILED = 0xa,
+    CXL_MBOX_FW_INVALID_SLOT = 0xb,
+    CXL_MBOX_FW_ROLLEDBACK = 0xc,
+    CXL_MBOX_FW_REST_REQD = 0xd,
+    CXL_MBOX_INVALID_HANDLE = 0xe,
+    CXL_MBOX_INVALID_PA = 0xf,
+    CXL_MBOX_INJECT_POISON_LIMIT = 0x10,
+    CXL_MBOX_PERMANENT_MEDIA_FAILURE = 0x11,
+    CXL_MBOX_ABORTED = 0x12,
+    CXL_MBOX_INVALID_SECURITY_STATE = 0x13,
+    CXL_MBOX_INCORRECT_PASSPHRASE = 0x14,
+    CXL_MBOX_UNSUPPORTED_MAILBOX = 0x15,
+    CXL_MBOX_INVALID_PAYLOAD_LENGTH = 0x16,
+    CXL_MBOX_MAX = 0x17
+} CXLRetCode;
+
 typedef struct cxl_device_state {
     MemoryRegion device_registers;
 
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v5 3/7] hw/cxl/events: Wire up get/clear event mailbox commands
  2023-04-23 16:51 [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
  2023-04-23 16:51 ` [PATCH v5 1/7] hw/cxl/events: Add event status register Jonathan Cameron via
  2023-04-23 16:51 ` [PATCH v5 2/7] hw/cxl: Move CXLRetCode definition to cxl_device.h Jonathan Cameron via
@ 2023-04-23 16:51 ` Jonathan Cameron via
  2023-04-23 16:51 ` [PATCH v5 4/7] hw/cxl/events: Add event interrupt support Jonathan Cameron via
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-04-23 16:51 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin, Fan Ni
  Cc: linux-cxl, linuxarm, Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
	Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
	Marc-André Lureau, Thomas Huth

From: Ira Weiny <ira.weiny@intel.com>

CXL testing is benefited from an artificial event log injection
mechanism.

Add an event log infrastructure to insert, get, and clear events from
the various logs available on a device.

Replace the stubbed out CXL Get/Clear Event mailbox commands with
commands that operate on the new infrastructure.

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

---
v5: Rebase
---
 hw/cxl/cxl-events.c         | 217 ++++++++++++++++++++++++++++++++++++
 hw/cxl/cxl-mailbox-utils.c  |  40 ++++++-
 hw/cxl/meson.build          |   1 +
 hw/mem/cxl_type3.c          |   1 +
 include/hw/cxl/cxl_device.h |  25 +++++
 include/hw/cxl/cxl_events.h |  55 +++++++++
 6 files changed, 337 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
new file mode 100644
index 0000000000..5da1b76b97
--- /dev/null
+++ b/hw/cxl/cxl-events.c
@@ -0,0 +1,217 @@
+/*
+ * CXL Event processing
+ *
+ * Copyright(C) 2023 Intel Corporation.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include <stdint.h>
+
+#include "qemu/osdep.h"
+#include "qemu/bswap.h"
+#include "qemu/typedefs.h"
+#include "qemu/error-report.h"
+#include "hw/cxl/cxl.h"
+#include "hw/cxl/cxl_events.h"
+
+/* Artificial limit on the number of events a log can hold */
+#define CXL_TEST_EVENT_OVERFLOW 8
+
+static void reset_overflow(CXLEventLog *log)
+{
+    log->overflow_err_count = 0;
+    log->first_overflow_timestamp = 0;
+    log->last_overflow_timestamp = 0;
+}
+
+void cxl_event_init(CXLDeviceState *cxlds)
+{
+    CXLEventLog *log;
+    int i;
+
+    for (i = 0; i < CXL_EVENT_TYPE_MAX; i++) {
+        log = &cxlds->event_logs[i];
+        log->next_handle = 1;
+        log->overflow_err_count = 0;
+        log->first_overflow_timestamp = 0;
+        log->last_overflow_timestamp = 0;
+        qemu_mutex_init(&log->lock);
+        QSIMPLEQ_INIT(&log->events);
+    }
+}
+
+static CXLEvent *cxl_event_get_head(CXLEventLog *log)
+{
+    return QSIMPLEQ_FIRST(&log->events);
+}
+
+static CXLEvent *cxl_event_get_next(CXLEvent *entry)
+{
+    return QSIMPLEQ_NEXT(entry, node);
+}
+
+static int cxl_event_count(CXLEventLog *log)
+{
+    CXLEvent *event;
+    int rc = 0;
+
+    QSIMPLEQ_FOREACH(event, &log->events, node) {
+        rc++;
+    }
+
+    return rc;
+}
+
+static bool cxl_event_empty(CXLEventLog *log)
+{
+    return QSIMPLEQ_EMPTY(&log->events);
+}
+
+static void cxl_event_delete_head(CXLDeviceState *cxlds,
+                                  CXLEventLogType log_type,
+                                  CXLEventLog *log)
+{
+    CXLEvent *entry = cxl_event_get_head(log);
+
+    reset_overflow(log);
+    QSIMPLEQ_REMOVE_HEAD(&log->events, node);
+    if (cxl_event_empty(log)) {
+        cxl_event_set_status(cxlds, log_type, false);
+    }
+    g_free(entry);
+}
+
+/*
+ * return true if an interrupt should be generated as a result
+ * of inserting this event.
+ */
+bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
+                      CXLEventRecordRaw *event)
+{
+    uint64_t time;
+    CXLEventLog *log;
+    CXLEvent *entry;
+
+    if (log_type >= CXL_EVENT_TYPE_MAX) {
+        return false;
+    }
+
+    time = cxl_device_get_timestamp(cxlds);
+
+    log = &cxlds->event_logs[log_type];
+
+    QEMU_LOCK_GUARD(&log->lock);
+
+    if (cxl_event_count(log) >= CXL_TEST_EVENT_OVERFLOW) {
+        if (log->overflow_err_count == 0) {
+            log->first_overflow_timestamp = time;
+        }
+        log->overflow_err_count++;
+        log->last_overflow_timestamp = time;
+        return false;
+    }
+
+    entry = g_new0(CXLEvent, 1);
+
+    memcpy(&entry->data, event, sizeof(*event));
+
+    entry->data.hdr.handle = cpu_to_le16(log->next_handle);
+    log->next_handle++;
+    /* 0 handle is never valid */
+    if (log->next_handle == 0) {
+        log->next_handle++;
+    }
+    entry->data.hdr.timestamp = cpu_to_le64(time);
+
+    QSIMPLEQ_INSERT_TAIL(&log->events, entry, node);
+    cxl_event_set_status(cxlds, log_type, true);
+
+    /* Count went from 0 to 1 */
+    return cxl_event_count(log) == 1;
+}
+
+CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
+                                 uint8_t log_type, int max_recs,
+                                 uint16_t *len)
+{
+    CXLEventLog *log;
+    CXLEvent *entry;
+    uint16_t nr;
+
+    if (log_type >= CXL_EVENT_TYPE_MAX) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    log = &cxlds->event_logs[log_type];
+
+    QEMU_LOCK_GUARD(&log->lock);
+
+    entry = cxl_event_get_head(log);
+    for (nr = 0; entry && nr < max_recs; nr++) {
+        memcpy(&pl->records[nr], &entry->data, CXL_EVENT_RECORD_SIZE);
+        entry = cxl_event_get_next(entry);
+    }
+
+    if (!cxl_event_empty(log)) {
+        pl->flags |= CXL_GET_EVENT_FLAG_MORE_RECORDS;
+    }
+
+    if (log->overflow_err_count) {
+        pl->flags |= CXL_GET_EVENT_FLAG_OVERFLOW;
+        pl->overflow_err_count = cpu_to_le16(log->overflow_err_count);
+        pl->first_overflow_timestamp = cpu_to_le64(log->first_overflow_timestamp);
+        pl->last_overflow_timestamp = cpu_to_le64(log->last_overflow_timestamp);
+    }
+
+    pl->record_count = cpu_to_le16(nr);
+    *len = CXL_EVENT_PAYLOAD_HDR_SIZE + (CXL_EVENT_RECORD_SIZE * nr);
+
+    return CXL_MBOX_SUCCESS;
+}
+
+CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds, CXLClearEventPayload *pl)
+{
+    CXLEventLog *log;
+    uint8_t log_type;
+    CXLEvent *entry;
+    int nr;
+
+    log_type = pl->event_log;
+
+    if (log_type >= CXL_EVENT_TYPE_MAX) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    log = &cxlds->event_logs[log_type];
+
+    QEMU_LOCK_GUARD(&log->lock);
+    /*
+     * Must itterate the queue twice.
+     * "The device shall verify the event record handles specified in the input
+     * payload are in temporal order. If the device detects an older event
+     * record that will not be cleared when Clear Event Records is executed,
+     * the device shall return the Invalid Handle return code and shall not
+     * clear any of the specified event records."
+     *   -- CXL 3.0 8.2.9.2.3
+     */
+    entry = cxl_event_get_head(log);
+    for (nr = 0; entry && nr < pl->nr_recs; nr++) {
+        uint16_t handle = pl->handle[nr];
+
+        /* NOTE: Both handles are little endian. */
+        if (handle == 0 || entry->data.hdr.handle != handle) {
+            return CXL_MBOX_INVALID_INPUT;
+        }
+        entry = cxl_event_get_next(entry);
+    }
+
+    entry = cxl_event_get_head(log);
+    for (nr = 0; entry && nr < pl->nr_recs; nr++) {
+        cxl_event_delete_head(cxlds, log_type, log);
+        entry = cxl_event_get_head(log);
+    }
+
+    return CXL_MBOX_SUCCESS;
+}
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index d7e114aaae..3f46538048 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -9,6 +9,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/cxl/cxl.h"
+#include "hw/cxl/cxl_events.h"
 #include "hw/pci/pci.h"
 #include "qemu/cutils.h"
 #include "qemu/log.h"
@@ -95,11 +96,46 @@ struct cxl_cmd {
         return CXL_MBOX_SUCCESS;                                          \
     }
 
-DEFINE_MAILBOX_HANDLER_ZEROED(events_get_records, 0x20);
-DEFINE_MAILBOX_HANDLER_NOP(events_clear_records);
 DEFINE_MAILBOX_HANDLER_ZEROED(events_get_interrupt_policy, 4);
 DEFINE_MAILBOX_HANDLER_NOP(events_set_interrupt_policy);
 
+static CXLRetCode cmd_events_get_records(struct cxl_cmd *cmd,
+                                         CXLDeviceState *cxlds,
+                                         uint16_t *len)
+{
+    CXLGetEventPayload *pl;
+    uint8_t log_type;
+    int max_recs;
+
+    if (cmd->in < sizeof(log_type)) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    log_type = *((uint8_t *)cmd->payload);
+
+    pl = (CXLGetEventPayload *)cmd->payload;
+    memset(pl, 0, sizeof(*pl));
+
+    max_recs = (cxlds->payload_size - CXL_EVENT_PAYLOAD_HDR_SIZE) /
+                CXL_EVENT_RECORD_SIZE;
+    if (max_recs > 0xFFFF) {
+        max_recs = 0xFFFF;
+    }
+
+    return cxl_event_get_records(cxlds, pl, log_type, max_recs, len);
+}
+
+static CXLRetCode cmd_events_clear_records(struct cxl_cmd *cmd,
+                                           CXLDeviceState *cxlds,
+                                           uint16_t *len)
+{
+    CXLClearEventPayload *pl;
+
+    pl = (CXLClearEventPayload *)cmd->payload;
+    *len = 0;
+    return cxl_event_clear_records(cxlds, pl);
+}
+
 /* 8.2.9.2.1 */
 static CXLRetCode cmd_firmware_update_get_info(struct cxl_cmd *cmd,
                                                CXLDeviceState *cxl_dstate,
diff --git a/hw/cxl/meson.build b/hw/cxl/meson.build
index cfa95ffd40..71059972d4 100644
--- a/hw/cxl/meson.build
+++ b/hw/cxl/meson.build
@@ -5,6 +5,7 @@ softmmu_ss.add(when: 'CONFIG_CXL',
                    'cxl-mailbox-utils.c',
                    'cxl-host.c',
                    'cxl-cdat.c',
+                   'cxl-events.c',
                ),
                if_false: files(
                    'cxl-host-stubs.c',
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index a247f506b7..ca165f6f7a 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -724,6 +724,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
         goto err_release_cdat;
     }
 
+    cxl_event_init(&ct3d->cxl_dstate);
     return;
 
 err_release_cdat:
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 9f8ee85f8a..d3aec1bc0e 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -111,6 +111,20 @@ typedef enum {
     CXL_MBOX_MAX = 0x17
 } CXLRetCode;
 
+typedef struct CXLEvent {
+    CXLEventRecordRaw data;
+    QSIMPLEQ_ENTRY(CXLEvent) node;
+} CXLEvent;
+
+typedef struct CXLEventLog {
+    uint16_t next_handle;
+    uint16_t overflow_err_count;
+    uint64_t first_overflow_timestamp;
+    uint64_t last_overflow_timestamp;
+    QemuMutex lock;
+    QSIMPLEQ_HEAD(, CXLEvent) events;
+} CXLEventLog;
+
 typedef struct cxl_device_state {
     MemoryRegion device_registers;
 
@@ -161,6 +175,8 @@ typedef struct cxl_device_state {
     uint64_t mem_size;
     uint64_t pmem_size;
     uint64_t vmem_size;
+
+    CXLEventLog event_logs[CXL_EVENT_TYPE_MAX];
 } CXLDeviceState;
 
 /* Initialize the register block for a device */
@@ -353,6 +369,15 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
 
 uint64_t cxl_device_get_timestamp(CXLDeviceState *cxlds);
 
+void cxl_event_init(CXLDeviceState *cxlds);
+bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
+                      CXLEventRecordRaw *event);
+CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
+                                 uint8_t log_type, int max_recs,
+                                 uint16_t *len);
+CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
+                                   CXLClearEventPayload *pl);
+
 void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d);
 
 #endif
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index aeb3b0590e..d4aaa894f1 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -10,6 +10,8 @@
 #ifndef CXL_EVENTS_H
 #define CXL_EVENTS_H
 
+#include "qemu/uuid.h"
+
 /*
  * CXL rev 3.0 section 8.2.9.2.2; Table 8-49
  *
@@ -25,4 +27,57 @@ typedef enum CXLEventLogType {
     CXL_EVENT_TYPE_MAX
 } CXLEventLogType;
 
+/*
+ * Common Event Record Format
+ * CXL rev 3.0 section 8.2.9.2.1; Table 8-42
+ */
+#define CXL_EVENT_REC_HDR_RES_LEN 0xf
+typedef struct CXLEventRecordHdr {
+    QemuUUID id;
+    uint8_t length;
+    uint8_t flags[3];
+    uint16_t handle;
+    uint16_t related_handle;
+    uint64_t timestamp;
+    uint8_t maint_op_class;
+    uint8_t reserved[CXL_EVENT_REC_HDR_RES_LEN];
+} QEMU_PACKED CXLEventRecordHdr;
+
+#define CXL_EVENT_RECORD_DATA_LENGTH 0x50
+typedef struct CXLEventRecordRaw {
+    CXLEventRecordHdr hdr;
+    uint8_t data[CXL_EVENT_RECORD_DATA_LENGTH];
+} QEMU_PACKED CXLEventRecordRaw;
+#define CXL_EVENT_RECORD_SIZE (sizeof(CXLEventRecordRaw))
+
+/*
+ * Get Event Records output payload
+ * CXL rev 3.0 section 8.2.9.2.2; Table 8-50
+ */
+#define CXL_GET_EVENT_FLAG_OVERFLOW     BIT(0)
+#define CXL_GET_EVENT_FLAG_MORE_RECORDS BIT(1)
+typedef struct CXLGetEventPayload {
+    uint8_t flags;
+    uint8_t reserved1;
+    uint16_t overflow_err_count;
+    uint64_t first_overflow_timestamp;
+    uint64_t last_overflow_timestamp;
+    uint16_t record_count;
+    uint8_t reserved2[0xa];
+    CXLEventRecordRaw records[];
+} QEMU_PACKED CXLGetEventPayload;
+#define CXL_EVENT_PAYLOAD_HDR_SIZE (sizeof(CXLGetEventPayload))
+
+/*
+ * Clear Event Records input payload
+ * CXL rev 3.0 section 8.2.9.2.3; Table 8-51
+ */
+typedef struct CXLClearEventPayload {
+    uint8_t event_log;      /* CXLEventLogType */
+    uint8_t clear_flags;
+    uint8_t nr_recs;
+    uint8_t reserved[3];
+    uint16_t handle[];
+} CXLClearEventPayload;
+
 #endif /* CXL_EVENTS_H */
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v5 4/7] hw/cxl/events: Add event interrupt support
  2023-04-23 16:51 [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
                   ` (2 preceding siblings ...)
  2023-04-23 16:51 ` [PATCH v5 3/7] hw/cxl/events: Wire up get/clear event mailbox commands Jonathan Cameron via
@ 2023-04-23 16:51 ` Jonathan Cameron via
  2023-04-23 16:51 ` [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events Jonathan Cameron via
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-04-23 16:51 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin, Fan Ni
  Cc: linux-cxl, linuxarm, Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
	Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
	Marc-André Lureau, Thomas Huth

From: Ira Weiny <ira.weiny@intel.com>

Replace the stubbed out CXL Get/Set Event interrupt policy mailbox
commands.  Enable those commands to control interrupts for each of the
event log types.

Skip the standard input mailbox length on the Set command due to DCD
being optional.  Perform the checks separately.

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

---
v5: Rebase
---
 hw/cxl/cxl-events.c         |  33 ++++++++++-
 hw/cxl/cxl-mailbox-utils.c  | 106 +++++++++++++++++++++++++++++-------
 hw/mem/cxl_type3.c          |   4 +-
 include/hw/cxl/cxl_device.h |   6 +-
 include/hw/cxl/cxl_events.h |  23 ++++++++
 5 files changed, 147 insertions(+), 25 deletions(-)

diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
index 5da1b76b97..d161d57456 100644
--- a/hw/cxl/cxl-events.c
+++ b/hw/cxl/cxl-events.c
@@ -13,6 +13,8 @@
 #include "qemu/bswap.h"
 #include "qemu/typedefs.h"
 #include "qemu/error-report.h"
+#include "hw/pci/msi.h"
+#include "hw/pci/msix.h"
 #include "hw/cxl/cxl.h"
 #include "hw/cxl/cxl_events.h"
 
@@ -26,7 +28,7 @@ static void reset_overflow(CXLEventLog *log)
     log->last_overflow_timestamp = 0;
 }
 
-void cxl_event_init(CXLDeviceState *cxlds)
+void cxl_event_init(CXLDeviceState *cxlds, int start_msg_num)
 {
     CXLEventLog *log;
     int i;
@@ -37,9 +39,16 @@ void cxl_event_init(CXLDeviceState *cxlds)
         log->overflow_err_count = 0;
         log->first_overflow_timestamp = 0;
         log->last_overflow_timestamp = 0;
+        log->irq_enabled = false;
+        log->irq_vec = start_msg_num++;
         qemu_mutex_init(&log->lock);
         QSIMPLEQ_INIT(&log->events);
     }
+
+    /* Override -- Dynamic Capacity uses the same vector as info */
+    cxlds->event_logs[CXL_EVENT_TYPE_DYNAMIC_CAP].irq_vec =
+                      cxlds->event_logs[CXL_EVENT_TYPE_INFO].irq_vec;
+
 }
 
 static CXLEvent *cxl_event_get_head(CXLEventLog *log)
@@ -215,3 +224,25 @@ CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds, CXLClearEventPayload *
 
     return CXL_MBOX_SUCCESS;
 }
+
+void cxl_event_irq_assert(CXLType3Dev *ct3d)
+{
+    CXLDeviceState *cxlds = &ct3d->cxl_dstate;
+    PCIDevice *pdev = &ct3d->parent_obj;
+    int i;
+
+    for (i = 0; i < CXL_EVENT_TYPE_MAX; i++) {
+        CXLEventLog *log = &cxlds->event_logs[i];
+
+        if (!log->irq_enabled || cxl_event_empty(log)) {
+            continue;
+        }
+
+        /*  Notifies interrupt, legacy IRQ is not supported */
+        if (msix_enabled(pdev)) {
+            msix_notify(pdev, log->irq_vec);
+        } else if (msi_enabled(pdev)) {
+            msi_notify(pdev, log->irq_vec);
+        }
+    }
+}
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 3f46538048..02f9b5a870 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -80,25 +80,6 @@ struct cxl_cmd {
     uint8_t *payload;
 };
 
-#define DEFINE_MAILBOX_HANDLER_ZEROED(name, size)                         \
-    uint16_t __zero##name = size;                                         \
-    static CXLRetCode cmd_##name(struct cxl_cmd *cmd,                       \
-                                 CXLDeviceState *cxl_dstate, uint16_t *len) \
-    {                                                                     \
-        *len = __zero##name;                                              \
-        memset(cmd->payload, 0, *len);                                    \
-        return CXL_MBOX_SUCCESS;                                          \
-    }
-#define DEFINE_MAILBOX_HANDLER_NOP(name)                                  \
-    static CXLRetCode cmd_##name(struct cxl_cmd *cmd,                       \
-                                 CXLDeviceState *cxl_dstate, uint16_t *len) \
-    {                                                                     \
-        return CXL_MBOX_SUCCESS;                                          \
-    }
-
-DEFINE_MAILBOX_HANDLER_ZEROED(events_get_interrupt_policy, 4);
-DEFINE_MAILBOX_HANDLER_NOP(events_set_interrupt_policy);
-
 static CXLRetCode cmd_events_get_records(struct cxl_cmd *cmd,
                                          CXLDeviceState *cxlds,
                                          uint16_t *len)
@@ -136,6 +117,88 @@ static CXLRetCode cmd_events_clear_records(struct cxl_cmd *cmd,
     return cxl_event_clear_records(cxlds, pl);
 }
 
+static CXLRetCode cmd_events_get_interrupt_policy(struct cxl_cmd *cmd,
+                                                  CXLDeviceState *cxlds,
+                                                  uint16_t *len)
+{
+    CXLEventInterruptPolicy *policy;
+    CXLEventLog *log;
+
+    policy = (CXLEventInterruptPolicy *)cmd->payload;
+    memset(policy, 0, sizeof(*policy));
+
+    log = &cxlds->event_logs[CXL_EVENT_TYPE_INFO];
+    if (log->irq_enabled) {
+        policy->info_settings = CXL_EVENT_INT_SETTING(log->irq_vec);
+    }
+
+    log = &cxlds->event_logs[CXL_EVENT_TYPE_WARN];
+    if (log->irq_enabled) {
+        policy->warn_settings = CXL_EVENT_INT_SETTING(log->irq_vec);
+    }
+
+    log = &cxlds->event_logs[CXL_EVENT_TYPE_FAIL];
+    if (log->irq_enabled) {
+        policy->failure_settings = CXL_EVENT_INT_SETTING(log->irq_vec);
+    }
+
+    log = &cxlds->event_logs[CXL_EVENT_TYPE_FATAL];
+    if (log->irq_enabled) {
+        policy->fatal_settings = CXL_EVENT_INT_SETTING(log->irq_vec);
+    }
+
+    log = &cxlds->event_logs[CXL_EVENT_TYPE_DYNAMIC_CAP];
+    if (log->irq_enabled) {
+        /* Dynamic Capacity borrows the same vector as info */
+        policy->dyn_cap_settings = CXL_INT_MSI_MSIX;
+    }
+
+    *len = sizeof(*policy);
+    return CXL_MBOX_SUCCESS;
+}
+
+static CXLRetCode cmd_events_set_interrupt_policy(struct cxl_cmd *cmd,
+                                                  CXLDeviceState *cxlds,
+                                                  uint16_t *len)
+{
+    CXLEventInterruptPolicy *policy;
+    CXLEventLog *log;
+
+    if (*len < CXL_EVENT_INT_SETTING_MIN_LEN) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
+    policy = (CXLEventInterruptPolicy *)cmd->payload;
+
+    log = &cxlds->event_logs[CXL_EVENT_TYPE_INFO];
+    log->irq_enabled = (policy->info_settings & CXL_EVENT_INT_MODE_MASK) ==
+                        CXL_INT_MSI_MSIX;
+
+    log = &cxlds->event_logs[CXL_EVENT_TYPE_WARN];
+    log->irq_enabled = (policy->warn_settings & CXL_EVENT_INT_MODE_MASK) ==
+                        CXL_INT_MSI_MSIX;
+
+    log = &cxlds->event_logs[CXL_EVENT_TYPE_FAIL];
+    log->irq_enabled = (policy->failure_settings & CXL_EVENT_INT_MODE_MASK) ==
+                        CXL_INT_MSI_MSIX;
+
+    log = &cxlds->event_logs[CXL_EVENT_TYPE_FATAL];
+    log->irq_enabled = (policy->fatal_settings & CXL_EVENT_INT_MODE_MASK) ==
+                        CXL_INT_MSI_MSIX;
+
+    /* DCD is optional */
+    if (*len < sizeof(*policy)) {
+        return CXL_MBOX_SUCCESS;
+    }
+
+    log = &cxlds->event_logs[CXL_EVENT_TYPE_DYNAMIC_CAP];
+    log->irq_enabled = (policy->dyn_cap_settings & CXL_EVENT_INT_MODE_MASK) ==
+                        CXL_INT_MSI_MSIX;
+
+    *len = sizeof(*policy);
+    return CXL_MBOX_SUCCESS;
+}
+
 /* 8.2.9.2.1 */
 static CXLRetCode cmd_firmware_update_get_info(struct cxl_cmd *cmd,
                                                CXLDeviceState *cxl_dstate,
@@ -611,9 +674,10 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
     [EVENTS][CLEAR_RECORDS] = { "EVENTS_CLEAR_RECORDS",
         cmd_events_clear_records, ~0, IMMEDIATE_LOG_CHANGE },
     [EVENTS][GET_INTERRUPT_POLICY] = { "EVENTS_GET_INTERRUPT_POLICY",
-        cmd_events_get_interrupt_policy, 0, 0 },
+                                      cmd_events_get_interrupt_policy, 0, 0 },
     [EVENTS][SET_INTERRUPT_POLICY] = { "EVENTS_SET_INTERRUPT_POLICY",
-        cmd_events_set_interrupt_policy, 4, IMMEDIATE_CONFIG_CHANGE },
+                                      cmd_events_set_interrupt_policy,
+                                      ~0, IMMEDIATE_CONFIG_CHANGE },
     [FIRMWARE_UPDATE][GET_INFO] = { "FIRMWARE_UPDATE_GET_INFO",
         cmd_firmware_update_get_info, 0, 0 },
     [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index ca165f6f7a..7ea38cadf6 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -659,7 +659,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
     ComponentRegisters *regs = &cxl_cstate->crb;
     MemoryRegion *mr = &regs->component_registers;
     uint8_t *pci_conf = pci_dev->config;
-    unsigned short msix_num = 1;
+    unsigned short msix_num = 6;
     int i, rc;
 
     QTAILQ_INIT(&ct3d->error_list);
@@ -723,8 +723,8 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
     if (rc) {
         goto err_release_cdat;
     }
+    cxl_event_init(&ct3d->cxl_dstate, 2);
 
-    cxl_event_init(&ct3d->cxl_dstate);
     return;
 
 err_release_cdat:
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index d3aec1bc0e..1978730fba 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -121,6 +121,8 @@ typedef struct CXLEventLog {
     uint16_t overflow_err_count;
     uint64_t first_overflow_timestamp;
     uint64_t last_overflow_timestamp;
+    bool irq_enabled;
+    int irq_vec;
     QemuMutex lock;
     QSIMPLEQ_HEAD(, CXLEvent) events;
 } CXLEventLog;
@@ -369,7 +371,7 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
 
 uint64_t cxl_device_get_timestamp(CXLDeviceState *cxlds);
 
-void cxl_event_init(CXLDeviceState *cxlds);
+void cxl_event_init(CXLDeviceState *cxlds, int start_msg_num);
 bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
                       CXLEventRecordRaw *event);
 CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
@@ -378,6 +380,8 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
 CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
                                    CXLClearEventPayload *pl);
 
+void cxl_event_irq_assert(CXLType3Dev *ct3d);
+
 void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d);
 
 #endif
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index d4aaa894f1..4bf8b7aa08 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -80,4 +80,27 @@ typedef struct CXLClearEventPayload {
     uint16_t handle[];
 } CXLClearEventPayload;
 
+/**
+ * Event Interrupt Policy
+ *
+ * CXL rev 3.0 section 8.2.9.2.4; Table 8-52
+ */
+typedef enum CXLEventIntMode {
+    CXL_INT_NONE     = 0x00,
+    CXL_INT_MSI_MSIX = 0x01,
+    CXL_INT_FW       = 0x02,
+    CXL_INT_RES      = 0x03,
+} CXLEventIntMode;
+#define CXL_EVENT_INT_MODE_MASK 0x3
+#define CXL_EVENT_INT_SETTING(vector) ((((uint8_t)vector & 0xf) << 4) | CXL_INT_MSI_MSIX)
+typedef struct CXLEventInterruptPolicy {
+    uint8_t info_settings;
+    uint8_t warn_settings;
+    uint8_t failure_settings;
+    uint8_t fatal_settings;
+    uint8_t dyn_cap_settings;
+} QEMU_PACKED CXLEventInterruptPolicy;
+/* DCD is optional but other fields are not */
+#define CXL_EVENT_INT_SETTING_MIN_LEN 4
+
 #endif /* CXL_EVENTS_H */
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events
  2023-04-23 16:51 [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
                   ` (3 preceding siblings ...)
  2023-04-23 16:51 ` [PATCH v5 4/7] hw/cxl/events: Add event interrupt support Jonathan Cameron via
@ 2023-04-23 16:51 ` Jonathan Cameron via
  2023-05-22  7:19   ` Markus Armbruster
  2023-04-23 16:51 ` [PATCH v5 6/7] hw/cxl/events: Add injection of DRAM events Jonathan Cameron via
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Jonathan Cameron via @ 2023-04-23 16:51 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin, Fan Ni
  Cc: linux-cxl, linuxarm, Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
	Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
	Marc-André Lureau, Thomas Huth

From: Ira Weiny <ira.weiny@intel.com>

To facilitate testing provide a QMP command to inject a general media
event.  The event can be added to the log specified.

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

---
v5:
* Rebase
* Update QMP Since entries to 8.1
---
 hw/mem/cxl_type3.c          | 111 ++++++++++++++++++++++++++++++++++++
 hw/mem/cxl_type3_stubs.c    |  10 ++++
 include/hw/cxl/cxl_events.h |  20 +++++++
 qapi/cxl.json               |  50 ++++++++++++++++
 4 files changed, 191 insertions(+)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 7ea38cadf6..f5a604975e 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1181,6 +1181,117 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
     pcie_aer_inject_error(PCI_DEVICE(obj), &err);
 }
 
+static void cxl_assign_event_header(CXLEventRecordHdr *hdr,
+                                    const QemuUUID *uuid, uint32_t flags,
+                                    uint8_t length, uint64_t timestamp)
+{
+    st24_le_p(&hdr->flags, flags);
+    hdr->length = length;
+    memcpy(&hdr->id, uuid, sizeof(hdr->id));
+    stq_le_p(&hdr->timestamp, timestamp);
+}
+
+static const QemuUUID gen_media_uuid = {
+    .data = UUID(0xfbcd0a77, 0xc260, 0x417f,
+                 0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6),
+};
+
+#define CXL_GMER_VALID_CHANNEL                          BIT(0)
+#define CXL_GMER_VALID_RANK                             BIT(1)
+#define CXL_GMER_VALID_DEVICE                           BIT(2)
+#define CXL_GMER_VALID_COMPONENT                        BIT(3)
+
+static int ct3d_qmp_cxl_event_log_enc(CxlEventLog log)
+{
+    switch (log) {
+    case CXL_EVENT_LOG_INFORMATIONAL:
+        return CXL_EVENT_TYPE_INFO;
+    case CXL_EVENT_LOG_WARNING:
+        return CXL_EVENT_TYPE_WARN;
+    case CXL_EVENT_LOG_FAILURE:
+        return CXL_EVENT_TYPE_FAIL;
+    case CXL_EVENT_LOG_FATAL:
+        return CXL_EVENT_TYPE_FATAL;
+/* DCD not yet supported */
+    default:
+        return -EINVAL;
+    }
+}
+/* Component ID is device specific.  Define this as a string. */
+void qmp_cxl_inject_gen_media_event(const char *path, CxlEventLog log,
+                                    uint8_t flags, uint64_t physaddr,
+                                    uint8_t descriptor, uint8_t type,
+                                    uint8_t transaction_type,
+                                    bool has_channel, uint8_t channel,
+                                    bool has_rank, uint8_t rank,
+                                    bool has_device, uint32_t device,
+                                    const char *component_id,
+                                    Error **errp)
+{
+    Object *obj = object_resolve_path(path, NULL);
+    CXLEventGenMedia gem;
+    CXLEventRecordHdr *hdr = &gem.hdr;
+    CXLDeviceState *cxlds;
+    CXLType3Dev *ct3d;
+    uint16_t valid_flags = 0;
+    uint8_t enc_log;
+    int rc;
+
+    if (!obj) {
+        error_setg(errp, "Unable to resolve path");
+        return;
+    }
+    if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
+        error_setg(errp, "Path does not point to a CXL type 3 device");
+        return;
+    }
+    ct3d = CXL_TYPE3(obj);
+    cxlds = &ct3d->cxl_dstate;
+
+    rc = ct3d_qmp_cxl_event_log_enc(log);
+    if (rc < 0) {
+        error_setg(errp, "Unhandled error log type");
+        return;
+    }
+    enc_log = rc;
+
+    memset(&gem, 0, sizeof(gem));
+    cxl_assign_event_header(hdr, &gen_media_uuid, flags, sizeof(gem),
+                            cxl_device_get_timestamp(&ct3d->cxl_dstate));
+
+    stq_le_p(&gem.phys_addr, physaddr);
+    gem.descriptor = descriptor;
+    gem.type = type;
+    gem.transaction_type = transaction_type;
+
+    if (has_channel) {
+        gem.channel = channel;
+        valid_flags |= CXL_GMER_VALID_CHANNEL;
+    }
+
+    if (has_rank) {
+        gem.rank = rank;
+        valid_flags |= CXL_GMER_VALID_RANK;
+    }
+
+    if (has_device) {
+        st24_le_p(gem.device, device);
+        valid_flags |= CXL_GMER_VALID_DEVICE;
+    }
+
+    if (component_id) {
+        strncpy((char *)gem.component_id, component_id,
+                sizeof(gem.component_id) - 1);
+        valid_flags |= CXL_GMER_VALID_COMPONENT;
+    }
+
+    stw_le_p(&gem.validity_flags, valid_flags);
+
+    if (cxl_event_insert(cxlds, enc_log, (CXLEventRecordRaw *)&gem)) {
+        cxl_event_irq_assert(ct3d);
+    }
+}
+
 static void ct3_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
index fd1166a610..55d19b0e03 100644
--- a/hw/mem/cxl_type3_stubs.c
+++ b/hw/mem/cxl_type3_stubs.c
@@ -3,6 +3,16 @@
 #include "qapi/error.h"
 #include "qapi/qapi-commands-cxl.h"
 
+void qmp_cxl_inject_gen_media_event(const char *path, CxlEventLog log,
+                                    uint8_t flags, uint64_t physaddr,
+                                    uint8_t descriptor, uint8_t type,
+                                    uint8_t transaction_type,
+                                    bool has_channel, uint8_t channel,
+                                    bool has_rank, uint8_t rank,
+                                    bool has_device, uint32_t device,
+                                    const char *component_id,
+                                    Error **errp) {}
+
 void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
                            Error **errp)
 {
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index 4bf8b7aa08..b189193f4c 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -103,4 +103,24 @@ typedef struct CXLEventInterruptPolicy {
 /* DCD is optional but other fields are not */
 #define CXL_EVENT_INT_SETTING_MIN_LEN 4
 
+/*
+ * General Media Event Record
+ * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
+ */
+#define CXL_EVENT_GEN_MED_COMP_ID_SIZE  0x10
+#define CXL_EVENT_GEN_MED_RES_SIZE      0x2e
+typedef struct CXLEventGenMedia {
+    CXLEventRecordHdr hdr;
+    uint64_t phys_addr;
+    uint8_t descriptor;
+    uint8_t type;
+    uint8_t transaction_type;
+    uint16_t validity_flags;
+    uint8_t channel;
+    uint8_t rank;
+    uint8_t device[3];
+    uint8_t component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
+    uint8_t reserved[CXL_EVENT_GEN_MED_RES_SIZE];
+} QEMU_PACKED CXLEventGenMedia;
+
 #endif /* CXL_EVENTS_H */
diff --git a/qapi/cxl.json b/qapi/cxl.json
index ca3af3f0b2..9dcd308a49 100644
--- a/qapi/cxl.json
+++ b/qapi/cxl.json
@@ -5,6 +5,56 @@
 # = CXL devices
 ##
 
+##
+# @CxlEventLog:
+#
+# CXL has a number of separate event logs for different types of event.
+# Each such event log is handled and signaled independently.
+#
+# @informational: Information Event Log
+# @warning: Warning Event Log
+# @failure: Failure Event Log
+# @fatal: Fatal Event Log
+#
+# Since: 8.1
+##
+{ 'enum': 'CxlEventLog',
+  'data': ['informational',
+           'warning',
+           'failure',
+           'fatal'
+           ]
+ }
+
+##
+# @cxl-inject-gen-media-event:
+#
+# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)
+# This event type is reported via one of the event logs specified via
+# the log parameter.
+#
+# @path: CXL type 3 device canonical QOM path
+# @log: Event Log to add the event to
+# @flags: header flags
+# @physaddr: Physical Address
+# @descriptor: Descriptor
+# @type: Type
+# @transaction-type: Transaction Type
+# @channel: Channel
+# @rank: Rank
+# @device: Device
+# @component-id: Device specific string
+#
+# Since: 8.1
+##
+{ 'command': 'cxl-inject-gen-media-event',
+  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags': 'uint8',
+            'physaddr': 'uint64', 'descriptor': 'uint8',
+            'type': 'uint8', 'transaction-type': 'uint8',
+            '*channel': 'uint8', '*rank': 'uint8',
+            '*device': 'uint32', '*component-id': 'str'
+            }}
+
 ##
 # @cxl-inject-poison:
 #
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v5 6/7] hw/cxl/events: Add injection of DRAM events
  2023-04-23 16:51 [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
                   ` (4 preceding siblings ...)
  2023-04-23 16:51 ` [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events Jonathan Cameron via
@ 2023-04-23 16:51 ` Jonathan Cameron via
  2023-05-22  7:43   ` Markus Armbruster
  2023-04-23 16:51 ` [PATCH v5 7/7] hw/cxl/events: Add injection of Memory Module Events Jonathan Cameron via
  2023-05-16 16:31 ` [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
  7 siblings, 1 reply; 20+ messages in thread
From: Jonathan Cameron via @ 2023-04-23 16:51 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin, Fan Ni
  Cc: linux-cxl, linuxarm, Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
	Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
	Marc-André Lureau, Thomas Huth

Defined in CXL r3.0 8.2.9.2.1.2 DRAM Event Record, this event
provides information related to DRAM devices.

Example injection command in QMP:

{ "execute": "cxl-inject-dram-event",
    "arguments": {
        "path": "/machine/peripheral/cxl-mem0",
        "log": "informational",
        "flags": 1,
        "physaddr": 1000,
        "descriptor": 3,
        "type": 3,
        "transaction-type": 192,
        "channel": 3,
        "rank": 17,
        "nibble-mask": 37421234,
        "bank-group": 7,
        "bank": 11,
        "row": 2,
        "column": 77,
        "correction-mask": [33, 44, 55,66]
    }}

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v5:
* Rebase
* Update Since entires for QMP.
---
 hw/mem/cxl_type3.c          | 116 ++++++++++++++++++++++++++++++++++++
 hw/mem/cxl_type3_stubs.c    |  13 ++++
 include/hw/cxl/cxl_events.h |  23 +++++++
 qapi/cxl.json               |  35 +++++++++++
 4 files changed, 187 insertions(+)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index f5a604975e..71df31917c 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1196,6 +1196,11 @@ static const QemuUUID gen_media_uuid = {
                  0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6),
 };
 
+static const QemuUUID dram_uuid = {
+    .data = UUID(0x601dcbb3, 0x9c06, 0x4eab, 0xb8, 0xaf,
+                 0x4e, 0x9b, 0xfb, 0x5c, 0x96, 0x24),
+};
+
 #define CXL_GMER_VALID_CHANNEL                          BIT(0)
 #define CXL_GMER_VALID_RANK                             BIT(1)
 #define CXL_GMER_VALID_DEVICE                           BIT(2)
@@ -1292,6 +1297,117 @@ void qmp_cxl_inject_gen_media_event(const char *path, CxlEventLog log,
     }
 }
 
+#define CXL_DRAM_VALID_CHANNEL                          BIT(0)
+#define CXL_DRAM_VALID_RANK                             BIT(1)
+#define CXL_DRAM_VALID_NIBBLE_MASK                      BIT(2)
+#define CXL_DRAM_VALID_BANK_GROUP                       BIT(3)
+#define CXL_DRAM_VALID_BANK                             BIT(4)
+#define CXL_DRAM_VALID_ROW                              BIT(5)
+#define CXL_DRAM_VALID_COLUMN                           BIT(6)
+#define CXL_DRAM_VALID_CORRECTION_MASK                  BIT(7)
+
+void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
+                               uint64_t physaddr, uint8_t descriptor,
+                               uint8_t type, uint8_t transaction_type,
+                               bool has_channel, uint8_t channel,
+                               bool has_rank, uint8_t rank,
+                               bool has_nibble_mask, uint32_t nibble_mask,
+                               bool has_bank_group, uint8_t bank_group,
+                               bool has_bank, uint8_t bank,
+                               bool has_row, uint32_t row,
+                               bool has_column, uint16_t column,
+                               bool has_correction_mask, uint64List *correction_mask,
+                               Error **errp)
+{
+    Object *obj = object_resolve_path(path, NULL);
+    CXLEventDram dram;
+    CXLEventRecordHdr *hdr = &dram.hdr;
+    CXLDeviceState *cxlds;
+    CXLType3Dev *ct3d;
+    uint16_t valid_flags = 0;
+    uint8_t enc_log;
+    int rc;
+
+    if (!obj) {
+        error_setg(errp, "Unable to resolve path");
+        return;
+    }
+    if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
+        error_setg(errp, "Path does not point to a CXL type 3 device");
+        return;
+    }
+    ct3d = CXL_TYPE3(obj);
+    cxlds = &ct3d->cxl_dstate;
+
+    rc = ct3d_qmp_cxl_event_log_enc(log);
+    if (rc < 0) {
+        error_setg(errp, "Unhandled error log type");
+        return;
+    }
+    enc_log = rc;
+
+    memset(&dram, 0, sizeof(dram));
+    cxl_assign_event_header(hdr, &dram_uuid, flags, sizeof(dram),
+                            cxl_device_get_timestamp(&ct3d->cxl_dstate));
+    stq_le_p(&dram.phys_addr, physaddr);
+    dram.descriptor = descriptor;
+    dram.type = type;
+    dram.transaction_type = transaction_type;
+
+    if (has_channel) {
+        dram.channel = channel;
+        valid_flags |= CXL_DRAM_VALID_CHANNEL;
+    }
+
+    if (has_rank) {
+        dram.rank = rank;
+        valid_flags |= CXL_DRAM_VALID_RANK;
+    }
+
+    if (has_nibble_mask) {
+        st24_le_p(dram.nibble_mask, nibble_mask);
+        valid_flags |= CXL_DRAM_VALID_NIBBLE_MASK;
+    }
+
+    if (has_bank_group) {
+        dram.bank_group = bank_group;
+        valid_flags |= CXL_DRAM_VALID_BANK_GROUP;
+    }
+
+    if (has_bank) {
+        dram.bank = bank;
+        valid_flags |= CXL_DRAM_VALID_BANK;
+    }
+
+    if (has_row) {
+        st24_le_p(dram.row, row);
+        valid_flags |= CXL_DRAM_VALID_ROW;
+    }
+
+    if (has_column) {
+        stw_le_p(&dram.column, column);
+        valid_flags |= CXL_DRAM_VALID_COLUMN;
+    }
+
+    if (has_correction_mask) {
+        int count = 0;
+        while (correction_mask && count < 4) {
+            stq_le_p(&dram.correction_mask[count],
+                     correction_mask->value);
+            count++;
+            correction_mask = correction_mask->next;
+        }
+        valid_flags |= CXL_DRAM_VALID_CORRECTION_MASK;
+    }
+
+    stw_le_p(&dram.validity_flags, valid_flags);
+
+    if (cxl_event_insert(cxlds, enc_log, (CXLEventRecordRaw *)&dram)) {
+        cxl_event_irq_assert(ct3d);
+    }
+    return;
+}
+
 static void ct3_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
index 55d19b0e03..235c171264 100644
--- a/hw/mem/cxl_type3_stubs.c
+++ b/hw/mem/cxl_type3_stubs.c
@@ -13,6 +13,19 @@ void qmp_cxl_inject_gen_media_event(const char *path, CxlEventLog log,
                                     const char *component_id,
                                     Error **errp) {}
 
+void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
+                               uint64_t physaddr, uint8_t descriptor,
+                               uint8_t type, uint8_t transaction_type,
+                               bool has_channel, uint8_t channel,
+                               bool has_rank, uint8_t rank,
+                               bool has_nibble_mask, uint32_t nibble_mask,
+                               bool has_bank_group, uint8_t bank_group,
+                               bool has_bank, uint8_t bank,
+                               bool has_row, uint32_t row,
+                               bool has_column, uint16_t column,
+                               bool has_correction_mask, uint64List *correction_mask,
+                               Error **errp) {}
+
 void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
                            Error **errp)
 {
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index b189193f4c..a39e30d973 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -123,4 +123,27 @@ typedef struct CXLEventGenMedia {
     uint8_t reserved[CXL_EVENT_GEN_MED_RES_SIZE];
 } QEMU_PACKED CXLEventGenMedia;
 
+/*
+ * DRAM Event Record
+ * CXL Rev 3.0 Section 8.2.9.2.1.2: Table 8-44
+ * All fields little endian.
+ */
+typedef struct CXLEventDram {
+    CXLEventRecordHdr hdr;
+    uint64_t phys_addr;
+    uint8_t descriptor;
+    uint8_t type;
+    uint8_t transaction_type;
+    uint16_t validity_flags;
+    uint8_t channel;
+    uint8_t rank;
+    uint8_t nibble_mask[3];
+    uint8_t bank_group;
+    uint8_t bank;
+    uint8_t row[3];
+    uint16_t column;
+    uint64_t correction_mask[4];
+    uint8_t reserved[0x17];
+} QEMU_PACKED CXLEventDram;
+
 #endif /* CXL_EVENTS_H */
diff --git a/qapi/cxl.json b/qapi/cxl.json
index 9dcd308a49..190db58385 100644
--- a/qapi/cxl.json
+++ b/qapi/cxl.json
@@ -55,6 +55,41 @@
             '*device': 'uint32', '*component-id': 'str'
             }}
 
+##
+# @cxl-inject-dram-event:
+#
+# Inject an event record for a DRAM Event (CXL r3.0 8.2.9.2.1.2)
+# This event type is reported via one of the event logs specified via
+# the log parameter.
+#
+# @path: CXL type 3 device canonical QOM path
+# @log: Event Log to add the event to
+# @flags: header flags
+# @physaddr: Physical Address
+# @descriptor: Descriptor
+# @type: Type
+# @transaction-type: Transaction Type
+# @channel: Channel
+# @rank: Rank
+# @nibble-mask: Identify one or more nibbles that the error affects
+# @bank-group: Bank group
+# @bank: Bank
+# @row: Row
+# @column: Column
+# @correction-mask: Bits within each nibble. Used in order of bits set
+#                   in the nibble-mask.  Up to 4 nibbles may be covered.
+#
+# Since: 8.1
+##
+{ 'command': 'cxl-inject-dram-event',
+  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags': 'uint8',
+            'physaddr': 'uint64', 'descriptor': 'uint8',
+            'type': 'uint8', 'transaction-type': 'uint8',
+            '*channel': 'uint8', '*rank': 'uint8', '*nibble-mask': 'uint32',
+            '*bank-group': 'uint8', '*bank': 'uint8', '*row': 'uint32',
+            '*column': 'uint16', '*correction-mask': [ 'uint64' ]
+           }}
+
 ##
 # @cxl-inject-poison:
 #
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v5 7/7] hw/cxl/events: Add injection of Memory Module Events
  2023-04-23 16:51 [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
                   ` (5 preceding siblings ...)
  2023-04-23 16:51 ` [PATCH v5 6/7] hw/cxl/events: Add injection of DRAM events Jonathan Cameron via
@ 2023-04-23 16:51 ` Jonathan Cameron via
  2023-05-19  7:06   ` Michael S. Tsirkin
  2023-05-22  7:47   ` Markus Armbruster
  2023-05-16 16:31 ` [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
  7 siblings, 2 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-04-23 16:51 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin, Fan Ni
  Cc: linux-cxl, linuxarm, Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
	Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
	Marc-André Lureau, Thomas Huth

These events include a copy of the device health information at the
time of the event. Actually using the emulated device health would
require a lot of controls to manipulate that state.  Given the aim
of this injection code is to just test the flows when events occur,
inject the contents of the device health state as well.

Future work may add more sophisticate device health emulation
including direct generation of these records when events occur
(such as a temperature threshold being crossed).  That does not
reduce the usefulness of this more basic generation of the events.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

--
v5:
* Rebase
* Update Since entries for QMP.
---
 hw/mem/cxl_type3.c          | 62 +++++++++++++++++++++++++++++++++++++
 hw/mem/cxl_type3_stubs.c    | 12 +++++++
 include/hw/cxl/cxl_events.h | 19 ++++++++++++
 qapi/cxl.json               | 35 +++++++++++++++++++++
 4 files changed, 128 insertions(+)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 71df31917c..5d1b3a5b9b 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1201,6 +1201,11 @@ static const QemuUUID dram_uuid = {
                  0x4e, 0x9b, 0xfb, 0x5c, 0x96, 0x24),
 };
 
+static const QemuUUID memory_module_uuid = {
+    .data = UUID(0xfe927475, 0xdd59, 0x4339, 0xa5, 0x86,
+                 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74),
+};
+
 #define CXL_GMER_VALID_CHANNEL                          BIT(0)
 #define CXL_GMER_VALID_RANK                             BIT(1)
 #define CXL_GMER_VALID_DEVICE                           BIT(2)
@@ -1408,6 +1413,63 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
     return;
 }
 
+void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
+                                        uint8_t flags, uint8_t type,
+                                        uint8_t health_status,
+                                        uint8_t media_status,
+                                        uint8_t additional_status,
+                                        uint8_t life_used,
+                                        int16_t temperature,
+                                        uint32_t dirty_shutdown_count,
+                                        uint32_t corrected_volatile_error_count,
+                                        uint32_t corrected_persistent_error_count,
+                                        Error **errp)
+{
+    Object *obj = object_resolve_path(path, NULL);
+    CXLEventMemoryModule module;
+    CXLEventRecordHdr *hdr = &module.hdr;
+    CXLDeviceState *cxlds;
+    CXLType3Dev *ct3d;
+    uint8_t enc_log;
+    int rc;
+
+    if (!obj) {
+        error_setg(errp, "Unable to resolve path");
+        return;
+    }
+    if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
+        error_setg(errp, "Path does not point to a CXL type 3 device");
+        return;
+    }
+    ct3d = CXL_TYPE3(obj);
+    cxlds = &ct3d->cxl_dstate;
+
+    rc = ct3d_qmp_cxl_event_log_enc(log);
+    if (rc < 0) {
+        error_setg(errp, "Unhandled error log type");
+        return;
+    }
+    enc_log = rc;
+
+    memset(&module, 0, sizeof(module));
+    cxl_assign_event_header(hdr, &memory_module_uuid, flags, sizeof(module),
+                            cxl_device_get_timestamp(&ct3d->cxl_dstate));
+
+    module.type = type;
+    module.health_status = health_status;
+    module.media_status = media_status;
+    module.additional_status = additional_status;
+    module.life_used = life_used;
+    stw_le_p(&module.temperature, temperature);
+    stl_le_p(&module.dirty_shutdown_count, dirty_shutdown_count);
+    stl_le_p(&module.corrected_volatile_error_count, corrected_volatile_error_count);
+    stl_le_p(&module.corrected_persistent_error_count, corrected_persistent_error_count);
+
+    if (cxl_event_insert(cxlds, enc_log, (CXLEventRecordRaw *)&module)) {
+        cxl_event_irq_assert(ct3d);
+    }
+}
+
 static void ct3_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
index 235c171264..2196bd841c 100644
--- a/hw/mem/cxl_type3_stubs.c
+++ b/hw/mem/cxl_type3_stubs.c
@@ -26,6 +26,18 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
                                bool has_correction_mask, uint64List *correction_mask,
                                Error **errp) {}
 
+void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
+                                        uint8_t flags, uint8_t type,
+                                        uint8_t health_status,
+                                        uint8_t media_status,
+                                        uint8_t additional_status,
+                                        uint8_t life_used,
+                                        int16_t temperature,
+                                        uint32_t dirty_shutdown_count,
+                                        uint32_t corrected_volatile_error_count,
+                                        uint32_t corrected_persistent_error_count,
+                                        Error **errp) {}
+
 void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
                            Error **errp)
 {
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index a39e30d973..089ba2091f 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -146,4 +146,23 @@ typedef struct CXLEventDram {
     uint8_t reserved[0x17];
 } QEMU_PACKED CXLEventDram;
 
+/*
+ * Memory Module Event Record
+ * CXL Rev 3.0 Section 8.2.9.2.1.3: Table 8-45
+ * All fields little endian.
+ */
+typedef struct CXLEventMemoryModule {
+    CXLEventRecordHdr hdr;
+    uint8_t type;
+    uint8_t health_status;
+    uint8_t media_status;
+    uint8_t additional_status;
+    uint8_t life_used;
+    int16_t temperature;
+    uint32_t dirty_shutdown_count;
+    uint32_t corrected_volatile_error_count;
+    uint32_t corrected_persistent_error_count;
+    uint8_t reserved[0x3d];
+} QEMU_PACKED CXLEventMemoryModule;
+
 #endif /* CXL_EVENTS_H */
diff --git a/qapi/cxl.json b/qapi/cxl.json
index 190db58385..aae70667c2 100644
--- a/qapi/cxl.json
+++ b/qapi/cxl.json
@@ -90,6 +90,41 @@
             '*column': 'uint16', '*correction-mask': [ 'uint64' ]
            }}
 
+##
+# @cxl-inject-memory-module-event:
+#
+# Inject an event record for a Memory Module Event (CXL r3.0 8.2.9.2.1.3)
+# This event includes a copy of the Device Health info at the time of
+# the event.
+#
+# @path: CXL type 3 device canonical QOM path
+# @log: Event Log to add the event to
+# @flags: header flags
+# @type: Device Event Type (see spec for permitted values)
+# @health-status: Overall health summary bitmap (see spec for permitted bits)
+# @media-status: Overall media health summary (see spec for permitted values)
+# @additional-status: Complex field (see spec for meaning)
+# @life-used: Percentage (0-100) of factory expected life span
+# @temperature: Device temperature in degrees Celsius
+# @dirty-shutdown-count: Counter incremented whenever device is unable
+#                        to determine if data loss may have occurred.
+# @corrected-volatile-error-count: Total number of correctable errors in
+#                                  volatile memory
+# @corrected-persistent-error-count: Total number correctable errors in
+#                                    persistent memory
+#
+# Since: 8.1
+##
+{ 'command': 'cxl-inject-memory-module-event',
+  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags' : 'uint8',
+            'type': 'uint8', 'health-status': 'uint8',
+            'media-status': 'uint8', 'additional-status': 'uint8',
+            'life-used': 'uint8', 'temperature' : 'int16',
+            'dirty-shutdown-count': 'uint32',
+            'corrected-volatile-error-count': 'uint32',
+            'corrected-persistent-error-count': 'uint32'
+            }}
+
 ##
 # @cxl-inject-poison:
 #
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support
  2023-04-23 16:51 [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
                   ` (6 preceding siblings ...)
  2023-04-23 16:51 ` [PATCH v5 7/7] hw/cxl/events: Add injection of Memory Module Events Jonathan Cameron via
@ 2023-05-16 16:31 ` Jonathan Cameron via
  7 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-05-16 16:31 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin, Fan Ni
  Cc: linux-cxl, Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Markus Armbruster,
	Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
	Marc-André Lureau, Thomas Huth

On Sun, 23 Apr 2023 17:51:33 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> v5: Rebase including version number updates to 8.1

Hi Michael,

I asked a few people to look at the remaining couple of patches where
there were only my tags so now I think all the following have
had some review and I'm not aware of any outstanding issues with them

I did postpone one piece of general feedback from Philippe for future
work as it wasn't specific to the patch but more general feedback on
older code. For reference, the question concerned why we have
cxl_dstate as a structure in the CXLType3Dev rather than using the 
QEMU device model and inheriting from it.

Majority of what we have in these 6 sets (including this one)
came from other contributors so I've reviewed and tested all those
as part of adding my SOB when gathering them up.

Anything you'd like to see in the way of additional review on these?

We have a number of other series, that will hopefully be in a position
to post for possible merge soon, but I'm guessing you'd rather we didn't
stack more than this in one go.

Other than fixes, this is all stuff left over from previous cycle.

Thanks,

Jonathan

> 
> Depends on 
> [PATCH 0/2] hw/cxl: CDAT file handling fixes.
> [PATCH v2 0/3] hw/cxl: Fix decoder commit and uncommit handling
> [PATCH 0/3] docs/cxl: Gathering of fixes for 8.0 CXL docs.
> [PATCH v5 0/3] hw/mem: CXL Type-3 Volatile Memory Support
> [PATCH v5 0/6] hw/cxl: Poison get, inject, clear
> 
> Based on: Message-ID: 20230421132020.7408-1-Jonathan.Cameron@huawei.com
> Based on: Message-ID: 20230421135906.3515-1-Jonathan.Cameron@huawei.com
> Based on: Message-ID: 20230421134507.26842-1-Jonathan.Cameron@huawei.com
> Based on: Message-ID: 20230421160827.2227-1-Jonathan.Cameron@huawei.com
> Based on: Message-ID: 20230423162013.4535-1-Jonathan.Cameron@huawei.com
> 
> Cover letter from earlier version
> 
> One challenge here is striking the right balance between lots of constraints
> in the injection code to enforce particular reserved bits etc by breaking
> out all the flags as individual parameters vs having a reasonably concise
> API.  I think this set strikes the right balance but others may well
> disagree :)   Note that Ira raised the question of whether we should be
> automatically establishing the volatile flag based on the Device Physical
> Address of the injected error. My proposal is to not do so for now, but
> to possibly revisit tightening the checking of injected errors in future.
> Whilst the volatile flag is straight forwards, some of the other flags that
> could be automatically set (or perhaps checked for validiaty) are much more
> complex. Adding verification at this stage would greatly increase the
> complexity of the patch + we are missing other elements that would interact
> with this.  I'm not concerned about potential breaking of backwards compatibility
> if it only related to the injection of errors that make no sense for a real
> device.
> 
> CXL Event records inform the OS of various CXL device events.  Thus far CXL
> memory devices are emulated and therefore don't naturally generate events.
> 
> Add an event infrastructure and mock event injection.  Previous versions
> included a bulk insertion of lots of events.  However, this series focuses on
> providing the ability to inject individual events through QMP.  Only the
> General Media Event is included in this series as an example.  Other events can
> be added pretty easily once the infrastructure is acceptable.
> 
> In addition, this version updates the code to be in line with the
> specification based on discussions around the kernel patches.
> 
> Injection examples;
> 
> { "execute": "cxl-inject-gen-media-event",
>     "arguments": {
>         "path": "/machine/peripheral/cxl-mem0",
>         "log": "informational",
>         "flags": 1,
>         "physaddr": 1000,
>         "descriptor": 3,
>         "type": 3,
>         "transaction-type": 192,
>         "channel": 3,
>         "device": 5,
>         "component-id": "iras mem"
>     }}
> 
> 
> { "execute": "cxl-inject-dram-event",
>     "arguments": {
>         "path": "/machine/peripheral/cxl-mem0",
>         "log": "informational",
>         "flags": 1,
>         "physaddr": 1000,
>         "descriptor": 3,
>         "type": 3,
>         "transaction-type": 192,
>         "channel": 3,
>         "rank": 17,
>         "nibble-mask": 37421234,
>         "bank-group": 7,
>         "bank": 11,
>         "row": 2,
>         "column": 77,
>         "correction-mask": [33, 44, 55, 66]
>     }}
> 
> { "execute": "cxl-inject-memory-module-event",
>   "arguments": {
>     "path": "/machine/peripheral/cxl-mem0",
>     "log": "informational",
>     "flags": 1,
>     "type": 3,
>     "health-status": 3,
>     "media-status": 7,
>     "additional-status": 33,
>     "life-used": 30,
>     "temperature": -15,
>     "dirty-shutdown-count": 4,
>     "corrected-volatile-error-count": 3233,
>     "corrected-persistent-error-count": 1300
>   }}
> 
> Ira Weiny (4):
>   hw/cxl/events: Add event status register
>   hw/cxl/events: Wire up get/clear event mailbox commands
>   hw/cxl/events: Add event interrupt support
>   hw/cxl/events: Add injection of General Media Events
> 
> Jonathan Cameron (3):
>   hw/cxl: Move CXLRetCode definition to cxl_device.h
>   hw/cxl/events: Add injection of DRAM events
>   hw/cxl/events: Add injection of Memory Module Events
> 
>  hw/cxl/cxl-device-utils.c   |  43 +++++-
>  hw/cxl/cxl-events.c         | 248 ++++++++++++++++++++++++++++++
>  hw/cxl/cxl-mailbox-utils.c  | 166 ++++++++++++++------
>  hw/cxl/meson.build          |   1 +
>  hw/mem/cxl_type3.c          | 292 +++++++++++++++++++++++++++++++++++-
>  hw/mem/cxl_type3_stubs.c    |  35 +++++
>  include/hw/cxl/cxl_device.h |  80 +++++++++-
>  include/hw/cxl/cxl_events.h | 168 +++++++++++++++++++++
>  qapi/cxl.json               | 120 +++++++++++++++
>  9 files changed, 1097 insertions(+), 56 deletions(-)
>  create mode 100644 hw/cxl/cxl-events.c
>  create mode 100644 include/hw/cxl/cxl_events.h
> 



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 7/7] hw/cxl/events: Add injection of Memory Module Events
  2023-04-23 16:51 ` [PATCH v5 7/7] hw/cxl/events: Add injection of Memory Module Events Jonathan Cameron via
@ 2023-05-19  7:06   ` Michael S. Tsirkin
  2023-05-22  7:47   ` Markus Armbruster
  1 sibling, 0 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2023-05-19  7:06 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: qemu-devel, Fan Ni, linux-cxl, linuxarm, Ira Weiny,
	Alison Schofield, Michael Roth, Philippe Mathieu-Daudé,
	Dave Jiang, Markus Armbruster, Daniel P . Berrangé,
	Eric Blake, Mike Maslenkin, Marc-André Lureau, Thomas Huth

On Sun, Apr 23, 2023 at 05:51:40PM +0100, Jonathan Cameron wrote:
> These events include a copy of the device health information at the
> time of the event. Actually using the emulated device health would
> require a lot of controls to manipulate that state.  Given the aim
> of this injection code is to just test the flows when events occur,
> inject the contents of the device health state as well.
> 
> Future work may add more sophisticate device health emulation
> including direct generation of these records when events occur
> (such as a temperature threshold being crossed).  That does not
> reduce the usefulness of this more basic generation of the events.
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> --

This should be --- not --, otherwise git am does not cut at this point.


> v5:
> * Rebase
> * Update Since entries for QMP.
> ---
>  hw/mem/cxl_type3.c          | 62 +++++++++++++++++++++++++++++++++++++
>  hw/mem/cxl_type3_stubs.c    | 12 +++++++
>  include/hw/cxl/cxl_events.h | 19 ++++++++++++
>  qapi/cxl.json               | 35 +++++++++++++++++++++
>  4 files changed, 128 insertions(+)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 71df31917c..5d1b3a5b9b 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1201,6 +1201,11 @@ static const QemuUUID dram_uuid = {
>                   0x4e, 0x9b, 0xfb, 0x5c, 0x96, 0x24),
>  };
>  
> +static const QemuUUID memory_module_uuid = {
> +    .data = UUID(0xfe927475, 0xdd59, 0x4339, 0xa5, 0x86,
> +                 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74),
> +};
> +
>  #define CXL_GMER_VALID_CHANNEL                          BIT(0)
>  #define CXL_GMER_VALID_RANK                             BIT(1)
>  #define CXL_GMER_VALID_DEVICE                           BIT(2)
> @@ -1408,6 +1413,63 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
>      return;
>  }
>  
> +void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> +                                        uint8_t flags, uint8_t type,
> +                                        uint8_t health_status,
> +                                        uint8_t media_status,
> +                                        uint8_t additional_status,
> +                                        uint8_t life_used,
> +                                        int16_t temperature,
> +                                        uint32_t dirty_shutdown_count,
> +                                        uint32_t corrected_volatile_error_count,
> +                                        uint32_t corrected_persistent_error_count,
> +                                        Error **errp)
> +{
> +    Object *obj = object_resolve_path(path, NULL);
> +    CXLEventMemoryModule module;
> +    CXLEventRecordHdr *hdr = &module.hdr;
> +    CXLDeviceState *cxlds;
> +    CXLType3Dev *ct3d;
> +    uint8_t enc_log;
> +    int rc;
> +
> +    if (!obj) {
> +        error_setg(errp, "Unable to resolve path");
> +        return;
> +    }
> +    if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
> +        error_setg(errp, "Path does not point to a CXL type 3 device");
> +        return;
> +    }
> +    ct3d = CXL_TYPE3(obj);
> +    cxlds = &ct3d->cxl_dstate;
> +
> +    rc = ct3d_qmp_cxl_event_log_enc(log);
> +    if (rc < 0) {
> +        error_setg(errp, "Unhandled error log type");
> +        return;
> +    }
> +    enc_log = rc;
> +
> +    memset(&module, 0, sizeof(module));
> +    cxl_assign_event_header(hdr, &memory_module_uuid, flags, sizeof(module),
> +                            cxl_device_get_timestamp(&ct3d->cxl_dstate));
> +
> +    module.type = type;
> +    module.health_status = health_status;
> +    module.media_status = media_status;
> +    module.additional_status = additional_status;
> +    module.life_used = life_used;
> +    stw_le_p(&module.temperature, temperature);
> +    stl_le_p(&module.dirty_shutdown_count, dirty_shutdown_count);
> +    stl_le_p(&module.corrected_volatile_error_count, corrected_volatile_error_count);
> +    stl_le_p(&module.corrected_persistent_error_count, corrected_persistent_error_count);
> +
> +    if (cxl_event_insert(cxlds, enc_log, (CXLEventRecordRaw *)&module)) {
> +        cxl_event_irq_assert(ct3d);
> +    }
> +}
> +
>  static void ct3_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
> index 235c171264..2196bd841c 100644
> --- a/hw/mem/cxl_type3_stubs.c
> +++ b/hw/mem/cxl_type3_stubs.c
> @@ -26,6 +26,18 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
>                                 bool has_correction_mask, uint64List *correction_mask,
>                                 Error **errp) {}
>  
> +void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> +                                        uint8_t flags, uint8_t type,
> +                                        uint8_t health_status,
> +                                        uint8_t media_status,
> +                                        uint8_t additional_status,
> +                                        uint8_t life_used,
> +                                        int16_t temperature,
> +                                        uint32_t dirty_shutdown_count,
> +                                        uint32_t corrected_volatile_error_count,
> +                                        uint32_t corrected_persistent_error_count,
> +                                        Error **errp) {}
> +
>  void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
>                             Error **errp)
>  {
> diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
> index a39e30d973..089ba2091f 100644
> --- a/include/hw/cxl/cxl_events.h
> +++ b/include/hw/cxl/cxl_events.h
> @@ -146,4 +146,23 @@ typedef struct CXLEventDram {
>      uint8_t reserved[0x17];
>  } QEMU_PACKED CXLEventDram;
>  
> +/*
> + * Memory Module Event Record
> + * CXL Rev 3.0 Section 8.2.9.2.1.3: Table 8-45
> + * All fields little endian.
> + */
> +typedef struct CXLEventMemoryModule {
> +    CXLEventRecordHdr hdr;
> +    uint8_t type;
> +    uint8_t health_status;
> +    uint8_t media_status;
> +    uint8_t additional_status;
> +    uint8_t life_used;
> +    int16_t temperature;
> +    uint32_t dirty_shutdown_count;
> +    uint32_t corrected_volatile_error_count;
> +    uint32_t corrected_persistent_error_count;
> +    uint8_t reserved[0x3d];
> +} QEMU_PACKED CXLEventMemoryModule;
> +
>  #endif /* CXL_EVENTS_H */
> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index 190db58385..aae70667c2 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -90,6 +90,41 @@
>              '*column': 'uint16', '*correction-mask': [ 'uint64' ]
>             }}
>  
> +##
> +# @cxl-inject-memory-module-event:
> +#
> +# Inject an event record for a Memory Module Event (CXL r3.0 8.2.9.2.1.3)
> +# This event includes a copy of the Device Health info at the time of
> +# the event.
> +#
> +# @path: CXL type 3 device canonical QOM path
> +# @log: Event Log to add the event to
> +# @flags: header flags
> +# @type: Device Event Type (see spec for permitted values)
> +# @health-status: Overall health summary bitmap (see spec for permitted bits)
> +# @media-status: Overall media health summary (see spec for permitted values)
> +# @additional-status: Complex field (see spec for meaning)
> +# @life-used: Percentage (0-100) of factory expected life span
> +# @temperature: Device temperature in degrees Celsius
> +# @dirty-shutdown-count: Counter incremented whenever device is unable
> +#                        to determine if data loss may have occurred.
> +# @corrected-volatile-error-count: Total number of correctable errors in
> +#                                  volatile memory
> +# @corrected-persistent-error-count: Total number correctable errors in
> +#                                    persistent memory
> +#
> +# Since: 8.1
> +##
> +{ 'command': 'cxl-inject-memory-module-event',
> +  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags' : 'uint8',
> +            'type': 'uint8', 'health-status': 'uint8',
> +            'media-status': 'uint8', 'additional-status': 'uint8',
> +            'life-used': 'uint8', 'temperature' : 'int16',
> +            'dirty-shutdown-count': 'uint32',
> +            'corrected-volatile-error-count': 'uint32',
> +            'corrected-persistent-error-count': 'uint32'
> +            }}
> +
>  ##
>  # @cxl-inject-poison:
>  #
> -- 
> 2.37.2



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events
  2023-04-23 16:51 ` [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events Jonathan Cameron via
@ 2023-05-22  7:19   ` Markus Armbruster
  2023-05-22 12:57     ` Jonathan Cameron via
  0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2023-05-22  7:19 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
	Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Daniel P . Berrangé,
	Eric Blake, Mike Maslenkin, Marc-André Lureau, Thomas Huth

Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:

> From: Ira Weiny <ira.weiny@intel.com>
>
> To facilitate testing provide a QMP command to inject a general media
> event.  The event can be added to the log specified.
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[...]

> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index ca3af3f0b2..9dcd308a49 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -5,6 +5,56 @@
>  # = CXL devices
>  ##
>  
> +##
> +# @CxlEventLog:
> +#
> +# CXL has a number of separate event logs for different types of event.

types of events

> +# Each such event log is handled and signaled independently.
> +#
> +# @informational: Information Event Log
> +# @warning: Warning Event Log
> +# @failure: Failure Event Log
> +# @fatal: Fatal Event Log

Are these proper nouns?  If not, the words should not be capitalized.

> +#
> +# Since: 8.1
> +##

Please format like

   ##
   # @CxlEventLog:
   #
   # CXL has a number of separate event logs for different types of
   # events.  Each such event log is handled and signaled independently.
   #
   # @informational: Information Event Log
   #
   # @warning: Warning Event Log
   #
   # @failure: Failure Event Log
   #
   # @fatal: Fatal Event Log
   #
   # Since: 8.1
   ##

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

The blank lines help with catching certain errors.  rST loves to
surprise...

> +{ 'enum': 'CxlEventLog',
> +  'data': ['informational',
> +           'warning',
> +           'failure',
> +           'fatal'
> +           ]

Make that
              'fatal']

> + }
> +
> +##
> +# @cxl-inject-gen-media-event:

Suggest cxl-inject-general-media-event, because we traditionally avoid
abbreviations in QMP, for better or worse.

> +#
> +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)

What's "CXL r3.0", and where could a reader find it?

Aside: the idea of a document with numbered section nested six levels
deep is kind of horrifying :)

Again, capitalize "General Media Event" only if it's a proper noun.  If
"CXL r3.0" capitalizes it this way, it is.

> +# This event type is reported via one of the event logs specified via
> +# the log parameter.
> +#
> +# @path: CXL type 3 device canonical QOM path
> +# @log: Event Log to add the event to

event log

> +# @flags: header flags

Either specify the header flags here, or point to specification.

> +# @physaddr: Physical Address

Perhaps "Guest physical address"

Address of what?

We have no consistent naming convention for guest physical addresses.  I
see @addr, @memaddr, @gpa.  Let's not add yet another name for the same
thing without need.

> +# @descriptor: Descriptor

No.

> +# @type: Type

No.

> +# @transaction-type: Transaction Type

No.

> +# @channel: Channel

No.

> +# @rank: Rank

No.

> +# @device: Device

No.

See, I can do too terse, too!

> +# @component-id: Device specific string

Probably no.

> +#
> +# Since: 8.1
> +##

Again, please format like

   ##
   # @cxl-inject-general-media-event:
   #
   # Inject an event record for a General Media Event (CXL r3.0
   # 8.2.9.2.1.1) This event type is reported via one of the event logs
   # specified via the log parameter.
   #
   # @path: CXL type 3 device canonical QOM path
   #
   # @log: Event Log to add the event to
   #
   # @flags: header flags
   #
   # @physaddr: Physical Address
   #
   # @descriptor: Descriptor
   #
   # @type: Type
   #
   # @transaction-type: Transaction Type
   #
   # @channel: Channel
   #
   # @rank: Rank
   #
   # @device: Device
   #
   # @component-id: Device specific string
   #
   # Since: 8.1
   ##

> +{ 'command': 'cxl-inject-gen-media-event',
> +  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags': 'uint8',
> +            'physaddr': 'uint64', 'descriptor': 'uint8',
> +            'type': 'uint8', 'transaction-type': 'uint8',
> +            '*channel': 'uint8', '*rank': 'uint8',
> +            '*device': 'uint32', '*component-id': 'str'
> +            }}

Make that

               '*device': 'uint32', '*component-id': 'str' } }

> +
>  ##
>  # @cxl-inject-poison:
>  #



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 6/7] hw/cxl/events: Add injection of DRAM events
  2023-04-23 16:51 ` [PATCH v5 6/7] hw/cxl/events: Add injection of DRAM events Jonathan Cameron via
@ 2023-05-22  7:43   ` Markus Armbruster
  2023-05-22 13:26     ` Jonathan Cameron via
  0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2023-05-22  7:43 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
	Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Daniel P . Berrangé,
	Eric Blake, Mike Maslenkin, Marc-André Lureau, Thomas Huth

Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:

> Defined in CXL r3.0 8.2.9.2.1.2 DRAM Event Record, this event
> provides information related to DRAM devices.
>
> Example injection command in QMP:
>
> { "execute": "cxl-inject-dram-event",
>     "arguments": {
>         "path": "/machine/peripheral/cxl-mem0",
>         "log": "informational",
>         "flags": 1,
>         "physaddr": 1000,
>         "descriptor": 3,
>         "type": 3,
>         "transaction-type": 192,
>         "channel": 3,
>         "rank": 17,
>         "nibble-mask": 37421234,
>         "bank-group": 7,
>         "bank": 11,
>         "row": 2,
>         "column": 77,
>         "correction-mask": [33, 44, 55,66]
>     }}
>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[...]

> index 9dcd308a49..190db58385 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -55,6 +55,41 @@
>              '*device': 'uint32', '*component-id': 'str'
>              }}
>  
> +##
> +# @cxl-inject-dram-event:
> +#
> +# Inject an event record for a DRAM Event (CXL r3.0 8.2.9.2.1.2)
> +# This event type is reported via one of the event logs specified via
> +# the log parameter.
> +#
> +# @path: CXL type 3 device canonical QOM path
> +# @log: Event Log to add the event to
> +# @flags: header flags
> +# @physaddr: Physical Address
> +# @descriptor: Descriptor
> +# @type: Type
> +# @transaction-type: Transaction Type
> +# @channel: Channel
> +# @rank: Rank
> +# @nibble-mask: Identify one or more nibbles that the error affects
> +# @bank-group: Bank group
> +# @bank: Bank
> +# @row: Row
> +# @column: Column

My comments on cxl-inject-gen-media-event's doc comment in the previous
patch apply.

> +# @correction-mask: Bits within each nibble. Used in order of bits set
> +#                   in the nibble-mask.  Up to 4 nibbles may be covered.
> +#
> +# Since: 8.1
> +##
> +{ 'command': 'cxl-inject-dram-event',
> +  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags': 'uint8',
> +            'physaddr': 'uint64', 'descriptor': 'uint8',
> +            'type': 'uint8', 'transaction-type': 'uint8',
> +            '*channel': 'uint8', '*rank': 'uint8', '*nibble-mask': 'uint32',
> +            '*bank-group': 'uint8', '*bank': 'uint8', '*row': 'uint32',
> +            '*column': 'uint16', '*correction-mask': [ 'uint64' ]
> +           }}
> +
>  ##
>  # @cxl-inject-poison:
>  #

Please format like

   ##
   # @cxl-inject-dram-event:
   #
   # Inject an event record for a DRAM Event (CXL r3.0 8.2.9.2.1.2) This
   # event type is reported via one of the event logs specified via the
   # log parameter.
   #
   # @path: CXL type 3 device canonical QOM path
   #
   # @log: Event Log to add the event to
   #
   # @flags: header flags
   #
   # @physaddr: Physical Address
   #
   # @descriptor: Descriptor
   #
   # @type: Type
   #
   # @transaction-type: Transaction Type
   #
   # @channel: Channel
   #
   # @rank: Rank
   #
   # @nibble-mask: Identify one or more nibbles that the error affects
   #
   # @bank-group: Bank group
   #
   # @bank: Bank
   #
   # @row: Row
   #
   # @column: Column
   #
   # @correction-mask: Bits within each nibble.  Used in order of bits
   #     set in the nibble-mask.  Up to 4 nibbles may be covered.
   #
   # Since: 8.1
   ##

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 7/7] hw/cxl/events: Add injection of Memory Module Events
  2023-04-23 16:51 ` [PATCH v5 7/7] hw/cxl/events: Add injection of Memory Module Events Jonathan Cameron via
  2023-05-19  7:06   ` Michael S. Tsirkin
@ 2023-05-22  7:47   ` Markus Armbruster
  1 sibling, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2023-05-22  7:47 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
	Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Daniel P . Berrangé,
	Eric Blake, Mike Maslenkin, Marc-André Lureau, Thomas Huth

Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:

> These events include a copy of the device health information at the
> time of the event. Actually using the emulated device health would
> require a lot of controls to manipulate that state.  Given the aim
> of this injection code is to just test the flows when events occur,
> inject the contents of the device health state as well.
>
> Future work may add more sophisticate device health emulation
> including direct generation of these records when events occur
> (such as a temperature threshold being crossed).  That does not
> reduce the usefulness of this more basic generation of the events.
>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[...]

> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index 190db58385..aae70667c2 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -90,6 +90,41 @@
>              '*column': 'uint16', '*correction-mask': [ 'uint64' ]
>             }}
>  
> +##
> +# @cxl-inject-memory-module-event:
> +#
> +# Inject an event record for a Memory Module Event (CXL r3.0 8.2.9.2.1.3)

End the sentence with a period, please.

> +# This event includes a copy of the Device Health info at the time of
> +# the event.
> +#
> +# @path: CXL type 3 device canonical QOM path
> +# @log: Event Log to add the event to
> +# @flags: header flags
> +# @type: Device Event Type (see spec for permitted values)
> +# @health-status: Overall health summary bitmap (see spec for permitted bits)
> +# @media-status: Overall media health summary (see spec for permitted values)
> +# @additional-status: Complex field (see spec for meaning)

"spec" is not a word.  Yes, typing out references gets tedious, but your
readers will appreciate tediously unambiguous references.

> +# @life-used: Percentage (0-100) of factory expected life span
> +# @temperature: Device temperature in degrees Celsius
> +# @dirty-shutdown-count: Counter incremented whenever device is unable

Suggest something like "number of times the device has been unable to
determine whether data loss ..."

> +#                        to determine if data loss may have occurred.
> +# @corrected-volatile-error-count: Total number of correctable errors in
> +#                                  volatile memory
> +# @corrected-persistent-error-count: Total number correctable errors in
> +#                                    persistent memory
> +#
> +# Since: 8.1
> +##


Please format like

   ##
   # @cxl-inject-memory-module-event:
   #
   # Inject an event record for a Memory Module Event (CXL r3.0
   # 8.2.9.2.1.3).  This event includes a copy of the Device Health info
   # at the time of the event.
   #
   # @path: CXL type 3 device canonical QOM path
   #
   # @log: Event Log to add the event to
   #
   # @flags: header flags
   #
   # @type: Device Event Type (see spec for permitted values)
   #
   # @health-status: Overall health summary bitmap (see spec for
   #     permitted bits)
   #
   # @media-status: Overall media health summary (see spec for permitted
   #     values)
   #
   # @additional-status: Complex field (see spec for meaning)
   #
   # @life-used: Percentage (0-100) of factory expected life span
   #
   # @temperature: Device temperature in degrees Celsius
   #
   # @dirty-shutdown-count: Counter incremented whenever device is unable
   #     to determine if data loss may have occurred.
   #
   # @corrected-volatile-error-count: Total number of correctable errors
   #     in volatile memory
   #
   # @corrected-persistent-error-count: Total number correctable errors
   #     in persistent memory
   #
   # Since: 8.1
   ##

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

> +{ 'command': 'cxl-inject-memory-module-event',
> +  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags' : 'uint8',
> +            'type': 'uint8', 'health-status': 'uint8',
> +            'media-status': 'uint8', 'additional-status': 'uint8',
> +            'life-used': 'uint8', 'temperature' : 'int16',
> +            'dirty-shutdown-count': 'uint32',
> +            'corrected-volatile-error-count': 'uint32',
> +            'corrected-persistent-error-count': 'uint32'
> +            }}
> +
>  ##
>  # @cxl-inject-poison:
>  #



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events
  2023-05-22  7:19   ` Markus Armbruster
@ 2023-05-22 12:57     ` Jonathan Cameron via
  2023-05-22 13:53       ` Jonathan Cameron via
  2023-05-23  8:10       ` Markus Armbruster
  0 siblings, 2 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-05-22 12:57 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
	Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Daniel P . Berrangé,
	Eric Blake, Mike Maslenkin, Marc-André Lureau, Thomas Huth

On Mon, 22 May 2023 09:19:57 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > To facilitate testing provide a QMP command to inject a general media
> > event.  The event can be added to the log specified.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> [...]
> 
> > diff --git a/qapi/cxl.json b/qapi/cxl.json
> > index ca3af3f0b2..9dcd308a49 100644
> > --- a/qapi/cxl.json
> > +++ b/qapi/cxl.json
> > @@ -5,6 +5,56 @@
> >  # = CXL devices
> >  ##
> >  
> > +##
> > +# @CxlEventLog:
> > +#
> > +# CXL has a number of separate event logs for different types of event.  
> 
> types of events
> 
> > +# Each such event log is handled and signaled independently.
> > +#
> > +# @informational: Information Event Log
> > +# @warning: Warning Event Log
> > +# @failure: Failure Event Log
> > +# @fatal: Fatal Event Log  
> 
> Are these proper nouns?  If not, the words should not be capitalized.

By your definition below of them being capitalized in the CXL spec then
yes, they are all proper nouns.


...

> > +#
> > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)  
> 
> What's "CXL r3.0", and where could a reader find it?

We have docs in docs/system/devices/cxl.rst that include the consortium
website which has download links on the front page.  I'm not sure we want to
have lots of references to the URL spread throughout QEMU.  I can add one
somewhere in cxl.json if you think it is important to have one here as well.

> 
> Aside: the idea of a document with numbered section nested six levels
> deep is kind of horrifying :)

Agreed!

> 
> Again, capitalize "General Media Event" only if it's a proper noun.  If
> "CXL r3.0" capitalizes it this way, it is.

It does capitalize it.

...

> 
> > +# @flags: header flags  
> 
> Either specify the header flags here, or point to specification.

Added a reference - same reason as below, the contents is being added to
with each version and we don't want to bake what is supported in this
interface if we can avoid it.

> 
> > +# @physaddr: Physical Address  
> 
> Perhaps "Guest physical address"
> 
> Address of what?

Changed already based on Phillipe's feedback on v6 to
Physical address (relative to @path device)

In CXL terms it's a Device Physical Address (DPA) which
are independent of the host (or guest) physical addresses with
runtime controllable mappings.
I'll change it to 

@dpa: Device Physical Address (relative to @path device)
(and Device Physical Address is capitalized like that in the CXL spec)

> 
> We have no consistent naming convention for guest physical addresses.  I
> see @addr, @memaddr, @gpa.  Let's not add yet another name for the same
> thing without need.

It's none of the above (except may addr which is so vague)

I'll change to dpa.

Also added a note that some of the lower bits encode flags
Not this is probably why the spec uses a different name - Physical
Address  to distinguish this from DPA - I'll keep that naming in the
implementation of the record, but it's not needed in the injection
interface where I think DPA is less confusing.

> 
> > +# @descriptor: Descriptor  
> 
> No.

Ok this indeed ended up sparse.

It is a tricky balance as I don't think it makes sense to just
duplicate large chunks of the spec. 
I'll have a go at summarizing what sort of things are in each.
As I mention below, we could break, these down fully at the cost
of constant updates as the CXL spec evolves to add new subfields
or values for existing fields.  This one for example currently has
3 bits, Uncorrectable Event, Threshold Event, Poison List Overflow event.
The next one currently has 3 bits defined as well, but there are 3 more
queued up for inclusion.

Realistically no one is going to write a descriptor without
looking at the specification for the field definitions and understanding
the physical geometry of their device (which will be device specific).

I'm fine with tweaking the balance though if you think that makes sense.

Jonathan





^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 6/7] hw/cxl/events: Add injection of DRAM events
  2023-05-22  7:43   ` Markus Armbruster
@ 2023-05-22 13:26     ` Jonathan Cameron via
  0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-05-22 13:26 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
	Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Daniel P . Berrangé,
	Eric Blake, Mike Maslenkin, Marc-André Lureau, Thomas Huth


>    # @bank-group: Bank group
>    #
>    # @bank: Bank

Just a quick note to point out that the CXL spec contains very little info
on some of these. They are considered generic enough DRAM terms that
people who care know what they are.  My explanations will be whatever
google gives me. Which brings risk of them being wrong.


>    #
>    # @row: Row
>    #
>    # @column: Column
>    #
>    # @correction-mask: Bits within each nibble.  Used in order of bits
>    #     set in the nibble-mask.  Up to 4 nibbles may be covered.
>    #
>    # Since: 8.1
>    ##
> 
> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
> to conform to current conventions).
> 



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events
  2023-05-22 12:57     ` Jonathan Cameron via
@ 2023-05-22 13:53       ` Jonathan Cameron via
  2023-05-23  8:10       ` Markus Armbruster
  1 sibling, 0 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-05-22 13:53 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, Ira Weiny,
	Alison Schofield, Michael Roth, Philippe Mathieu-Daudé,
	Dave Jiang, Daniel P . Berrangé, Eric Blake, Mike Maslenkin,
	Marc-André Lureau, Thomas Huth

On Mon, 22 May 2023 13:57:37 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon, 22 May 2023 09:19:57 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:
> >   
> > > From: Ira Weiny <ira.weiny@intel.com>
> > >
> > > To facilitate testing provide a QMP command to inject a general media
> > > event.  The event can be added to the log specified.
> > >
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>    
> > 
> > [...]
> >   
> > > diff --git a/qapi/cxl.json b/qapi/cxl.json
> > > index ca3af3f0b2..9dcd308a49 100644
> > > --- a/qapi/cxl.json
> > > +++ b/qapi/cxl.json
> > > @@ -5,6 +5,56 @@
> > >  # = CXL devices
> > >  ##
> > >  
> > > +##
> > > +# @CxlEventLog:
> > > +#
> > > +# CXL has a number of separate event logs for different types of event.    
> > 
> > types of events
> >   
> > > +# Each such event log is handled and signaled independently.
> > > +#
> > > +# @informational: Information Event Log
> > > +# @warning: Warning Event Log
> > > +# @failure: Failure Event Log
> > > +# @fatal: Fatal Event Log    
> > 
> > Are these proper nouns?  If not, the words should not be capitalized.  
> 
> By your definition below of them being capitalized in the CXL spec then
> yes, they are all proper nouns.
> 
> 
> ...
> 
> > > +#
> > > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)    
> > 
> > What's "CXL r3.0", and where could a reader find it?  
> 
> We have docs in docs/system/devices/cxl.rst that include the consortium
> website which has download links on the front page.  I'm not sure we want to
> have lots of references to the URL spread throughout QEMU.  I can add one
> somewhere in cxl.json if you think it is important to have one here as well.

FWIW I tried adding some top level docs by adding stuff directly under the
 = CXL devices 
at the top of the file and building the html docs.

Looks fine in the actual page, but is picked up in the index generation as
a heading we should be able to link to alongside the CxlEventLog and
command definitions.

So, I'll not do that for now.

Jonathan

> 
> > 
> > Aside: the idea of a document with numbered section nested six levels
> > deep is kind of horrifying :)  
> 
> Agreed!
> 
> > 
> > Again, capitalize "General Media Event" only if it's a proper noun.  If
> > "CXL r3.0" capitalizes it this way, it is.  
> 
> It does capitalize it.
> 
> ...
> 
> >   
> > > +# @flags: header flags    
> > 
> > Either specify the header flags here, or point to specification.  
> 
> Added a reference - same reason as below, the contents is being added to
> with each version and we don't want to bake what is supported in this
> interface if we can avoid it.
> 
> >   
> > > +# @physaddr: Physical Address    
> > 
> > Perhaps "Guest physical address"
> > 
> > Address of what?  
> 
> Changed already based on Phillipe's feedback on v6 to
> Physical address (relative to @path device)
> 
> In CXL terms it's a Device Physical Address (DPA) which
> are independent of the host (or guest) physical addresses with
> runtime controllable mappings.
> I'll change it to 
> 
> @dpa: Device Physical Address (relative to @path device)
> (and Device Physical Address is capitalized like that in the CXL spec)
> 
> > 
> > We have no consistent naming convention for guest physical addresses.  I
> > see @addr, @memaddr, @gpa.  Let's not add yet another name for the same
> > thing without need.  
> 
> It's none of the above (except may addr which is so vague)
> 
> I'll change to dpa.
> 
> Also added a note that some of the lower bits encode flags
> Not this is probably why the spec uses a different name - Physical
> Address  to distinguish this from DPA - I'll keep that naming in the
> implementation of the record, but it's not needed in the injection
> interface where I think DPA is less confusing.
> 
> >   
> > > +# @descriptor: Descriptor    
> > 
> > No.  
> 
> Ok this indeed ended up sparse.
> 
> It is a tricky balance as I don't think it makes sense to just
> duplicate large chunks of the spec. 
> I'll have a go at summarizing what sort of things are in each.
> As I mention below, we could break, these down fully at the cost
> of constant updates as the CXL spec evolves to add new subfields
> or values for existing fields.  This one for example currently has
> 3 bits, Uncorrectable Event, Threshold Event, Poison List Overflow event.
> The next one currently has 3 bits defined as well, but there are 3 more
> queued up for inclusion.
> 
> Realistically no one is going to write a descriptor without
> looking at the specification for the field definitions and understanding
> the physical geometry of their device (which will be device specific).
> 
> I'm fine with tweaking the balance though if you think that makes sense.
> 
> Jonathan
> 
> 
> 
> 



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events
  2023-05-22 12:57     ` Jonathan Cameron via
  2023-05-22 13:53       ` Jonathan Cameron via
@ 2023-05-23  8:10       ` Markus Armbruster
       [not found]         ` <20230523113543.00006a1f@Huawei.com>
  1 sibling, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2023-05-23  8:10 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
	Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, Daniel P . Berrangé,
	Eric Blake, Mike Maslenkin, Marc-André Lureau, Thomas Huth

Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:

> On Mon, 22 May 2023 09:19:57 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:
>> 
>> > From: Ira Weiny <ira.weiny@intel.com>
>> >
>> > To facilitate testing provide a QMP command to inject a general media
>> > event.  The event can be added to the log specified.
>> >
>> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
>> 
>> [...]
>> 
>> > diff --git a/qapi/cxl.json b/qapi/cxl.json
>> > index ca3af3f0b2..9dcd308a49 100644
>> > --- a/qapi/cxl.json
>> > +++ b/qapi/cxl.json
>> > @@ -5,6 +5,56 @@
>> >  # = CXL devices
>> >  ##
>> >  
>> > +##
>> > +# @CxlEventLog:
>> > +#
>> > +# CXL has a number of separate event logs for different types of event.  
>> 
>> types of events
>> 
>> > +# Each such event log is handled and signaled independently.
>> > +#
>> > +# @informational: Information Event Log
>> > +# @warning: Warning Event Log
>> > +# @failure: Failure Event Log
>> > +# @fatal: Fatal Event Log  
>> 
>> Are these proper nouns?  If not, the words should not be capitalized.
>
> By your definition below of them being capitalized in the CXL spec then
> yes, they are all proper nouns.

Good.

> ...
>
>> > +#
>> > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)  
>> 
>> What's "CXL r3.0", and where could a reader find it?
>
> We have docs in docs/system/devices/cxl.rst that include the consortium
> website which has download links on the front page.

cxl.rst has

    References
    ----------

     - Consortium website for specifications etc:
       http://www.computeexpresslink.org
     - Compute Express link Revision 2 specification, October 2020
     - CEDT CFMWS & QTG _DSM ECN May 2021

Should the second reference be updated to 3.0?  Exact title seems to be
"The Compute Express Link™ (CXL™) 3.0 specification".  Not sure we need
to bother with the "™" in a reference.

>                                                      I'm not sure we want to
> have lots of references to the URL spread throughout QEMU.  I can add one
> somewhere in cxl.json if you think it is important to have one here as well.

You could add an introduction right under the "# = CXL devices" heading,
and include a full reference to the specification there.  Suitably
abbreviated references like the ones you use in this patch should then
be fine.

Please link to cxl.rst, too: add a label to cxl.rst, :ref: it from
cxl.json.

>> Aside: the idea of a document with numbered section nested six levels
>> deep is kind of horrifying :)
>
> Agreed!
>
>> 
>> Again, capitalize "General Media Event" only if it's a proper noun.  If
>> "CXL r3.0" capitalizes it this way, it is.
>
> It does capitalize it.
>
> ...
>
>> 
>> > +# @flags: header flags  
>> 
>> Either specify the header flags here, or point to specification.
>
> Added a reference - same reason as below, the contents is being added to
> with each version and we don't want to bake what is supported in this
> interface if we can avoid it.

Symbolic flags are a much friendlier interface, but limit the interface
to what QEMU understands.

With a numeric encoding of flags, QEMU can serve as dumb transport
between peers who may understand more flags than QEMU does.  One peer is
the QMP client.  Who is the other peer?  Guest software?

Can flags be useful even though the QEMU device model doesn't understand
them?  Are they safe?  See below for a more general take on this.

>> > +# @physaddr: Physical Address  
>> 
>> Perhaps "Guest physical address"
>> 
>> Address of what?
>
> Changed already based on Phillipe's feedback on v6 to
> Physical address (relative to @path device)
>
> In CXL terms it's a Device Physical Address (DPA) which
> are independent of the host (or guest) physical addresses with
> runtime controllable mappings.
> I'll change it to 
>
> @dpa: Device Physical Address (relative to @path device)
> (and Device Physical Address is capitalized like that in the CXL spec)

Better, because the difference to guest physical address is now clear.

>> We have no consistent naming convention for guest physical addresses.  I
>> see @addr, @memaddr, @gpa.  Let's not add yet another name for the same
>> thing without need.
>
> It's none of the above (except may addr which is so vague)
>
> I'll change to dpa.
>
> Also added a note that some of the lower bits encode flags
> Not this is probably why the spec uses a different name - Physical
> Address  to distinguish this from DPA - I'll keep that naming in the
> implementation of the record, but it's not needed in the injection
> interface where I think DPA is less confusing.
>
>> 
>> > +# @descriptor: Descriptor  
>> 
>> No.
>
> Ok this indeed ended up sparse.
>
> It is a tricky balance as I don't think it makes sense to just
> duplicate large chunks of the spec. 
> I'll have a go at summarizing what sort of things are in each.
> As I mention below, we could break, these down fully at the cost
> of constant updates as the CXL spec evolves to add new subfields
> or values for existing fields.  This one for example currently has
> 3 bits, Uncorrectable Event, Threshold Event, Poison List Overflow event.
> The next one currently has 3 bits defined as well, but there are 3 more
> queued up for inclusion.
>
> Realistically no one is going to write a descriptor without
> looking at the specification for the field definitions and understanding
> the physical geometry of their device (which will be device specific).
>
> I'm fine with tweaking the balance though if you think that makes sense.

This is about picking an appropriate level of abstraction for the QMP
interface.

In your patch, it is basically a few named sequences of bits.  The
interface changes only when new named entities get added to the spec.
Spec revisions may also add new uses of existing entities' bits, but the
interface doesn't care.

The lowest imaginable level is a single sequence of bits.  Basically the
named bit sequences pasted together.  Now the interface changes only
when we run out of bits.  Mind, I'm merely exploring the limits here.

At higher levels we use symbols rather than bits.  This interface needs
to change when symbols get added to the spec.

I figure the deciding question is the QEMU device model's role in all
this.

When something can be used safely only when the device model knows it,
providing a symbolic interface doesn't add to the things QEMU needs to
know.  Moreover, the interface can't be misused.

For things where the device model acts as a dumb transport, i.e. only
management application and guest need to know it, not having to put he
knowledge into QEMU just to enable it to transport bits makes some
sense.  It may enable misuse.

So, can you tell us a bit more about what the device model needs to
know to function?



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events
       [not found]         ` <20230523113543.00006a1f@Huawei.com>
@ 2023-05-23 12:46           ` Markus Armbruster
  2023-05-24  9:11             ` Jonathan Cameron via
  0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2023-05-23 12:46 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
	Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, berrange, Eric Blake,
	Mike Maslenkin, Marc-André Lureau, Thomas Huth

Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:

>> >  
>> >> > +#
>> >> > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)    
>> >> 
>> >> What's "CXL r3.0", and where could a reader find it?  
>> >
>> > We have docs in docs/system/devices/cxl.rst that include the consortium
>> > website which has download links on the front page.  
>> 
>> cxl.rst has
>> 
>>     References
>>     ----------
>> 
>>      - Consortium website for specifications etc:
>>        http://www.computeexpresslink.org
>>      - Compute Express link Revision 2 specification, October 2020
>>      - CEDT CFMWS & QTG _DSM ECN May 2021
>> 
>> Should the second reference be updated to 3.0?  Exact title seems to be
>> "The Compute Express Link™ (CXL™) 3.0 specification".  Not sure we need
>> to bother with the "™" in a reference.
>
> Yes. On the todo list is to update all the references to latest released
> specification because old ones are unobtainable to non consortium members
> unless they grabbed a copy in the past.
>
> Annoyingly this will be a repeated requirement as new spec versions are released
> but the cadence should be fairly low.
>
>> 
>> >                                                      I'm not sure we want to
>> > have lots of references to the URL spread throughout QEMU.  I can add one
>> > somewhere in cxl.json if you think it is important to have one here as well.  
>> 
>> You could add an introduction right under the "# = CXL devices" heading,
>> and include a full reference to the specification there.  Suitably
>> abbreviated references like the ones you use in this patch should then
>> be fine.
>
> I tried doing that - it resulted in the index including an entry with all the text.
> So on the webpage, the contents list to the left includes whatever text you put
> in that block.
>  
> I'm not sure why, or how to fix that.

Show me what you tried (as a patch or something I can git-pull), and
I'll have a look.

[...]



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events
  2023-05-23 12:46           ` Markus Armbruster
@ 2023-05-24  9:11             ` Jonathan Cameron via
  2023-05-24  9:20               ` Jonathan Cameron via
  0 siblings, 1 reply; 20+ messages in thread
From: Jonathan Cameron via @ 2023-05-24  9:11 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
	Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, berrange, Eric Blake,
	Mike Maslenkin, Marc-André Lureau, Thomas Huth

On Tue, 23 May 2023 14:46:37 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:
> 
> >> >    
> >> >> > +#
> >> >> > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)      
> >> >> 
> >> >> What's "CXL r3.0", and where could a reader find it?    
> >> >
> >> > We have docs in docs/system/devices/cxl.rst that include the consortium
> >> > website which has download links on the front page.    
> >> 
> >> cxl.rst has
> >> 
> >>     References
> >>     ----------
> >> 
> >>      - Consortium website for specifications etc:
> >>        http://www.computeexpresslink.org
> >>      - Compute Express link Revision 2 specification, October 2020
> >>      - CEDT CFMWS & QTG _DSM ECN May 2021
> >> 
> >> Should the second reference be updated to 3.0?  Exact title seems to be
> >> "The Compute Express Link™ (CXL™) 3.0 specification".  Not sure we need
> >> to bother with the "™" in a reference.  
> >
> > Yes. On the todo list is to update all the references to latest released
> > specification because old ones are unobtainable to non consortium members
> > unless they grabbed a copy in the past.
> >
> > Annoyingly this will be a repeated requirement as new spec versions are released
> > but the cadence should be fairly low.
> >  
> >>   
> >> >                                                      I'm not sure we want to
> >> > have lots of references to the URL spread throughout QEMU.  I can add one
> >> > somewhere in cxl.json if you think it is important to have one here as well.    
> >> 
> >> You could add an introduction right under the "# = CXL devices" heading,
> >> and include a full reference to the specification there.  Suitably
> >> abbreviated references like the ones you use in this patch should then
> >> be fine.  
> >
> > I tried doing that - it resulted in the index including an entry with all the text.
> > So on the webpage, the contents list to the left includes whatever text you put
> > in that block.
> >  
> > I'm not sure why, or how to fix that.  
> 
> Show me what you tried (as a patch or something I can git-pull), and
> I'll have a look.
> 
> [...]
> 
Very simple to replicate the issue - see below.

Result of this is an extra
"Test text" entry in the index to the left of the html docs.

In qemu-qmp-ref.html we have

<li class="toctree-l3"><a class="reference internal" href="#qapidoc-3297">CXL devices</a><ul>
<li class="toctree-l4"><a class="reference internal" href="#qapidoc-3298">Test text</a></li>
<li class="toctree-l4"><a class="reference internal" href="#qapidoc-3301"><code class="docutils literal notranslate"><span class="pre">CxlEventLog</span></code> (Enum)</a></li>

diff --git a/qapi/cxl.json b/qapi/cxl.json
index 05c560cfe5..9f7f0485ae 100644
--- a/qapi/cxl.json
+++ b/qapi/cxl.json
@@ -3,6 +3,8 @@

 ##
 # = CXL devices
+#
+# Test text
 ##

 ##


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events
  2023-05-24  9:11             ` Jonathan Cameron via
@ 2023-05-24  9:20               ` Jonathan Cameron via
  0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron via @ 2023-05-24  9:20 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm,
	Ira Weiny, Alison Schofield, Michael Roth,
	Philippe Mathieu-Daudé, Dave Jiang, berrange, Eric Blake,
	Mike Maslenkin, Marc-André Lureau, Thomas Huth

On Wed, 24 May 2023 10:11:58 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Tue, 23 May 2023 14:46:37 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:
> > 
> > >> >    
> > >> >> > +#
> > >> >> > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)      
> > >> >> 
> > >> >> What's "CXL r3.0", and where could a reader find it?    
> > >> >
> > >> > We have docs in docs/system/devices/cxl.rst that include the consortium
> > >> > website which has download links on the front page.    
> > >> 
> > >> cxl.rst has
> > >> 
> > >>     References
> > >>     ----------
> > >> 
> > >>      - Consortium website for specifications etc:
> > >>        http://www.computeexpresslink.org
> > >>      - Compute Express link Revision 2 specification, October 2020
> > >>      - CEDT CFMWS & QTG _DSM ECN May 2021
> > >> 
> > >> Should the second reference be updated to 3.0?  Exact title seems to be
> > >> "The Compute Express Link™ (CXL™) 3.0 specification".  Not sure we need
> > >> to bother with the "™" in a reference.  
> > >
> > > Yes. On the todo list is to update all the references to latest released
> > > specification because old ones are unobtainable to non consortium members
> > > unless they grabbed a copy in the past.
> > >
> > > Annoyingly this will be a repeated requirement as new spec versions are released
> > > but the cadence should be fairly low.
> > >  
> > >>   
> > >> >                                                      I'm not sure we want to
> > >> > have lots of references to the URL spread throughout QEMU.  I can add one
> > >> > somewhere in cxl.json if you think it is important to have one here as well.    
> > >> 
> > >> You could add an introduction right under the "# = CXL devices" heading,
> > >> and include a full reference to the specification there.  Suitably
> > >> abbreviated references like the ones you use in this patch should then
> > >> be fine.  
> > >
> > > I tried doing that - it resulted in the index including an entry with all the text.
> > > So on the webpage, the contents list to the left includes whatever text you put
> > > in that block.
> > >  
> > > I'm not sure why, or how to fix that.  
> > 
> > Show me what you tried (as a patch or something I can git-pull), and
> > I'll have a look.
> > 
> > [...]
> > 
> Very simple to replicate the issue - see below.
> 
> Result of this is an extra
> "Test text" entry in the index to the left of the html docs.
> 
> In qemu-qmp-ref.html we have
> 
> <li class="toctree-l3"><a class="reference internal" href="#qapidoc-3297">CXL devices</a><ul>
> <li class="toctree-l4"><a class="reference internal" href="#qapidoc-3298">Test text</a></li>
> <li class="toctree-l4"><a class="reference internal" href="#qapidoc-3301"><code class="docutils literal notranslate"><span class="pre">CxlEventLog</span></code> (Enum)</a></li>

Experimented and dived in the code a bit more and got reminded of how bad my python is
(which is why I was avoiding looking at this :)
One option would be to give the extra text a meaningful title so that ends up in the contents list.

> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -3,6 +3,8 @@
> 
>  ##
>  # = CXL devices
> +#
> +# CXL Background
> +#
> +# Actually useful text.
>  ##
> 
>  ##

Can't do
== CXL Background

though to get it nicely formatted as then we loose all the actual entries from the
contents list.  I guess they dropped down one too many levels.

> 
> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index 05c560cfe5..9f7f0485ae 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -3,6 +3,8 @@
> 
>  ##
>  # = CXL devices
> +#
> +# Test text
>  ##
> 
>  ##



^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2023-05-24  9:21 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-23 16:51 [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via
2023-04-23 16:51 ` [PATCH v5 1/7] hw/cxl/events: Add event status register Jonathan Cameron via
2023-04-23 16:51 ` [PATCH v5 2/7] hw/cxl: Move CXLRetCode definition to cxl_device.h Jonathan Cameron via
2023-04-23 16:51 ` [PATCH v5 3/7] hw/cxl/events: Wire up get/clear event mailbox commands Jonathan Cameron via
2023-04-23 16:51 ` [PATCH v5 4/7] hw/cxl/events: Add event interrupt support Jonathan Cameron via
2023-04-23 16:51 ` [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events Jonathan Cameron via
2023-05-22  7:19   ` Markus Armbruster
2023-05-22 12:57     ` Jonathan Cameron via
2023-05-22 13:53       ` Jonathan Cameron via
2023-05-23  8:10       ` Markus Armbruster
     [not found]         ` <20230523113543.00006a1f@Huawei.com>
2023-05-23 12:46           ` Markus Armbruster
2023-05-24  9:11             ` Jonathan Cameron via
2023-05-24  9:20               ` Jonathan Cameron via
2023-04-23 16:51 ` [PATCH v5 6/7] hw/cxl/events: Add injection of DRAM events Jonathan Cameron via
2023-05-22  7:43   ` Markus Armbruster
2023-05-22 13:26     ` Jonathan Cameron via
2023-04-23 16:51 ` [PATCH v5 7/7] hw/cxl/events: Add injection of Memory Module Events Jonathan Cameron via
2023-05-19  7:06   ` Michael S. Tsirkin
2023-05-22  7:47   ` Markus Armbruster
2023-05-16 16:31 ` [PATCH v5 0/7] QEMU CXL Provide mock CXL events and irq support Jonathan Cameron via

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).