[PATCH -qemu 0/4] hw/cxl: Support for scan media

[PATCH -qemu 0/4] hw/cxl: Support for scan media

[Davidlohr Bueso]

Hello,

Patch 1 is a fix I found which should be picked up regardless of
the rest. As a side note, does it make sense to have a clear poison
equivalent for the qmp interface?

This is a (tardy) followup series to the rfc version[0]. With the
exception of the get scan media caps, the whole thing has been mostly
redone to 1) just use the qmp interface we have for poison injection and
extend it to add to a backup list when the poison limit is reached; and
2) The valid address range chunks obtained by scan media are moved from
the backup to a results list, which in turn is consumed by the get scan
media results operation.

As with the current state of things, DRAM Event Logs are not generated
for every new poisoned dpa.

Applies against https://gitlab.com/jic23/qemu/-/tree/cxl-2023-08-30

Testing - I hacked up kernel side equivalent to trigger the scan:

1. With an imaginary poison limit of 2 and 5 poisoned dpas.

<get-poison-list>
cxl_poison: memdev=mem1 host=0000:0d:00.0 serial=0 trace_type=List region= region_uuid=... hpa=0xffffffffffffffff dpa=0x800 dpa_length=0x40 source=Internal flags=Overflow  overflow_time=1694153028389446574
cxl_poison: memdev=mem1 host=0000:0d:00.0 serial=0 trace_type=List region= region_uuid=... hpa=0xffffffffffffffff dpa=0x200 dpa_length=0x40 source=Internal flags=Overflow overflow_time=1694153028389446574
<scan-media> + <get-poison-list>
cxl_poison: memdev=mem1 host=0000:0d:00.0 serial=0 trace_type=List region= region_uuid=... hpa=0xffffffffffffffff dpa=0x100 dpa_length=0x40 source=Internal flags=Overflow overflow_time=1694153028389446574
cxl_poison: memdev=mem1 host=0000:0d:00.0 serial=0 trace_type=List region= region_uuid=... hpa=0xffffffffffffffff dpa=0x80 dpa_length=0x40 source=Internal flags=Overflow overflow_time=1694153028389446574

2. With an imaginary poison limit of 2 and 3 poisoned dpas.
<get-poison-list>
cxl_poison: memdev=mem1 host=0000:0d:00.0 serial=0 trace_type=List region= region_uuid=... hpa=0xffffffffffffffff dpa=0x400 dpa_length=0x40 source=Internal flags=Overflow overflow_time=1694153398720616022
cxl_poison: memdev=mem1 host=0000:0d:00.0 serial=0 trace_type=List region= region_uuid=... hpa=0xffffffffffffffff dpa=0x80 dpa_length=0x40 source=Internal flags=Overflow overflow_time=1694153398720616022
<scan-media> + <get-poison-list>
cxl_poison: memdev=mem1 host=0000:0d:00.0 serial=0 trace_type=List region= region_uuid=... hpa=0xffffffffffffffff dpa=0x100 dpa_length=0x40 source=Internal flags= overflow_time=0

Thanks!

[0] https://lore.kernel.org/linux-cxl/20230426021418.10186-1-dave@stgolabs.net

Davidlohr Bueso (4):
  cxl/type3: Fix crash in set_cacheline()
  hw/cxl: Add get scan media capabilities cmd support
  hw/cxl: Add scan media cmd support
  hw/cxl: Add get scan media results cmd support

 hw/cxl/cxl-mailbox-utils.c  | 298 +++++++++++++++++++++++++++++++++++-
 hw/mem/cxl_type3.c          |  24 ++-
 include/hw/cxl/cxl_device.h |  10 ++
 3 files changed, 317 insertions(+), 15 deletions(-)

--
2.42.0

[PATCH 1/4] cxl/type3: Fix crash in set_cacheline()

[Davidlohr Bueso]

Use the correct vmr_size, otherwise a clear poison operation, for
example, can crash the emulator.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 hw/mem/cxl_type3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index fd9d134d468f..b90a7397d62f 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1417,7 +1417,7 @@ static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
         as = &ct3d->hostvmem_as;
     } else if (dpa_offset < vmr_size + pmr_size) {
         as = &ct3d->hostpmem_as;
-        dpa_offset -= vmr->size;
+        dpa_offset -= vmr_size;
     } else {
         as = &ct3d->dc.host_dc_as;
         dpa_offset -= (vmr_size + pmr_size);
-- 
2.42.0

[PATCH 2/4] hw/cxl: Add get scan media capabilities cmd support

[Davidlohr Bueso]

Use simple heuristics to determine the cost of scanning any given
chunk, assuming cost is equal across the whole device, without
differentiating between volatile or persistent partitions. This
is aligned to the fact that these constraints are not enforced
in respective poison query commands.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 hw/cxl/cxl-mailbox-utils.c | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 4e8651ebe2e9..3073222060ab 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -80,6 +80,7 @@ enum {
         #define GET_POISON_LIST        0x0
         #define INJECT_POISON          0x1
         #define CLEAR_POISON           0x2
+        #define GET_SCAN_MEDIA_CAPABILITIES 0x3
     DCD_CONFIG  = 0x48,
         #define GET_DC_CONFIG          0x0
         #define GET_DYN_CAP_EXT_LIST   0x1
@@ -1174,6 +1175,56 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+/*
+ * CXL r3.0 section 8.2.9.8.4.4: Get Scan Media Capabilities
+ */
+static CXLRetCode
+cmd_media_get_scan_media_capabilities(const struct cxl_cmd *cmd,
+                                      uint8_t *payload_in,
+                                      size_t len_in,
+                                      uint8_t *payload_out,
+                                      size_t *len_out,
+                                      CXLCCI *cci)
+{
+    struct get_scan_media_capabilities_pl {
+        uint64_t pa;
+        uint64_t length;
+    } QEMU_PACKED;
+
+    struct get_scan_media_capabilities_out_pl {
+        uint32_t estimated_runtime_ms;
+    } QEMU_PACKED;
+
+    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+    CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
+    struct get_scan_media_capabilities_pl *in = (void *)payload_in;
+    struct get_scan_media_capabilities_out_pl *out = (void *)payload_out;
+    uint64_t query_start;
+    uint64_t query_length;
+
+    query_start = ldq_le_p(&in->pa);
+    /* 64 byte alignment required */
+    if (query_start & 0x3f) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+    query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
+
+    if (query_start + query_length > cxl_dstate->static_mem_size) {
+        return CXL_MBOX_INVALID_PA;
+    }
+
+    /*
+     * Just use 400 nanosecond access/read latency + 100 ns for
+     * the cost of updating the poison list. For small enough
+     * chunks return at least 1 ms.
+     */
+    stl_le_p(&out->estimated_runtime_ms,
+             MAX(1, query_length * (0.0005L/64)));
+
+    *len_out = sizeof(*out);
+    return CXL_MBOX_SUCCESS;
+}
+
 /*
  * CXL r3.0 section 8.2.9.8.9.1: Dynamic Capacity Configuration
  */
@@ -1602,6 +1653,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
         cmd_media_inject_poison, 8, 0 },
     [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON",
         cmd_media_clear_poison, 72, 0 },
+    [MEDIA_AND_POISON][GET_SCAN_MEDIA_CAPABILITIES] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
+        cmd_media_get_scan_media_capabilities, 16, 0 },
 };
 
 static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
-- 
2.42.0

[PATCH 3/4] hw/cxl: Add scan media cmd support

[Davidlohr Bueso]

Implement the poison list backup functionality by extending the
qmp poison injection interface to add chunks beyond the poison
limit and use it to recreate the poison list upon overflow.

To this end two new lists are added, bkp and results, which are
drained by moving elements from the backup to the results, which
are consumed by the respective get scan media results cmd.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 hw/cxl/cxl-mailbox-utils.c  | 161 ++++++++++++++++++++++++++++++++++--
 hw/mem/cxl_type3.c          |  22 +++--
 include/hw/cxl/cxl_device.h |   9 ++
 3 files changed, 178 insertions(+), 14 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 3073222060ab..399ac03962db 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -81,6 +81,7 @@ enum {
         #define INJECT_POISON          0x1
         #define CLEAR_POISON           0x2
         #define GET_SCAN_MEDIA_CAPABILITIES 0x3
+        #define SCAN_MEDIA             0x4
     DCD_CONFIG  = 0x48,
         #define GET_DC_CONFIG          0x0
         #define GET_DYN_CAP_EXT_LIST   0x1
@@ -1037,6 +1038,10 @@ static CXLRetCode cmd_media_get_poison_list(const struct cxl_cmd *cmd,
         out->flags = (1 << 1);
         stq_le_p(&out->overflow_timestamp, ct3d->poison_list_overflow_ts);
     }
+    if (scan_media_running(cci)) {
+        out->flags |= (1 << 2);
+    }
+
     stw_le_p(&out->count, record_count);
     *len_out = out_pl_len;
     return CXL_MBOX_SUCCESS;
@@ -1065,6 +1070,16 @@ static CXLRetCode cmd_media_inject_poison(const struct cxl_cmd *cmd,
             return CXL_MBOX_SUCCESS;
         }
     }
