[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 08/12] hw/cxl/cxl-mailbox-utils: Add mailbox commands to s
|
From: |
Jonathan Cameron |
|
Subject: |
Re: [PATCH v6 08/12] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response |
|
Date: |
Fri, 5 Apr 2024 12:39:02 +0100 |
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 },
> };
>