Re: [PATCH v6 08/12] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response

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

On Mon, 25 Mar 2024 12:02:26 -0700
nifan.cxl@gmail.com wrote:

> From: Fan Ni <fan.ni@samsung.com>
> 
> Per CXL spec 3.1, two mailbox commands are implemented:
> Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
> 
> For the process of the above two commands, we use two-pass approach.
> Pass 1: Check whether the input payload is valid or not; if not, skip
>         Pass 2 and return mailbox process error.
> Pass 2: Do the real work--add or release extents, respectively.
> 
> Signed-off-by: Fan Ni <fan.ni@samsung.com>

A few additional comments from me.

Jonathan


> +/*
> + * For the extents in the extent list to operate, check whether they are valid
> + * 1. The extent should be in the range of a valid DC region;
> + * 2. The extent should not cross multiple regions;
> + * 3. The start DPA and the length of the extent should align with the block
> + * size of the region;
> + * 4. The address range of multiple extents in the list should not overlap.
> + */
> +static CXLRetCode cxl_detect_malformed_extent_list(CXLType3Dev *ct3d,
> +        const CXLUpdateDCExtentListInPl *in)
> +{
> +    uint64_t min_block_size = UINT64_MAX;
> +    CXLDCRegion *region = &ct3d->dc.regions[0];

This is immediately overwritten if num_regions != 0 (Which I think is checked before
calling this function).  So no need to initialize it.

> +    CXLDCRegion *lastregion = &ct3d->dc.regions[ct3d->dc.num_regions - 1];
> +    g_autofree unsigned long *blk_bitmap = NULL;
> +    uint64_t dpa, len;
> +    uint32_t i;
> +
> +    for (i = 0; i < ct3d->dc.num_regions; i++) {
> +        region = &ct3d->dc.regions[i];
> +        min_block_size = MIN(min_block_size, region->block_size);
> +    }
> +
> +    blk_bitmap = bitmap_new((lastregion->base + lastregion->len -
> +                             ct3d->dc.regions[0].base) / min_block_size);
> +
> +    for (i = 0; i < in->num_entries_updated; i++) {
> +        dpa = in->updated_entries[i].start_dpa;
> +        len = in->updated_entries[i].len;
> +
> +        region = cxl_find_dc_region(ct3d, dpa, len);
> +        if (!region) {
> +            return CXL_MBOX_INVALID_PA;
> +        }
> +
> +        dpa -= ct3d->dc.regions[0].base;
> +        if (dpa % region->block_size || len % region->block_size) {
> +            return CXL_MBOX_INVALID_EXTENT_LIST;
> +        }
> +        /* the dpa range already covered by some other extents in the list */
> +        if (test_any_bits_set(blk_bitmap, dpa / min_block_size,
> +            len / min_block_size)) {
> +            return CXL_MBOX_INVALID_EXTENT_LIST;
> +        }
> +        bitmap_set(blk_bitmap, dpa / min_block_size, len / min_block_size);
> +   }
> +
> +    return CXL_MBOX_SUCCESS;
> +}



> +/*
> + * CXL r3.1 section 8.2.9.9.9.3: Add Dynamic Capacity Response (Opcode 4802h)
> + * An extent is added to the extent list and becomes usable only after the
> + * response is processed successfully
> + */
> +static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
> +                                          uint8_t *payload_in,
> +                                          size_t len_in,
> +                                          uint8_t *payload_out,
> +                                          size_t *len_out,
> +                                          CXLCCI *cci)
> +{
> +    CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +    CXLDCExtentList *extent_list = &ct3d->dc.extents;
> +    uint32_t i;
> +    uint64_t dpa, len;
> +    CXLRetCode ret;
> +
> +    if (in->num_entries_updated == 0) {
> +        return CXL_MBOX_SUCCESS;
> +    }


A zero length response is a rejection of an offered set of extents.
Probably want a todo here to say this will wipe out part of the pending list
(similar to the one you have below).

> +
> +    /* Adding extents causes exceeding device's extent tracking ability. */
> +    if (in->num_entries_updated + ct3d->dc.total_extent_count >
> +        CXL_NUM_EXTENTS_SUPPORTED) {
> +        return CXL_MBOX_RESOURCES_EXHAUSTED;
> +    }
> +
> +    ret = cxl_detect_malformed_extent_list(ct3d, in);
> +    if (ret != CXL_MBOX_SUCCESS) {
> +        return ret;
> +    }
> +
> +    ret = cxl_dcd_add_dyn_cap_rsp_dry_run(ct3d, in);
> +    if (ret != CXL_MBOX_SUCCESS) {
> +        return ret;
> +    }
> +
> +    for (i = 0; i < in->num_entries_updated; i++) {
> +        dpa = in->updated_entries[i].start_dpa;
> +        len = in->updated_entries[i].len;
> +
> +        cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
> +        ct3d->dc.total_extent_count += 1;
> +        /*
> +         * TODO: we will add a pending extent list based on event log record
> +         * and process the list according here.
> +         */
> +    }
> +
> +    return CXL_MBOX_SUCCESS;
> +}