+    /*
+     * Freeze the list if there is an on-going scan media operation.
+     */
+    if (scan_media_running(cci)) {
+        /*
+         * XXX: Spec is ambiguous - is this case considered
+         * a successful return despite not adding to the list?
+         */
+        goto success;
+    }
 
     if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
         return CXL_MBOX_INJECT_POISON_LIMIT;
@@ -1080,6 +1095,7 @@ static CXLRetCode cmd_media_inject_poison(const struct cxl_cmd *cmd,
      */
     QLIST_INSERT_HEAD(poison_list, p, node);
     ct3d->poison_list_cnt++;
+success:
     *len_out = 0;
 
     return CXL_MBOX_SUCCESS;
@@ -1123,6 +1139,17 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
         }
     }
 
+    /*
+     * Freeze the list if there is an on-going scan media operation.
+     */
+    if (scan_media_running(cci)) {
+        /*
+         * XXX: Spec is ambiguous - is this case considered
+         * a successful return despite not removing from the list?
+         */
+        goto success;
+    }
+
     QLIST_FOREACH(ent, poison_list, node) {
         /*
          * Test for contained in entry. Simpler than general case
@@ -1133,7 +1160,7 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
         }
     }
     if (!ent) {
-        return CXL_MBOX_SUCCESS;
+        goto success;
     }
 
     QLIST_REMOVE(ent, node);
@@ -1170,6 +1197,7 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
     }
     /* Any fragments have been added, free original entry */
     g_free(ent);
+success:
     *len_out = 0;
 
     return CXL_MBOX_SUCCESS;
@@ -1225,6 +1253,124 @@ cmd_media_get_scan_media_capabilities(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+static void __do_scan_media(CXLType3Dev *ct3d)
+{
+    CXLPoison *ent;
+    unsigned int results_cnt = 0;
+
+    QLIST_FOREACH(ent, &ct3d->scan_media_results, node) {
+        results_cnt++;
+    }
+
+    /* only scan media may clear the overflow */
+    if (ct3d->poison_list_overflowed &&
+        ct3d->poison_list_cnt == results_cnt) {
+        cxl_clear_poison_list_overflowed(ct3d);
+    }
+}
+
+/*
+ * CXL r3.0 section 8.2.9.8.4.5: Scan Media
+ */
+static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
+                                       uint8_t *payload_in,
+                                       size_t len_in,
+                                       uint8_t *payload_out,
+                                       size_t *len_out,
+                                       CXLCCI *cci)
+{
+    struct scan_media_pl {
+        uint64_t pa;
+        uint64_t length;
+        uint8_t flags;
+    } QEMU_PACKED;
+
+    struct scan_media_pl *in = (void *)payload_in;
+    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+    CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
+    uint64_t query_start;
+    uint64_t query_length;
+    CXLPoison *ent, *next;
+
+    query_start = ldq_le_p(&in->pa);
+    /* 64 byte alignment required */
+    if (query_start & 0x3f) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+    query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
+
+    if (query_start + query_length > cxl_dstate->static_mem_size) {
+        return CXL_MBOX_INVALID_PA;
+    }
+    if (ct3d->dc.num_regions && query_start + query_length >=
+            cxl_dstate->static_mem_size + ct3d->dc.total_capacity) {
+        return CXL_MBOX_INVALID_PA;
+    }
+
+    if (in->flags == 0) { /* TODO */
+        qemu_log_mask(LOG_UNIMP,
+                      "Scan Media Event Log is unsupported\n");
+    }
+
+    /* any previous results are discarded upon a new Scan Media */
+    QLIST_FOREACH_SAFE(ent, &ct3d->scan_media_results, node, next) {
+        QLIST_REMOVE(ent, node);
+        g_free(ent);
+    }
+
+    /* kill the poison list - it will be recreated */
+    if (ct3d->poison_list_overflowed) {
+        QLIST_FOREACH_SAFE(ent, &ct3d->poison_list, node, next) {
+            QLIST_REMOVE(ent, node);
+            g_free(ent);
+            ct3d->poison_list_cnt--;
+        }
+    }
+
+    /*
+     * Scan the backup list and move corresponding entries
+     * into the results list, updating the poison list
+     * when possible.
+     */
+    QLIST_FOREACH_SAFE(ent, &ct3d->poison_list_bkp, node, next) {
+        CXLPoison *res;
+
+        if (ent->start >= query_start + query_length ||
+            ent->start + ent->length <= query_start) {
+            continue;
+        }
+
+        /*
+         * If a Get Poison List cmd comes in while this
+         * scan is being done, it will see the new complete
+         * list, while setting the respective flag.
+         */
+        if (ct3d->poison_list_cnt < CXL_POISON_LIST_LIMIT) {
+            CXLPoison *p = g_new0(CXLPoison, 1);
+
+            p->start = ent->start;
+            p->length = ent->length;
+            p->type = ent->type;
+            QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
+            ct3d->poison_list_cnt++;
+        }
+
+        res = g_new0(CXLPoison, 1);
+        res->start = ent->start;
+        res->length = ent->length;
+        res->type = ent->type;
+        QLIST_INSERT_HEAD(&ct3d->scan_media_results, res, node);
+
+        QLIST_REMOVE(ent, node);
+        g_free(ent);
+    }
+
+    cci->bg.runtime = MAX(1, query_length * (0.0005L/64));
+    *len_out = 0;
+
+    return CXL_MBOX_BG_STARTED;
+}
+
 /*
  * CXL r3.0 section 8.2.9.8.9.1: Dynamic Capacity Configuration
  */
@@ -1655,6 +1801,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
         cmd_media_clear_poison, 72, 0 },
     [MEDIA_AND_POISON][GET_SCAN_MEDIA_CAPABILITIES] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
         cmd_media_get_scan_media_capabilities, 16, 0 },
+    [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA",
+        cmd_media_scan_media, 17, BACKGROUND_OPERATION },
 };
 
 static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
@@ -1783,16 +1931,15 @@ static void bg_timercb(void *opaque)
         cci->bg.complete_pct = 100;
         cci->bg.ret_code = ret;
         if (ret == CXL_MBOX_SUCCESS) {
+            CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+
             switch (cci->bg.opcode) {
             case 0x4400: /* sanitize */
-            {
-                CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
-
                 __do_sanitization(ct3d);
                 cxl_dev_enable_media(&ct3d->cxl_dstate);
-            }
-            break;
-            case 0x4304: /* TODO: scan media */
+                break;
+            case 0x4304: /* scan media */
+                __do_scan_media(ct3d);
                 break;
             default:
                 __builtin_unreachable();
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index b90a7397d62f..aa9fdde1436e 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1435,6 +1435,12 @@ void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d)
             cxl_device_get_timestamp(&ct3d->cxl_dstate);
 }
 
+void cxl_clear_poison_list_overflowed(CXLType3Dev *ct3d)
+{
+    ct3d->poison_list_overflowed = false;
+    ct3d->poison_list_overflow_ts = 0;
+}
+
 void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
                            Error **errp)
 {
@@ -1470,18 +1476,20 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
         }
     }
 
-    if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
-        cxl_set_poison_list_overflowed(ct3d);
-        return;
-    }
-
     p = g_new0(CXLPoison, 1);
     p->length = length;
     p->start = start;
     p->type = CXL_POISON_TYPE_INTERNAL; /* Different from injected via the mbox */
 
-    QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
-    ct3d->poison_list_cnt++;
+    if (ct3d->poison_list_cnt < CXL_POISON_LIST_LIMIT) {
+        QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
+        ct3d->poison_list_cnt++;
+    } else {
+        if (!ct3d->poison_list_overflowed) {
+            cxl_set_poison_list_overflowed(ct3d);
+        }
+        QLIST_INSERT_HEAD(&ct3d->poison_list_bkp, p, node);
+    }
 }
 
 /* For uncorrectable errors include support for multiple header recording */
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index e824c5ade8a1..eb5c5284fa9f 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -385,6 +385,11 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val)
 #define cxl_dev_enable_media(cxlds)                     \
         do { __toggle_media((cxlds), 0x1); } while (0)
 
