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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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;