On Mon, Mar 25, 2024 at 12:02:27PM -0700, nifan....@gmail.com wrote:
> From: Fan Ni <fan...@samsung.com>
>
> To simulate FM functionalities for initiating Dynamic Capacity Add
> (Opcode 5604h) and Dynamic Capacity Release (Opcode 5605h) as in CXL spec
> r3.1 7.6.7.6.5 and 7.6.7.6.6, we implemented two QMP interfaces to issue
> add/release dynamic capacity extents requests.
>
... snip
> +
> +/*
> + * The main function to process dynamic capacity event. Currently DC extents
> + * add/release requests are processed.
> + */
> +static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog
> log,
> + CXLDCEventType type, uint16_t
> hid,
> + uint8_t rid,
> + CXLDCExtentRecordList *records,
> + Error **errp)
> +{
... snip
> + /* Sanity check and count the extents */
> + list = records;
> + while (list) {
> + offset = list->value->offset;
> + len = list->value->len;
> + dpa = offset + dcd->dc.regions[rid].base;
> +
> + if (len == 0) {
> + error_setg(errp, "extent with 0 length is not allowed");
> + return;
> + }
> +
> + if (offset % block_size || len % block_size) {
> + error_setg(errp, "dpa or len is not aligned to region block
> size");
> + return;
> + }
> +
> + if (offset + len > dcd->dc.regions[rid].len) {
> + error_setg(errp, "extent range is beyond the region end");
> + return;
> + }
> +
> + /* No duplicate or overlapped extents are allowed */
> + if (test_any_bits_set(blk_bitmap, offset / block_size,
> + len / block_size)) {
> + error_setg(errp, "duplicate or overlapped extents are detected");
> + return;
> + }
> + bitmap_set(blk_bitmap, offset / block_size, len / block_size);
> +
> + num_extents++;
I think num_extents is always equal to the length of the list, otherwise
this code will return with error.
Nitpick:
This can be moved to the bottom w/ `list = list->next` to express that a
little more clearly.
> + if (type == DC_EVENT_RELEASE_CAPACITY) {
> + if (cxl_extents_overlaps_dpa_range(&dcd->dc.extents_pending,
> + dpa, len)) {
> + error_setg(errp,
> + "cannot release extent with pending DPA range");
> + return;
> + }
> + if (!cxl_extents_contains_dpa_range(&dcd->dc.extents,
> + dpa, len)) {
> + error_setg(errp,
> + "cannot release extent with non-existing DPA
> range");
> + return;
> + }
> + }
> + list = list->next;
> + }
> +
> + if (num_extents == 0) {
Since num_extents is always the length of the list, this is equivalent to
`if (!records)` prior to the while loop. Makes it a little more clear that:
1. There must be at least 1 extent
2. All extents must be valid for the command to be serviced.
> + error_setg(errp, "no valid extents to send to process");
> + return;
> + }
> +
I'm looking at adding the MHD extensions around this point, e.g.:
/* If MHD cannot allocate requested extents, the cmd fails */
if (type == DC_EVENT_ADD_CAPACITY && dcd->mhd_dcd_extents_allocate &&
num_extents != dcd->mhd_dcd_extents_allocate(...))
return;
where mhd_dcd_extents_allocate checks the MHD block bitmap and tags
for correctness (shared // no double-allocations, etc). On success,
it garuantees proper ownership.
the release path would then be done in the release response path from
the host, as opposed to the release event injection.
Do you see any issues with that flow?
> + /* Create extent list for event being passed to host */
> + i = 0;
> + list = records;
> + extents = g_new0(CXLDCExtentRaw, num_extents);
> + while (list) {
> + offset = list->value->offset;
> + len = list->value->len;
> + dpa = dcd->dc.regions[rid].base + offset;
> +
> + extents[i].start_dpa = dpa;
> + extents[i].len = len;
> + memset(extents[i].tag, 0, 0x10);
> + extents[i].shared_seq = 0;
> + list = list->next;
> + i++;
> + }
> +
> + /*
> + * CXL r3.1 section 8.2.9.2.1.6: Dynamic Capacity Event Record
> + *
> + * All Dynamic Capacity event records shall set the Event Record Severity
> + * field in the Common Event Record Format to Informational Event. All
> + * Dynamic Capacity related events shall be logged in the Dynamic
> Capacity
> + * Event Log.
> + */
> + cxl_assign_event_header(hdr, &dynamic_capacity_uuid, flags, sizeof(dCap),
> + cxl_device_get_timestamp(&dcd->cxl_dstate));
> +
> + dCap.type = type;
> + /* FIXME: for now, validity flag is cleared */
> + dCap.validity_flags = 0;
> + stw_le_p(&dCap.host_id, hid);
> + /* only valid for DC_REGION_CONFIG_UPDATED event */
> + dCap.updated_region_id = 0;
> + /*
> + * FIXME: for now, the "More" flag is cleared as there is only one
> + * extent associating with each record and tag-based release is
> + * not supported.
> + */
> + dCap.flags = 0;
> + for (i = 0; i < num_extents; i++) {
> + memcpy(&dCap.dynamic_capacity_extent, &extents[i],
> + sizeof(CXLDCExtentRaw));
> +
> + if (type == DC_EVENT_ADD_CAPACITY) {
> + cxl_insert_extent_to_extent_list(&dcd->dc.extents_pending,
> + extents[i].start_dpa,
> + extents[i].len,
> + extents[i].tag,
> + extents[i].shared_seq);
> + }
> +
> + if (cxl_event_insert(&dcd->cxl_dstate, enc_log,
> + (CXLEventRecordRaw *)&dCap)) {
Pardon if I missed a prior discussion about this, but what happens to
pending events in the scenario where cxl_event_insert fails?
~Gregory