+static inline bool scan_media_running(CXLCCI *cci)
+{
+    return !!cci->bg.runtime && cci->bg.opcode == 0x4304;
+}
+
 static inline bool sanitize_running(CXLCCI *cci)
 {
     return !!cci->bg.runtime && cci->bg.opcode == 0x4400;
@@ -476,6 +481,9 @@ struct CXLType3Dev {
     unsigned int poison_list_cnt;
     bool poison_list_overflowed;
     uint64_t poison_list_overflow_ts;
+    /* Poison Injection - backup */
+    CXLPoisonList poison_list_bkp;
+    CXLPoisonList scan_media_results;
 
     struct dynamic_capacity {
         HostMemoryBackend *host_dc;
@@ -538,6 +546,7 @@ CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
 void cxl_event_irq_assert(CXLType3Dev *ct3d);
 
 void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d);
+void cxl_clear_poison_list_overflowed(CXLType3Dev *ct3d);
 
 CXLDCDRegion *cxl_find_dc_region(CXLType3Dev *ct3d, uint64_t dpa, uint64_t len);
 
-- 
2.42.0

[PATCH 4/4] hw/cxl: Add get scan media results cmd support

[Davidlohr Bueso]

Iterate over the list keeping the output payload size into account,
returning the results from a previous scan media operation. The
scan media operation does not fail prematurely due to device being
out of storage, so this implementation does not deal with the
retry/restart functionality.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 hw/cxl/cxl-mailbox-utils.c  | 84 +++++++++++++++++++++++++++++++++++++
 include/hw/cxl/cxl_device.h |  1 +
 2 files changed, 85 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 399ac03962db..109b9ecfabf1 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -82,6 +82,7 @@ enum {
         #define CLEAR_POISON           0x2
         #define GET_SCAN_MEDIA_CAPABILITIES 0x3
         #define SCAN_MEDIA             0x4
+        #define GET_SCAN_MEDIA_RESULTS 0x5
     DCD_CONFIG  = 0x48,
         #define GET_DC_CONFIG          0x0
         #define GET_DYN_CAP_EXT_LIST   0x1
@@ -1267,6 +1268,8 @@ static void __do_scan_media(CXLType3Dev *ct3d)
         ct3d->poison_list_cnt == results_cnt) {
         cxl_clear_poison_list_overflowed(ct3d);
     }
+    /* scan media has run since last conventional reset */
+    ct3d->scan_media_hasrun = true;
 }
 
 /*
@@ -1371,6 +1374,85 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
     return CXL_MBOX_BG_STARTED;
 }
 
+/*
+ * CXL r3.0 section 8.2.9.8.4.6: Get Scan Media Results
+ */
+static CXLRetCode cmd_media_get_scan_media_results(const struct cxl_cmd *cmd,
+                                                   uint8_t *payload_in,
+                                                   size_t len_in,
+                                                   uint8_t *payload_out,
+                                                   size_t *len_out,
+                                                   CXLCCI *cci)
+{
+    struct get_scan_media_results_out_pl {
+        uint64_t dpa_restart;
+        uint64_t length;
+        uint8_t flags;
+        uint8_t rsvd1;
+        uint16_t count;
+        uint8_t rsvd2[0xc];
+        struct {
+            uint64_t addr;
+            uint32_t length;
+            uint32_t resv;
+        } QEMU_PACKED records[];
+    } QEMU_PACKED;
+
+    struct get_scan_media_results_out_pl *out = (void *)payload_out;
+    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+    CXLPoisonList *scan_media_results = &ct3d->scan_media_results;
+    CXLPoison *ent, *next;
+    uint16_t total_count = 0, record_count = 0, i = 0;
+    uint16_t out_pl_len;
+
+    if (!ct3d->scan_media_hasrun) {
+        return CXL_MBOX_UNSUPPORTED;
+    }
+
+    /*
+     * Calculate limits, all entries are within the same
+     * address range of the last scan media call.
+     */
+    QLIST_FOREACH(ent, scan_media_results, node) {
+        size_t rec_size = record_count * sizeof(out->records[0]);
+
+        if (sizeof(*out) + rec_size < CXL_MAILBOX_MAX_PAYLOAD_SIZE) {
+            record_count++;
+        }
+        total_count++;
+    }
+
+    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
+    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
+
+    memset(out, 0, out_pl_len);
+    QLIST_FOREACH_SAFE(ent, scan_media_results, node, next) {
+        uint64_t start, stop;
+
+        if (i == record_count) {
+            break;
+        }
+
+        start = ROUND_DOWN(ent->start, 64ull);
+        stop = ROUND_DOWN(ent->start, 64ull) + ent->length;
+        stq_le_p(&out->records[i].addr, start | (ent->type & 0x7));
+        stl_le_p(&out->records[i].length, (stop - start) / CXL_CACHE_LINE_SIZE);
+        i++;
+
+        /* consume the returning entry */
+        QLIST_REMOVE(ent, node);
+        g_free(ent);
+    }
+
+    stw_le_p(&out->count, record_count);
+    if (total_count > record_count) {
+        out->flags = (1 << 0); /* More Media Error Records */
+    }
+
+    *len_out = out_pl_len;
+    return CXL_MBOX_SUCCESS;
+}
+
 /*
  * CXL r3.0 section 8.2.9.8.9.1: Dynamic Capacity Configuration
  */
@@ -1803,6 +1885,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
         cmd_media_get_scan_media_capabilities, 16, 0 },
     [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA",
         cmd_media_scan_media, 17, BACKGROUND_OPERATION },
