diff mbox series

[3/3] hw/cxl: Add scan media mailbox command support

Message ID 20230426021418.10186-4-dave@stgolabs.net
State New, archived
Headers show
Series hw/cxl: Add support for Scan Media | expand

Commit Message

Davidlohr Bueso April 26, 2023, 2:14 a.m. UTC 
Allow the caller to retrieve the results of a previous scan media
operation, with similar semantics to the get poison list command.
Number of returned records is to the max payload output size.

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

Comments

nifan@outlook.com April 28, 2023, 5 p.m. UTC | #1 
The 04/25/2023 19:14, Davidlohr Bueso wrote:
> Allow the caller to retrieve the results of a previous scan media
> operation, with similar semantics to the get poison list command.
> Number of returned records is to the max payload output size.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  hw/cxl/cxl-mailbox-utils.c | 55 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 7f29b840a2c9..f978180d8e21 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -81,6 +81,7 @@ enum {
>          #define CLEAR_POISON           0x2
>          #define GET_SCAN_MEDIA_CAPABILITIES 0x3
>          #define SCAN_MEDIA             0x4
> +        #define GET_SCAN_MEDIA_RESULTS 0x5
>      PHYSICAL_SWITCH = 0x51
>          #define IDENTIFY_SWITCH_DEVICE      0x0
>  };
> @@ -1079,6 +1080,58 @@ cmd_media_scan_media(struct cxl_cmd *cmd,
>      return CXL_MBOX_BG_STARTED;
>  }
>  
> +static CXLRetCode cmd_media_get_scan_media_results(struct cxl_cmd *cmd,
> +                                                   CXLDeviceState *cxl_dstate,
> +                                                   uint16_t *len)
> +{
> +    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[20];
20-->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 *)cmd->payload;
> +    CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> +    uint16_t record_count = 0, i = 0;
> +    CXLPoisonList *poison_list = &ct3d->poison_list;
> +    CXLPoison *ent;
> +    uint16_t out_pl_len;
> +
> +    QLIST_FOREACH(ent, poison_list, node) {
> +        if (ent->type == CXL_POISON_TYPE_SCANMEDIA) {
> +            record_count++;
> +        }
> +    }
> +
> +    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> +    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
Add TODO to cover the case when eror list is larger than output payload
size??
> +
> +    memset(out, 0, out_pl_len);
> +    QLIST_FOREACH(ent, poison_list, node) {
> +        if (ent->type == CXL_POISON_TYPE_SCANMEDIA) {
> +            uint64_t start, stop;
> +
> +            start = ent->start & 0xffffffffffffffc0;
> +            stop = ent->start & 0xffffffffffffffc0;
so start == stop??? missing ent->length for stop???
> +            stq_le_p(&out->records[i].addr, start | (ent->type & 0x7));
> +            stl_le_p(&out->records[i].length, (stop - start) / 64);
length always be 0.
> +            i++;
> +        }
> +    }
> +
> +    stw_le_p(&out->count, record_count);
> +
> +    *len = out_pl_len;
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
>  #define IMMEDIATE_DATA_CHANGE (1 << 2)
>  #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -1121,6 +1174,8 @@ static 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, 0 },
> +    [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
> +        cmd_media_get_scan_media_results, 0, 0 },
>  };
>  
>  static struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> -- 
> 2.40.0
>
Davidlohr Bueso April 28, 2023, 7:30 p.m. UTC | #2 
On Fri, 28 Apr 2023, Fan Ni wrote:

>The 04/25/2023 19:14, Davidlohr Bueso wrote:
>> +static CXLRetCode cmd_media_get_scan_media_results(struct cxl_cmd *cmd,
>> +                                                   CXLDeviceState *cxl_dstate,
>> +                                                   uint16_t *len)
>> +{
>> +    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[20];
>20-->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 *)cmd->payload;
>> +    CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
>> +    uint16_t record_count = 0, i = 0;
>> +    CXLPoisonList *poison_list = &ct3d->poison_list;
>> +    CXLPoison *ent;
>> +    uint16_t out_pl_len;
>> +
>> +    QLIST_FOREACH(ent, poison_list, node) {
>> +        if (ent->type == CXL_POISON_TYPE_SCANMEDIA) {
>> +            record_count++;
>> +        }
>> +    }
>> +
>> +    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
>> +    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
>Add TODO to cover the case when eror list is larger than output payload
>size??

Sure, but this is why I referenced this limitation in the cover letter.