> +static CXLRetCode cxl_dc_extent_release_dry_run(CXLType3Dev *ct3d,
> +        const CXLUpdateDCExtentListInPl *in)
> +{
> +    CXLDCExtent *ent, *ent_next;
> +    uint64_t dpa, len;
> +    uint32_t i;
> +    int cnt_delta = 0;
> +    CXLDCExtentList tmp_list;
> +    CXLRetCode ret = CXL_MBOX_SUCCESS;
> +
> +    if (in->num_entries_updated == 0) {

This is only used in paths where we already checked this. I don't hink
we need to repeat.

> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    QTAILQ_INIT(&tmp_list);
> +    copy_extent_list(&tmp_list, &ct3d->dc.extents);
> +
> +    for (i = 0; i < in->num_entries_updated; i++) {
> +        Range range;
> +
> +        dpa = in->updated_entries[i].start_dpa;
> +        len = in->updated_entries[i].len;
> +
> +        while (len > 0) {
> +            QTAILQ_FOREACH(ent, &tmp_list, node) 
> +                range_init_nofail(&range, ent->start_dpa, ent->len);
> +
> +                if (range_contains(&range, dpa)) {
> +                    uint64_t len1, len2, len_done = 0;
> +                    uint64_t ent_start_dpa = ent->start_dpa;
> +                    uint64_t ent_len = ent->len;
> +                    /*
> +                     * Found the exact extent or the subset of an existing
> +                     * extent.
> +                     */
> +                    if (range_contains(&range, dpa + len - 1)) {
> +                        len1 = dpa - ent->start_dpa;
> +                        len2 = ent_start_dpa + ent_len - dpa - len;
> +                        len_done = ent_len - len1 - len2;
I'd like this to look a bit more like the real run - possibly allowing code
sharing. Though definitely see if there is a way to share more as Jorgen suggested.

                        len1 = dpa - ent_start_dpa;
                        if (range_contains(&range, dpa + len - 1) {
                            len 2 = ent_start_dpa + ent_len - dpa - len;
                        } else { /* maybe add an if (dry_run) here to allow code reuse */
                            /*
                             * TODO: we reject the attempt to remove an extent
                             * that overlaps with multiple extents in the device 
                             * for now, we will allow it once superset release
                             * support is added.
                             */
                             ret = CXL_MBOX_INVALID_PA;
                             goto free_and_exit;
			}
> +
> +                        cxl_remove_extent_from_extent_list(&tmp_list, ent);
> +                        cnt_delta--;
> +
> +                        if (len1) {
> +                            cxl_insert_extent_to_extent_list(&tmp_list,
> +                                                             ent_start_dpa,
> +                                                             len1, NULL, 0);
> +                            cnt_delta++;
> +                        }
> +                        if (len2) {
> +                            cxl_insert_extent_to_extent_list(&tmp_list,
> +                                                             dpa + len,
> +                                                             len2, NULL, 0);
> +                            cnt_delta++;
> +                        }
> +
> +                        if (cnt_delta + ct3d->dc.total_extent_count >
> +                            CXL_NUM_EXTENTS_SUPPORTED) {
> +                            ret = CXL_MBOX_RESOURCES_EXHAUSTED;
> +                            goto free_and_exit;
> +                        }
> +                    } else {
> +                        /*
> +                         * TODO: we reject the attempt to remove an extent
> +                         * that overlaps with multiple extents in the device
> +                         * for now, we will allow it once superset release
> +                         * support is added.
> +                         */
> +                        ret = CXL_MBOX_INVALID_PA;
> +                        goto free_and_exit;
> +                    }
> +
> +                    len -= len_done;
> +                    /* len == 0 here until superset release is added */
> +                    break;
> +                }
> +            }
> +            if (len) {
> +                ret = CXL_MBOX_INVALID_PA;
> +                goto free_and_exit;
> +            }
> +        }
> +    }
> +free_and_exit:
> +    QTAILQ_FOREACH_SAFE(ent, &tmp_list, node, ent_next) {
> +        cxl_remove_extent_from_extent_list(&tmp_list, ent);
> +    }
> +
> +    return ret;
> +}
> +
> +/*
> + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (Opcode 4803h)
> + */
> +static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> +                                          uint8_t *payload_in,
> +                                          size_t len_in,
> +                                          uint8_t *payload_out,
> +                                          size_t *len_out,
> +                                          CXLCCI *cci)
> +{
> +    CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +    CXLDCExtentList *extent_list = &ct3d->dc.extents;
> +    CXLDCExtent *ent;
> +    uint32_t i;
> +    uint64_t dpa, len;
> +    CXLRetCode ret;
> +
> +    if (in->num_entries_updated == 0) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    ret = cxl_detect_malformed_extent_list(ct3d, in);
> +    if (ret != CXL_MBOX_SUCCESS) {
> +        return ret;
> +    }
> +
> +    ret = cxl_dc_extent_release_dry_run(ct3d, in);
> +    if (ret != CXL_MBOX_SUCCESS) {
> +        return ret;
> +    }
> +
> +    /* From this point, all the extents to release are valid */

known to be valid

> +    for (i = 0; i < in->num_entries_updated; i++) {
> +        Range range;
Perhaps factor out the handling of each extent?  Will reduce indent
and give more readable code I think. 

> +
> +        dpa = in->updated_entries[i].start_dpa;
> +        len = in->updated_entries[i].len;
> +
> +        while (len > 0) {
> +            QTAILQ_FOREACH(ent, extent_list, node) {
> +                range_init_nofail(&range, ent->start_dpa, ent->len);
> +
> +                /* Found the extent overlapping with */
> +                if (range_contains(&range, dpa)) {
> +                    uint64_t len1, len2 = 0, len_done = 0;
> +                    uint64_t ent_start_dpa = ent->start_dpa;
> +                    uint64_t ent_len = ent->len;
> +
> +                    len1 = dpa - ent_start_dpa;
> +                    if (range_contains(&range, dpa + len - 1)) {
> +                        len2 = ent_start_dpa + ent_len - dpa - len;
> +                    }
> +                    len_done = ent_len - len1 - len2;
> +
> +                    cxl_remove_extent_from_extent_list(extent_list, ent);
> +                    ct3d->dc.total_extent_count -= 1;
> +
> +                    if (len1) {
> +                        cxl_insert_extent_to_extent_list(extent_list,
> +                                                         ent_start_dpa,
> +                                                         len1, NULL, 0);
> +                        ct3d->dc.total_extent_count += 1;
> +                    }
> +                    if (len2) {
> +                        cxl_insert_extent_to_extent_list(extent_list,
> +                                                         dpa + len,
> +                                                         len2, NULL, 0);
> +                        ct3d->dc.total_extent_count += 1;
> +                    }
> +
> +                    len -= len_done;
> +                    /*
> +                     * len will always be 0 until superset release is add.
> +                     * TODO: superset release will be added.
> +                     */
> +                    break;
> +                }
> +            }
> +        }
> +    }
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
>  #define IMMEDIATE_DATA_CHANGE (1 << 2)
>  #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -1413,15 +1832,15 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>      [EVENTS][CLEAR_RECORDS] = { "EVENTS_CLEAR_RECORDS",
>          cmd_events_clear_records, ~0, IMMEDIATE_LOG_CHANGE },
>      [EVENTS][GET_INTERRUPT_POLICY] = { "EVENTS_GET_INTERRUPT_POLICY",
> -                                      cmd_events_get_interrupt_policy, 0, 0 },
> +        cmd_events_get_interrupt_policy, 0, 0 },
>      [EVENTS][SET_INTERRUPT_POLICY] = { "EVENTS_SET_INTERRUPT_POLICY",
> -                                      cmd_events_set_interrupt_policy,
> -                                      ~0, IMMEDIATE_CONFIG_CHANGE },
> +        cmd_events_set_interrupt_policy,
> +        ~0, IMMEDIATE_CONFIG_CHANGE },