+    [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
+        cmd_media_get_scan_media_results, 0, 0 },
 };
 
 static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index eb5c5284fa9f..e9d130e5c988 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -484,6 +484,7 @@ struct CXLType3Dev {
     /* Poison Injection - backup */
     CXLPoisonList poison_list_bkp;
     CXLPoisonList scan_media_results;
+    bool scan_media_hasrun;
 
     struct dynamic_capacity {
         HostMemoryBackend *host_dc;
-- 
2.42.0

Re: [PATCH 1/4] cxl/type3: Fix crash in set_cacheline()

[Fan Ni]

On Fri, Sep 08, 2023 at 12:31:49AM -0700, Davidlohr Bueso wrote:
> Use the correct vmr_size, otherwise a clear poison operation, for
> example, can crash the emulator.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  hw/mem/cxl_type3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index fd9d134d468f..b90a7397d62f 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1417,7 +1417,7 @@ static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
>          as = &ct3d->hostvmem_as;
>      } else if (dpa_offset < vmr_size + pmr_size) {
>          as = &ct3d->hostpmem_as;
> -        dpa_offset -= vmr->size;
> +        dpa_offset -= vmr_size;

Good catch. It is a typo from my DCD patch, not upstreamed yet.

Fan
>      } else {
>          as = &ct3d->dc.host_dc_as;
>          dpa_offset -= (vmr_size + pmr_size);
> --
> 2.42.0
>

Re: [PATCH 2/4] hw/cxl: Add get scan media capabilities cmd support

[Fan Ni]

On Fri, Sep 08, 2023 at 12:31:50AM -0700, Davidlohr Bueso wrote:
> Use simple heuristics to determine the cost of scanning any given
> chunk, assuming cost is equal across the whole device, without
> differentiating between volatile or persistent partitions. This
> is aligned to the fact that these constraints are not enforced
> in respective poison query commands.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  hw/cxl/cxl-mailbox-utils.c | 53 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 4e8651ebe2e9..3073222060ab 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -80,6 +80,7 @@ enum {
>          #define GET_POISON_LIST        0x0
>          #define INJECT_POISON          0x1
>          #define CLEAR_POISON           0x2
> +        #define GET_SCAN_MEDIA_CAPABILITIES 0x3
>      DCD_CONFIG  = 0x48,
>          #define GET_DC_CONFIG          0x0
>          #define GET_DYN_CAP_EXT_LIST   0x1
> @@ -1174,6 +1175,56 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>
> +/*
> + * CXL r3.0 section 8.2.9.8.4.4: Get Scan Media Capabilities
> + */
> +static CXLRetCode
> +cmd_media_get_scan_media_capabilities(const struct cxl_cmd *cmd,
> +                                      uint8_t *payload_in,
> +                                      size_t len_in,
> +                                      uint8_t *payload_out,
> +                                      size_t *len_out,
> +                                      CXLCCI *cci)
> +{
> +    struct get_scan_media_capabilities_pl {
> +        uint64_t pa;
> +        uint64_t length;
> +    } QEMU_PACKED;
> +
> +    struct get_scan_media_capabilities_out_pl {
> +        uint32_t estimated_runtime_ms;
> +    } QEMU_PACKED;
> +
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +    CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
> +    struct get_scan_media_capabilities_pl *in = (void *)payload_in;
> +    struct get_scan_media_capabilities_out_pl *out = (void *)payload_out;
> +    uint64_t query_start;
> +    uint64_t query_length;
> +
> +    query_start = ldq_le_p(&in->pa);
> +    /* 64 byte alignment required */
> +    if (query_start & 0x3f) {
> +        return CXL_MBOX_INVALID_INPUT;

From the spec, it is not clear to me whether we should return invalid
input or invalid PA.

Fan

> +    }
> +    query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
> +
> +    if (query_start + query_length > cxl_dstate->static_mem_size) {
> +        return CXL_MBOX_INVALID_PA;
> +    }
> +
> +    /*
> +     * Just use 400 nanosecond access/read latency + 100 ns for
> +     * the cost of updating the poison list. For small enough
> +     * chunks return at least 1 ms.
> +     */
> +    stl_le_p(&out->estimated_runtime_ms,
> +             MAX(1, query_length * (0.0005L/64)));
> +
> +    *len_out = sizeof(*out);
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  /*
>   * CXL r3.0 section 8.2.9.8.9.1: Dynamic Capacity Configuration
>   */
> @@ -1602,6 +1653,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>          cmd_media_inject_poison, 8, 0 },
>      [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON",
>          cmd_media_clear_poison, 72, 0 },
> +    [MEDIA_AND_POISON][GET_SCAN_MEDIA_CAPABILITIES] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
> +        cmd_media_get_scan_media_capabilities, 16, 0 },
>  };
>
>  static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
> --
> 2.42.0
>

Re: [PATCH 2/4] hw/cxl: Add get scan media capabilities cmd support

[Davidlohr Bueso]

On Fri, 08 Sep 2023, Fan Ni wrote:

>On Fri, Sep 08, 2023 at 12:31:50AM -0700, Davidlohr Bueso wrote:
>> +static CXLRetCode
>> +cmd_media_get_scan_media_capabilities(const struct cxl_cmd *cmd,
>> +                                      uint8_t *payload_in,
>> +                                      size_t len_in,
>> +                                      uint8_t *payload_out,
>> +                                      size_t *len_out,
>> +                                      CXLCCI *cci)
>> +{
>> +    struct get_scan_media_capabilities_pl {
>> +        uint64_t pa;
>> +        uint64_t length;
>> +    } QEMU_PACKED;
>> +
>> +    struct get_scan_media_capabilities_out_pl {
>> +        uint32_t estimated_runtime_ms;
>> +    } QEMU_PACKED;
>> +
>> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
>> +    CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
>> +    struct get_scan_media_capabilities_pl *in = (void *)payload_in;
>> +    struct get_scan_media_capabilities_out_pl *out = (void *)payload_out;
>> +    uint64_t query_start;
>> +    uint64_t query_length;
>> +
>> +    query_start = ldq_le_p(&in->pa);
>> +    /* 64 byte alignment required */
>> +    if (query_start & 0x3f) {
>> +        return CXL_MBOX_INVALID_INPUT;
>
>From the spec, it is not clear to me whether we should return invalid
>input or invalid PA.

The way I interpret it, which is also what get poison list does, is
invalid input is better served as invalid pa mostly means that the
actual address is invalid to the dpa space in question.

Thanks,
Davidlohr

Re: [PATCH 1/4] cxl/type3: Fix crash in set_cacheline()

[Jonathan Cameron]

On Fri, 8 Sep 2023 11:37:31 -0700
Fan Ni <fan.ni@gmx.us> wrote:

> On Fri, Sep 08, 2023 at 12:31:49AM -0700, Davidlohr Bueso wrote:
> > Use the correct vmr_size, otherwise a clear poison operation, for
> > example, can crash the emulator.
> >
> > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> > ---
> >  hw/mem/cxl_type3.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index fd9d134d468f..b90a7397d62f 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -1417,7 +1417,7 @@ static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
> >          as = &ct3d->hostvmem_as;
> >      } else if (dpa_offset < vmr_size + pmr_size) {
> >          as = &ct3d->hostpmem_as;
> > -        dpa_offset -= vmr->size;
> > +        dpa_offset -= vmr_size;  
> 
> Good catch. It is a typo from my DCD patch, not upstreamed yet.
In meantime I squish this into the version of your series I'm carrying and
push out a new tree later today.

Thanks,

Jonathan

> 
> Fan
> >      } else {
> >          as = &ct3d->dc.host_dc_as;
> >          dpa_offset -= (vmr_size + pmr_size);
> > --
> > 2.42.0
> >  

Re: [PATCH 2/4] hw/cxl: Add get scan media capabilities cmd support

[Jonathan Cameron]

On Fri,  8 Sep 2023 00:31:50 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Use simple heuristics to determine the cost of scanning any given
> chunk, assuming cost is equal across the whole device, without
> differentiating between volatile or persistent partitions. This
> is aligned to the fact that these constraints are not enforced
> in respective poison query commands.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Looks good to me I'll queue this up on my gitlab tree for now
given it is dependent on the CCI rework at least.

I should figure out how to move that forward independent of
solving the question of how to provide the MCTP hosts adapters.
Still need the NVME series to land first though and seems that
has had some more feedback today.

I might shuffle the tree around to pull this ahead of DCD
(possibly carrying the rename of static_mem_size along with
 it to simplify the rebase).  I suspect this will be easier
to get upstream as it is short and sweet.

Jonathan

> ---
>  hw/cxl/cxl-mailbox-utils.c | 53 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 4e8651ebe2e9..3073222060ab 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -80,6 +80,7 @@ enum {
>          #define GET_POISON_LIST        0x0
>          #define INJECT_POISON          0x1
>          #define CLEAR_POISON           0x2
> +        #define GET_SCAN_MEDIA_CAPABILITIES 0x3
>      DCD_CONFIG  = 0x48,
>          #define GET_DC_CONFIG          0x0
>          #define GET_DYN_CAP_EXT_LIST   0x1
> @@ -1174,6 +1175,56 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +/*
> + * CXL r3.0 section 8.2.9.8.4.4: Get Scan Media Capabilities
> + */
> +static CXLRetCode
> +cmd_media_get_scan_media_capabilities(const struct cxl_cmd *cmd,
> +                                      uint8_t *payload_in,
> +                                      size_t len_in,
> +                                      uint8_t *payload_out,
> +                                      size_t *len_out,
> +                                      CXLCCI *cci)
> +{
> +    struct get_scan_media_capabilities_pl {
> +        uint64_t pa;
> +        uint64_t length;
> +    } QEMU_PACKED;
> +
> +    struct get_scan_media_capabilities_out_pl {
> +        uint32_t estimated_runtime_ms;
> +    } QEMU_PACKED;
> +
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +    CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
> +    struct get_scan_media_capabilities_pl *in = (void *)payload_in;
> +    struct get_scan_media_capabilities_out_pl *out = (void *)payload_out;
> +    uint64_t query_start;
> +    uint64_t query_length;
> +
> +    query_start = ldq_le_p(&in->pa);
> +    /* 64 byte alignment required */
> +    if (query_start & 0x3f) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +    query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
> +
> +    if (query_start + query_length > cxl_dstate->static_mem_size) {
> +        return CXL_MBOX_INVALID_PA;
> +    }
> +
> +    /*
> +     * Just use 400 nanosecond access/read latency + 100 ns for
> +     * the cost of updating the poison list. For small enough
> +     * chunks return at least 1 ms.
> +     */
> +    stl_le_p(&out->estimated_runtime_ms,
> +             MAX(1, query_length * (0.0005L/64)));
> +
> +    *len_out = sizeof(*out);
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  /*
>   * CXL r3.0 section 8.2.9.8.9.1: Dynamic Capacity Configuration
>   */
> @@ -1602,6 +1653,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>          cmd_media_inject_poison, 8, 0 },
>      [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON",
>          cmd_media_clear_poison, 72, 0 },
> +    [MEDIA_AND_POISON][GET_SCAN_MEDIA_CAPABILITIES] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
> +        cmd_media_get_scan_media_capabilities, 16, 0 },
>  };
>  
>  static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {

Re: [PATCH 2/4] hw/cxl: Add get scan media capabilities cmd support

[Jonathan Cameron]

On Fri,  8 Sep 2023 00:31:50 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Use simple heuristics to determine the cost of scanning any given
> chunk, assuming cost is equal across the whole device, without
> differentiating between volatile or persistent partitions. This
> is aligned to the fact that these constraints are not enforced
> in respective poison query commands.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  hw/cxl/cxl-mailbox-utils.c | 53 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 4e8651ebe2e9..3073222060ab 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -80,6 +80,7 @@ enum {
>          #define GET_POISON_LIST        0x0
>          #define INJECT_POISON          0x1
>          #define CLEAR_POISON           0x2
> +        #define GET_SCAN_MEDIA_CAPABILITIES 0x3
>      DCD_CONFIG  = 0x48,
>          #define GET_DC_CONFIG          0x0
>          #define GET_DYN_CAP_EXT_LIST   0x1
> @@ -1174,6 +1175,56 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +/*
> + * CXL r3.0 section 8.2.9.8.4.4: Get Scan Media Capabilities
> + */
> +static CXLRetCode
> +cmd_media_get_scan_media_capabilities(const struct cxl_cmd *cmd,
> +                                      uint8_t *payload_in,
> +                                      size_t len_in,
> +                                      uint8_t *payload_out,
> +                                      size_t *len_out,
> +                                      CXLCCI *cci)
> +{
> +    struct get_scan_media_capabilities_pl {
> +        uint64_t pa;
> +        uint64_t length;
> +    } QEMU_PACKED;
> +
> +    struct get_scan_media_capabilities_out_pl {
> +        uint32_t estimated_runtime_ms;
> +    } QEMU_PACKED;
> +
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +    CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
> +    struct get_scan_media_capabilities_pl *in = (void *)payload_in;
> +    struct get_scan_media_capabilities_out_pl *out = (void *)payload_out;
> +    uint64_t query_start;
> +    uint64_t query_length;
> +
> +    query_start = ldq_le_p(&in->pa);
> +    /* 64 byte alignment required */
> +    if (query_start & 0x3f) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +    query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
> +
> +    if (query_start + query_length > cxl_dstate->static_mem_size) {
> +        return CXL_MBOX_INVALID_PA;
> +    }
> +
> +    /*
> +     * Just use 400 nanosecond access/read latency + 100 ns for
> +     * the cost of updating the poison list. For small enough
> +     * chunks return at least 1 ms.
> +     */
> +    stl_le_p(&out->estimated_runtime_ms,
> +             MAX(1, query_length * (0.0005L/64)));
Forgot to say I added spaces around the /

> +
> +    *len_out = sizeof(*out);
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  /*
>   * CXL r3.0 section 8.2.9.8.9.1: Dynamic Capacity Configuration
>   */
> @@ -1602,6 +1653,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>          cmd_media_inject_poison, 8, 0 },
>      [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON",
>          cmd_media_clear_poison, 72, 0 },
> +    [MEDIA_AND_POISON][GET_SCAN_MEDIA_CAPABILITIES] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
I rewrapped this to keep under 80 chars.

Jonathan

> +        cmd_media_get_scan_media_capabilities, 16, 0 },
>  };
>  
>  static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {

Re: [PATCH 3/4] hw/cxl: Add scan media cmd support

[Jonathan Cameron]

On Fri,  8 Sep 2023 00:31:51 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Implement the poison list backup functionality by extending the
> qmp poison injection interface to add chunks beyond the poison
> limit and use it to recreate the poison list upon overflow.
> 
> To this end two new lists are added, bkp and results, which are
> drained by moving elements from the backup to the results, which
> are consumed by the respective get scan media results cmd.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Hi Davidlohr,

A few comments inline, but all are superficial so I'll start carrying this
on my tree.  Note I haven't yet tidied anything I commented on up.

Jonathan

> ---
>  hw/cxl/cxl-mailbox-utils.c  | 161 ++++++++++++++++++++++++++++++++++--
>  hw/mem/cxl_type3.c          |  22 +++--
>  include/hw/cxl/cxl_device.h |   9 ++
>  3 files changed, 178 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 3073222060ab..399ac03962db 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -81,6 +81,7 @@ enum {
>          #define INJECT_POISON          0x1
>          #define CLEAR_POISON           0x2
>          #define GET_SCAN_MEDIA_CAPABILITIES 0x3
> +        #define SCAN_MEDIA             0x4
>      DCD_CONFIG  = 0x48,
>          #define GET_DC_CONFIG          0x0
>          #define GET_DYN_CAP_EXT_LIST   0x1
> @@ -1037,6 +1038,10 @@ static CXLRetCode cmd_media_get_poison_list(const struct cxl_cmd *cmd,
>          out->flags = (1 << 1);
>          stq_le_p(&out->overflow_timestamp, ct3d->poison_list_overflow_ts);
>      }
> +    if (scan_media_running(cci)) {
> +        out->flags |= (1 << 2);

Define needed for the flag.

> +    }
> +
>      stw_le_p(&out->count, record_count);
>      *len_out = out_pl_len;
>      return CXL_MBOX_SUCCESS;
> @@ -1065,6 +1070,16 @@ static CXLRetCode cmd_media_inject_poison(const struct cxl_cmd *cmd,
>              return CXL_MBOX_SUCCESS;
>          }
>      }
> +    /*
> +     * Freeze the list if there is an on-going scan media operation.
> +     */
> +    if (scan_media_running(cci)) {
> +        /*
> +         * XXX: Spec is ambiguous - is this case considered
> +         * a successful return despite not adding to the list?
> +         */

As below. Can't see this one getting clarified.

> +        goto success;
> +    }
>  
>      if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
>          return CXL_MBOX_INJECT_POISON_LIMIT;
> @@ -1080,6 +1095,7 @@ static CXLRetCode cmd_media_inject_poison(const struct cxl_cmd *cmd,
>       */
>      QLIST_INSERT_HEAD(poison_list, p, node);
>      ct3d->poison_list_cnt++;
> +success:
>      *len_out = 0;
>  
>      return CXL_MBOX_SUCCESS;
> @@ -1123,6 +1139,17 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
>          }
>      }
>  
> +    /*
> +     * Freeze the list if there is an on-going scan media operation.

Single line comment.

> +     */
> +    if (scan_media_running(cci)) {
> +        /*
> +         * XXX: Spec is ambiguous - is this case considered
> +         * a successful return despite not removing from the list?
> +         */

I think we are in impdef territory here, though happy to join the conversation
if you want to raise it in CXL consortium.  Would need an errata though so
I suspect it will stay impdef with likely reasoning being.

OS should only use scan_media if it know poison list is overflowed.
It should know it issued scan media.
It should not do anything so silly has hit the device with a poison clear
in parallel to that.  Hence doesn't matter that this corner is impdef.

I'd like to see a comment to say it is though!

> +        goto success;
> +    }
> +
>      QLIST_FOREACH(ent, poison_list, node) {
>          /*
>           * Test for contained in entry. Simpler than general case
> @@ -1133,7 +1160,7 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
>          }
>      }
>      if (!ent) {
> -        return CXL_MBOX_SUCCESS;
> +        goto success;

Is this a bug fix?  I think the explicit setting of len_out came in somewhere but don't
think it is necessary for commands that have no output payload.  If it is, then we should
set it at the top of the function as they never have a payload.

>      }
>  
>      QLIST_REMOVE(ent, node);
> @@ -1170,6 +1197,7 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
>      }
>      /* Any fragments have been added, free original entry */
>      g_free(ent);
> +success:
>      *len_out = 0;
>  
>      return CXL_MBOX_SUCCESS;
> @@ -1225,6 +1253,124 @@ cmd_media_get_scan_media_capabilities(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +static void __do_scan_media(CXLType3Dev *ct3d)
> +{
> +    CXLPoison *ent;
> +    unsigned int results_cnt = 0;
> +
> +    QLIST_FOREACH(ent, &ct3d->scan_media_results, node) {
> +        results_cnt++;
> +    }
> +
> +    /* only scan media may clear the overflow */
> +    if (ct3d->poison_list_overflowed &&
> +        ct3d->poison_list_cnt == results_cnt) {
> +        cxl_clear_poison_list_overflowed(ct3d);
> +    }
> +}
> +
> +/*
> + * CXL r3.0 section 8.2.9.8.4.5: Scan Media
> + */
> +static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
> +                                       uint8_t *payload_in,
> +                                       size_t len_in,
> +                                       uint8_t *payload_out,
> +                                       size_t *len_out,
> +                                       CXLCCI *cci)
> +{
> +    struct scan_media_pl {
> +        uint64_t pa;
> +        uint64_t length;
> +        uint8_t flags;
> +    } QEMU_PACKED;
> +
> +    struct scan_media_pl *in = (void *)payload_in;
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +    CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
> +    uint64_t query_start;
> +    uint64_t query_length;
> +    CXLPoison *ent, *next;
> +
> +    query_start = ldq_le_p(&in->pa);
> +    /* 64 byte alignment required */
> +    if (query_start & 0x3f) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +    query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
> +
> +    if (query_start + query_length > cxl_dstate->static_mem_size) {
> +        return CXL_MBOX_INVALID_PA;
> +    }
> +    if (ct3d->dc.num_regions && query_start + query_length >=
> +            cxl_dstate->static_mem_size + ct3d->dc.total_capacity) {

If in DCD range we will never get to this test as we already failed
on matching against static_mem_size above.

Can we just drop the earlier test?

> +        return CXL_MBOX_INVALID_PA;
> +    }
> +
> +    if (in->flags == 0) { /* TODO */
Good to separate match on first bit vs seeing reserved bits.
Makes no difference today, but will make things potentially easier to extend
in long run.

> +        qemu_log_mask(LOG_UNIMP,
> +                      "Scan Media Event Log is unsupported\n");
> +    }
> +
> +    /* any previous results are discarded upon a new Scan Media */
> +    QLIST_FOREACH_SAFE(ent, &ct3d->scan_media_results, node, next) {
> +        QLIST_REMOVE(ent, node);
> +        g_free(ent);
> +    }
> +
> +    /* kill the poison list - it will be recreated */

Perhaps call out that the spec says a device 'may update it's Poison List...'

> +    if (ct3d->poison_list_overflowed) {
> +        QLIST_FOREACH_SAFE(ent, &ct3d->poison_list, node, next) {
> +            QLIST_REMOVE(ent, node);
> +            g_free(ent);
> +            ct3d->poison_list_cnt--;
> +        }
> +    }
> +
> +    /*
> +     * Scan the backup list and move corresponding entries
> +     * into the results list, updating the poison list
> +     * when possible.
> +     */
> +    QLIST_FOREACH_SAFE(ent, &ct3d->poison_list_bkp, node, next) {
> +        CXLPoison *res;
> +
> +        if (ent->start >= query_start + query_length ||
> +            ent->start + ent->length <= query_start) {
> +            continue;
> +        }
> +
> +        /*
> +         * If a Get Poison List cmd comes in while this
> +         * scan is being done, it will see the new complete
> +         * list, while setting the respective flag.

I'd be more specific
"while setting the Scan Media in Progress flag in the Get Poison
List Output Payload."


> +         */
> +        if (ct3d->poison_list_cnt < CXL_POISON_LIST_LIMIT) {
> +            CXLPoison *p = g_new0(CXLPoison, 1);
> +
> +            p->start = ent->start;
> +            p->length = ent->length;
> +            p->type = ent->type;
> +            QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
> +            ct3d->poison_list_cnt++;

Maybe worth a utility function for this adding one to the poison
list.  Or duplicate the memory and insert that to the poison list?

> +        }
> +
> +        res = g_new0(CXLPoison, 1);
> +        res->start = ent->start;
> +        res->length = ent->length;
> +        res->type = ent->type;
> +        QLIST_INSERT_HEAD(&ct3d->scan_media_results, res, node);
> +
> +        QLIST_REMOVE(ent, node);

Can't we just remove from the poison_list_bkp first then add it
to the scan_media_results list?  I.e. Don't make a new one.

> +        g_free(ent);
> +    }
> +
> +    cci->bg.runtime = MAX(1, query_length * (0.0005L/64));
Spaces around /

> +    *len_out = 0;
> +
> +    return CXL_MBOX_BG_STARTED;
> +}
> +
>  /*
>   * CXL r3.0 section 8.2.9.8.9.1: Dynamic Capacity Configuration
>   */
> @@ -1655,6 +1801,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>          cmd_media_clear_poison, 72, 0 },
>      [MEDIA_AND_POISON][GET_SCAN_MEDIA_CAPABILITIES] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
>          cmd_media_get_scan_media_capabilities, 16, 0 },
> +    [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA",
> +        cmd_media_scan_media, 17, BACKGROUND_OPERATION },
>  };
>  
>  static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
> @@ -1783,16 +1931,15 @@ static void bg_timercb(void *opaque)
>          cci->bg.complete_pct = 100;
>          cci->bg.ret_code = ret;
>          if (ret == CXL_MBOX_SUCCESS) {
> +            CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +
>              switch (cci->bg.opcode) {
>              case 0x4400: /* sanitize */
> -            {
> -                CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> -
>                  __do_sanitization(ct3d);
>                  cxl_dev_enable_media(&ct3d->cxl_dstate);
> -            }
> -            break;
> -            case 0x4304: /* TODO: scan media */
> +                break;
> +            case 0x4304: /* scan media */
> +                __do_scan_media(ct3d);
>                  break;
>              default:
>                  __builtin_unreachable();
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b90a7397d62f..aa9fdde1436e 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1435,6 +1435,12 @@ void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d)
>              cxl_device_get_timestamp(&ct3d->cxl_dstate);
>  }
>  
> +void cxl_clear_poison_list_overflowed(CXLType3Dev *ct3d)
> +{
> +    ct3d->poison_list_overflowed = false;
> +    ct3d->poison_list_overflow_ts = 0;
> +}
> +
>  void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
>                             Error **errp)
>  {
> @@ -1470,18 +1476,20 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
>          }
>      }
>  
> -    if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
> -        cxl_set_poison_list_overflowed(ct3d);
> -        return;
> -    }
> -
>      p = g_new0(CXLPoison, 1);
>      p->length = length;
>      p->start = start;
>      p->type = CXL_POISON_TYPE_INTERNAL; /* Different from injected via the mbox */
>  
> -    QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
> -    ct3d->poison_list_cnt++;
> +    if (ct3d->poison_list_cnt < CXL_POISON_LIST_LIMIT) {
> +        QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
> +        ct3d->poison_list_cnt++;
> +    } else {
> +        if (!ct3d->poison_list_overflowed) {
> +            cxl_set_poison_list_overflowed(ct3d);
> +        }
> +        QLIST_INSERT_HEAD(&ct3d->poison_list_bkp, p, node);
> +    }
>  }
>  
>  /* For uncorrectable errors include support for multiple header recording */
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index e824c5ade8a1..eb5c5284fa9f 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -385,6 +385,11 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val)
>  #define cxl_dev_enable_media(cxlds)                     \
>          do { __toggle_media((cxlds), 0x1); } while (0)
>  
> +static inline bool scan_media_running(CXLCCI *cci)
> +{
> +    return !!cci->bg.runtime && cci->bg.opcode == 0x4304;

I'd prefer the opcode being built which would mean moving this
implementation down to where we can see the defines.

(MEDIA_AND_POISON << 8) | SCAN_MEDIA 

I considered suggesting a macro, but if we do it should be separate
patch and include the other cases in this file.

Hence one for another day.

> +}
> +
>  static inline bool sanitize_running(CXLCCI *cci)
>  {
>      return !!cci->bg.runtime && cci->bg.opcode == 0x4400;
> @@ -476,6 +481,9 @@ struct CXLType3Dev {
>      unsigned int poison_list_cnt;
>      bool poison_list_overflowed;
>      uint64_t poison_list_overflow_ts;
> +    /* Poison Injection - backup */
> +    CXLPoisonList poison_list_bkp;
> +    CXLPoisonList scan_media_results;
>  
>      struct dynamic_capacity {
>          HostMemoryBackend *host_dc;
> @@ -538,6 +546,7 @@ CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
>  void cxl_event_irq_assert(CXLType3Dev *ct3d);
>  
>  void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d);
> +void cxl_clear_poison_list_overflowed(CXLType3Dev *ct3d);
>  
>  CXLDCDRegion *cxl_find_dc_region(CXLType3Dev *ct3d, uint64_t dpa, uint64_t len);
>  

Re: [PATCH 4/4] hw/cxl: Add get scan media results cmd support

[Jonathan Cameron]

On Fri,  8 Sep 2023 00:31:52 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Iterate over the list keeping the output payload size into account,
> returning the results from a previous scan media operation. The
> scan media operation does not fail prematurely due to device being
> out of storage, so this implementation does not deal with the
> retry/restart functionality.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
A few comments inline but nothing to stop me carrying this on my
staging tree. (gitlab.com/jic23/qemu cxl-*latestdate*

So I'll apply it there.  Updates welcome or I'll act on comments when
this gets to the top of our queue for upstreaming.

Jonathan

> ---
>  hw/cxl/cxl-mailbox-utils.c  | 84 +++++++++++++++++++++++++++++++++++++
>  include/hw/cxl/cxl_device.h |  1 +
>  2 files changed, 85 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 399ac03962db..109b9ecfabf1 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -82,6 +82,7 @@ enum {
>          #define CLEAR_POISON           0x2
>          #define GET_SCAN_MEDIA_CAPABILITIES 0x3
>          #define SCAN_MEDIA             0x4
> +        #define GET_SCAN_MEDIA_RESULTS 0x5
>      DCD_CONFIG  = 0x48,
>          #define GET_DC_CONFIG          0x0
>          #define GET_DYN_CAP_EXT_LIST   0x1
> @@ -1267,6 +1268,8 @@ static void __do_scan_media(CXLType3Dev *ct3d)
>          ct3d->poison_list_cnt == results_cnt) {
>          cxl_clear_poison_list_overflowed(ct3d);
>      }
> +    /* scan media has run since last conventional reset */
> +    ct3d->scan_media_hasrun = true;
>  }
>  
>  /*
> @@ -1371,6 +1374,85 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
>      return CXL_MBOX_BG_STARTED;
>  }
>  
> +/*
> + * CXL r3.0 section 8.2.9.8.4.6: Get Scan Media Results
> + */
> +static CXLRetCode cmd_media_get_scan_media_results(const struct cxl_cmd *cmd,
> +                                                   uint8_t *payload_in,
> +                                                   size_t len_in,
> +                                                   uint8_t *payload_out,
> +                                                   size_t *len_out,
> +                                                   CXLCCI *cci)
> +{
> +    struct get_scan_media_results_out_pl {
> +        uint64_t dpa_restart;
> +        uint64_t length;
> +        uint8_t flags;
> +        uint8_t rsvd1;
> +        uint16_t count;
> +        uint8_t rsvd2[0xc];
> +        struct {
> +            uint64_t addr;
> +            uint32_t length;
> +            uint32_t resv;
> +        } QEMU_PACKED records[];
> +    } QEMU_PACKED;
> +
> +    struct get_scan_media_results_out_pl *out = (void *)payload_out;
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +    CXLPoisonList *scan_media_results = &ct3d->scan_media_results;
> +    CXLPoison *ent, *next;
> +    uint16_t total_count = 0, record_count = 0, i = 0;
> +    uint16_t out_pl_len;
> +
> +    if (!ct3d->scan_media_hasrun) {
> +        return CXL_MBOX_UNSUPPORTED;
> +    }
> +
> +    /*
> +     * Calculate limits, all entries are within the same
> +     * address range of the last scan media call.
> +     */
> +    QLIST_FOREACH(ent, scan_media_results, node) {
> +        size_t rec_size = record_count * sizeof(out->records[0]);
> +
> +        if (sizeof(*out) + rec_size < CXL_MAILBOX_MAX_PAYLOAD_SIZE) {
> +            record_count++;
> +        }
> +        total_count++;
> +    }
> +
> +    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> +    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);

Isn't aim of the if in the loop above to ensure this never happens?
I'd hope that code is obvious enough we don't need the additional assert
here.

> +
> +    memset(out, 0, out_pl_len);
> +    QLIST_FOREACH_SAFE(ent, scan_media_results, node, next) {
> +        uint64_t start, stop;
> +
> +        if (i == record_count) {
> +            break;
> +        }
> +
> +        start = ROUND_DOWN(ent->start, 64ull);
> +        stop = ROUND_DOWN(ent->start, 64ull) + ent->length;

I think we control the way these all turn up so they are multiples of 64.
So this rounding shouldn't be needed (or am I missing something?)

> +        stq_le_p(&out->records[i].addr, start | (ent->type & 0x7)); 

define for that 0x7

> +        stl_le_p(&out->records[i].length, (stop - start) / CXL_CACHE_LINE_SIZE);
> +        i++;
> +
> +        /* consume the returning entry */
> +        QLIST_REMOVE(ent, node);
> +        g_free(ent);
> +    }
> +
> +    stw_le_p(&out->count, record_count);
> +    if (total_count > record_count) {
> +        out->flags = (1 << 0); /* More Media Error Records */
Define perhaps.

> +    }
> +
> +    *len_out = out_pl_len;
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  /*
>   * CXL r3.0 section 8.2.9.8.9.1: Dynamic Capacity Configuration
>   */
> @@ -1803,6 +1885,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>          cmd_media_get_scan_media_capabilities, 16, 0 },
>      [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA",
>          cmd_media_scan_media, 17, BACKGROUND_OPERATION },
> +    [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
I'll wrap this line whilst applying.

Shortly I'm going to send out a series hammering down
all the line lengths in the CXL code. 

> +        cmd_media_get_scan_media_results, 0, 0 },
>  };
>  
>  static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index eb5c5284fa9f..e9d130e5c988 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -484,6 +484,7 @@ struct CXLType3Dev {
>      /* Poison Injection - backup */
>      CXLPoisonList poison_list_bkp;
>      CXLPoisonList scan_media_results;
> +    bool scan_media_hasrun;
>  
>      struct dynamic_capacity {
>          HostMemoryBackend *host_dc;

Re: [PATCH 4/4] hw/cxl: Add get scan media results cmd support

[Davidlohr Bueso]

On Tue, 12 Sep 2023, Jonathan Cameron wrote:

>On Fri,  8 Sep 2023 00:31:52 -0700
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> Iterate over the list keeping the output payload size into account,
>> returning the results from a previous scan media operation. The
>> scan media operation does not fail prematurely due to device being
>> out of storage, so this implementation does not deal with the
>> retry/restart functionality.
>>
>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>A few comments inline but nothing to stop me carrying this on my
>staging tree. (gitlab.com/jic23/qemu cxl-*latestdate*
>
>So I'll apply it there.  Updates welcome or I'll act on comments when
>this gets to the top of our queue for upstreaming.

I'll send follow up patches for this series once your new latest
branch is published.

Thanks,
Davidlohr

Re: [PATCH 3/4] hw/cxl: Add scan media cmd support

[Davidlohr Bueso]

On Tue, 12 Sep 2023, Jonathan Cameron wrote:

>On Fri,  8 Sep 2023 00:31:51 -0700
>> +    if (scan_media_running(cci)) {
>> +        /*
>> +         * XXX: Spec is ambiguous - is this case considered
>> +         * a successful return despite not removing from the list?
>> +         */
>
>I think we are in impdef territory here, though happy to join the conversation
>if you want to raise it in CXL consortium.  Would need an errata though so
>I suspect it will stay impdef with likely reasoning being.
>
>OS should only use scan_media if it know poison list is overflowed.
>It should know it issued scan media.
>It should not do anything so silly has hit the device with a poison clear
>in parallel to that.  Hence doesn't matter that this corner is impdef.

Right, this is very much along the lines of what I have the kernel currently
doing. That is, directly start the scan from get poison list if the overflow
bit returns set. This does not follow userspace policy once discussed and
hence no possibility of calling scan media under bogus conditions. Inject/clear
are also serialized vs this, so no funny business either.

>
>I'd like to see a comment to say it is though!
>
>> +        goto success;
>> +    }
>> +
>>      QLIST_FOREACH(ent, poison_list, node) {
>>          /*
>>           * Test for contained in entry. Simpler than general case
>> @@ -1133,7 +1160,7 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
>>          }
>>      }
>>      if (!ent) {
>> -        return CXL_MBOX_SUCCESS;
>> +        goto success;
>
>Is this a bug fix?  I think the explicit setting of len_out came in somewhere but don't
>think it is necessary for commands that have no output payload.  If it is, then we should
>set it at the top of the function as they never have a payload.

Yeah it doesn't matter for this case.

Thanks,
Davidlohr

Re: [PATCH 4/4] hw/cxl: Add get scan media results cmd support

[Jonathan Cameron]

On Tue, 12 Sep 2023 20:33:52 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Tue, 12 Sep 2023, Jonathan Cameron wrote:
> 
> >On Fri,  8 Sep 2023 00:31:52 -0700
> >Davidlohr Bueso <dave@stgolabs.net> wrote:
> >  
> >> Iterate over the list keeping the output payload size into account,
> >> returning the results from a previous scan media operation. The
> >> scan media operation does not fail prematurely due to device being
> >> out of storage, so this implementation does not deal with the
> >> retry/restart functionality.
> >>
> >> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>  
> >A few comments inline but nothing to stop me carrying this on my
> >staging tree. (gitlab.com/jic23/qemu cxl-*latestdate*
> >
> >So I'll apply it there.  Updates welcome or I'll act on comments when
> >this gets to the top of our queue for upstreaming.  
> 
> I'll send follow up patches for this series once your new latest
> branch is published.
> 
> Thanks,
> Davidlohr

https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13

Fresh and tasty.  Includes 3 series that I've posted with the hope
of them merging soon and another one I'll post in a few mins to
deal with over long lines...

Jonathan

Re: [PATCH 4/4] hw/cxl: Add get scan media results cmd support

[Davidlohr Bueso]

On Wed, 13 Sep 2023, Jonathan Cameron wrote:

>https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13
>
>Fresh and tasty.  Includes 3 series that I've posted with the hope
>of them merging soon and another one I'll post in a few mins to
>deal with over long lines...

fyi I'm getting the below:

/usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_reset':
/home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:477: undefined reference to `ct3d_reset'
/usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_exit':
/home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:466: undefined reference to `ct3_exit'
/usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_realize':
/home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:428: undefined reference to `ct3_realize'
/usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_reset':
/home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:478: undefined reference to `cxl_add_cci_commands'

Re: [PATCH 4/4] hw/cxl: Add get scan media results cmd support

[Jonathan Cameron]

On Fri, 15 Sep 2023 17:11:27 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Wed, 13 Sep 2023, Jonathan Cameron wrote:
> 
> >https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13
> >
> >Fresh and tasty.  Includes 3 series that I've posted with the hope
> >of them merging soon and another one I'll post in a few mins to
> >deal with over long lines...  
> 
> fyi I'm getting the below:
> 
> /usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_reset':
> /home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:477: undefined reference to `ct3d_reset'
> /usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_exit':
> /home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:466: undefined reference to `ct3_exit'
> /usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_realize':
> /home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:428: undefined reference to `ct3_realize'
> /usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_reset':
> /home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:478: undefined reference to `cxl_add_cci_commands'

Thanks.  I'd not noticed the niagara stuff doesn't have it's own kconfig with dependencies.
Gregory, want to send a proper fix defining the various Kconfigs down to the actual build directory?

I'd guess the lazy approach of making CXL_VENDOR
depends on CXL_MEM_DEVICE
should work in meantime.

Jonathan

Re: [PATCH 4/4] hw/cxl: Add get scan media results cmd support

[Gregory Price]

On Mon, Sep 18, 2023 at 12:11:28PM +0100, Jonathan Cameron wrote:
> On Fri, 15 Sep 2023 17:11:27 -0700
> Davidlohr Bueso <dave@stgolabs.net> wrote:
> 
> > On Wed, 13 Sep 2023, Jonathan Cameron wrote:
> > 
> > >https://gitlab.com/jic23/qemu/-/tree/cxl-2023-09-13
> > >
> > >Fresh and tasty.  Includes 3 series that I've posted with the hope
> > >of them merging soon and another one I'll post in a few mins to
> > >deal with over long lines...  
> > 
> > fyi I'm getting the below:
> > 
> > /usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_reset':
> > /home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:477: undefined reference to `ct3d_reset'
> > /usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_exit':
> > /home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:466: undefined reference to `ct3_exit'
> > /usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_realize':
> > /home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:428: undefined reference to `ct3_realize'
> > /usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: libcommon.fa.p/hw_cxl_vendor_skhynix_skhynix_niagara.c.o: in function `cxl_niagara_reset':
> > /home/dave/git/qemu-jic/build/../hw/cxl/vendor/skhynix/skhynix_niagara.c:478: undefined reference to `cxl_add_cci_commands'
> 
> Thanks.  I'd not noticed the niagara stuff doesn't have it's own kconfig with dependencies.
> Gregory, want to send a proper fix defining the various Kconfigs down to the actual build directory?
> 
> I'd guess the lazy approach of making CXL_VENDOR
> depends on CXL_MEM_DEVICE
> should work in meantime.
> 
> Jonathan
>

woops! My bad.  I will push up a patch later today with that + limiting
niagara to linux-only (already fixed that up, just haven't pushed it).

I don't think any other changes have been made to that line, so i'll
just push an update to the Niagara patch itself and you can hot-swap
them on the branch if you like.

~Gregory

[PATCH 4/4] hw/cxl: Add get scan media results cmd support

[Jonathan Cameron]

From: Davidlohr Bueso <dave@stgolabs.net>

Iterate over the list keeping the output payload size into account,
returning the results from a previous scan media operation. The
scan media operation does not fail prematurely due to device being
out of storage, so this implementation does not deal with the
retry/restart functionality.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Link: https://lore.kernel.org/r/20230908073152.4386-5-dave@stgolabs.net
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Tweak - update references to point to CXL r3.1 given lack of
easy availability of r3.0.
---
 include/hw/cxl/cxl_device.h |  1 +
 hw/cxl/cxl-mailbox-utils.c  | 85 +++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+)

diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 0509d961c3..cc98553583 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -499,6 +499,7 @@ struct CXLType3Dev {
     /* Poison Injection - backup */
     CXLPoisonList poison_list_bkp;
     CXLPoisonList scan_media_results;
+    bool scan_media_hasrun;
 
     struct dynamic_capacity {
         HostMemoryBackend *host_dc;
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index e3d3575b02..e87089474a 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -85,6 +85,7 @@ enum {
         #define CLEAR_POISON           0x2
         #define GET_SCAN_MEDIA_CAPABILITIES 0x3
         #define SCAN_MEDIA             0x4
+        #define GET_SCAN_MEDIA_RESULTS 0x5
     DCD_CONFIG  = 0x48,
         #define GET_DC_CONFIG          0x0
         #define GET_DYN_CAP_EXT_LIST   0x1
@@ -1346,6 +1347,8 @@ static void __do_scan_media(CXLType3Dev *ct3d)
         ct3d->poison_list_cnt == results_cnt) {
         cxl_clear_poison_list_overflowed(ct3d);
     }
+    /* scan media has run since last conventional reset */
+    ct3d->scan_media_hasrun = true;
 }
 
 /*
@@ -1450,6 +1453,85 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
     return CXL_MBOX_BG_STARTED;
 }
 
+/*
+ * CXL r3.1 section 8.2.9.9.4.6: Get Scan Media Results
+ */
+static CXLRetCode cmd_media_get_scan_media_results(const struct cxl_cmd *cmd,
+                                                   uint8_t *payload_in,
+                                                   size_t len_in,
+                                                   uint8_t *payload_out,
+                                                   size_t *len_out,
+                                                   CXLCCI *cci)
+{
+    struct get_scan_media_results_out_pl {
+        uint64_t dpa_restart;
+        uint64_t length;
+        uint8_t flags;
+        uint8_t rsvd1;
+        uint16_t count;
+        uint8_t rsvd2[0xc];
+        struct {
+            uint64_t addr;
+            uint32_t length;
+            uint32_t resv;
+        } QEMU_PACKED records[];
+    } QEMU_PACKED;
+
+    struct get_scan_media_results_out_pl *out = (void *)payload_out;
+    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+    CXLPoisonList *scan_media_results = &ct3d->scan_media_results;
+    CXLPoison *ent, *next;
+    uint16_t total_count = 0, record_count = 0, i = 0;
+    uint16_t out_pl_len;
+
+    if (!ct3d->scan_media_hasrun) {
+        return CXL_MBOX_UNSUPPORTED;
+    }
+
+    /*
+     * Calculate limits, all entries are within the same address range of the
+     * last scan media call.
+     */
+    QLIST_FOREACH(ent, scan_media_results, node) {
+        size_t rec_size = record_count * sizeof(out->records[0]);
+
+        if (sizeof(*out) + rec_size < CXL_MAILBOX_MAX_PAYLOAD_SIZE) {
+            record_count++;
+        }
+        total_count++;
+    }
+
+    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
+    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
+
+    memset(out, 0, out_pl_len);
+    QLIST_FOREACH_SAFE(ent, scan_media_results, node, next) {
+        uint64_t start, stop;
+
+        if (i == record_count) {
+            break;
+        }
+
+        start = ROUND_DOWN(ent->start, 64ull);
+        stop = ROUND_DOWN(ent->start, 64ull) + ent->length;
+        stq_le_p(&out->records[i].addr, start | (ent->type & 0x7));
+        stl_le_p(&out->records[i].length, (stop - start) / CXL_CACHE_LINE_SIZE);
+        i++;
+
+        /* consume the returning entry */
+        QLIST_REMOVE(ent, node);
+        g_free(ent);
+    }
+
+    stw_le_p(&out->count, record_count);
+    if (total_count > record_count) {
+        out->flags = (1 << 0); /* More Media Error Records */
+    }
+
+    *len_out = out_pl_len;
+    return CXL_MBOX_SUCCESS;
+}
+
 /*
  * CXL r3.1 section 8.2.9.9.9.1: Get Dynamic Capacity Configuration
  * (Opcode: 4800h)
@@ -2067,6 +2149,9 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
         cmd_media_get_scan_media_capabilities, 16, 0 },
     [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA",
         cmd_media_scan_media, 17, BACKGROUND_OPERATION },
+    [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = {
+        "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
+        cmd_media_get_scan_media_results, 0, 0 },
 };
 
 static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
-- 
2.43.0