>> +
>> +    memset(out, 0, out_pl_len);
>> +    QLIST_FOREACH(ent, poison_list, node) {
>> +        if (ent->type == CXL_POISON_TYPE_SCANMEDIA) {
>> +            uint64_t start, stop;
>> +
>> +            start = ent->start & 0xffffffffffffffc0;
>> +            stop = ent->start & 0xffffffffffffffc0;
>so start == stop??? missing ent->length for stop???

duh yes should be ent->start + ent->length. I did say all this was untested :)

Thanks,
Davidlor
Jonathan Cameron May 15, 2023, 12:04 p.m. UTC | #3 
On Tue, 25 Apr 2023 19:14:18 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Allow the caller to retrieve the results of a previous scan media
> operation, with similar semantics to the get poison list command.
> Number of returned records is to the max payload output size.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

The special poison type is an issue here as well. As in previous patch
I don't think we need it.  Poison doesn't go away on it's own, so
Scan Media (results) should see all poison that Get Poison List does +
others that didn't fit in the tracking list.

> ---
>  hw/cxl/cxl-mailbox-utils.c | 55 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 7f29b840a2c9..f978180d8e21 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -81,6 +81,7 @@ enum {
>          #define CLEAR_POISON           0x2
>          #define GET_SCAN_MEDIA_CAPABILITIES 0x3
>          #define SCAN_MEDIA             0x4
> +        #define GET_SCAN_MEDIA_RESULTS 0x5
>      PHYSICAL_SWITCH = 0x51
>          #define IDENTIFY_SWITCH_DEVICE      0x0
>  };
> @@ -1079,6 +1080,58 @@ cmd_media_scan_media(struct cxl_cmd *cmd,
>      return CXL_MBOX_BG_STARTED;
>  }
>  
> +static CXLRetCode cmd_media_get_scan_media_results(struct cxl_cmd *cmd,
> +                                                   CXLDeviceState *cxl_dstate,
> +                                                   uint16_t *len)
> +{
> +    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[20];
> +        struct {
> +            uint64_t addr;
> +            uint32_t length;
> +            uint32_t resv;
> +        } QEMU_PACKED records[];
> +    } QEMU_PACKED;
> +    struct get_scan_media_results_out_pl *out = (void *)cmd->payload;
> +    CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> +    uint16_t record_count = 0, i = 0;
> +    CXLPoisonList *poison_list = &ct3d->poison_list;
> +    CXLPoison *ent;
> +    uint16_t out_pl_len;
> +
> +    QLIST_FOREACH(ent, poison_list, node) {
> +        if (ent->type == CXL_POISON_TYPE_SCANMEDIA) {
> +            record_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(ent, poison_list, node) {
> +        if (ent->type == CXL_POISON_TYPE_SCANMEDIA) {
> +            uint64_t start, stop;
> +
> +            start = ent->start & 0xffffffffffffffc0;
> +            stop = ent->start & 0xffffffffffffffc0;
> +            stq_le_p(&out->records[i].addr, start | (ent->type & 0x7));

Given type is 0x4 that mask doesn't block us having a value defined in spec as
'all other encodings are reserved'

> +            stl_le_p(&out->records[i].length, (stop - start) / 64);
> +            i++;
> +        }
> +    }
> +
> +    stw_le_p(&out->count, record_count);
> +
> +    *len = out_pl_len;
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
>  #define IMMEDIATE_DATA_CHANGE (1 << 2)
>  #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -1121,6 +1174,8 @@ static 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, 0 },
> +    [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
> +        cmd_media_get_scan_media_results, 0, 0 },
>  };
>  
>  static struct cxl_cmd cxl_cmd_set_sw[256][256] = {
diff mbox series

Patch

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 7f29b840a2c9..f978180d8e21 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -81,6 +81,7 @@  enum {
         #define CLEAR_POISON           0x2
         #define GET_SCAN_MEDIA_CAPABILITIES 0x3
         #define SCAN_MEDIA             0x4
+        #define GET_SCAN_MEDIA_RESULTS 0x5
     PHYSICAL_SWITCH = 0x51
         #define IDENTIFY_SWITCH_DEVICE      0x0
 };
@@ -1079,6 +1080,58 @@  cmd_media_scan_media(struct cxl_cmd *cmd,
     return CXL_MBOX_BG_STARTED;
 }
 
+static CXLRetCode cmd_media_get_scan_media_results(struct cxl_cmd *cmd,
+                                                   CXLDeviceState *cxl_dstate,
+                                                   uint16_t *len)
+{
+    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[20];
+        struct {
+            uint64_t addr;
+            uint32_t length;
+            uint32_t resv;
+        } QEMU_PACKED records[];
+    } QEMU_PACKED;
+    struct get_scan_media_results_out_pl *out = (void *)cmd->payload;
+    CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
+    uint16_t record_count = 0, i = 0;
+    CXLPoisonList *poison_list = &ct3d->poison_list;
+    CXLPoison *ent;
+    uint16_t out_pl_len;
+
+    QLIST_FOREACH(ent, poison_list, node) {
+        if (ent->type == CXL_POISON_TYPE_SCANMEDIA) {
+            record_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(ent, poison_list, node) {
+        if (ent->type == CXL_POISON_TYPE_SCANMEDIA) {
+            uint64_t start, stop;
+
+            start = ent->start & 0xffffffffffffffc0;
+            stop = ent->start & 0xffffffffffffffc0;
+            stq_le_p(&out->records[i].addr, start | (ent->type & 0x7));
+            stl_le_p(&out->records[i].length, (stop - start) / 64);
+            i++;
+        }
+    }
+
+    stw_le_p(&out->count, record_count);
+
+    *len = out_pl_len;
+    return CXL_MBOX_SUCCESS;
+}
+
 #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
 #define IMMEDIATE_DATA_CHANGE (1 << 2)
 #define IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -1121,6 +1174,8 @@  static 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, 0 },
+    [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = { "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
+        cmd_media_get_scan_media_results, 0, 0 },
 };
 
 static struct cxl_cmd cxl_cmd_set_sw[256][256] = {