Avoid the reformatting in a patch that does other stuff.
Adds noise and hides any actual changes in the blocks re indented.

>      [FIRMWARE_UPDATE][GET_INFO] = { "FIRMWARE_UPDATE_GET_INFO",
>          cmd_firmware_update_get_info, 0, 0 },
>      [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
>      [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set,
> -                         8, IMMEDIATE_POLICY_CHANGE },
> +        8, IMMEDIATE_POLICY_CHANGE },
>      [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported,
>                                0, 0 },
>      [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 },
> @@ -1450,6 +1869,12 @@ static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
>      [DCD_CONFIG][GET_DYN_CAP_EXT_LIST] = {
>          "DCD_GET_DYNAMIC_CAPACITY_EXTENT_LIST", cmd_dcd_get_dyn_cap_ext_list,
>          8, 0 },
> +    [DCD_CONFIG][ADD_DYN_CAP_RSP] = {
> +        "DCD_ADD_DYNAMIC_CAPACITY_RESPONSE", cmd_dcd_add_dyn_cap_rsp,
> +        ~0, IMMEDIATE_DATA_CHANGE },
> +    [DCD_CONFIG][RELEASE_DYN_CAP] = {
> +        "DCD_RELEASE_DYNAMIC_CAPACITY", cmd_dcd_release_dyn_cap,
> +        ~0, IMMEDIATE_DATA_CHANGE },
>  };
>