* [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605
@ 2025-03-17 16:31 anisa.su887
2025-03-17 16:31 ` [PATCH 1/9] cxl/type3: Add supported block sizes bitmask to CXLDCRegion struct anisa.su887
` (8 more replies)
0 siblings, 9 replies; 34+ messages in thread
From: anisa.su887 @ 2025-03-17 16:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jonathan.cameron, nifan.cxl, dave, linux-cxl, Anisa Su
From: Anisa Su <anisa.su@samsung.com>
This patchset adds support for 6 FM API DCD Management commands (0x5600-0x5605)
according to the CXL r3.2 Spec. It is based on the following branch:
https://gitlab.com/jic23/qemu/-/tree/cxl-2025-02-20.
The code was tested with libcxlmi, which runs in the QEMU VM and sends 56xxh
commands to the device (QEMU emulated) through MCTP messages over I2C
bus. To perform end-to-end tests, both MCTP and DCD support are needed
for the kernel, so the needed MCTP patches are applied on top of Ira's DCD
branch https://github.com/weiny2/linux-kernel/tree/dcd-v4-2024-12-11.
For the tests of commands 0x5600 (Get DCD Info), 0x5601 (Get Host DC Region
Config), and 0x5603 (Get DC Region Extent Lists), DCD kernel code is not involved.
The libcxlmi test program is used to send the command to the device and results
are collected and verified.
For command 0x5602 (Set DC Region Config): device creates an event record with type
DC_EVENT_REGION_CONFIG_UPDATED and triggers an interrupt to the host
if the configuration changes as a result of the command. Currently, the kernel
version used to test this only supports Add/Release type events. Thus, this
request essentially gets ignored but did not cause problems besides the host
not knowing about the configuration change when tested.
For the command 0x5604 (Initiate DC Add) and 0x5605 (Initiate DC Release), the
tests involve libcxlmi test program (acting as the FM), kernel DCD
code (host) and QEMU device. The test workflow follows that in cxl r3.2 section
7.6.7.6.5 and 7.6.7.6.6. More specifically, the tests involve following
steps,
1. Start a VM with CXL topology: https://github.com/moking/cxl-test-tool/blob/main/utils/cxl.py#L54.
2. Load the CXL related drivers in the VM;
3. Create a DC region for the DCD device attached.
4. add/release DC extents by sending 0x5604 and 0x5605 respectively through
the out-of-tree libcxlmi test program
(https://github.com/anisa-su993/libcxlmi/blob/dcd_management_cmds/tests/test-fmapi.c).
5. Check and verify the extents by retrieving the extents list through
command 0x5603 in the test program.
The remaining 3 commands in this series (0x5606-0x5608) are related to tags
and sharing, thus have not been implemented.
Anisa Su (9):
cxl/type3: Add supported block sizes bitmask to CXLDCRegion struct
cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info
cxl/type3: Add dsmas_flags to CXLDCRegion struct
cxl-mailbox-utils: 0x5601 - FMAPI Get Host Region Config
cxl_events.h: move definition for dynamic_capacity_uuid and enum for
DC event types
cxl-mailbox-utils: 0x5602 - FMAPI Set DC Region Config
cxl-mailbox-utils: 0x5603 - FMAPI Get DC Region Extent Lists
cxl-mailbox-utils: 0x5604 - FMAPI Initiate DC Add
cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release
hw/cxl/cxl-mailbox-utils.c | 615 +++++++++++++++++++++++++++++++++++
hw/cxl/i2c_mctp_cxl.c | 6 +-
hw/mem/cxl_type3.c | 30 +-
include/hw/cxl/cxl_device.h | 9 +
include/hw/cxl/cxl_events.h | 15 +
include/hw/cxl/cxl_mailbox.h | 6 +
6 files changed, 660 insertions(+), 21 deletions(-)
--
2.47.2
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/9] cxl/type3: Add supported block sizes bitmask to CXLDCRegion struct
2025-03-17 16:31 [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605 anisa.su887
@ 2025-03-17 16:31 ` anisa.su887
2025-04-24 10:11 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info anisa.su887
` (7 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: anisa.su887 @ 2025-03-17 16:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jonathan.cameron, nifan.cxl, dave, linux-cxl, Anisa Su
From: Anisa Su <anisa.su@samsung.com>
Add supported_blk_size field to CXLDCRegion struct in preparation for
next patch. It is needed by command 0x5600 Get DC Region Config.
Signed-off-by: Anisa Su <anisa.su@samsung.com>
---
hw/mem/cxl_type3.c | 3 +++
include/hw/cxl/cxl_device.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 11c38a9292..731497ebda 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -8,6 +8,7 @@
*
* SPDX-License-Identifier: GPL-v2-only
*/
+#include <math.h>
#include "qemu/osdep.h"
#include "qemu/units.h"
@@ -766,6 +767,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
uint64_t region_len;
uint64_t decode_len;
uint64_t blk_size = 2 * MiB;
+ uint64_t supported_blk_size_bitmask = BIT((int) log2(blk_size));
CXLDCRegion *region;
MemoryRegion *mr;
uint64_t dc_size;
@@ -811,6 +813,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
.block_size = blk_size,
/* dsmad_handle set when creating CDAT table entries */
.flags = 0,
+ .supported_blk_size_bitmask = supported_blk_size_bitmask,
};
ct3d->dc.total_capacity += region->len;
region->blk_bitmap = bitmap_new(region->len / region->block_size);
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index ca515cab13..bebed04085 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -608,6 +608,7 @@ typedef struct CXLDCRegion {
uint32_t dsmadhandle;
uint8_t flags;
unsigned long *blk_bitmap;
+ uint64_t supported_blk_size_bitmask;
} CXLDCRegion;
typedef struct CXLSetFeatureInfo {
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info
2025-03-17 16:31 [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605 anisa.su887
2025-03-17 16:31 ` [PATCH 1/9] cxl/type3: Add supported block sizes bitmask to CXLDCRegion struct anisa.su887
@ 2025-03-17 16:31 ` anisa.su887
2025-03-18 15:56 ` Jonathan Cameron via
2025-04-24 10:30 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct anisa.su887
` (6 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: anisa.su887 @ 2025-03-17 16:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jonathan.cameron, nifan.cxl, dave, linux-cxl, Anisa Su
From: Anisa Su <anisa.su@samsung.com>
FM DCD Management command 0x5600 implemented per CXL 3.2 Spec Section 7.6.7.6.1
Signed-off-by: Anisa Su <anisa.su@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 67 ++++++++++++++++++++++++++++++++++++++
hw/cxl/i2c_mctp_cxl.c | 6 +++-
2 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 1b62d36101..e9991fd1a7 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -122,6 +122,8 @@ enum {
#define MANAGEMENT_COMMAND 0x0
MHD = 0x55,
#define GET_MHD_INFO 0x0
+ FMAPI_DCD_MGMT = 0x56,
+ #define GET_DCD_INFO 0x0
};
/* CCI Message Format CXL r3.1 Figure 7-19 */
@@ -3341,6 +3343,62 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+/*
+ * CXL r3.2 section 7.6.7.6.1: Get DCD Info (Opcode 5600h)
+ */
+static CXLRetCode cmd_fm_get_dcd_info(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ struct {
+ uint8_t num_hosts;
+ uint8_t num_regions_supported;
+ uint8_t rsvd1[2];
+ uint16_t add_select_policy_bitmask;
+ uint8_t rsvd2[2];
+ uint16_t release_select_policy_bitmask;
+ uint8_t sanitize_on_release_bitmask;
+ uint8_t rsvd3;
+ uint64_t total_dynamic_capacity;
+ uint64_t region_blk_size_bitmasks[8];
+ } QEMU_PACKED *out;
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ CXLDCRegion region;
+ int i;
+
+ if (ct3d->dc.num_regions == 0) {
+ return CXL_MBOX_UNSUPPORTED;
+ }
+
+ out = (void *)payload_out;
+
+ /* TODO: num hosts set to 1 for now */
+ out->num_hosts = 1;
+ out->num_regions_supported = ct3d->dc.num_regions;
+ /* TODO: only prescriptive supported for now */
+ stw_le_p(&out->add_select_policy_bitmask,
+ CXL_EXTENT_SELECTION_POLICY_PRESCRIPTIVE);
+ stw_le_p(&out->release_select_policy_bitmask,
+ CXL_EXTENT_REMOVAL_POLICY_PRESCRIPTIVE);
+ /* TODO: sanitize on release bitmask cleared for now */
+ out->sanitize_on_release_bitmask = 0;
+
+ stq_le_p(&out->total_dynamic_capacity,
+ ct3d->dc.total_capacity / CXL_CAPACITY_MULTIPLIER);
+
+ for (i = 0; i < ct3d->dc.num_regions; i++) {
+ region = ct3d->dc.regions[i];
+ memcpy(&out->region_blk_size_bitmasks[i],
+ ®ion.supported_blk_size_bitmask, 8);
+ }
+
+ *len_out = sizeof(*out);
+ return CXL_MBOX_SUCCESS;
+}
+
static const struct cxl_cmd cxl_cmd_set[256][256] = {
[INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
cmd_infostat_bg_op_abort, 0, 0 },
@@ -3462,6 +3520,11 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
cmd_tunnel_management_cmd, ~0, 0 },
};
+static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
+ [FMAPI_DCD_MGMT][GET_DCD_INFO] = { "GET_DCD_INFO",
+ cmd_fm_get_dcd_info, 0, 0},
+};
+
/*
* While the command is executing in the background, the device should
* update the percentage complete in the Background Command Status Register
@@ -3764,7 +3827,11 @@ void cxl_initialize_t3_fm_owned_ld_mctpcci(CXLCCI *cci, DeviceState *d,
DeviceState *intf,
size_t payload_max)
{
+ CXLType3Dev *ct3d = CXL_TYPE3(d);
cxl_copy_cci_commands(cci, cxl_cmd_set_t3_fm_owned_ld_mctp);
+ if (ct3d->dc.num_regions) {
+ cxl_copy_cci_commands(cci, cxl_cmd_set_fm_dcd);
+ }
cci->d = d;
cci->intf = intf;
cxl_init_cci(cci, payload_max);
diff --git a/hw/cxl/i2c_mctp_cxl.c b/hw/cxl/i2c_mctp_cxl.c
index 7d2cbc3b75..df95182925 100644
--- a/hw/cxl/i2c_mctp_cxl.c
+++ b/hw/cxl/i2c_mctp_cxl.c
@@ -46,6 +46,9 @@
/* Implementation choice - may make this configurable */
#define MCTP_CXL_MAILBOX_BYTES 512
+/* Supported FMAPI Cmds */
+#define FMAPI_CMD_MAX_OPCODE 0x57
+
typedef struct CXLMCTPMessage {
/*
* DSP0236 (MCTP Base) Integrity Check + Message Type
@@ -200,7 +203,8 @@ static void i2c_mctp_cxl_handle_message(MCTPI2CEndpoint *mctp)
if (!(msg->message_type == MCTP_MT_CXL_TYPE3 &&
msg->command_set < 0x51) &&
!(msg->message_type == MCTP_MT_CXL_FMAPI &&
- msg->command_set >= 0x51 && msg->command_set < 0x56)) {
+ msg->command_set >= 0x51 &&
+ msg->command_set < FMAPI_CMD_MAX_OPCODE)) {
buf->rc = CXL_MBOX_UNSUPPORTED;
st24_le_p(buf->pl_length, len_out);
s->len = s->pos;
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct
2025-03-17 16:31 [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605 anisa.su887
2025-03-17 16:31 ` [PATCH 1/9] cxl/type3: Add supported block sizes bitmask to CXLDCRegion struct anisa.su887
2025-03-17 16:31 ` [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info anisa.su887
@ 2025-03-17 16:31 ` anisa.su887
2025-04-24 10:42 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 4/9] cxl-mailbox-utils: 0x5601 - FMAPI Get Host Region Config anisa.su887
` (5 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: anisa.su887 @ 2025-03-17 16:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jonathan.cameron, nifan.cxl, dave, linux-cxl, Anisa Su
From: Anisa Su <anisa.su@samsung.com>
Add dsmas_flags field to DC Region struct in preparation for next
command, which returns the dsmas flags in the response.
Signed-off-by: Anisa Su <anisa.su@samsung.com>
---
hw/mem/cxl_type3.c | 2 ++
include/hw/cxl/cxl_device.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 731497ebda..452a0c101a 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -237,6 +237,8 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
ct3d->dc.regions[i].len,
false, true, region_base);
ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1;
+ CDATDsmas *dsmas = (CDATDsmas *) table[cur_ent + CT3_CDAT_DSMAS];
+ ct3d->dc.regions[i].dsmas_flags = dsmas->flags;
cur_ent += CT3_CDAT_NUM_ENTRIES;
region_base += ct3d->dc.regions[i].len;
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index bebed04085..81b826f570 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -609,6 +609,7 @@ typedef struct CXLDCRegion {
uint8_t flags;
unsigned long *blk_bitmap;
uint64_t supported_blk_size_bitmask;
+ uint8_t dsmas_flags;
} CXLDCRegion;
typedef struct CXLSetFeatureInfo {
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 4/9] cxl-mailbox-utils: 0x5601 - FMAPI Get Host Region Config
2025-03-17 16:31 [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605 anisa.su887
` (2 preceding siblings ...)
2025-03-17 16:31 ` [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct anisa.su887
@ 2025-03-17 16:31 ` anisa.su887
2025-04-24 10:53 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 5/9] cxl_events.h: move definition for dynamic_capacity_uuid and enum for DC event types anisa.su887
` (4 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: anisa.su887 @ 2025-03-17 16:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jonathan.cameron, nifan.cxl, dave, linux-cxl, Anisa Su
From: Anisa Su <anisa.su@samsung.com>
FM DCD Management command 0x5601 implemented per CXL r3.2 Spec Section 7.6.7.6.2
Signed-off-by: Anisa Su <anisa.su@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 97 ++++++++++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index e9991fd1a7..4bb51bf3e8 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -124,6 +124,7 @@ enum {
#define GET_MHD_INFO 0x0
FMAPI_DCD_MGMT = 0x56,
#define GET_DCD_INFO 0x0
+ #define GET_HOST_DC_REGION_CONFIG 0x1
};
/* CCI Message Format CXL r3.1 Figure 7-19 */
@@ -3399,6 +3400,100 @@ static CXLRetCode cmd_fm_get_dcd_info(const struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+/*
+ * CXL r3.2 section 7.6.7.6.2: Get Host DC Region Configuration (Opcode 5601h)
+ */
+static CXLRetCode cmd_fm_get_host_dc_region_config(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ struct {
+ uint16_t host_id;
+ uint8_t region_cnt;
+ uint8_t start_rid;
+ } QEMU_PACKED *in;
+ struct {
+ uint16_t host_id;
+ uint8_t num_regions;
+ uint8_t regions_returned;
+ struct {
+ uint64_t base;
+ uint64_t decode_len;
+ uint64_t region_len;
+ uint64_t block_size;
+ uint8_t dsmas_flags;
+ uint8_t rsvd1[3];
+ uint8_t sanitize;
+ uint8_t rsvd2[3];
+ } QEMU_PACKED records[];
+ } QEMU_PACKED *out;
+ struct {
+ uint32_t num_extents_supported;
+ uint32_t num_extents_available;
+ uint32_t num_tags_supported;
+ uint32_t num_tags_available;
+ } QEMU_PACKED *extra_out;
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ uint16_t record_count, out_pl_len, i;
+ uint8_t start_rid;
+
+ if (ct3d->dc.num_regions == 0) {
+ return CXL_MBOX_UNSUPPORTED;
+ }
+
+ in = (void *)payload_in;
+ if (in->start_rid >= ct3d->dc.num_regions) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ start_rid = in->start_rid;
+ record_count = MIN(ct3d->dc.num_regions - start_rid, in->region_cnt);
+
+ out = (void *)payload_out;
+ out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
+
+ extra_out = (void *)(out) + out_pl_len;
+ out_pl_len += sizeof(*extra_out);
+
+ assert(*len_out <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
+
+ /* TODO: For now, fix host id to be 0 */
+ stw_le_p(&out->host_id, 0);
+ out->num_regions = ct3d->dc.num_regions;
+ out->regions_returned = record_count;
+
+ for (i = 0; i < record_count; i++) {
+ stq_le_p(&out->records[i].base,
+ ct3d->dc.regions[start_rid + i].base);
+ stq_le_p(&out->records[i].decode_len,
+ ct3d->dc.regions[start_rid + i].decode_len /
+ CXL_CAPACITY_MULTIPLIER);
+ stq_le_p(&out->records[i].region_len,
+ ct3d->dc.regions[start_rid + i].len);
+ stq_le_p(&out->records[i].block_size,
+ ct3d->dc.regions[start_rid + i].block_size);
+ out->records[i].dsmas_flags =
+ ct3d->dc.regions[start_rid + i].dsmas_flags;
+ out->records[i].sanitize = 0;
+ }
+ /*
+ * TODO: Assign values once extents and tags are introduced
+ * to use.
+ */
+ stl_le_p(&extra_out->num_extents_supported, CXL_NUM_EXTENTS_SUPPORTED);
+ stl_le_p(&extra_out->num_extents_available, CXL_NUM_EXTENTS_SUPPORTED -
+ ct3d->dc.total_extent_count);
+ stl_le_p(&extra_out->num_tags_supported, CXL_NUM_TAGS_SUPPORTED);
+ stl_le_p(&extra_out->num_tags_available, CXL_NUM_TAGS_SUPPORTED);
+
+ *len_out = out_pl_len;
+
+ return CXL_MBOX_SUCCESS;
+}
+
static const struct cxl_cmd cxl_cmd_set[256][256] = {
[INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
cmd_infostat_bg_op_abort, 0, 0 },
@@ -3523,6 +3618,8 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
[FMAPI_DCD_MGMT][GET_DCD_INFO] = { "GET_DCD_INFO",
cmd_fm_get_dcd_info, 0, 0},
+ [FMAPI_DCD_MGMT][GET_HOST_DC_REGION_CONFIG] = { "GET_HOST_DC_REGION_CONFIG",
+ cmd_fm_get_host_dc_region_config, 4, 0},
};
/*
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 5/9] cxl_events.h: move definition for dynamic_capacity_uuid and enum for DC event types
2025-03-17 16:31 [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605 anisa.su887
` (3 preceding siblings ...)
2025-03-17 16:31 ` [PATCH 4/9] cxl-mailbox-utils: 0x5601 - FMAPI Get Host Region Config anisa.su887
@ 2025-03-17 16:31 ` anisa.su887
2025-04-24 10:55 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 6/9] cxl-mailbox-utils: 0x5602 - FMAPI Set DC Region Config anisa.su887
` (3 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: anisa.su887 @ 2025-03-17 16:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jonathan.cameron, nifan.cxl, dave, linux-cxl, Anisa Su
From: Anisa Su <anisa.su@samsung.com>
move definition for dynamic_capacity_uuid and enum for DC event types to
cxl_events.h from cxl_type3.c for shared use in next patch
Signed-off-by: Anisa Su <anisa.su@samsung.com>
---
hw/mem/cxl_type3.c | 15 ---------------
include/hw/cxl/cxl_events.h | 15 +++++++++++++++
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 452a0c101a..ac74e62875 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1977,21 +1977,6 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
}
}
-/* CXL r3.1 Table 8-50: Dynamic Capacity Event Record */
-static const QemuUUID dynamic_capacity_uuid = {
- .data = UUID(0xca95afa7, 0xf183, 0x4018, 0x8c, 0x2f,
- 0x95, 0x26, 0x8e, 0x10, 0x1a, 0x2a),
-};
-
-typedef enum CXLDCEventType {
- DC_EVENT_ADD_CAPACITY = 0x0,
- DC_EVENT_RELEASE_CAPACITY = 0x1,
- DC_EVENT_FORCED_RELEASE_CAPACITY = 0x2,
- DC_EVENT_REGION_CONFIG_UPDATED = 0x3,
- DC_EVENT_ADD_CAPACITY_RSP = 0x4,
- DC_EVENT_CAPACITY_RELEASED = 0x5,
-} CXLDCEventType;
-
/*
* Check whether the range [dpa, dpa + len - 1] has overlaps with extents in
* the list.
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index 38cadaa0f3..758b075a64 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -184,4 +184,19 @@ typedef struct CXLEventDynamicCapacity {
uint32_t tags_avail;
} QEMU_PACKED CXLEventDynamicCapacity;
+/* CXL r3.1 Table 8-50: Dynamic Capacity Event Record */
+static const QemuUUID dynamic_capacity_uuid = {
+ .data = UUID(0xca95afa7, 0xf183, 0x4018, 0x8c, 0x2f,
+ 0x95, 0x26, 0x8e, 0x10, 0x1a, 0x2a),
+};
+
+typedef enum CXLDCEventType {
+ DC_EVENT_ADD_CAPACITY = 0x0,
+ DC_EVENT_RELEASE_CAPACITY = 0x1,
+ DC_EVENT_FORCED_RELEASE_CAPACITY = 0x2,
+ DC_EVENT_REGION_CONFIG_UPDATED = 0x3,
+ DC_EVENT_ADD_CAPACITY_RSP = 0x4,
+ DC_EVENT_CAPACITY_RELEASED = 0x5,
+} CXLDCEventType;
+
#endif /* CXL_EVENTS_H */
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 6/9] cxl-mailbox-utils: 0x5602 - FMAPI Set DC Region Config
2025-03-17 16:31 [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605 anisa.su887
` (4 preceding siblings ...)
2025-03-17 16:31 ` [PATCH 5/9] cxl_events.h: move definition for dynamic_capacity_uuid and enum for DC event types anisa.su887
@ 2025-03-17 16:31 ` anisa.su887
2025-04-16 21:33 ` Anisa Su
2025-04-24 11:05 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 7/9] cxl-mailbox-utils: 0x5603 - FMAPI Get DC Region Extent Lists anisa.su887
` (2 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: anisa.su887 @ 2025-03-17 16:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jonathan.cameron, nifan.cxl, dave, linux-cxl, Anisa Su
From: Anisa Su <anisa.su@samsung.com>
FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
Signed-off-by: Anisa Su <anisa.su@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 100 +++++++++++++++++++++++++++++++++++
hw/mem/cxl_type3.c | 2 +-
include/hw/cxl/cxl_device.h | 3 ++
include/hw/cxl/cxl_mailbox.h | 6 +++
4 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 4bb51bf3e8..51ead2b152 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -125,6 +125,7 @@ enum {
FMAPI_DCD_MGMT = 0x56,
#define GET_DCD_INFO 0x0
#define GET_HOST_DC_REGION_CONFIG 0x1
+ #define SET_DC_REGION_CONFIG 0x2
};
/* CCI Message Format CXL r3.1 Figure 7-19 */
@@ -3494,6 +3495,98 @@ static CXLRetCode cmd_fm_get_host_dc_region_config(const struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+static void cxl_mbox_dc_event_create_record_hdr(CXLType3Dev *ct3d,
+ CXLEventRecordHdr *hdr)
+{
+ /*
+ * 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.
+ */
+ uint8_t flags = 1 << CXL_EVENT_TYPE_INFO;
+
+ st24_le_p(&hdr->flags, flags);
+ hdr->length = sizeof(struct CXLEventDynamicCapacity);
+ memcpy(&hdr->id, &dynamic_capacity_uuid, sizeof(hdr->id));
+ stq_le_p(&hdr->timestamp, cxl_device_get_timestamp(&ct3d->cxl_dstate));
+}
+
+/*
+ * CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602)
+ */
+static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ struct {
+ uint8_t reg_id;
+ uint8_t rsvd[3];
+ uint64_t block_sz;
+ uint8_t flags;
+ uint8_t rsvd2[3];
+ } QEMU_PACKED *in;
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ CXLEventDynamicCapacity dcEvent = {};
+ CXLDCRegion *region;
+
+ if (ct3d->dc.num_regions == 0) {
+ return CXL_MBOX_UNSUPPORTED;
+ }
+
+ /*
+ * TODO: Fail with CXL_MBOX_INVALID_SECURITY_STATE if
+ * device has been locked
+ */
+
+ in = (void *)payload_in;
+ region = &ct3d->dc.regions[in->reg_id];
+
+ /*
+ * TODO: Fail if sanitize bit doesn't match current config and "the device
+ * does not support reconfiguration of the Sanitize on Release setting."
+ * Currently not reconfigurable, so always fail if sanitize bit
+ * doesn't match.
+ */
+ if ((in->flags & 1) != (region->flags & 1)) {
+ return CXL_MBOX_UNSUPPORTED;
+ }
+
+ if (in->reg_id >= DCD_MAX_NUM_REGION) {
+ return CXL_MBOX_UNSUPPORTED;
+ }
+
+ /* Check that no extents are in the region being reconfigured */
+ if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
+ return CXL_MBOX_UNSUPPORTED;
+ }
+
+ /* Free bitmap and create new one for new block size. */
+ g_free(region->blk_bitmap);
+ region->blk_bitmap = bitmap_new(region->len / in->block_sz);
+
+ /* Create event record and insert into event log */
+ cxl_mbox_dc_event_create_record_hdr(ct3d, &dcEvent.hdr);
+ dcEvent.type = DC_EVENT_REGION_CONFIG_UPDATED;
+ /* FIXME: for now, validity flag is cleared */
+ dcEvent.validity_flags = 0;
+ /* FIXME: Currently only support 1 host */
+ dcEvent.host_id = 0;
+ dcEvent.updated_region_id = in->reg_id;
+
+ if (cxl_event_insert(&ct3d->cxl_dstate,
+ CXL_EVENT_TYPE_DYNAMIC_CAP,
+ (CXLEventRecordRaw *)&dcEvent)) {
+ cxl_event_irq_assert(ct3d);
+ }
+ return CXL_MBOX_SUCCESS;
+}
+
static const struct cxl_cmd cxl_cmd_set[256][256] = {
[INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
cmd_infostat_bg_op_abort, 0, 0 },
@@ -3620,6 +3713,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
cmd_fm_get_dcd_info, 0, 0},
[FMAPI_DCD_MGMT][GET_HOST_DC_REGION_CONFIG] = { "GET_HOST_DC_REGION_CONFIG",
cmd_fm_get_host_dc_region_config, 4, 0},
+ [FMAPI_DCD_MGMT][SET_DC_REGION_CONFIG] = { "SET_DC_REGION_CONFIG",
+ cmd_fm_set_dc_region_config, 16,
+ (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
+ CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
+ CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
+ CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
+ CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
};
/*
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index ac74e62875..b742b2bb8d 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1686,7 +1686,7 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
pcie_aer_inject_error(PCI_DEVICE(obj), &err);
}
-static void cxl_assign_event_header(CXLEventRecordHdr *hdr,
+void cxl_assign_event_header(CXLEventRecordHdr *hdr,
const QemuUUID *uuid, uint32_t flags,
uint8_t length, uint64_t timestamp)
{
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 81b826f570..217003a29d 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -806,4 +806,7 @@ void ct3_clear_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
uint64_t len);
bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
uint64_t len);
+void cxl_assign_event_header(CXLEventRecordHdr *hdr,
+ const QemuUUID *uuid, uint32_t flags,
+ uint8_t length, uint64_t timestamp);
#endif
diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
index 8e1c7c5f15..820c411cbb 100644
--- a/include/hw/cxl/cxl_mailbox.h
+++ b/include/hw/cxl/cxl_mailbox.h
@@ -8,6 +8,7 @@
#ifndef CXL_MAILBOX_H
#define CXL_MAILBOX_H
+#define CXL_MBOX_CONFIG_CHANGE_COLD_RESET (1)
#define CXL_MBOX_IMMEDIATE_CONFIG_CHANGE (1 << 1)
#define CXL_MBOX_IMMEDIATE_DATA_CHANGE (1 << 2)
#define CXL_MBOX_IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -15,6 +16,11 @@
#define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5)
#define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
#define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
+#define CXL_MBOX_SECONDARY_MBOX_SUPPORTED (1 << 8)
+#define CXL_MBOX_REQUEST_ABORT_BACKGROUND_OP_SUPPORTED (1 << 9)
+#define CXL_MBOX_CEL_10_TO_11_VALID (1 << 10)
+#define CXL_MBOX_CONFIG_CHANGE_CONV_RESET (1 << 11)
+#define CXL_MBOX_CONFIG_CHANGE_CXL_RESET (1 << 12)
#define CXL_LOG_CAP_CLEAR_SUPPORTED (1 << 0)
#define CXL_LOG_CAP_POPULATE_SUPPORTED (1 << 1)
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 7/9] cxl-mailbox-utils: 0x5603 - FMAPI Get DC Region Extent Lists
2025-03-17 16:31 [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605 anisa.su887
` (5 preceding siblings ...)
2025-03-17 16:31 ` [PATCH 6/9] cxl-mailbox-utils: 0x5602 - FMAPI Set DC Region Config anisa.su887
@ 2025-03-17 16:31 ` anisa.su887
2025-04-24 11:10 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 8/9] cxl-mailbox-utils: 0x5604 - FMAPI Initiate DC Add anisa.su887
2025-03-17 16:31 ` [PATCH 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release anisa.su887
8 siblings, 1 reply; 34+ messages in thread
From: anisa.su887 @ 2025-03-17 16:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jonathan.cameron, nifan.cxl, dave, linux-cxl, Anisa Su
From: Anisa Su <anisa.su@samsung.com>
FM DCD Management command 0x5603 implemented per CXL r3.2 Spec Section 7.6.7.6.4
Very similar to previously implemented command 0x4801.
Signed-off-by: Anisa Su <anisa.su@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 84 ++++++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 51ead2b152..77cf0fdb15 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -126,6 +126,7 @@ enum {
#define GET_DCD_INFO 0x0
#define GET_HOST_DC_REGION_CONFIG 0x1
#define SET_DC_REGION_CONFIG 0x2
+ #define GET_DC_REGION_EXTENT_LIST 0x3
};
/* CCI Message Format CXL r3.1 Figure 7-19 */
@@ -3587,6 +3588,87 @@ static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+/*
+ * CXL r3.2 section 7.6.7.6.4 Get DC Region Extent Lists (Opcode 5603h)
+ */
+static CXLRetCode cmd_fm_get_dc_region_extent_list(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ struct {
+ uint16_t host_id;
+ uint8_t rsvd[2];
+ uint32_t extent_cnt;
+ uint32_t start_extent_id;
+ } QEMU_PACKED *in = (void *)payload_in;
+ struct {
+ uint16_t host_id;
+ uint8_t rsvd[2];
+ uint32_t start_extent_id;
+ uint32_t extents_returned;
+ uint32_t total_extents;
+ uint32_t list_generation_num;
+ uint8_t rsvd2[4];
+ CXLDCExtentRaw records[];
+ } QEMU_PACKED *out = (void *)payload_out;
+ QEMU_BUILD_BUG_ON(sizeof(*in) != 0xc);
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ CXLDCExtent *ent;
+ CXLDCExtentRaw *out_rec;
+ uint16_t record_count = 0, record_done = 0, i = 0;
+ uint16_t out_pl_len, max_size;
+
+ if (ct3d->dc.num_regions == 0) {
+ return CXL_MBOX_UNSUPPORTED;
+ }
+
+ if (in->host_id != 0) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ if (in->start_extent_id > ct3d->dc.total_extent_count) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ record_count = MIN(in->extent_cnt,
+ ct3d->dc.total_extent_count - in->start_extent_id);
+ max_size = CXL_MAILBOX_MAX_PAYLOAD_SIZE - sizeof(*out);
+ record_count = MIN(record_count, max_size / sizeof(out->records[0]));
+ out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
+
+ stw_le_p(&out->host_id, in->host_id);
+ stl_le_p(&out->start_extent_id, in->start_extent_id);
+ stl_le_p(&out->extents_returned, record_count);
+ stl_le_p(&out->total_extents, ct3d->dc.total_extent_count);
+ stl_le_p(&out->list_generation_num, ct3d->dc.ext_list_gen_seq);
+
+ if (record_count > 0) {
+ out_rec = &out->records[record_done];
+
+ QTAILQ_FOREACH(ent, &ct3d->dc.extents, node) {
+ if (i++ < in->start_extent_id) {
+ continue;
+ }
+ stq_le_p(&out_rec->start_dpa, ent->start_dpa);
+ stq_le_p(&out_rec->len, ent->len);
+ memcpy(&out_rec->tag, ent->tag, 0x10);
+ stw_le_p(&out_rec->shared_seq, ent->shared_seq);
+
+ record_done++;
+ out_rec++;
+ if (record_done == record_count) {
+ break;
+ }
+ }
+ }
+
+ *len_out = out_pl_len;
+ return CXL_MBOX_SUCCESS;
+}
+
static const struct cxl_cmd cxl_cmd_set[256][256] = {
[INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
cmd_infostat_bg_op_abort, 0, 0 },
@@ -3720,6 +3802,8 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
+ [FMAPI_DCD_MGMT][GET_DC_REGION_EXTENT_LIST] = { "GET_DC_REGION_EXTENT_LIST",
+ cmd_fm_get_dc_region_extent_list, 12, 0},
};
/*
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 8/9] cxl-mailbox-utils: 0x5604 - FMAPI Initiate DC Add
2025-03-17 16:31 [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605 anisa.su887
` (6 preceding siblings ...)
2025-03-17 16:31 ` [PATCH 7/9] cxl-mailbox-utils: 0x5603 - FMAPI Get DC Region Extent Lists anisa.su887
@ 2025-03-17 16:31 ` anisa.su887
2025-04-24 11:19 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release anisa.su887
8 siblings, 1 reply; 34+ messages in thread
From: anisa.su887 @ 2025-03-17 16:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jonathan.cameron, nifan.cxl, dave, linux-cxl, Anisa Su
From: Anisa Su <anisa.su@samsung.com>
FM DCD Management command 0x5604 implemented per CXL r3.2 Spec Section 7.6.7.6.5
Signed-off-by: Anisa Su <anisa.su@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 173 ++++++++++++++++++++++++++++++++++++
hw/mem/cxl_type3.c | 8 +-
include/hw/cxl/cxl_device.h | 4 +
3 files changed, 181 insertions(+), 4 deletions(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 77cf0fdb15..5bcbd434b7 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -127,6 +127,8 @@ enum {
#define GET_HOST_DC_REGION_CONFIG 0x1
#define SET_DC_REGION_CONFIG 0x2
#define GET_DC_REGION_EXTENT_LIST 0x3
+ #define INITIATE_DC_ADD 0x4
+
};
/* CCI Message Format CXL r3.1 Figure 7-19 */
@@ -3669,6 +3671,170 @@ static CXLRetCode cmd_fm_get_dc_region_extent_list(const struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+static CXLRetCode cxl_mbox_dc_prescriptive_sanity_check(CXLType3Dev *dcd,
+ uint16_t host_id,
+ uint32_t ext_count,
+ CXLDCExtentRaw extents[],
+ CXLDCEventType type)
+{
+ CXLDCExtentRaw ext;
+ CXLDCRegion *reg = NULL;
+ int i, j;
+
+ if (dcd->dc.num_regions == 0) {
+ qemu_log_mask(LOG_UNIMP,
+ "No dynamic capacity support from the device.\n");
+ return CXL_MBOX_UNSUPPORTED;
+ }
+
+ /* TODO: host id check will be added later, currently host id must be 0. */
+ if (host_id != 0) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ for (i = 0; i < ext_count; i++) {
+ ext = extents[i];
+
+ if (ext.len == 0) {
+ return CXL_MBOX_INVALID_EXTENT_LIST;
+ }
+
+ reg = cxl_find_dc_region(dcd, ext.start_dpa, ext.len);
+ if (!reg) {
+ return CXL_MBOX_INVALID_EXTENT_LIST;
+ }
+
+ if (ext.len % reg->block_size || ext.start_dpa % reg->block_size) {
+ return CXL_MBOX_INVALID_EXTENT_LIST;
+ }
+
+ /* Check requested extents do not overlap with each other. */
+ for (j = i + 1; j < ext_count; j++) {
+ if (ranges_overlap(ext.start_dpa, ext.len, extents[j].start_dpa,
+ extents[j].len)) {
+ return CXL_MBOX_INVALID_EXTENT_LIST;
+ }
+ }
+
+ if (type == DC_EVENT_ADD_CAPACITY) {
+ /* Check requested extents do not overlap with existing extents. */
+ if (cxl_extents_overlaps_dpa_range(&dcd->dc.extents,
+ ext.start_dpa, ext.len)) {
+ return CXL_MBOX_INVALID_EXTENT_LIST;
+ }
+ }
+ }
+
+ return CXL_MBOX_SUCCESS;
+}
+
+/*
+ * CXL r3.2 Section 7.6.7.6.5 Initiate Dynamic Capacity Add (Opcode 5604h)
+ */
+static CXLRetCode cmd_fm_initiate_dc_add(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ struct {
+ uint16_t host_id;
+ uint8_t selection_policy;
+ uint8_t reg_num;
+ uint64_t length;
+ uint8_t tag[0x10];
+ uint32_t ext_count;
+ CXLDCExtentRaw extents[];
+ } QEMU_PACKED *in = (void *)payload_in;
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ g_autofree CXLDCExtentRaw *event_rec_exts = NULL;
+ CXLEventDynamicCapacity event_rec = {};
+ CXLDCExtentGroup *group = NULL;
+ CXLDCExtentRaw ext;
+ int rc, i;
+
+ switch (in->selection_policy) {
+ case CXL_EXTENT_SELECTION_POLICY_PRESCRIPTIVE:
+ /* Adding extents causes exceeding device's extent tracking ability. */
+ if (in->ext_count + ct3d->dc.total_extent_count >
+ CXL_NUM_EXTENTS_SUPPORTED) {
+ return CXL_MBOX_RESOURCES_EXHAUSTED;
+ }
+ rc = cxl_mbox_dc_prescriptive_sanity_check(ct3d,
+ in->host_id,
+ in->ext_count,
+ in->extents,
+ DC_EVENT_ADD_CAPACITY);
+ if (rc) {
+ return rc;
+ }
+
+ /* Prepare extents for Event Record */
+ event_rec_exts = g_new0(CXLDCExtentRaw, in->ext_count);
+ for (i = 0; i < in->ext_count; i++) {
+ ext = in->extents[i];
+ event_rec_exts[i].start_dpa = ext.start_dpa;
+ event_rec_exts[i].len = ext.len;
+ memset(event_rec_exts[i].tag, 0, 0x10);
+ event_rec_exts[i].shared_seq = 0;
+
+ /* Check requested extents do not overlap with pending extents. */
+ if (cxl_extent_groups_overlaps_dpa_range(&ct3d->dc.extents_pending,
+ ext.start_dpa, ext.len)) {
+ return CXL_MBOX_INVALID_EXTENT_LIST;
+ }
+
+ /* Create extent group to add to pending list. */
+ group = cxl_insert_extent_to_extent_group(group,
+ event_rec_exts[i].start_dpa,
+ event_rec_exts[i].len,
+ event_rec_exts[i].tag,
+ event_rec_exts[i].shared_seq);
+ }
+
+ /* Add requested extents to pending list. */
+ if (group) {
+ cxl_extent_group_list_insert_tail(&ct3d->dc.extents_pending, group);
+ }
+
+ /* Create event record and insert to event log */
+ cxl_mbox_dc_event_create_record_hdr(ct3d, &event_rec.hdr);
+ event_rec.type = DC_EVENT_ADD_CAPACITY;
+ /* FIXME: for now, validity flag is cleared */
+ event_rec.validity_flags = 0;
+ /* FIXME: Currently only support 1 host */
+ event_rec.host_id = 0;
+ /* updated_region_id only valid for DC_EVENT_REGION_CONFIG_UPDATED */
+ event_rec.updated_region_id = 0;
+ for (i = 0; i < in->ext_count; i++) {
+ memcpy(&event_rec.dynamic_capacity_extent,
+ &event_rec_exts[i],
+ sizeof(CXLDCExtentRaw));
+
+ event_rec.flags = 0;
+ if (i < in->ext_count - 1) {
+ /* Set "More" flag */
+ event_rec.flags |= BIT(0);
+ }
+
+ if (cxl_event_insert(&ct3d->cxl_dstate,
+ CXL_EVENT_TYPE_DYNAMIC_CAP,
+ (CXLEventRecordRaw *)&event_rec)) {
+ cxl_event_irq_assert(ct3d);
+ }
+ }
+
+ return CXL_MBOX_SUCCESS;
+ default:
+ qemu_log_mask(LOG_UNIMP,
+ "CXL extent selection policy not supported.\n");
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ return CXL_MBOX_SUCCESS;
+}
+
static const struct cxl_cmd cxl_cmd_set[256][256] = {
[INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
cmd_infostat_bg_op_abort, 0, 0 },
@@ -3804,6 +3970,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
[FMAPI_DCD_MGMT][GET_DC_REGION_EXTENT_LIST] = { "GET_DC_REGION_EXTENT_LIST",
cmd_fm_get_dc_region_extent_list, 12, 0},
+ [FMAPI_DCD_MGMT][INITIATE_DC_ADD] = { "INIT_DC_ADD",
+ cmd_fm_initiate_dc_add, ~0,
+ (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
+ CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
+ CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
+ CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
+ CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
};
/*
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index b742b2bb8d..ccc619fe10 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1982,8 +1982,8 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
* the list.
* Return value: return true if has overlaps; otherwise, return false
*/
-static bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
- uint64_t dpa, uint64_t len)
+bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
+ uint64_t dpa, uint64_t len)
{
CXLDCExtent *ent;
Range range1, range2;
@@ -2028,8 +2028,8 @@ bool cxl_extents_contains_dpa_range(CXLDCExtentList *list,
return false;
}
-static bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
- uint64_t dpa, uint64_t len)
+bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
+ uint64_t dpa, uint64_t len)
{
CXLDCExtentGroup *group;
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 217003a29d..1d5831a0b6 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -809,4 +809,8 @@ bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
void cxl_assign_event_header(CXLEventRecordHdr *hdr,
const QemuUUID *uuid, uint32_t flags,
uint8_t length, uint64_t timestamp);
+bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
+ uint64_t dpa, uint64_t len);
+bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
+ uint64_t dpa, uint64_t len);
#endif
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release
2025-03-17 16:31 [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605 anisa.su887
` (7 preceding siblings ...)
2025-03-17 16:31 ` [PATCH 8/9] cxl-mailbox-utils: 0x5604 - FMAPI Initiate DC Add anisa.su887
@ 2025-03-17 16:31 ` anisa.su887
2025-04-24 11:23 ` Jonathan Cameron via
8 siblings, 1 reply; 34+ messages in thread
From: anisa.su887 @ 2025-03-17 16:31 UTC (permalink / raw)
To: qemu-devel; +Cc: jonathan.cameron, nifan.cxl, dave, linux-cxl, Anisa Su
From: Anisa Su <anisa.su@samsung.com>
FM DCD Managment command 0x5605 implemented per CXL r3.2 Spec Section 7.6.7.6.6
Signed-off-by: Anisa Su <anisa.su@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 94 ++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 5bcbd434b7..37810d892f 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -128,6 +128,7 @@ enum {
#define SET_DC_REGION_CONFIG 0x2
#define GET_DC_REGION_EXTENT_LIST 0x3
#define INITIATE_DC_ADD 0x4
+ #define INITIATE_DC_RELEASE 0x5
};
@@ -3722,6 +3723,10 @@ static CXLRetCode cxl_mbox_dc_prescriptive_sanity_check(CXLType3Dev *dcd,
ext.start_dpa, ext.len)) {
return CXL_MBOX_INVALID_EXTENT_LIST;
}
+ } else if (type == DC_EVENT_RELEASE_CAPACITY) {
+ if (!ct3_test_region_block_backed(dcd, ext.start_dpa, ext.len)) {
+ return CXL_MBOX_INVALID_PA;
+ }
}
}
@@ -3835,6 +3840,88 @@ static CXLRetCode cmd_fm_initiate_dc_add(const struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+/*
+ * CXL r3.2 Section 7.6.7.6.6 Initiate Dynamic Capacity Release (Opcode 5605h)
+ */
+static CXLRetCode cmd_fm_initiate_dc_release(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ struct {
+ uint16_t host_id;
+ uint8_t flags;
+ uint8_t reg_num;
+ uint64_t length;
+ uint8_t tag[0x10];
+ uint32_t ext_count;
+ CXLDCExtentRaw extents[];
+ } QEMU_PACKED *in = (void *)payload_in;
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ g_autofree CXLDCExtentRaw *event_rec_exts = NULL;
+ CXLEventDynamicCapacity event_rec = {};
+ CXLDCExtentRaw ext;
+ int i, rc = 0;
+
+ switch (in->flags & 0x7) {
+ case CXL_EXTENT_REMOVAL_POLICY_PRESCRIPTIVE:
+ rc = cxl_mbox_dc_prescriptive_sanity_check(ct3d,
+ in->host_id,
+ in->ext_count,
+ in->extents,
+ DC_EVENT_RELEASE_CAPACITY);
+ if (rc) {
+ return rc;
+ }
+
+ /* Create extents for Event Record */
+ event_rec_exts = g_new0(CXLDCExtentRaw, in->ext_count);
+ for (i = 0; i < in->ext_count; i++) {
+ ext = in->extents[i];
+ event_rec_exts[i].start_dpa = ext.start_dpa;
+ event_rec_exts[i].len = ext.len;
+ memset(event_rec_exts[i].tag, 0, 0x10);
+ event_rec_exts[i].shared_seq = 0;
+ }
+
+ /* Create event record and insert to event log */
+ cxl_mbox_dc_event_create_record_hdr(ct3d, &event_rec.hdr);
+ event_rec.type = DC_EVENT_RELEASE_CAPACITY;
+ /* FIXME: for now, validity flag is cleared */
+ event_rec.validity_flags = 0;
+ /* FIXME: Currently only support 1 host */
+ event_rec.host_id = 0;
+ /* updated_region_id only valid for DC_EVENT_REGION_CONFIG_UPDATED */
+ event_rec.updated_region_id = 0;
+ for (i = 0; i < in->ext_count; i++) {
+ memcpy(&event_rec.dynamic_capacity_extent,
+ &event_rec_exts[i],
+ sizeof(CXLDCExtentRaw));
+
+ event_rec.flags = 0;
+ if (i < in->ext_count - 1) {
+ /* Set "More" flag */
+ event_rec.flags |= BIT(0);
+ }
+
+ if (cxl_event_insert(&ct3d->cxl_dstate,
+ CXL_EVENT_TYPE_DYNAMIC_CAP,
+ (CXLEventRecordRaw *)&event_rec)) {
+ cxl_event_irq_assert(ct3d);
+ }
+ }
+ return CXL_MBOX_SUCCESS;
+ default:
+ qemu_log_mask(LOG_UNIMP,
+ "CXL extent selection policy not supported.\n");
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ return CXL_MBOX_SUCCESS;
+}
+
static const struct cxl_cmd cxl_cmd_set[256][256] = {
[INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
cmd_infostat_bg_op_abort, 0, 0 },
@@ -3977,6 +4064,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
+ [FMAPI_DCD_MGMT][INITIATE_DC_RELEASE] = { "INIT_DC_RELEASE",
+ cmd_fm_initiate_dc_release, ~0,
+ (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
+ CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
+ CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
+ CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
+ CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
};
/*
--
2.47.2
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info
2025-03-17 16:31 ` [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info anisa.su887
@ 2025-03-18 15:56 ` Jonathan Cameron via
2025-03-31 19:38 ` Anisa Su
2025-04-16 21:25 ` Anisa Su
2025-04-24 10:30 ` Jonathan Cameron via
1 sibling, 2 replies; 34+ messages in thread
From: Jonathan Cameron via @ 2025-03-18 15:56 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Mon, 17 Mar 2025 16:31:29 +0000
anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> FM DCD Management command 0x5600 implemented per CXL 3.2 Spec Section 7.6.7.6.1
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 67 ++++++++++++++++++++++++++++++++++++++
> hw/cxl/i2c_mctp_cxl.c | 6 +++-
> 2 files changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 1b62d36101..e9991fd1a7 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -122,6 +122,8 @@ enum {
> #define MANAGEMENT_COMMAND 0x0
> MHD = 0x55,
> #define GET_MHD_INFO 0x0
> + FMAPI_DCD_MGMT = 0x56,
> + #define GET_DCD_INFO 0x0
> };
>
> /* CCI Message Format CXL r3.1 Figure 7-19 */
> @@ -3341,6 +3343,62 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * CXL r3.2 section 7.6.7.6.1: Get DCD Info (Opcode 5600h)
> + */
Single line comment should be fine here.
> +static CXLRetCode cmd_fm_get_dcd_info(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + struct {
> + uint8_t num_hosts;
> + uint8_t num_regions_supported;
> + uint8_t rsvd1[2];
> + uint16_t add_select_policy_bitmask;
> + uint8_t rsvd2[2];
> + uint16_t release_select_policy_bitmask;
> + uint8_t sanitize_on_release_bitmask;
> + uint8_t rsvd3;
> + uint64_t total_dynamic_capacity;
> + uint64_t region_blk_size_bitmasks[8];
> + } QEMU_PACKED *out;
} QEMU_PACKED *out = (void *)payload_out;
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + CXLDCRegion region;
> + int i;
> +
> + if (ct3d->dc.num_regions == 0) {
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + out = (void *)payload_out;
Why not just do this at declaration above?
It is harmless to set it then even if we exit earlier
I think.
> +
> + /* TODO: num hosts set to 1 for now */
Unless this changes later in the set, no need for a todo here.
This simply denotes what we are emulating. Maybe we will make
it more flexible in future, maybe not.
> + out->num_hosts = 1;
> + out->num_regions_supported = ct3d->dc.num_regions;
> + /* TODO: only prescriptive supported for now */
Likewise, not a todo that needs comment. Just a current setting.
As long as we never make it nor support this we are fine for
compatibility etc. The CXL stuff doesn't support migration anyway
so not problems there.
> + stw_le_p(&out->add_select_policy_bitmask,
> + CXL_EXTENT_SELECTION_POLICY_PRESCRIPTIVE);
> + stw_le_p(&out->release_select_policy_bitmask,
> + CXL_EXTENT_REMOVAL_POLICY_PRESCRIPTIVE);
> + /* TODO: sanitize on release bitmask cleared for now */
As with above, not really a todo, more of a choice made for now.
> + out->sanitize_on_release_bitmask = 0;
> +
> + stq_le_p(&out->total_dynamic_capacity,
> + ct3d->dc.total_capacity / CXL_CAPACITY_MULTIPLIER);
> +
> + for (i = 0; i < ct3d->dc.num_regions; i++) {
> + region = ct3d->dc.regions[i];
> + memcpy(&out->region_blk_size_bitmasks[i],
> + ®ion.supported_blk_size_bitmask, 8);
sizeof(out->region_blk_size_bitmasks[i])
> + }
> +
> + *len_out = sizeof(*out);
> + return CXL_MBOX_SUCCESS;
> +}
> +
> static const struct cxl_cmd cxl_cmd_set[256][256] = {
> [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> cmd_infostat_bg_op_abort, 0, 0 },
> @@ -3462,6 +3520,11 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> cmd_tunnel_management_cmd, ~0, 0 },
> };
>
> +static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> + [FMAPI_DCD_MGMT][GET_DCD_INFO] = { "GET_DCD_INFO",
> + cmd_fm_get_dcd_info, 0, 0},
> +};
> +
> /*
> * While the command is executing in the background, the device should
> * update the percentage complete in the Background Command Status Register
> @@ -3764,7 +3827,11 @@ void cxl_initialize_t3_fm_owned_ld_mctpcci(CXLCCI *cci, DeviceState *d,
> DeviceState *intf,
> size_t payload_max)
> {
> + CXLType3Dev *ct3d = CXL_TYPE3(d);
> cxl_copy_cci_commands(cci, cxl_cmd_set_t3_fm_owned_ld_mctp);
> + if (ct3d->dc.num_regions) {
> + cxl_copy_cci_commands(cci, cxl_cmd_set_fm_dcd);
> + }
> cci->d = d;
> cci->intf = intf;
> cxl_init_cci(cci, payload_max);
> diff --git a/hw/cxl/i2c_mctp_cxl.c b/hw/cxl/i2c_mctp_cxl.c
> index 7d2cbc3b75..df95182925 100644
> --- a/hw/cxl/i2c_mctp_cxl.c
> +++ b/hw/cxl/i2c_mctp_cxl.c
> @@ -46,6 +46,9 @@
> /* Implementation choice - may make this configurable */
> #define MCTP_CXL_MAILBOX_BYTES 512
>
> +/* Supported FMAPI Cmds */
> +#define FMAPI_CMD_MAX_OPCODE 0x57
> +
> typedef struct CXLMCTPMessage {
> /*
> * DSP0236 (MCTP Base) Integrity Check + Message Type
> @@ -200,7 +203,8 @@ static void i2c_mctp_cxl_handle_message(MCTPI2CEndpoint *mctp)
> if (!(msg->message_type == MCTP_MT_CXL_TYPE3 &&
> msg->command_set < 0x51) &&
> !(msg->message_type == MCTP_MT_CXL_FMAPI &&
> - msg->command_set >= 0x51 && msg->command_set < 0x56)) {
> + msg->command_set >= 0x51 &&
> + msg->command_set < FMAPI_CMD_MAX_OPCODE)) {
Hmm. There is a visibility problem here we should address but probably not
by introducing a new define. Maybe we should move the enum from
cxl-mailbox-utils.c in a precursor patch.
Jonathan
> buf->rc = CXL_MBOX_UNSUPPORTED;
> st24_le_p(buf->pl_length, len_out);
> s->len = s->pos;
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info
2025-03-18 15:56 ` Jonathan Cameron via
@ 2025-03-31 19:38 ` Anisa Su
2025-04-14 16:52 ` Anisa Su
2025-04-24 10:21 ` Jonathan Cameron via
2025-04-16 21:25 ` Anisa Su
1 sibling, 2 replies; 34+ messages in thread
From: Anisa Su @ 2025-03-31 19:38 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: anisa.su887, qemu-devel, nifan.cxl, dave, linux-cxl
On Tue, Mar 18, 2025 at 03:56:24PM +0000, Jonathan Cameron wrote:
> On Mon, 17 Mar 2025 16:31:29 +0000
> anisa.su887@gmail.com wrote:
>
> > From: Anisa Su <anisa.su@samsung.com>
> >
> > FM DCD Management command 0x5600 implemented per CXL 3.2 Spec Section 7.6.7.6.1
> >
> > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > --- a/hw/cxl/i2c_mctp_cxl.c
> > +++ b/hw/cxl/i2c_mctp_cxl.c
> > @@ -46,6 +46,9 @@
> > /* Implementation choice - may make this configurable */
> > #define MCTP_CXL_MAILBOX_BYTES 512
> >
> > +/* Supported FMAPI Cmds */
> > +#define FMAPI_CMD_MAX_OPCODE 0x57
> > +
> > typedef struct CXLMCTPMessage {
> > /*
> > * DSP0236 (MCTP Base) Integrity Check + Message Type
> > @@ -200,7 +203,8 @@ static void i2c_mctp_cxl_handle_message(MCTPI2CEndpoint *mctp)
> > if (!(msg->message_type == MCTP_MT_CXL_TYPE3 &&
> > msg->command_set < 0x51) &&
> > !(msg->message_type == MCTP_MT_CXL_FMAPI &&
> > - msg->command_set >= 0x51 && msg->command_set < 0x56)) {
> > + msg->command_set >= 0x51 &&
> > + msg->command_set < FMAPI_CMD_MAX_OPCODE)) {
>
> Hmm. There is a visibility problem here we should address but probably not
> by introducing a new define. Maybe we should move the enum from
> cxl-mailbox-utils.c in a precursor patch.
>
> Jonathan
Thanks for the feedback and review Jonathan.
According to the comment above this condition, "Any command forming part
of the CXL FM-API command set... is valid only with the CXL Fabric
Manager API over MCTP binding (DSP0234)."
From my understanding, this check is to ensure that any message
sent from the FM API command set (0x51 - 0x59) has the MCTP_MT_CXL_FMAPI
binding and all other commands (opcode < 0x51) are are sent with the
MCTP_MT_CXL_TYPE3 binding.
Although I see from r3.2 Table 8-230 CXL Defined FM API Command Opcodes
that commands from sets 0x57-0x59 are prohibited from being implemented
in the MCTP CCI, would it be more correct to change the condition for
FMAPI commands to msg->command_set < 0x59? Then if/when commands from sets
0x57-0x59 are implemented, if they are implemented according to the spec, they
should not be added to the FM MCTP CCI.
Please correct my understanding if this is incorrect.
Regarding the visibility problem, I intend to move the enum defining all the
opcodes in cxl-mailbox.utils.c to cxl-mailbox.h and including cxl-mailbox.h
in i2c_mctp_cxl.c
Let me know if that is what you intended.
Other than that, I have removed the extraneous TO-DO's from the other
patches and plan to send out v2 with relevant corrections soon.
Hopefully that makes the remaining patches easier for you to review.
Thanks,
Anisa
>
>
> > buf->rc = CXL_MBOX_UNSUPPORTED;
> > st24_le_p(buf->pl_length, len_out);
> > s->len = s->pos;
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info
2025-03-31 19:38 ` Anisa Su
@ 2025-04-14 16:52 ` Anisa Su
2025-04-24 10:21 ` Jonathan Cameron via
1 sibling, 0 replies; 34+ messages in thread
From: Anisa Su @ 2025-04-14 16:52 UTC (permalink / raw)
To: Anisa Su; +Cc: Jonathan Cameron, qemu-devel, nifan.cxl, dave, linux-cxl
[-- Attachment #1: Type: text/plain, Size: 3215 bytes --]
Hi Jonathan,
Any update on this?
Thanks,
Anisa
On Mon, Mar 31, 2025 at 12:38 PM Anisa Su <anisa.su@samsung.com> wrote:
> On Tue, Mar 18, 2025 at 03:56:24PM +0000, Jonathan Cameron wrote:
> > On Mon, 17 Mar 2025 16:31:29 +0000
> > anisa.su887@gmail.com wrote:
> >
> > > From: Anisa Su <anisa.su@samsung.com>
> > >
> > > FM DCD Management command 0x5600 implemented per CXL 3.2 Spec Section
> 7.6.7.6.1
> > >
> > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > --- a/hw/cxl/i2c_mctp_cxl.c
> > > +++ b/hw/cxl/i2c_mctp_cxl.c
> > > @@ -46,6 +46,9 @@
> > > /* Implementation choice - may make this configurable */
> > > #define MCTP_CXL_MAILBOX_BYTES 512
> > >
> > > +/* Supported FMAPI Cmds */
> > > +#define FMAPI_CMD_MAX_OPCODE 0x57
> > > +
> > > typedef struct CXLMCTPMessage {
> > > /*
> > > * DSP0236 (MCTP Base) Integrity Check + Message Type
> > > @@ -200,7 +203,8 @@ static void
> i2c_mctp_cxl_handle_message(MCTPI2CEndpoint *mctp)
> > > if (!(msg->message_type == MCTP_MT_CXL_TYPE3 &&
> > > msg->command_set < 0x51) &&
> > > !(msg->message_type == MCTP_MT_CXL_FMAPI &&
> > > - msg->command_set >= 0x51 && msg->command_set < 0x56)) {
> > > + msg->command_set >= 0x51 &&
> > > + msg->command_set < FMAPI_CMD_MAX_OPCODE)) {
> >
> > Hmm. There is a visibility problem here we should address but probably
> not
> > by introducing a new define. Maybe we should move the enum from
> > cxl-mailbox-utils.c in a precursor patch.
> >
> > Jonathan
> Thanks for the feedback and review Jonathan.
>
> According to the comment above this condition, "Any command forming part
> of the CXL FM-API command set... is valid only with the CXL Fabric
> Manager API over MCTP binding (DSP0234)."
>
> From my understanding, this check is to ensure that any message
> sent from the FM API command set (0x51 - 0x59) has the MCTP_MT_CXL_FMAPI
> binding and all other commands (opcode < 0x51) are are sent with the
> MCTP_MT_CXL_TYPE3 binding.
>
> Although I see from r3.2 Table 8-230 CXL Defined FM API Command Opcodes
> that commands from sets 0x57-0x59 are prohibited from being implemented
> in the MCTP CCI, would it be more correct to change the condition for
> FMAPI commands to msg->command_set < 0x59? Then if/when commands from sets
> 0x57-0x59 are implemented, if they are implemented according to the spec,
> they
> should not be added to the FM MCTP CCI.
>
> Please correct my understanding if this is incorrect.
>
> Regarding the visibility problem, I intend to move the enum defining all
> the
> opcodes in cxl-mailbox.utils.c to cxl-mailbox.h and including cxl-mailbox.h
> in i2c_mctp_cxl.c
>
> Let me know if that is what you intended.
>
> Other than that, I have removed the extraneous TO-DO's from the other
> patches and plan to send out v2 with relevant corrections soon.
> Hopefully that makes the remaining patches easier for you to review.
>
> Thanks,
> Anisa
>
>
> >
> >
> > > buf->rc = CXL_MBOX_UNSUPPORTED;
> > > st24_le_p(buf->pl_length, len_out);
> > > s->len = s->pos;
> >
>
[-- Attachment #2: Type: text/html, Size: 4288 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info
2025-03-18 15:56 ` Jonathan Cameron via
2025-03-31 19:38 ` Anisa Su
@ 2025-04-16 21:25 ` Anisa Su
1 sibling, 0 replies; 34+ messages in thread
From: Anisa Su @ 2025-04-16 21:25 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: anisa.su887, qemu-devel, nifan.cxl, dave, linux-cxl
On Tue, Mar 18, 2025 at 03:56:24PM +0000, Jonathan Cameron wrote:
> On Mon, 17 Mar 2025 16:31:29 +0000
> anisa.su887@gmail.com wrote:
>
> > From: Anisa Su <anisa.su@samsung.com>
> >
> > FM DCD Management command 0x5600 implemented per CXL 3.2 Spec Section 7.6.7.6.1
> >
> > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 67 ++++++++++++++++++++++++++++++++++++++
> > hw/cxl/i2c_mctp_cxl.c | 6 +++-
> > 2 files changed, 72 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 1b62d36101..e9991fd1a7 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -122,6 +122,8 @@ enum {
> > #define MANAGEMENT_COMMAND 0x0
> > MHD = 0x55,
> > #define GET_MHD_INFO 0x0
> > + FMAPI_DCD_MGMT = 0x56,
> > + #define GET_DCD_INFO 0x0
> > };
> >
> > /* CCI Message Format CXL r3.1 Figure 7-19 */
> > @@ -3341,6 +3343,62 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> > return CXL_MBOX_SUCCESS;
> > }
> >
> > +/*
> > + * CXL r3.2 section 7.6.7.6.1: Get DCD Info (Opcode 5600h)
> > + */
>
> Single line comment should be fine here.
>
> > +static CXLRetCode cmd_fm_get_dcd_info(const struct cxl_cmd *cmd,
> > + uint8_t *payload_in,
> > + size_t len_in,
> > + uint8_t *payload_out,
> > + size_t *len_out,
> > + CXLCCI *cci)
> > +{
> > + struct {
> > + uint8_t num_hosts;
> > + uint8_t num_regions_supported;
> > + uint8_t rsvd1[2];
> > + uint16_t add_select_policy_bitmask;
> > + uint8_t rsvd2[2];
> > + uint16_t release_select_policy_bitmask;
> > + uint8_t sanitize_on_release_bitmask;
> > + uint8_t rsvd3;
> > + uint64_t total_dynamic_capacity;
> > + uint64_t region_blk_size_bitmasks[8];
> > + } QEMU_PACKED *out;
> } QEMU_PACKED *out = (void *)payload_out;
> > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > + CXLDCRegion region;
> > + int i;
> > +
> > + if (ct3d->dc.num_regions == 0) {
> > + return CXL_MBOX_UNSUPPORTED;
> > + }
> > +
> > + out = (void *)payload_out;
> Why not just do this at declaration above?
> It is harmless to set it then even if we exit earlier
> I think.
>
> > +
> > + /* TODO: num hosts set to 1 for now */
>
> Unless this changes later in the set, no need for a todo here.
> This simply denotes what we are emulating. Maybe we will make
> it more flexible in future, maybe not.
>
> > + out->num_hosts = 1;
> > + out->num_regions_supported = ct3d->dc.num_regions;
> > + /* TODO: only prescriptive supported for now */
>
> Likewise, not a todo that needs comment. Just a current setting.
> As long as we never make it nor support this we are fine for
> compatibility etc. The CXL stuff doesn't support migration anyway
> so not problems there.
>
> > + stw_le_p(&out->add_select_policy_bitmask,
> > + CXL_EXTENT_SELECTION_POLICY_PRESCRIPTIVE);
should be
BIT(CXL_EXTENT_SELECTION_POLICY_PRESCRIPTIVE)
> > + stw_le_p(&out->release_select_policy_bitmask,
> > + CXL_EXTENT_REMOVAL_POLICY_PRESCRIPTIVE);
should be
BIT(CXL_EXTENT_REMOVAL_POLICY_PRESCRIPTIVE)
-Anisa
> > + /* TODO: sanitize on release bitmask cleared for now */
>
> As with above, not really a todo, more of a choice made for now.
>
> > + out->sanitize_on_release_bitmask = 0;
> > +
> > + stq_le_p(&out->total_dynamic_capacity,
> > + ct3d->dc.total_capacity / CXL_CAPACITY_MULTIPLIER);
> > +
> > + for (i = 0; i < ct3d->dc.num_regions; i++) {
> > + region = ct3d->dc.regions[i];
> > + memcpy(&out->region_blk_size_bitmasks[i],
> > + ®ion.supported_blk_size_bitmask, 8);
>
> sizeof(out->region_blk_size_bitmasks[i])
>
> > + }
> > +
> > + *len_out = sizeof(*out);
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
> > static const struct cxl_cmd cxl_cmd_set[256][256] = {
> > [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> > cmd_infostat_bg_op_abort, 0, 0 },
> > @@ -3462,6 +3520,11 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> > cmd_tunnel_management_cmd, ~0, 0 },
> > };
> >
> > +static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> > + [FMAPI_DCD_MGMT][GET_DCD_INFO] = { "GET_DCD_INFO",
> > + cmd_fm_get_dcd_info, 0, 0},
> > +};
> > +
> > /*
> > * While the command is executing in the background, the device should
> > * update the percentage complete in the Background Command Status Register
> > @@ -3764,7 +3827,11 @@ void cxl_initialize_t3_fm_owned_ld_mctpcci(CXLCCI *cci, DeviceState *d,
> > DeviceState *intf,
> > size_t payload_max)
> > {
> > + CXLType3Dev *ct3d = CXL_TYPE3(d);
> > cxl_copy_cci_commands(cci, cxl_cmd_set_t3_fm_owned_ld_mctp);
> > + if (ct3d->dc.num_regions) {
> > + cxl_copy_cci_commands(cci, cxl_cmd_set_fm_dcd);
> > + }
> > cci->d = d;
> > cci->intf = intf;
> > cxl_init_cci(cci, payload_max);
> > diff --git a/hw/cxl/i2c_mctp_cxl.c b/hw/cxl/i2c_mctp_cxl.c
> > index 7d2cbc3b75..df95182925 100644
> > --- a/hw/cxl/i2c_mctp_cxl.c
> > +++ b/hw/cxl/i2c_mctp_cxl.c
> > @@ -46,6 +46,9 @@
> > /* Implementation choice - may make this configurable */
> > #define MCTP_CXL_MAILBOX_BYTES 512
> >
> > +/* Supported FMAPI Cmds */
> > +#define FMAPI_CMD_MAX_OPCODE 0x57
> > +
> > typedef struct CXLMCTPMessage {
> > /*
> > * DSP0236 (MCTP Base) Integrity Check + Message Type
> > @@ -200,7 +203,8 @@ static void i2c_mctp_cxl_handle_message(MCTPI2CEndpoint *mctp)
> > if (!(msg->message_type == MCTP_MT_CXL_TYPE3 &&
> > msg->command_set < 0x51) &&
> > !(msg->message_type == MCTP_MT_CXL_FMAPI &&
> > - msg->command_set >= 0x51 && msg->command_set < 0x56)) {
> > + msg->command_set >= 0x51 &&
> > + msg->command_set < FMAPI_CMD_MAX_OPCODE)) {
>
> Hmm. There is a visibility problem here we should address but probably not
> by introducing a new define. Maybe we should move the enum from
> cxl-mailbox-utils.c in a precursor patch.
>
> Jonathan
>
>
> > buf->rc = CXL_MBOX_UNSUPPORTED;
> > st24_le_p(buf->pl_length, len_out);
> > s->len = s->pos;
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 6/9] cxl-mailbox-utils: 0x5602 - FMAPI Set DC Region Config
2025-03-17 16:31 ` [PATCH 6/9] cxl-mailbox-utils: 0x5602 - FMAPI Set DC Region Config anisa.su887
@ 2025-04-16 21:33 ` Anisa Su
2025-04-24 11:05 ` Jonathan Cameron via
1 sibling, 0 replies; 34+ messages in thread
From: Anisa Su @ 2025-04-16 21:33 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, jonathan.cameron, nifan.cxl, dave, linux-cxl
On Mon, Mar 17, 2025 at 04:31:33PM +0000, anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 100 +++++++++++++++++++++++++++++++++++
> hw/mem/cxl_type3.c | 2 +-
> include/hw/cxl/cxl_device.h | 3 ++
> include/hw/cxl/cxl_mailbox.h | 6 +++
> 4 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 4bb51bf3e8..51ead2b152 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -125,6 +125,7 @@ enum {
> FMAPI_DCD_MGMT = 0x56,
> #define GET_DCD_INFO 0x0
> #define GET_HOST_DC_REGION_CONFIG 0x1
> + #define SET_DC_REGION_CONFIG 0x2
> };
>
> /* CCI Message Format CXL r3.1 Figure 7-19 */
> @@ -3494,6 +3495,98 @@ static CXLRetCode cmd_fm_get_host_dc_region_config(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +static void cxl_mbox_dc_event_create_record_hdr(CXLType3Dev *ct3d,
> + CXLEventRecordHdr *hdr)
> +{
> + /*
> + * 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.
> + */
> + uint8_t flags = 1 << CXL_EVENT_TYPE_INFO;
> +
> + st24_le_p(&hdr->flags, flags);
> + hdr->length = sizeof(struct CXLEventDynamicCapacity);
> + memcpy(&hdr->id, &dynamic_capacity_uuid, sizeof(hdr->id));
> + stq_le_p(&hdr->timestamp, cxl_device_get_timestamp(&ct3d->cxl_dstate));
> +}
> +
> +/*
> + * CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602)
> + */
> +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + struct {
> + uint8_t reg_id;
> + uint8_t rsvd[3];
> + uint64_t block_sz;
> + uint8_t flags;
> + uint8_t rsvd2[3];
> + } QEMU_PACKED *in;
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + CXLEventDynamicCapacity dcEvent = {};
> + CXLDCRegion *region;
> +
> + if (ct3d->dc.num_regions == 0) {
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + /*
> + * TODO: Fail with CXL_MBOX_INVALID_SECURITY_STATE if
> + * device has been locked
> + */
> +
> + in = (void *)payload_in;
> + region = &ct3d->dc.regions[in->reg_id];
> +
> + /*
> + * TODO: Fail if sanitize bit doesn't match current config and "the device
> + * does not support reconfiguration of the Sanitize on Release setting."
> + * Currently not reconfigurable, so always fail if sanitize bit
> + * doesn't match.
> + */
> + if ((in->flags & 1) != (region->flags & 1)) {
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + if (in->reg_id >= DCD_MAX_NUM_REGION) {
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + /* Check that no extents are in the region being reconfigured */
> + if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + /* Free bitmap and create new one for new block size. */
> + g_free(region->blk_bitmap);
> + region->blk_bitmap = bitmap_new(region->len / in->block_sz);
> +
forgot to update region blk_size
region->block_size = in->block_sz;
- Anisa
> + /* Create event record and insert into event log */
> + cxl_mbox_dc_event_create_record_hdr(ct3d, &dcEvent.hdr);
> + dcEvent.type = DC_EVENT_REGION_CONFIG_UPDATED;
> + /* FIXME: for now, validity flag is cleared */
> + dcEvent.validity_flags = 0;
> + /* FIXME: Currently only support 1 host */
> + dcEvent.host_id = 0;
> + dcEvent.updated_region_id = in->reg_id;
> +
> + if (cxl_event_insert(&ct3d->cxl_dstate,
> + CXL_EVENT_TYPE_DYNAMIC_CAP,
> + (CXLEventRecordRaw *)&dcEvent)) {
> + cxl_event_irq_assert(ct3d);
> + }
> + return CXL_MBOX_SUCCESS;
> +}
> +
> static const struct cxl_cmd cxl_cmd_set[256][256] = {
> [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> cmd_infostat_bg_op_abort, 0, 0 },
> @@ -3620,6 +3713,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> cmd_fm_get_dcd_info, 0, 0},
> [FMAPI_DCD_MGMT][GET_HOST_DC_REGION_CONFIG] = { "GET_HOST_DC_REGION_CONFIG",
> cmd_fm_get_host_dc_region_config, 4, 0},
> + [FMAPI_DCD_MGMT][SET_DC_REGION_CONFIG] = { "SET_DC_REGION_CONFIG",
> + cmd_fm_set_dc_region_config, 16,
> + (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
> + CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
> + CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> + CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> + CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> };
>
> /*
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index ac74e62875..b742b2bb8d 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1686,7 +1686,7 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
> pcie_aer_inject_error(PCI_DEVICE(obj), &err);
> }
>
> -static void cxl_assign_event_header(CXLEventRecordHdr *hdr,
> +void cxl_assign_event_header(CXLEventRecordHdr *hdr,
> const QemuUUID *uuid, uint32_t flags,
> uint8_t length, uint64_t timestamp)
> {
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 81b826f570..217003a29d 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -806,4 +806,7 @@ void ct3_clear_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> uint64_t len);
> bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> uint64_t len);
> +void cxl_assign_event_header(CXLEventRecordHdr *hdr,
> + const QemuUUID *uuid, uint32_t flags,
> + uint8_t length, uint64_t timestamp);
> #endif
> diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
> index 8e1c7c5f15..820c411cbb 100644
> --- a/include/hw/cxl/cxl_mailbox.h
> +++ b/include/hw/cxl/cxl_mailbox.h
> @@ -8,6 +8,7 @@
> #ifndef CXL_MAILBOX_H
> #define CXL_MAILBOX_H
>
> +#define CXL_MBOX_CONFIG_CHANGE_COLD_RESET (1)
> #define CXL_MBOX_IMMEDIATE_CONFIG_CHANGE (1 << 1)
> #define CXL_MBOX_IMMEDIATE_DATA_CHANGE (1 << 2)
> #define CXL_MBOX_IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -15,6 +16,11 @@
> #define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5)
> #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
> #define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
> +#define CXL_MBOX_SECONDARY_MBOX_SUPPORTED (1 << 8)
> +#define CXL_MBOX_REQUEST_ABORT_BACKGROUND_OP_SUPPORTED (1 << 9)
> +#define CXL_MBOX_CEL_10_TO_11_VALID (1 << 10)
> +#define CXL_MBOX_CONFIG_CHANGE_CONV_RESET (1 << 11)
> +#define CXL_MBOX_CONFIG_CHANGE_CXL_RESET (1 << 12)
>
> #define CXL_LOG_CAP_CLEAR_SUPPORTED (1 << 0)
> #define CXL_LOG_CAP_POPULATE_SUPPORTED (1 << 1)
> --
> 2.47.2
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] cxl/type3: Add supported block sizes bitmask to CXLDCRegion struct
2025-03-17 16:31 ` [PATCH 1/9] cxl/type3: Add supported block sizes bitmask to CXLDCRegion struct anisa.su887
@ 2025-04-24 10:11 ` Jonathan Cameron via
2025-04-29 17:56 ` Anisa Su
0 siblings, 1 reply; 34+ messages in thread
From: Jonathan Cameron via @ 2025-04-24 10:11 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Mon, 17 Mar 2025 16:31:28 +0000
anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> Add supported_blk_size field to CXLDCRegion struct in preparation for
> next patch. It is needed by command 0x5600 Get DC Region Config.
Hi Anisa,
Sorry it took me so long to your series!
Squash this with patch 2. It isn't complex enough addition to justify
as separate patch where we need to explain it is there for that patch
in the cover letter.
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---
> hw/mem/cxl_type3.c | 3 +++
> include/hw/cxl/cxl_device.h | 1 +
> 2 files changed, 4 insertions(+)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 11c38a9292..731497ebda 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -8,6 +8,7 @@
> *
> * SPDX-License-Identifier: GPL-v2-only
> */
> +#include <math.h>
>
> #include "qemu/osdep.h"
> #include "qemu/units.h"
> @@ -766,6 +767,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
> uint64_t region_len;
> uint64_t decode_len;
> uint64_t blk_size = 2 * MiB;
> + uint64_t supported_blk_size_bitmask = BIT((int) log2(blk_size));
Isn't this going in circles? I guess it sort of acts as documentation that it
is a bitmap but then the name is already doing that.
Maybe set it to blk_size and add a comment that for now only a fixed block size
is supported?
I'm a little confused on what this is for given you don't check it in patch 6
which is changing the block size?
> CXLDCRegion *region;
> MemoryRegion *mr;
> uint64_t dc_size;
> @@ -811,6 +813,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
> .block_size = blk_size,
> /* dsmad_handle set when creating CDAT table entries */
> .flags = 0,
> + .supported_blk_size_bitmask = supported_blk_size_bitmask,
> };
> ct3d->dc.total_capacity += region->len;
> region->blk_bitmap = bitmap_new(region->len / region->block_size);
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index ca515cab13..bebed04085 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -608,6 +608,7 @@ typedef struct CXLDCRegion {
> uint32_t dsmadhandle;
> uint8_t flags;
> unsigned long *blk_bitmap;
> + uint64_t supported_blk_size_bitmask;
> } CXLDCRegion;
>
> typedef struct CXLSetFeatureInfo {
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info
2025-03-31 19:38 ` Anisa Su
2025-04-14 16:52 ` Anisa Su
@ 2025-04-24 10:21 ` Jonathan Cameron via
1 sibling, 0 replies; 34+ messages in thread
From: Jonathan Cameron via @ 2025-04-24 10:21 UTC (permalink / raw)
To: Anisa Su; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl
On Mon, 31 Mar 2025 19:38:45 +0000
Anisa Su <anisa.su887@gmail.com> wrote:
> On Tue, Mar 18, 2025 at 03:56:24PM +0000, Jonathan Cameron wrote:
> > On Mon, 17 Mar 2025 16:31:29 +0000
> > anisa.su887@gmail.com wrote:
> >
> > > From: Anisa Su <anisa.su@samsung.com>
> > >
> > > FM DCD Management command 0x5600 implemented per CXL 3.2 Spec Section 7.6.7.6.1
> > >
> > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > --- a/hw/cxl/i2c_mctp_cxl.c
> > > +++ b/hw/cxl/i2c_mctp_cxl.c
> > > @@ -46,6 +46,9 @@
> > > /* Implementation choice - may make this configurable */
> > > #define MCTP_CXL_MAILBOX_BYTES 512
> > >
> > > +/* Supported FMAPI Cmds */
> > > +#define FMAPI_CMD_MAX_OPCODE 0x57
> > > +
> > > typedef struct CXLMCTPMessage {
> > > /*
> > > * DSP0236 (MCTP Base) Integrity Check + Message Type
> > > @@ -200,7 +203,8 @@ static void i2c_mctp_cxl_handle_message(MCTPI2CEndpoint *mctp)
> > > if (!(msg->message_type == MCTP_MT_CXL_TYPE3 &&
> > > msg->command_set < 0x51) &&
> > > !(msg->message_type == MCTP_MT_CXL_FMAPI &&
> > > - msg->command_set >= 0x51 && msg->command_set < 0x56)) {
> > > + msg->command_set >= 0x51 &&
> > > + msg->command_set < FMAPI_CMD_MAX_OPCODE)) {
> >
> > Hmm. There is a visibility problem here we should address but probably not
> > by introducing a new define. Maybe we should move the enum from
> > cxl-mailbox-utils.c in a precursor patch.
> >
> > Jonathan
> Thanks for the feedback and review Jonathan.
>
> According to the comment above this condition, "Any command forming part
> of the CXL FM-API command set... is valid only with the CXL Fabric
> Manager API over MCTP binding (DSP0234)."
>
> From my understanding, this check is to ensure that any message
> sent from the FM API command set (0x51 - 0x59) has the MCTP_MT_CXL_FMAPI
> binding and all other commands (opcode < 0x51) are are sent with the
> MCTP_MT_CXL_TYPE3 binding.
Yes. That is the intent. Why the spec requires this distinction is a long
story we won't go into here...
>
> Although I see from r3.2 Table 8-230 CXL Defined FM API Command Opcodes
> that commands from sets 0x57-0x59 are prohibited from being implemented
> in the MCTP CCI, would it be more correct to change the condition for
> FMAPI commands to msg->command_set < 0x59? Then if/when commands from sets
> 0x57-0x59 are implemented, if they are implemented according to the spec, they
> should not be added to the FM MCTP CCI.
Agreed. For this check, if we are changing it we should update it to
incorporate the additional FM_API commands so < 0x59 to include the
various CXL fabrics things that have been added.
>
> Please correct my understanding if this is incorrect.
>
> Regarding the visibility problem, I intend to move the enum defining all the
> opcodes in cxl-mailbox.utils.c to cxl-mailbox.h and including cxl-mailbox.h
> in i2c_mctp_cxl.c
Ok. I was going to suggest a separate header to avoid info about mailboxes
that wasn't relevant bleeding over into this mctp bridge device but that
header has very little in it so we should be fine. We can reorganizing things
in some future set if that header gains lots of other stuff.
Thanks
Jonathan
>
> Let me know if that is what you intended.
>
> Other than that, I have removed the extraneous TO-DO's from the other
> patches and plan to send out v2 with relevant corrections soon.
> Hopefully that makes the remaining patches easier for you to review.
>
> Thanks,
> Anisa
>
>
> >
> >
> > > buf->rc = CXL_MBOX_UNSUPPORTED;
> > > st24_le_p(buf->pl_length, len_out);
> > > s->len = s->pos;
> >
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info
2025-03-17 16:31 ` [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info anisa.su887
2025-03-18 15:56 ` Jonathan Cameron via
@ 2025-04-24 10:30 ` Jonathan Cameron via
1 sibling, 0 replies; 34+ messages in thread
From: Jonathan Cameron via @ 2025-04-24 10:30 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Mon, 17 Mar 2025 16:31:29 +0000
anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> FM DCD Management command 0x5600 implemented per CXL 3.2 Spec Section 7.6.7.6.1
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
It's been too long so I'll look over this again as a precursor
to looking at the later patches... Might well duplicate things
I said before. :(
> ---
> hw/cxl/cxl-mailbox-utils.c | 67 ++++++++++++++++++++++++++++++++++++++
> hw/cxl/i2c_mctp_cxl.c | 6 +++-
> 2 files changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 1b62d36101..e9991fd1a7 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -122,6 +122,8 @@ enum {
> #define MANAGEMENT_COMMAND 0x0
> MHD = 0x55,
> #define GET_MHD_INFO 0x0
> + FMAPI_DCD_MGMT = 0x56,
> + #define GET_DCD_INFO 0x0
> };
>
> /* CCI Message Format CXL r3.1 Figure 7-19 */
> @@ -3341,6 +3343,62 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * CXL r3.2 section 7.6.7.6.1: Get DCD Info (Opcode 5600h)
> + */
> +static CXLRetCode cmd_fm_get_dcd_info(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + struct {
> + uint8_t num_hosts;
> + uint8_t num_regions_supported;
> + uint8_t rsvd1[2];
> + uint16_t add_select_policy_bitmask;
> + uint8_t rsvd2[2];
> + uint16_t release_select_policy_bitmask;
> + uint8_t sanitize_on_release_bitmask;
> + uint8_t rsvd3;
> + uint64_t total_dynamic_capacity;
> + uint64_t region_blk_size_bitmasks[8];
> + } QEMU_PACKED *out;
You could set this directly to (void *)payload_out; here
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + CXLDCRegion region;
Why not use a pointer? Only copying from the region
I think.
> + int i;
> +
> + if (ct3d->dc.num_regions == 0) {
This check shouldn't be needed as we don't register the commands
if we have no regions.
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + out = (void *)payload_out;
> +
> + /* TODO: num hosts set to 1 for now */
> + out->num_hosts = 1;
> + out->num_regions_supported = ct3d->dc.num_regions;
> + stq_le_p(&out->total_dynamic_capacity,
> + ct3d->dc.total_capacity / CXL_CAPACITY_MULTIPLIER);
> +
> + for (i = 0; i < ct3d->dc.num_regions; i++) {
> + region = ct3d->dc.regions[i];
> + memcpy(&out->region_blk_size_bitmasks[i],
> + ®ion.supported_blk_size_bitmask, 8);
> + }
> +
> + *len_out = sizeof(*out);
> + return CXL_MBOX_SUCCESS;
> +}
> +
> static const struct cxl_cmd cxl_cmd_set[256][256] = {
> [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> cmd_infostat_bg_op_abort, 0, 0 },
> @@ -3462,6 +3520,11 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> cmd_tunnel_management_cmd, ~0, 0 },
> };
>
> +static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> + [FMAPI_DCD_MGMT][GET_DCD_INFO] = { "GET_DCD_INFO",
> + cmd_fm_get_dcd_info, 0, 0},
space before }
> +};
> +
> /*
> * While the command is executing in the background, the device should
> * update the percentage complete in the Background Command Status Register
> @@ -3764,7 +3827,11 @@ void cxl_initialize_t3_fm_owned_ld_mctpcci(CXLCCI *cci, DeviceState *d,
> DeviceState *intf,
> size_t payload_max)
> {
> + CXLType3Dev *ct3d = CXL_TYPE3(d);
Blank line here.
Run checkpatch over the patch series. Might catch little things like this.
> cxl_copy_cci_commands(cci, cxl_cmd_set_t3_fm_owned_ld_mctp);
> + if (ct3d->dc.num_regions) {
length, len_out);
> s->len = s->pos;
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct
2025-03-17 16:31 ` [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct anisa.su887
@ 2025-04-24 10:42 ` Jonathan Cameron via
2025-05-01 20:21 ` Fan Ni
0 siblings, 1 reply; 34+ messages in thread
From: Jonathan Cameron via @ 2025-04-24 10:42 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Mon, 17 Mar 2025 16:31:30 +0000
anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> Add dsmas_flags field to DC Region struct in preparation for next
> command, which returns the dsmas flags in the response.
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---
> hw/mem/cxl_type3.c | 2 ++
> include/hw/cxl/cxl_device.h | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 731497ebda..452a0c101a 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -237,6 +237,8 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> ct3d->dc.regions[i].len,
> false, true, region_base);
> ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1;
> + CDATDsmas *dsmas = (CDATDsmas *) table[cur_ent + CT3_CDAT_DSMAS];
> + ct3d->dc.regions[i].dsmas_flags = dsmas->flags;
This is relying to much on the ordering of creating fields in
ct3_build_cdat_entries_for_mr().
I'd rather you just stored the information flags is built from in CXLDCRegion
and then built the field that is wonderfully called 'Note' in the DC region
configuration in 6.2 spec. I've sent a mail to see if we can clean that
'what is the field called' question for future spec releases.
Whilst the flag definitions cross refer the CDAT spec, the actual locations
of those flags matches, but doesn't cross refer so maybe in the future
we will have other flags in here and locations might not match.
>
> cur_ent += CT3_CDAT_NUM_ENTRIES;
> region_base += ct3d->dc.regions[i].len;
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index bebed04085..81b826f570 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -609,6 +609,7 @@ typedef struct CXLDCRegion {
> uint8_t flags;
> unsigned long *blk_bitmap;
> uint64_t supported_blk_size_bitmask;
> + uint8_t dsmas_flags;
> } CXLDCRegion;
>
> typedef struct CXLSetFeatureInfo {
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/9] cxl-mailbox-utils: 0x5601 - FMAPI Get Host Region Config
2025-03-17 16:31 ` [PATCH 4/9] cxl-mailbox-utils: 0x5601 - FMAPI Get Host Region Config anisa.su887
@ 2025-04-24 10:53 ` Jonathan Cameron via
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Cameron via @ 2025-04-24 10:53 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Mon, 17 Mar 2025 16:31:31 +0000
anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> FM DCD Management command 0x5601 implemented per CXL r3.2 Spec Section 7.6.7.6.2
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
Hi Anisa,
Just a few trivial things in here.
> ---
> hw/cxl/cxl-mailbox-utils.c | 97 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 97 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index e9991fd1a7..4bb51bf3e8 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -124,6 +124,7 @@ enum {
> #define GET_MHD_INFO 0x0
> FMAPI_DCD_MGMT = 0x56,
> #define GET_DCD_INFO 0x0
> + #define GET_HOST_DC_REGION_CONFIG 0x1
> };
>
> /* CCI Message Format CXL r3.1 Figure 7-19 */
> @@ -3399,6 +3400,100 @@ static CXLRetCode cmd_fm_get_dcd_info(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * CXL r3.2 section 7.6.7.6.2: Get Host DC Region Configuration (Opcode 5601h)
> + */
> +static CXLRetCode cmd_fm_get_host_dc_region_config(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + struct {
> + uint16_t host_id;
> + uint8_t region_cnt;
> + uint8_t start_rid;
> + } QEMU_PACKED *in;
= (void *)payload_in;
> + struct {
> + uint16_t host_id;
> + uint8_t num_regions;
> + uint8_t regions_returned;
> + struct {
> + uint64_t base;
> + uint64_t decode_len;
> + uint64_t region_len;
> + uint64_t block_size;
> + uint8_t dsmas_flags;
> + uint8_t rsvd1[3];
> + uint8_t sanitize;
> + uint8_t rsvd2[3];
> + } QEMU_PACKED records[];
> + } QEMU_PACKED *out;
= (void *)payload_out;
> + struct {
> + uint32_t num_extents_supported;
> + uint32_t num_extents_available;
> + uint32_t num_tags_supported;
> + uint32_t num_tags_available;
> + } QEMU_PACKED *extra_out;
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + uint16_t record_count, out_pl_len, i;
> + uint8_t start_rid;
> +
> + if (ct3d->dc.num_regions == 0) {
As in earlier patch - we don't register these commands
at all unless num_regions > 0 so probably don't need this
check.
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + in = (void *)payload_in;
> + if (in->start_rid >= ct3d->dc.num_regions) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + start_rid = in->start_rid;
The saving in characters for having the local variable doesn't seem
worthwhile to me. I'd just in->start_rid where ever you need it.
> + record_count = MIN(ct3d->dc.num_regions - start_rid, in->region_cnt);
> +
> + out = (void *)payload_out;
> + out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> +
> + extra_out = (void *)(out) + out_pl_len;
Do we need the brackets around out?
> + out_pl_len += sizeof(*extra_out);
> +
> + assert(*len_out <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> +
> + /* TODO: For now, fix host id to be 0 */
> + stw_le_p(&out->host_id, 0);
> + out->num_regions = ct3d->dc.num_regions;
> + out->regions_returned = record_count;
> +
> + for (i = 0; i < record_count; i++) {
> + stq_le_p(&out->records[i].base,
> + ct3d->dc.regions[start_rid + i].base);
> + stq_le_p(&out->records[i].decode_len,
> + ct3d->dc.regions[start_rid + i].decode_len /
> + CXL_CAPACITY_MULTIPLIER);
> + stq_le_p(&out->records[i].region_len,
> + ct3d->dc.regions[start_rid + i].len);
> + stq_le_p(&out->records[i].block_size,
> + ct3d->dc.regions[start_rid + i].block_size);
> + out->records[i].dsmas_flags =
> + ct3d->dc.regions[start_rid + i].dsmas_flags;
As in previous patch. I'd build this here from specific booleans
as the relationship to the dmsas flags isn't defined that tightly.
> + out->records[i].sanitize = 0;
> + }
> + /*
> + * TODO: Assign values once extents and tags are introduced
> + * to use.
> + */
> + stl_le_p(&extra_out->num_extents_supported, CXL_NUM_EXTENTS_SUPPORTED);
> + stl_le_p(&extra_out->num_extents_available, CXL_NUM_EXTENTS_SUPPORTED -
> + ct3d->dc.total_extent_count);
> + stl_le_p(&extra_out->num_tags_supported, CXL_NUM_TAGS_SUPPORTED);
> + stl_le_p(&extra_out->num_tags_available, CXL_NUM_TAGS_SUPPORTED);
> +
> + *len_out = out_pl_len;
> +
> + return CXL_MBOX_SUCCESS;
> +}
> +
> static const struct cxl_cmd cxl_cmd_set[256][256] = {
> [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> cmd_infostat_bg_op_abort, 0, 0 },
> @@ -3523,6 +3618,8 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> [FMAPI_DCD_MGMT][GET_DCD_INFO] = { "GET_DCD_INFO",
> cmd_fm_get_dcd_info, 0, 0},
> + [FMAPI_DCD_MGMT][GET_HOST_DC_REGION_CONFIG] = { "GET_HOST_DC_REGION_CONFIG",
> + cmd_fm_get_host_dc_region_config, 4, 0},
Space before }
Check for any other instances of this in the rest of the series.
> };
>
> /*
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 5/9] cxl_events.h: move definition for dynamic_capacity_uuid and enum for DC event types
2025-03-17 16:31 ` [PATCH 5/9] cxl_events.h: move definition for dynamic_capacity_uuid and enum for DC event types anisa.su887
@ 2025-04-24 10:55 ` Jonathan Cameron via
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Cameron via @ 2025-04-24 10:55 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Mon, 17 Mar 2025 16:31:32 +0000
anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> move definition for dynamic_capacity_uuid and enum for DC event types to
Move
> cxl_events.h from cxl_type3.c for shared use in next patch
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---
> hw/mem/cxl_type3.c | 15 ---------------
> include/hw/cxl/cxl_events.h | 15 +++++++++++++++
> 2 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 452a0c101a..ac74e62875 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1977,21 +1977,6 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> }
> }
>
> -/* CXL r3.1 Table 8-50: Dynamic Capacity Event Record */
> -static const QemuUUID dynamic_capacity_uuid = {
> - .data = UUID(0xca95afa7, 0xf183, 0x4018, 0x8c, 0x2f,
> - 0x95, 0x26, 0x8e, 0x10, 0x1a, 0x2a),
> -};
> -
> -typedef enum CXLDCEventType {
> - DC_EVENT_ADD_CAPACITY = 0x0,
> - DC_EVENT_RELEASE_CAPACITY = 0x1,
> - DC_EVENT_FORCED_RELEASE_CAPACITY = 0x2,
> - DC_EVENT_REGION_CONFIG_UPDATED = 0x3,
> - DC_EVENT_ADD_CAPACITY_RSP = 0x4,
> - DC_EVENT_CAPACITY_RELEASED = 0x5,
> -} CXLDCEventType;
> -
> /*
> * Check whether the range [dpa, dpa + len - 1] has overlaps with extents in
> * the list.
> diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
> index 38cadaa0f3..758b075a64 100644
> --- a/include/hw/cxl/cxl_events.h
> +++ b/include/hw/cxl/cxl_events.h
> @@ -184,4 +184,19 @@ typedef struct CXLEventDynamicCapacity {
> uint32_t tags_avail;
> } QEMU_PACKED CXLEventDynamicCapacity;
>
> +/* CXL r3.1 Table 8-50: Dynamic Capacity Event Record */
> +static const QemuUUID dynamic_capacity_uuid = {
> + .data = UUID(0xca95afa7, 0xf183, 0x4018, 0x8c, 0x2f,
> + 0x95, 0x26, 0x8e, 0x10, 0x1a, 0x2a),
> +};
> +
> +typedef enum CXLDCEventType {
> + DC_EVENT_ADD_CAPACITY = 0x0,
> + DC_EVENT_RELEASE_CAPACITY = 0x1,
> + DC_EVENT_FORCED_RELEASE_CAPACITY = 0x2,
> + DC_EVENT_REGION_CONFIG_UPDATED = 0x3,
> + DC_EVENT_ADD_CAPACITY_RSP = 0x4,
> + DC_EVENT_CAPACITY_RELEASED = 0x5,
> +} CXLDCEventType;
> +
> #endif /* CXL_EVENTS_H */
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 6/9] cxl-mailbox-utils: 0x5602 - FMAPI Set DC Region Config
2025-03-17 16:31 ` [PATCH 6/9] cxl-mailbox-utils: 0x5602 - FMAPI Set DC Region Config anisa.su887
2025-04-16 21:33 ` Anisa Su
@ 2025-04-24 11:05 ` Jonathan Cameron via
1 sibling, 0 replies; 34+ messages in thread
From: Jonathan Cameron via @ 2025-04-24 11:05 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Mon, 17 Mar 2025 16:31:33 +0000
anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 100 +++++++++++++++++++++++++++++++++++
> hw/mem/cxl_type3.c | 2 +-
> include/hw/cxl/cxl_device.h | 3 ++
> include/hw/cxl/cxl_mailbox.h | 6 +++
> 4 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 4bb51bf3e8..51ead2b152 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -125,6 +125,7 @@ enum {
> FMAPI_DCD_MGMT = 0x56,
> #define GET_DCD_INFO 0x0
> #define GET_HOST_DC_REGION_CONFIG 0x1
> + #define SET_DC_REGION_CONFIG 0x2
> };
>
> /* CCI Message Format CXL r3.1 Figure 7-19 */
> @@ -3494,6 +3495,98 @@ static CXLRetCode cmd_fm_get_host_dc_region_config(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +static void cxl_mbox_dc_event_create_record_hdr(CXLType3Dev *ct3d,
> + CXLEventRecordHdr *hdr)
> +{
> + /*
> + * 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.
> + */
> + uint8_t flags = 1 << CXL_EVENT_TYPE_INFO;
> +
> + st24_le_p(&hdr->flags, flags);
> + hdr->length = sizeof(struct CXLEventDynamicCapacity);
> + memcpy(&hdr->id, &dynamic_capacity_uuid, sizeof(hdr->id));
> + stq_le_p(&hdr->timestamp, cxl_device_get_timestamp(&ct3d->cxl_dstate));
> +}
> +
> +/*
> + * CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602)
> + */
> +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + struct {
> + uint8_t reg_id;
> + uint8_t rsvd[3];
> + uint64_t block_sz;
> + uint8_t flags;
> + uint8_t rsvd2[3];
> + } QEMU_PACKED *in;
= (void *)payload_in;
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + CXLEventDynamicCapacity dcEvent = {};
> + CXLDCRegion *region;
> +
> + if (ct3d->dc.num_regions == 0) {
As before. I don't think we need to check this.
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + /*
> + * TODO: Fail with CXL_MBOX_INVALID_SECURITY_STATE if
> + * device has been locked
> + */
> +
> + in = (void *)payload_in;
> + region = &ct3d->dc.regions[in->reg_id];
> +
> + /*
> + * TODO: Fail if sanitize bit doesn't match current config and "the device
> + * does not support reconfiguration of the Sanitize on Release setting."
> + * Currently not reconfigurable, so always fail if sanitize bit
> + * doesn't match.
> + */
> + if ((in->flags & 1) != (region->flags & 1)) {
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + if (in->reg_id >= DCD_MAX_NUM_REGION) {
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + /* Check that no extents are in the region being reconfigured */
> + if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + /* Free bitmap and create new one for new block size. */
Maybe need locking if we enable extent creation on a different CCI
to this one. That's only going to happen in fairly complex setups however..
> + g_free(region->blk_bitmap);
> + region->blk_bitmap = bitmap_new(region->len / in->block_sz);
> +
> + /* Create event record and insert into event log */
> + cxl_mbox_dc_event_create_record_hdr(ct3d, &dcEvent.hdr);
> + dcEvent.type = DC_EVENT_REGION_CONFIG_UPDATED;
> + /* FIXME: for now, validity flag is cleared */
Given we are just saying we don't know how many tags are available that
is presumably valid?
> + dcEvent.validity_flags = 0;
> + /* FIXME: Currently only support 1 host */
Not a fixme, a fact of life :)
> + dcEvent.host_id = 0;
> + dcEvent.updated_region_id = in->reg_id;
> +
> + if (cxl_event_insert(&ct3d->cxl_dstate,
> + CXL_EVENT_TYPE_DYNAMIC_CAP,
> + (CXLEventRecordRaw *)&dcEvent)) {
> + cxl_event_irq_assert(ct3d);
> + }
> + return CXL_MBOX_SUCCESS;
> +}
> +
> static const struct cxl_cmd cxl_cmd_set[256][256] = {
> [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> cmd_infostat_bg_op_abort, 0, 0 },
> @@ -3620,6 +3713,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> cmd_fm_get_dcd_info, 0, 0},
> [FMAPI_DCD_MGMT][GET_HOST_DC_REGION_CONFIG] = { "GET_HOST_DC_REGION_CONFIG",
> cmd_fm_get_host_dc_region_config, 4, 0},
> + [FMAPI_DCD_MGMT][SET_DC_REGION_CONFIG] = { "SET_DC_REGION_CONFIG",
> + cmd_fm_set_dc_region_config, 16,
> + (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
> + CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
> + CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> + CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> + CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
Space.
> };
>
> /*
> diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
> index 8e1c7c5f15..820c411cbb 100644
> --- a/include/hw/cxl/cxl_mailbox.h
> +++ b/include/hw/cxl/cxl_mailbox.h
> @@ -8,6 +8,7 @@
> #ifndef CXL_MAILBOX_H
> #define CXL_MAILBOX_H
>
> +#define CXL_MBOX_CONFIG_CHANGE_COLD_RESET (1)
> #define CXL_MBOX_IMMEDIATE_CONFIG_CHANGE (1 << 1)
> #define CXL_MBOX_IMMEDIATE_DATA_CHANGE (1 << 2)
> #define CXL_MBOX_IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -15,6 +16,11 @@
> #define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5)
> #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
> #define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
> +#define CXL_MBOX_SECONDARY_MBOX_SUPPORTED (1 << 8)
> +#define CXL_MBOX_REQUEST_ABORT_BACKGROUND_OP_SUPPORTED (1 << 9)
> +#define CXL_MBOX_CEL_10_TO_11_VALID (1 << 10)
> +#define CXL_MBOX_CONFIG_CHANGE_CONV_RESET (1 << 11)
> +#define CXL_MBOX_CONFIG_CHANGE_CXL_RESET (1 << 12)
Are there any other commands that need updating to include the
more detailed stuff these flags adds around resets?
>
> #define CXL_LOG_CAP_CLEAR_SUPPORTED (1 << 0)
> #define CXL_LOG_CAP_POPULATE_SUPPORTED (1 << 1)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 7/9] cxl-mailbox-utils: 0x5603 - FMAPI Get DC Region Extent Lists
2025-03-17 16:31 ` [PATCH 7/9] cxl-mailbox-utils: 0x5603 - FMAPI Get DC Region Extent Lists anisa.su887
@ 2025-04-24 11:10 ` Jonathan Cameron via
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Cameron via @ 2025-04-24 11:10 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Mon, 17 Mar 2025 16:31:34 +0000
anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> FM DCD Management command 0x5603 implemented per CXL r3.2 Spec Section 7.6.7.6.4
> Very similar to previously implemented command 0x4801.
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 84 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 84 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 51ead2b152..77cf0fdb15 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -126,6 +126,7 @@ enum {
> #define GET_DCD_INFO 0x0
> #define GET_HOST_DC_REGION_CONFIG 0x1
> #define SET_DC_REGION_CONFIG 0x2
> + #define GET_DC_REGION_EXTENT_LIST 0x3
> };
>
> /* CCI Message Format CXL r3.1 Figure 7-19 */
> @@ -3587,6 +3588,87 @@ static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * CXL r3.2 section 7.6.7.6.4 Get DC Region Extent Lists (Opcode 5603h)
Single line comment fine here.
> + */
> +static CXLRetCode cmd_fm_get_dc_region_extent_list(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + struct {
> + uint16_t host_id;
> + uint8_t rsvd[2];
> + uint32_t extent_cnt;
> + uint32_t start_extent_id;
> + } QEMU_PACKED *in = (void *)payload_in;
> + struct {
> + uint16_t host_id;
> + uint8_t rsvd[2];
> + uint32_t start_extent_id;
> + uint32_t extents_returned;
> + uint32_t total_extents;
> + uint32_t list_generation_num;
> + uint8_t rsvd2[4];
> + CXLDCExtentRaw records[];
> + } QEMU_PACKED *out = (void *)payload_out;
> + QEMU_BUILD_BUG_ON(sizeof(*in) != 0xc);
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + CXLDCExtent *ent;
> + CXLDCExtentRaw *out_rec;
> + uint16_t record_count = 0, record_done = 0, i = 0;
> + uint16_t out_pl_len, max_size;
> +
> + if (ct3d->dc.num_regions == 0) {
As in previous.
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + if (in->host_id != 0) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + if (in->start_extent_id > ct3d->dc.total_extent_count) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + record_count = MIN(in->extent_cnt,
> + ct3d->dc.total_extent_count - in->start_extent_id);
> + max_size = CXL_MAILBOX_MAX_PAYLOAD_SIZE - sizeof(*out);
> + record_count = MIN(record_count, max_size / sizeof(out->records[0]));
> + out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> +
> + stw_le_p(&out->host_id, in->host_id);
> + stl_le_p(&out->start_extent_id, in->start_extent_id);
> + stl_le_p(&out->extents_returned, record_count);
> + stl_le_p(&out->total_extents, ct3d->dc.total_extent_count);
> + stl_le_p(&out->list_generation_num, ct3d->dc.ext_list_gen_seq);
> +
> + if (record_count > 0) {
> + out_rec = &out->records[record_done];
Given this isn't in the loop, record_done is always 0, so maybe just
use that here?
Or maybe drag that into the loop and...
> +
> + QTAILQ_FOREACH(ent, &ct3d->dc.extents, node) {
> + if (i++ < in->start_extent_id) {
> + continue;
> + }
> + stq_le_p(&out_rec->start_dpa, ent->start_dpa);
> + stq_le_p(&out_rec->len, ent->len);
> + memcpy(&out_rec->tag, ent->tag, 0x10);
> + stw_le_p(&out_rec->shared_seq, ent->shared_seq);
> +
> + record_done++;
> + out_rec++;
Then no need to increment out_rec here.
> + if (record_done == record_count) {
> + break;
> + }
> + }
> + }
> +
> + *len_out = out_pl_len;
> + return CXL_MBOX_SUCCESS;
> +}
> +
> static const struct cxl_cmd cxl_cmd_set[256][256] = {
> [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> cmd_infostat_bg_op_abort, 0, 0 },
> @@ -3720,6 +3802,8 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> + [FMAPI_DCD_MGMT][GET_DC_REGION_EXTENT_LIST] = { "GET_DC_REGION_EXTENT_LIST",
> + cmd_fm_get_dc_region_extent_list, 12, 0},
space before }
> };
>
> /*
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] cxl-mailbox-utils: 0x5604 - FMAPI Initiate DC Add
2025-03-17 16:31 ` [PATCH 8/9] cxl-mailbox-utils: 0x5604 - FMAPI Initiate DC Add anisa.su887
@ 2025-04-24 11:19 ` Jonathan Cameron via
2025-04-28 20:41 ` Fan Ni
2025-05-05 16:40 ` Anisa Su
0 siblings, 2 replies; 34+ messages in thread
From: Jonathan Cameron via @ 2025-04-24 11:19 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Mon, 17 Mar 2025 16:31:35 +0000
anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> FM DCD Management command 0x5604 implemented per CXL r3.2 Spec Section 7.6.7.6.5
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 173 ++++++++++++++++++++++++++++++++++++
> hw/mem/cxl_type3.c | 8 +-
> include/hw/cxl/cxl_device.h | 4 +
> 3 files changed, 181 insertions(+), 4 deletions(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 77cf0fdb15..5bcbd434b7 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -127,6 +127,8 @@ enum {
> #define GET_HOST_DC_REGION_CONFIG 0x1
> #define SET_DC_REGION_CONFIG 0x2
> #define GET_DC_REGION_EXTENT_LIST 0x3
> + #define INITIATE_DC_ADD 0x4
> +
No blank line here.
> };
>
> /* CCI Message Format CXL r3.1 Figure 7-19 */
> @@ -3669,6 +3671,170 @@ static CXLRetCode cmd_fm_get_dc_region_extent_list(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +static CXLRetCode cxl_mbox_dc_prescriptive_sanity_check(CXLType3Dev *dcd,
> + uint16_t host_id,
> + uint32_t ext_count,
> + CXLDCExtentRaw extents[],
> + CXLDCEventType type)
> +{
> + CXLDCExtentRaw ext;
> + CXLDCRegion *reg = NULL;
> + int i, j;
> +
> + if (dcd->dc.num_regions == 0) {
Can we actually get here like that?
> + qemu_log_mask(LOG_UNIMP,
> + "No dynamic capacity support from the device.\n");
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + /* TODO: host id check will be added later, currently host id must be 0. */
> + if (host_id != 0) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + for (i = 0; i < ext_count; i++) {
> + ext = extents[i];
> +
> + if (ext.len == 0) {
> + return CXL_MBOX_INVALID_EXTENT_LIST;
> + }
> +
> + reg = cxl_find_dc_region(dcd, ext.start_dpa, ext.len);
> + if (!reg) {
> + return CXL_MBOX_INVALID_EXTENT_LIST;
> + }
> +
> + if (ext.len % reg->block_size || ext.start_dpa % reg->block_size) {
> + return CXL_MBOX_INVALID_EXTENT_LIST;
> + }
> +
> + /* Check requested extents do not overlap with each other. */
> + for (j = i + 1; j < ext_count; j++) {
> + if (ranges_overlap(ext.start_dpa, ext.len, extents[j].start_dpa,
> + extents[j].len)) {
> + return CXL_MBOX_INVALID_EXTENT_LIST;
> + }
> + }
> +
> + if (type == DC_EVENT_ADD_CAPACITY) {
> + /* Check requested extents do not overlap with existing extents. */
> + if (cxl_extents_overlaps_dpa_range(&dcd->dc.extents,
> + ext.start_dpa, ext.len)) {
> + return CXL_MBOX_INVALID_EXTENT_LIST;
> + }
> + }
> + }
> +
> + return CXL_MBOX_SUCCESS;
> +}
> +
> +/*
> + * CXL r3.2 Section 7.6.7.6.5 Initiate Dynamic Capacity Add (Opcode 5604h)
> + */
> +static CXLRetCode cmd_fm_initiate_dc_add(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + struct {
> + uint16_t host_id;
> + uint8_t selection_policy;
> + uint8_t reg_num;
> + uint64_t length;
> + uint8_t tag[0x10];
> + uint32_t ext_count;
> + CXLDCExtentRaw extents[];
> + } QEMU_PACKED *in = (void *)payload_in;
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + g_autofree CXLDCExtentRaw *event_rec_exts = NULL;
Move some of this into the scope of the case below
Or maybe factor out that whole thing as a new function?
> + CXLEventDynamicCapacity event_rec = {};
> + CXLDCExtentGroup *group = NULL;
> + CXLDCExtentRaw ext;
> + int rc, i;
> +
> + switch (in->selection_policy) {
> + case CXL_EXTENT_SELECTION_POLICY_PRESCRIPTIVE:
> + /* Adding extents causes exceeding device's extent tracking ability. */
> + if (in->ext_count + ct3d->dc.total_extent_count >
> + CXL_NUM_EXTENTS_SUPPORTED) {
> + return CXL_MBOX_RESOURCES_EXHAUSTED;
> + }
> + rc = cxl_mbox_dc_prescriptive_sanity_check(ct3d,
> + in->host_id,
> + in->ext_count,
> + in->extents,
> + DC_EVENT_ADD_CAPACITY);
> + if (rc) {
> + return rc;
> + }
> +
> + /* Prepare extents for Event Record */
> + event_rec_exts = g_new0(CXLDCExtentRaw, in->ext_count);
> + for (i = 0; i < in->ext_count; i++) {
> + ext = in->extents[i];
> + event_rec_exts[i].start_dpa = ext.start_dpa;
> + event_rec_exts[i].len = ext.len;
> + memset(event_rec_exts[i].tag, 0, 0x10);
> + event_rec_exts[i].shared_seq = 0;
> +
> + /* Check requested extents do not overlap with pending extents. */
> + if (cxl_extent_groups_overlaps_dpa_range(&ct3d->dc.extents_pending,
> + ext.start_dpa, ext.len)) {
> + return CXL_MBOX_INVALID_EXTENT_LIST;
> + }
> +
> + /* Create extent group to add to pending list. */
> + group = cxl_insert_extent_to_extent_group(group,
> + event_rec_exts[i].start_dpa,
> + event_rec_exts[i].len,
> + event_rec_exts[i].tag,
> + event_rec_exts[i].shared_seq);
> + }
> +
> + /* Add requested extents to pending list. */
> + if (group) {
> + cxl_extent_group_list_insert_tail(&ct3d->dc.extents_pending, group);
> + }
> +
> + /* Create event record and insert to event log */
> + cxl_mbox_dc_event_create_record_hdr(ct3d, &event_rec.hdr);
> + event_rec.type = DC_EVENT_ADD_CAPACITY;
> + /* FIXME: for now, validity flag is cleared */
This stuff is probably all valid. If we can return remaining extents though we might
as well.
> + event_rec.validity_flags = 0;
> + /* FIXME: Currently only support 1 host */
> + event_rec.host_id = 0;
> + /* updated_region_id only valid for DC_EVENT_REGION_CONFIG_UPDATED */
> + event_rec.updated_region_id = 0;
The event_rec is zeroed anyway so probably just don't set this at all
and no need for the comment.
> + for (i = 0; i < in->ext_count; i++) {
Why can't we combine this with the earlier loop and avoid the
need for separate storage of extents in event_rec_exts?
> + memcpy(&event_rec.dynamic_capacity_extent,
> + &event_rec_exts[i],
> + sizeof(CXLDCExtentRaw));
> +
> + event_rec.flags = 0;
> + if (i < in->ext_count - 1) {
> + /* Set "More" flag */
> + event_rec.flags |= BIT(0);
> + }
> +
> + if (cxl_event_insert(&ct3d->cxl_dstate,
> + CXL_EVENT_TYPE_DYNAMIC_CAP,
> + (CXLEventRecordRaw *)&event_rec)) {
> + cxl_event_irq_assert(ct3d);
> + }
> + }
> +
> + return CXL_MBOX_SUCCESS;
> + default:
> + qemu_log_mask(LOG_UNIMP,
> + "CXL extent selection policy not supported.\n");
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + return CXL_MBOX_SUCCESS;
> +}
> +
> static const struct cxl_cmd cxl_cmd_set[256][256] = {
> [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> cmd_infostat_bg_op_abort, 0, 0 },
> @@ -3804,6 +3970,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> [FMAPI_DCD_MGMT][GET_DC_REGION_EXTENT_LIST] = { "GET_DC_REGION_EXTENT_LIST",
> cmd_fm_get_dc_region_extent_list, 12, 0},
> + [FMAPI_DCD_MGMT][INITIATE_DC_ADD] = { "INIT_DC_ADD",
> + cmd_fm_initiate_dc_add, ~0,
> + (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
> + CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
> + CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> + CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> + CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> };
>
> /*
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b742b2bb8d..ccc619fe10 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1982,8 +1982,8 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> * the list.
> * Return value: return true if has overlaps; otherwise, return false
> */
> -static bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> - uint64_t dpa, uint64_t len)
> +bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> + uint64_t dpa, uint64_t len)
> {
> CXLDCExtent *ent;
> Range range1, range2;
> @@ -2028,8 +2028,8 @@ bool cxl_extents_contains_dpa_range(CXLDCExtentList *list,
> return false;
> }
>
> -static bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> - uint64_t dpa, uint64_t len)
> +bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> + uint64_t dpa, uint64_t len)
> {
> CXLDCExtentGroup *group;
>
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 217003a29d..1d5831a0b6 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -809,4 +809,8 @@ bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> void cxl_assign_event_header(CXLEventRecordHdr *hdr,
> const QemuUUID *uuid, uint32_t flags,
> uint8_t length, uint64_t timestamp);
> +bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> + uint64_t dpa, uint64_t len);
> +bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> + uint64_t dpa, uint64_t len);
> #endif
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release
2025-03-17 16:31 ` [PATCH 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release anisa.su887
@ 2025-04-24 11:23 ` Jonathan Cameron via
2025-04-28 20:44 ` Fan Ni
0 siblings, 1 reply; 34+ messages in thread
From: Jonathan Cameron via @ 2025-04-24 11:23 UTC (permalink / raw)
To: anisa.su887; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Mon, 17 Mar 2025 16:31:36 +0000
anisa.su887@gmail.com wrote:
> From: Anisa Su <anisa.su@samsung.com>
>
> FM DCD Managment command 0x5605 implemented per CXL r3.2 Spec Section 7.6.7.6.6
>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
Similar comments to patch 8 around the double loop.
I'd also like Fan to take a look through these (v2 probably)
as he's messed with DCD a lot more than me!
Thanks,
Jonathan
> ---
> hw/cxl/cxl-mailbox-utils.c | 94 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 94 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 5bcbd434b7..37810d892f 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -128,6 +128,7 @@ enum {
> #define SET_DC_REGION_CONFIG 0x2
> #define GET_DC_REGION_EXTENT_LIST 0x3
> #define INITIATE_DC_ADD 0x4
> + #define INITIATE_DC_RELEASE 0x5
>
> };
>
> @@ -3722,6 +3723,10 @@ static CXLRetCode cxl_mbox_dc_prescriptive_sanity_check(CXLType3Dev *dcd,
> ext.start_dpa, ext.len)) {
> return CXL_MBOX_INVALID_EXTENT_LIST;
> }
> + } else if (type == DC_EVENT_RELEASE_CAPACITY) {
> + if (!ct3_test_region_block_backed(dcd, ext.start_dpa, ext.len)) {
> + return CXL_MBOX_INVALID_PA;
> + }
> }
> }
>
> @@ -3835,6 +3840,88 @@ static CXLRetCode cmd_fm_initiate_dc_add(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * CXL r3.2 Section 7.6.7.6.6 Initiate Dynamic Capacity Release (Opcode 5605h)
> + */
> +static CXLRetCode cmd_fm_initiate_dc_release(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + struct {
> + uint16_t host_id;
> + uint8_t flags;
> + uint8_t reg_num;
> + uint64_t length;
> + uint8_t tag[0x10];
> + uint32_t ext_count;
> + CXLDCExtentRaw extents[];
> + } QEMU_PACKED *in = (void *)payload_in;
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + g_autofree CXLDCExtentRaw *event_rec_exts = NULL;
> + CXLEventDynamicCapacity event_rec = {};
> + CXLDCExtentRaw ext;
> + int i, rc = 0;
Prefer not to combine cases where you init and ones where you don't.
Just use 2 lines instead.
> +
> + switch (in->flags & 0x7) {
That 7 needs a define.
> + case CXL_EXTENT_REMOVAL_POLICY_PRESCRIPTIVE:
> + rc = cxl_mbox_dc_prescriptive_sanity_check(ct3d,
> + in->host_id,
> + in->ext_count,
> + in->extents,
> + DC_EVENT_RELEASE_CAPACITY);
> + if (rc) {
> + return rc;
> + }
> +
> + /* Create extents for Event Record */
> + event_rec_exts = g_new0(CXLDCExtentRaw, in->ext_count);
> + for (i = 0; i < in->ext_count; i++) {
> + ext = in->extents[i];
> + event_rec_exts[i].start_dpa = ext.start_dpa;
> + event_rec_exts[i].len = ext.len;
> + memset(event_rec_exts[i].tag, 0, 0x10);
> + event_rec_exts[i].shared_seq = 0;
> + }
Similar to before. I'm not currently following the reason for the local
storage.
> +
> + /* Create event record and insert to event log */
> + cxl_mbox_dc_event_create_record_hdr(ct3d, &event_rec.hdr);
> + event_rec.type = DC_EVENT_RELEASE_CAPACITY;
> + /* FIXME: for now, validity flag is cleared */
> + event_rec.validity_flags = 0;
> + /* FIXME: Currently only support 1 host */
> + event_rec.host_id = 0;
> + /* updated_region_id only valid for DC_EVENT_REGION_CONFIG_UPDATED */
> + event_rec.updated_region_id = 0;
> + for (i = 0; i < in->ext_count; i++) {
> + memcpy(&event_rec.dynamic_capacity_extent,
> + &event_rec_exts[i],
> + sizeof(CXLDCExtentRaw));
> +
> + event_rec.flags = 0;
> + if (i < in->ext_count - 1) {
> + /* Set "More" flag */
> + event_rec.flags |= BIT(0);
> + }
> +
> + if (cxl_event_insert(&ct3d->cxl_dstate,
> + CXL_EVENT_TYPE_DYNAMIC_CAP,
> + (CXLEventRecordRaw *)&event_rec)) {
> + cxl_event_irq_assert(ct3d);
> + }
> + }
> + return CXL_MBOX_SUCCESS;
> + default:
> + qemu_log_mask(LOG_UNIMP,
> + "CXL extent selection policy not supported.\n");
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + return CXL_MBOX_SUCCESS;
> +}
> +
> static const struct cxl_cmd cxl_cmd_set[256][256] = {
> [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> cmd_infostat_bg_op_abort, 0, 0 },
> @@ -3977,6 +4064,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> + [FMAPI_DCD_MGMT][INITIATE_DC_RELEASE] = { "INIT_DC_RELEASE",
> + cmd_fm_initiate_dc_release, ~0,
> + (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
> + CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
> + CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> + CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> + CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> };
>
> /*
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] cxl-mailbox-utils: 0x5604 - FMAPI Initiate DC Add
2025-04-24 11:19 ` Jonathan Cameron via
@ 2025-04-28 20:41 ` Fan Ni
2025-05-05 16:40 ` Anisa Su
1 sibling, 0 replies; 34+ messages in thread
From: Fan Ni @ 2025-04-28 20:41 UTC (permalink / raw)
To: Jonathan Cameron
Cc: anisa.su887, qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Thu, Apr 24, 2025 at 12:19:59PM +0100, Jonathan Cameron wrote:
> On Mon, 17 Mar 2025 16:31:35 +0000
> anisa.su887@gmail.com wrote:
>
> > From: Anisa Su <anisa.su@samsung.com>
> >
> > FM DCD Management command 0x5604 implemented per CXL r3.2 Spec Section 7.6.7.6.5
> >
> > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > ---
...
> > + CXL_NUM_EXTENTS_SUPPORTED) {
> > + return CXL_MBOX_RESOURCES_EXHAUSTED;
> > + }
> > + rc = cxl_mbox_dc_prescriptive_sanity_check(ct3d,
> > + in->host_id,
> > + in->ext_count,
> > + in->extents,
> > + DC_EVENT_ADD_CAPACITY);
> > + if (rc) {
> > + return rc;
> > + }
> > +
> > + /* Prepare extents for Event Record */
> > + event_rec_exts = g_new0(CXLDCExtentRaw, in->ext_count);
> > + for (i = 0; i < in->ext_count; i++) {
> > + ext = in->extents[i];
> > + event_rec_exts[i].start_dpa = ext.start_dpa;
> > + event_rec_exts[i].len = ext.len;
> > + memset(event_rec_exts[i].tag, 0, 0x10);
> > + event_rec_exts[i].shared_seq = 0;
> > +
> > + /* Check requested extents do not overlap with pending extents. */
> > + if (cxl_extent_groups_overlaps_dpa_range(&ct3d->dc.extents_pending,
> > + ext.start_dpa, ext.len)) {
> > + return CXL_MBOX_INVALID_EXTENT_LIST;
> > + }
The logic here seems not right. We need to have a separate pass to ensure all
the extents are valid, meaning aligned and no overlap themselves, AND no overlap
with accepted as well as pending extents. There should be no early error exit
like above when inserting into pending extent group.
> > +
> > + /* Create extent group to add to pending list. */
> > + group = cxl_insert_extent_to_extent_group(group,
> > + event_rec_exts[i].start_dpa,
> > + event_rec_exts[i].len,
> > + event_rec_exts[i].tag,
> > + event_rec_exts[i].shared_seq);
> > + }
> > +
> > + /* Add requested extents to pending list. */
> > + if (group) {
> > + cxl_extent_group_list_insert_tail(&ct3d->dc.extents_pending, group);
> > + }
> > +
> > + /* Create event record and insert to event log */
> > + cxl_mbox_dc_event_create_record_hdr(ct3d, &event_rec.hdr);
> > + event_rec.type = DC_EVENT_ADD_CAPACITY;
> > + /* FIXME: for now, validity flag is cleared */
>
> This stuff is probably all valid. If we can return remaining extents though we might
> as well.
>
> > + event_rec.validity_flags = 0;
> > + /* FIXME: Currently only support 1 host */
> > + event_rec.host_id = 0;
> > + /* updated_region_id only valid for DC_EVENT_REGION_CONFIG_UPDATED */
> > + event_rec.updated_region_id = 0;
>
> The event_rec is zeroed anyway so probably just don't set this at all
> and no need for the comment.
>
> > + for (i = 0; i < in->ext_count; i++) {
>
> Why can't we combine this with the earlier loop and avoid the
> need for separate storage of extents in event_rec_exts?
As long as we make sure all the extents are valid, we should be able to do it
in one pass.
Fan
>
> > + memcpy(&event_rec.dynamic_capacity_extent,
> > + &event_rec_exts[i],
> > + sizeof(CXLDCExtentRaw));
> > +
> > + event_rec.flags = 0;
> > + if (i < in->ext_count - 1) {
> > + /* Set "More" flag */
> > + event_rec.flags |= BIT(0);
> > + }
> > +
> > + if (cxl_event_insert(&ct3d->cxl_dstate,
> > + CXL_EVENT_TYPE_DYNAMIC_CAP,
> > + (CXLEventRecordRaw *)&event_rec)) {
> > + cxl_event_irq_assert(ct3d);
> > + }
> > + }
> > +
> > + return CXL_MBOX_SUCCESS;
> > + default:
> > + qemu_log_mask(LOG_UNIMP,
> > + "CXL extent selection policy not supported.\n");
> > + return CXL_MBOX_INVALID_INPUT;
> > + }
> > +
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
> > static const struct cxl_cmd cxl_cmd_set[256][256] = {
> > [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> > cmd_infostat_bg_op_abort, 0, 0 },
> > @@ -3804,6 +3970,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> > CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> > [FMAPI_DCD_MGMT][GET_DC_REGION_EXTENT_LIST] = { "GET_DC_REGION_EXTENT_LIST",
> > cmd_fm_get_dc_region_extent_list, 12, 0},
> > + [FMAPI_DCD_MGMT][INITIATE_DC_ADD] = { "INIT_DC_ADD",
> > + cmd_fm_initiate_dc_add, ~0,
> > + (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
> > + CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
> > + CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> > + CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> > + CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> > };
> >
> > /*
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index b742b2bb8d..ccc619fe10 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -1982,8 +1982,8 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> > * the list.
> > * Return value: return true if has overlaps; otherwise, return false
> > */
> > -static bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> > - uint64_t dpa, uint64_t len)
> > +bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> > + uint64_t dpa, uint64_t len)
> > {
> > CXLDCExtent *ent;
> > Range range1, range2;
> > @@ -2028,8 +2028,8 @@ bool cxl_extents_contains_dpa_range(CXLDCExtentList *list,
> > return false;
> > }
> >
> > -static bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> > - uint64_t dpa, uint64_t len)
> > +bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> > + uint64_t dpa, uint64_t len)
> > {
> > CXLDCExtentGroup *group;
> >
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 217003a29d..1d5831a0b6 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -809,4 +809,8 @@ bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> > void cxl_assign_event_header(CXLEventRecordHdr *hdr,
> > const QemuUUID *uuid, uint32_t flags,
> > uint8_t length, uint64_t timestamp);
> > +bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> > + uint64_t dpa, uint64_t len);
> > +bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> > + uint64_t dpa, uint64_t len);
> > #endif
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release
2025-04-24 11:23 ` Jonathan Cameron via
@ 2025-04-28 20:44 ` Fan Ni
0 siblings, 0 replies; 34+ messages in thread
From: Fan Ni @ 2025-04-28 20:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: anisa.su887, qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Thu, Apr 24, 2025 at 12:23:08PM +0100, Jonathan Cameron wrote:
> On Mon, 17 Mar 2025 16:31:36 +0000
> anisa.su887@gmail.com wrote:
>
> > From: Anisa Su <anisa.su@samsung.com>
> >
> > FM DCD Managment command 0x5605 implemented per CXL r3.2 Spec Section 7.6.7.6.6
> >
> > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> Similar comments to patch 8 around the double loop.
>
> I'd also like Fan to take a look through these (v2 probably)
> as he's messed with DCD a lot more than me!
It is true we can avoid the double loop here.
Since the input payload is already CXLDCExtentRaw.
Previous in QMP implementation, we need it as the input is different, we did
not have start_dpa directly, but an offset to the start of a region.
Fan
>
> Thanks,
>
> Jonathan
>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 94 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 94 insertions(+)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 5bcbd434b7..37810d892f 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -128,6 +128,7 @@ enum {
> > #define SET_DC_REGION_CONFIG 0x2
> > #define GET_DC_REGION_EXTENT_LIST 0x3
> > #define INITIATE_DC_ADD 0x4
> > + #define INITIATE_DC_RELEASE 0x5
> >
> > };
> >
> > @@ -3722,6 +3723,10 @@ static CXLRetCode cxl_mbox_dc_prescriptive_sanity_check(CXLType3Dev *dcd,
> > ext.start_dpa, ext.len)) {
> > return CXL_MBOX_INVALID_EXTENT_LIST;
> > }
> > + } else if (type == DC_EVENT_RELEASE_CAPACITY) {
> > + if (!ct3_test_region_block_backed(dcd, ext.start_dpa, ext.len)) {
> > + return CXL_MBOX_INVALID_PA;
> > + }
> > }
> > }
> >
> > @@ -3835,6 +3840,88 @@ static CXLRetCode cmd_fm_initiate_dc_add(const struct cxl_cmd *cmd,
> > return CXL_MBOX_SUCCESS;
> > }
> >
> > +/*
> > + * CXL r3.2 Section 7.6.7.6.6 Initiate Dynamic Capacity Release (Opcode 5605h)
> > + */
> > +static CXLRetCode cmd_fm_initiate_dc_release(const struct cxl_cmd *cmd,
> > + uint8_t *payload_in,
> > + size_t len_in,
> > + uint8_t *payload_out,
> > + size_t *len_out,
> > + CXLCCI *cci)
> > +{
> > + struct {
> > + uint16_t host_id;
> > + uint8_t flags;
> > + uint8_t reg_num;
> > + uint64_t length;
> > + uint8_t tag[0x10];
> > + uint32_t ext_count;
> > + CXLDCExtentRaw extents[];
> > + } QEMU_PACKED *in = (void *)payload_in;
> > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > + g_autofree CXLDCExtentRaw *event_rec_exts = NULL;
> > + CXLEventDynamicCapacity event_rec = {};
> > + CXLDCExtentRaw ext;
> > + int i, rc = 0;
>
> Prefer not to combine cases where you init and ones where you don't.
> Just use 2 lines instead.
>
> > +
> > + switch (in->flags & 0x7) {
>
> That 7 needs a define.
>
> > + case CXL_EXTENT_REMOVAL_POLICY_PRESCRIPTIVE:
> > + rc = cxl_mbox_dc_prescriptive_sanity_check(ct3d,
> > + in->host_id,
> > + in->ext_count,
> > + in->extents,
> > + DC_EVENT_RELEASE_CAPACITY);
> > + if (rc) {
> > + return rc;
> > + }
> > +
> > + /* Create extents for Event Record */
> > + event_rec_exts = g_new0(CXLDCExtentRaw, in->ext_count);
> > + for (i = 0; i < in->ext_count; i++) {
> > + ext = in->extents[i];
> > + event_rec_exts[i].start_dpa = ext.start_dpa;
> > + event_rec_exts[i].len = ext.len;
> > + memset(event_rec_exts[i].tag, 0, 0x10);
> > + event_rec_exts[i].shared_seq = 0;
> > + }
>
> Similar to before. I'm not currently following the reason for the local
> storage.
>
> > +
> > + /* Create event record and insert to event log */
> > + cxl_mbox_dc_event_create_record_hdr(ct3d, &event_rec.hdr);
> > + event_rec.type = DC_EVENT_RELEASE_CAPACITY;
> > + /* FIXME: for now, validity flag is cleared */
> > + event_rec.validity_flags = 0;
> > + /* FIXME: Currently only support 1 host */
> > + event_rec.host_id = 0;
> > + /* updated_region_id only valid for DC_EVENT_REGION_CONFIG_UPDATED */
> > + event_rec.updated_region_id = 0;
> > + for (i = 0; i < in->ext_count; i++) {
> > + memcpy(&event_rec.dynamic_capacity_extent,
> > + &event_rec_exts[i],
> > + sizeof(CXLDCExtentRaw));
> > +
> > + event_rec.flags = 0;
> > + if (i < in->ext_count - 1) {
> > + /* Set "More" flag */
> > + event_rec.flags |= BIT(0);
> > + }
> > +
> > + if (cxl_event_insert(&ct3d->cxl_dstate,
> > + CXL_EVENT_TYPE_DYNAMIC_CAP,
> > + (CXLEventRecordRaw *)&event_rec)) {
> > + cxl_event_irq_assert(ct3d);
> > + }
> > + }
> > + return CXL_MBOX_SUCCESS;
> > + default:
> > + qemu_log_mask(LOG_UNIMP,
> > + "CXL extent selection policy not supported.\n");
> > + return CXL_MBOX_INVALID_INPUT;
> > + }
> > +
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
> > static const struct cxl_cmd cxl_cmd_set[256][256] = {
> > [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> > cmd_infostat_bg_op_abort, 0, 0 },
> > @@ -3977,6 +4064,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> > CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> > CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> > CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> > + [FMAPI_DCD_MGMT][INITIATE_DC_RELEASE] = { "INIT_DC_RELEASE",
> > + cmd_fm_initiate_dc_release, ~0,
> > + (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
> > + CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
> > + CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> > + CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> > + CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> > };
> >
> > /*
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/9] cxl/type3: Add supported block sizes bitmask to CXLDCRegion struct
2025-04-24 10:11 ` Jonathan Cameron via
@ 2025-04-29 17:56 ` Anisa Su
0 siblings, 0 replies; 34+ messages in thread
From: Anisa Su @ 2025-04-29 17:56 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: anisa.su887, qemu-devel, nifan.cxl, dave, linux-cxl
On Thu, Apr 24, 2025 at 11:11:31AM +0100, Jonathan Cameron wrote:
> On Mon, 17 Mar 2025 16:31:28 +0000
> anisa.su887@gmail.com wrote:
>
> > From: Anisa Su <anisa.su@samsung.com>
> >
> > Add supported_blk_size field to CXLDCRegion struct in preparation for
> > next patch. It is needed by command 0x5600 Get DC Region Config.
...
> > @@ -8,6 +8,7 @@
> > *
> > * SPDX-License-Identifier: GPL-v2-only
> > */
> > +#include <math.h>
> >
> > #include "qemu/osdep.h"
> > #include "qemu/units.h"
> > @@ -766,6 +767,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
> > uint64_t region_len;
> > uint64_t decode_len;
> > uint64_t blk_size = 2 * MiB;
> > + uint64_t supported_blk_size_bitmask = BIT((int) log2(blk_size));
>
> Isn't this going in circles? I guess it sort of acts as documentation that it
> is a bitmap but then the name is already doing that.
> Maybe set it to blk_size and add a comment that for now only a fixed block size
> is supported?
>
> I'm a little confused on what this is for given you don't check it in patch 6
> which is changing the block size?
Good catch! It doesn't seem to specify this check in Section 7.6.7.6.3
Set DC Region Configuration (Opcode 5602h) in the 3.2 spec, but it would
make sense to me to fail with the same rc as when the region has not
been cleared, which is CXL_MBOX_UNSUPPORTED.
Will have the fix in v2.
> > CXLDCRegion *region;
> > MemoryRegion *mr;
> > uint64_t dc_size;
> > @@ -811,6 +813,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
> > .block_size = blk_size,
> > /* dsmad_handle set when creating CDAT table entries */
> > .flags = 0,
> > + .supported_blk_size_bitmask = supported_blk_size_bitmask,
> > };
> > ct3d->dc.total_capacity += region->len;
> > region->blk_bitmap = bitmap_new(region->len / region->block_size);
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index ca515cab13..bebed04085 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -608,6 +608,7 @@ typedef struct CXLDCRegion {
> > uint32_t dsmadhandle;
> > uint8_t flags;
> > unsigned long *blk_bitmap;
> > + uint64_t supported_blk_size_bitmask;
> > } CXLDCRegion;
> >
> > typedef struct CXLSetFeatureInfo {
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct
2025-04-24 10:42 ` Jonathan Cameron via
@ 2025-05-01 20:21 ` Fan Ni
2025-05-02 9:01 ` Jonathan Cameron via
0 siblings, 1 reply; 34+ messages in thread
From: Fan Ni @ 2025-05-01 20:21 UTC (permalink / raw)
To: Jonathan Cameron
Cc: anisa.su887, qemu-devel, nifan.cxl, dave, linux-cxl, Anisa Su
On Thu, Apr 24, 2025 at 11:42:59AM +0100, Jonathan Cameron wrote:
> On Mon, 17 Mar 2025 16:31:30 +0000
> anisa.su887@gmail.com wrote:
>
> > From: Anisa Su <anisa.su@samsung.com>
> >
> > Add dsmas_flags field to DC Region struct in preparation for next
> > command, which returns the dsmas flags in the response.
> >
> > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > ---
> > hw/mem/cxl_type3.c | 2 ++
> > include/hw/cxl/cxl_device.h | 1 +
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 731497ebda..452a0c101a 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -237,6 +237,8 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> > ct3d->dc.regions[i].len,
> > false, true, region_base);
> > ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1;
> > + CDATDsmas *dsmas = (CDATDsmas *) table[cur_ent + CT3_CDAT_DSMAS];
> > + ct3d->dc.regions[i].dsmas_flags = dsmas->flags;
>
Hi Jonathan,
Thanks for the feedback.
> This is relying to much on the ordering of creating fields in
> ct3_build_cdat_entries_for_mr().
I am not sure whether I understand this clearly.
In current qemu implemtation, each mr (ram,pmem or dc region) will have the
whole set of cdat table entries (dsmas, dslbis0-3, etc), so as long as we point
to the right table entry, we can get the table correctly.
What do you mean "the ordering of creating fields"?
>
> I'd rather you just stored the information flags is built from in CXLDCRegion
> and then built the field that is wonderfully called 'Note' in the DC region
This sentence is kind of broken for me, not totally clear what you are
suggesting :-(. Can you explain more?
Are you suggesting not directly take dsmas->flags as dsmas_flags, but
use bit op to generate the value used in Table 7-66 in cxl spec 3.2?
> configuration in 6.2 spec. I've sent a mail to see if we can clean that
6.2 spec???
> 'what is the field called' question for future spec releases.
>
> Whilst the flag definitions cross refer the CDAT spec, the actual locations
> of those flags matches, but doesn't cross refer so maybe in the future
> we will have other flags in here and locations might not match.
For the flags stored in dsmas table, do we expect there can be more than those
defined in Table 7-66 in spec 3.2?
Fan
>
> >
> > cur_ent += CT3_CDAT_NUM_ENTRIES;
> > region_base += ct3d->dc.regions[i].len;
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index bebed04085..81b826f570 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -609,6 +609,7 @@ typedef struct CXLDCRegion {
> > uint8_t flags;
> > unsigned long *blk_bitmap;
> > uint64_t supported_blk_size_bitmask;
> > + uint8_t dsmas_flags;
> > } CXLDCRegion;
> >
> > typedef struct CXLSetFeatureInfo {
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct
2025-05-01 20:21 ` Fan Ni
@ 2025-05-02 9:01 ` Jonathan Cameron via
2025-05-02 15:57 ` Fan Ni
0 siblings, 1 reply; 34+ messages in thread
From: Jonathan Cameron via @ 2025-05-02 9:01 UTC (permalink / raw)
To: Fan Ni; +Cc: anisa.su887, qemu-devel, dave, linux-cxl, Anisa Su
On Thu, 1 May 2025 20:21:56 +0000
Fan Ni <nifan.cxl@gmail.com> wrote:
> On Thu, Apr 24, 2025 at 11:42:59AM +0100, Jonathan Cameron wrote:
> > On Mon, 17 Mar 2025 16:31:30 +0000
> > anisa.su887@gmail.com wrote:
> >
> > > From: Anisa Su <anisa.su@samsung.com>
> > >
> > > Add dsmas_flags field to DC Region struct in preparation for next
> > > command, which returns the dsmas flags in the response.
> > >
> > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > ---
> > > hw/mem/cxl_type3.c | 2 ++
> > > include/hw/cxl/cxl_device.h | 1 +
> > > 2 files changed, 3 insertions(+)
> > >
> > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > index 731497ebda..452a0c101a 100644
> > > --- a/hw/mem/cxl_type3.c
> > > +++ b/hw/mem/cxl_type3.c
> > > @@ -237,6 +237,8 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> > > ct3d->dc.regions[i].len,
> > > false, true, region_base);
> > > ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1;
> > > + CDATDsmas *dsmas = (CDATDsmas *) table[cur_ent + CT3_CDAT_DSMAS];
> > > + ct3d->dc.regions[i].dsmas_flags = dsmas->flags;
> >
> Hi Jonathan,
> Thanks for the feedback.
> > This is relying to much on the ordering of creating fields in
> > ct3_build_cdat_entries_for_mr().
> I am not sure whether I understand this clearly.
> In current qemu implemtation, each mr (ram,pmem or dc region) will have the
> whole set of cdat table entries (dsmas, dslbis0-3, etc), so as long as we point
> to the right table entry, we can get the table correctly.
> What do you mean "the ordering of creating fields"?
It is an implementation detail only that the first bit of that table is
the DSMAS entry. I think we shouldn't rely on that.
> >
> > I'd rather you just stored the information flags is built from in CXLDCRegion
> > and then built the field that is wonderfully called 'Note' in the DC region
I got distracted by the spec oddity :)
> This sentence is kind of broken for me, not totally clear what you are
> suggesting :-(. Can you explain more?
> Are you suggesting not directly take dsmas->flags as dsmas_flags, but
> use bit op to generate the value used in Table 7-66 in cxl spec 3.2?
No. Just store the various bools etc that become dsmas->flags in the
CXLDCRegion structure directly rather than reading back from dsmas->flags.
Probably as explicit bools etc not a single value.
Then pass those in to ct3_build_cdat_entries_for_mr() . Mostly they overlap
with current true / false parameters that are hard coded.
> > configuration in 6.2 spec. I've sent a mail to see if we can clean that
> 6.2 spec???
> > 'what is the field called' question for future spec releases.
> >
> > Whilst the flag definitions cross refer the CDAT spec, the actual locations
> > of those flags matches, but doesn't cross refer so maybe in the future
> > we will have other flags in here and locations might not match.
> For the flags stored in dsmas table, do we expect there can be more than those
> defined in Table 7-66 in spec 3.2?
Not for now. Though I'm sure something will come along at some point.
The comment is about there being particular reason the flag locations should match
between CDAT and what we report via the commands being added here. The definitions
of individual bits cross refer between specs, the register as a whole does not.
Jonathan
>
> Fan
>
> >
> > >
> > > cur_ent += CT3_CDAT_NUM_ENTRIES;
> > > region_base += ct3d->dc.regions[i].len;
> > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > > index bebed04085..81b826f570 100644
> > > --- a/include/hw/cxl/cxl_device.h
> > > +++ b/include/hw/cxl/cxl_device.h
> > > @@ -609,6 +609,7 @@ typedef struct CXLDCRegion {
> > > uint8_t flags;
> > > unsigned long *blk_bitmap;
> > > uint64_t supported_blk_size_bitmask;
> > > + uint8_t dsmas_flags;
> > > } CXLDCRegion;
> > >
> > > typedef struct CXLSetFeatureInfo {
> >
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct
2025-05-02 9:01 ` Jonathan Cameron via
@ 2025-05-02 15:57 ` Fan Ni
2025-05-06 16:53 ` Jonathan Cameron via
0 siblings, 1 reply; 34+ messages in thread
From: Fan Ni @ 2025-05-02 15:57 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Fan Ni, anisa.su887, qemu-devel, dave, linux-cxl, Anisa Su
On Fri, May 02, 2025 at 10:01:55AM +0100, Jonathan Cameron wrote:
> On Thu, 1 May 2025 20:21:56 +0000
> Fan Ni <nifan.cxl@gmail.com> wrote:
>
> > On Thu, Apr 24, 2025 at 11:42:59AM +0100, Jonathan Cameron wrote:
> > > On Mon, 17 Mar 2025 16:31:30 +0000
> > > anisa.su887@gmail.com wrote:
> > >
> > > > From: Anisa Su <anisa.su@samsung.com>
> > > >
> > > > Add dsmas_flags field to DC Region struct in preparation for next
> > > > command, which returns the dsmas flags in the response.
> > > >
> > > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > > ---
> > > > hw/mem/cxl_type3.c | 2 ++
> > > > include/hw/cxl/cxl_device.h | 1 +
> > > > 2 files changed, 3 insertions(+)
> > > >
> > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > > index 731497ebda..452a0c101a 100644
> > > > --- a/hw/mem/cxl_type3.c
> > > > +++ b/hw/mem/cxl_type3.c
> > > > @@ -237,6 +237,8 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> > > > ct3d->dc.regions[i].len,
> > > > false, true, region_base);
> > > > ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1;
> > > > + CDATDsmas *dsmas = (CDATDsmas *) table[cur_ent + CT3_CDAT_DSMAS];
> > > > + ct3d->dc.regions[i].dsmas_flags = dsmas->flags;
> > >
> > Hi Jonathan,
> > Thanks for the feedback.
> > > This is relying to much on the ordering of creating fields in
> > > ct3_build_cdat_entries_for_mr().
> > I am not sure whether I understand this clearly.
> > In current qemu implemtation, each mr (ram,pmem or dc region) will have the
> > whole set of cdat table entries (dsmas, dslbis0-3, etc), so as long as we point
> > to the right table entry, we can get the table correctly.
> > What do you mean "the ordering of creating fields"?
>
> It is an implementation detail only that the first bit of that table is
> the DSMAS entry. I think we shouldn't rely on that.
>
> > >
> > > I'd rather you just stored the information flags is built from in CXLDCRegion
> > > and then built the field that is wonderfully called 'Note' in the DC region
> I got distracted by the spec oddity :)
>
> > This sentence is kind of broken for me, not totally clear what you are
> > suggesting :-(. Can you explain more?
> > Are you suggesting not directly take dsmas->flags as dsmas_flags, but
> > use bit op to generate the value used in Table 7-66 in cxl spec 3.2?
>
> No. Just store the various bools etc that become dsmas->flags in the
> CXLDCRegion structure directly rather than reading back from dsmas->flags.
> Probably as explicit bools etc not a single value.
>
> Then pass those in to ct3_build_cdat_entries_for_mr() . Mostly they overlap
> with current true / false parameters that are hard coded.
OK. Since some flags are not support yet, can we hard coded them for now?
Fan
>
>
> > > configuration in 6.2 spec. I've sent a mail to see if we can clean that
> > 6.2 spec???
> > > 'what is the field called' question for future spec releases.
> > >
> > > Whilst the flag definitions cross refer the CDAT spec, the actual locations
> > > of those flags matches, but doesn't cross refer so maybe in the future
> > > we will have other flags in here and locations might not match.
> > For the flags stored in dsmas table, do we expect there can be more than those
> > defined in Table 7-66 in spec 3.2?
>
> Not for now. Though I'm sure something will come along at some point.
> The comment is about there being particular reason the flag locations should match
> between CDAT and what we report via the commands being added here. The definitions
> of individual bits cross refer between specs, the register as a whole does not.
>
> Jonathan
>
> >
> > Fan
> >
> > >
> > > >
> > > > cur_ent += CT3_CDAT_NUM_ENTRIES;
> > > > region_base += ct3d->dc.regions[i].len;
> > > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > > > index bebed04085..81b826f570 100644
> > > > --- a/include/hw/cxl/cxl_device.h
> > > > +++ b/include/hw/cxl/cxl_device.h
> > > > @@ -609,6 +609,7 @@ typedef struct CXLDCRegion {
> > > > uint8_t flags;
> > > > unsigned long *blk_bitmap;
> > > > uint64_t supported_blk_size_bitmask;
> > > > + uint8_t dsmas_flags;
> > > > } CXLDCRegion;
> > > >
> > > > typedef struct CXLSetFeatureInfo {
> > >
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] cxl-mailbox-utils: 0x5604 - FMAPI Initiate DC Add
2025-04-24 11:19 ` Jonathan Cameron via
2025-04-28 20:41 ` Fan Ni
@ 2025-05-05 16:40 ` Anisa Su
2025-05-06 16:55 ` Jonathan Cameron via
1 sibling, 1 reply; 34+ messages in thread
From: Anisa Su @ 2025-05-05 16:40 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: anisa.su887, qemu-devel, nifan.cxl, dave, linux-cxl
On Thu, Apr 24, 2025 at 12:19:59PM +0100, Jonathan Cameron wrote:
> On Mon, 17 Mar 2025 16:31:35 +0000
> anisa.su887@gmail.com wrote:
>
> > From: Anisa Su <anisa.su@samsung.com>
> >
> > FM DCD Management command 0x5604 implemented per CXL r3.2 Spec Section 7.6.7.6.5
> >
> > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > ---
...
> > + /* Create event record and insert to event log */
> > + cxl_mbox_dc_event_create_record_hdr(ct3d, &event_rec.hdr);
> > + event_rec.type = DC_EVENT_ADD_CAPACITY;
> > + /* FIXME: for now, validity flag is cleared */
>
> This stuff is probably all valid. If we can return remaining extents though we might
> as well.
>
> > + event_rec.validity_flags = 0;
> > + /* FIXME: Currently only support 1 host */
> > + event_rec.host_id = 0;
> > + /* updated_region_id only valid for DC_EVENT_REGION_CONFIG_UPDATED */
> > + event_rec.updated_region_id = 0;
>
> The event_rec is zeroed anyway so probably just don't set this at all
> and no need for the comment.
>
> > + for (i = 0; i < in->ext_count; i++) {
>
> Why can't we combine this with the earlier loop and avoid the
> need for separate storage of extents in event_rec_exts?
>
I discussed with Fan and for add specifically, we will need 2 loops
because the pending list is of type CXLDCExtentGroupList. We must use the
first loop to create a CXLDCExtentGroup from all of the extents and
append the entire thing to the pending list before triggering any interrupts for
event records.
This is necessary to preserve the ordering/grouping in order for the
memdev_add_rsp command to know what to remove from the pending list if
no extents were accepted.
But the storage of extents in event_rec_exts is unnecessary and for
release, we only need 1 loop.
Thanks,
Anisa
> > + memcpy(&event_rec.dynamic_capacity_extent,
> > + &event_rec_exts[i],
> > + sizeof(CXLDCExtentRaw));
> > +
> > + event_rec.flags = 0;
> > + if (i < in->ext_count - 1) {
> > + /* Set "More" flag */
> > + event_rec.flags |= BIT(0);
> > + }
> > +
> > + if (cxl_event_insert(&ct3d->cxl_dstate,
> > + CXL_EVENT_TYPE_DYNAMIC_CAP,
> > + (CXLEventRecordRaw *)&event_rec)) {
> > + cxl_event_irq_assert(ct3d);
> > + }
> > + }
> > +
> > + return CXL_MBOX_SUCCESS;
> > + default:
> > + qemu_log_mask(LOG_UNIMP,
> > + "CXL extent selection policy not supported.\n");
> > + return CXL_MBOX_INVALID_INPUT;
> > + }
> > +
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
> > static const struct cxl_cmd cxl_cmd_set[256][256] = {
> > [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> > cmd_infostat_bg_op_abort, 0, 0 },
> > @@ -3804,6 +3970,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> > CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> > [FMAPI_DCD_MGMT][GET_DC_REGION_EXTENT_LIST] = { "GET_DC_REGION_EXTENT_LIST",
> > cmd_fm_get_dc_region_extent_list, 12, 0},
> > + [FMAPI_DCD_MGMT][INITIATE_DC_ADD] = { "INIT_DC_ADD",
> > + cmd_fm_initiate_dc_add, ~0,
> > + (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
> > + CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
> > + CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> > + CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> > + CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> > };
> >
> > /*
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index b742b2bb8d..ccc619fe10 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -1982,8 +1982,8 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> > * the list.
> > * Return value: return true if has overlaps; otherwise, return false
> > */
> > -static bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> > - uint64_t dpa, uint64_t len)
> > +bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> > + uint64_t dpa, uint64_t len)
> > {
> > CXLDCExtent *ent;
> > Range range1, range2;
> > @@ -2028,8 +2028,8 @@ bool cxl_extents_contains_dpa_range(CXLDCExtentList *list,
> > return false;
> > }
> >
> > -static bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> > - uint64_t dpa, uint64_t len)
> > +bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> > + uint64_t dpa, uint64_t len)
> > {
> > CXLDCExtentGroup *group;
> >
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 217003a29d..1d5831a0b6 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -809,4 +809,8 @@ bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> > void cxl_assign_event_header(CXLEventRecordHdr *hdr,
> > const QemuUUID *uuid, uint32_t flags,
> > uint8_t length, uint64_t timestamp);
> > +bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> > + uint64_t dpa, uint64_t len);
> > +bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> > + uint64_t dpa, uint64_t len);
> > #endif
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct
2025-05-02 15:57 ` Fan Ni
@ 2025-05-06 16:53 ` Jonathan Cameron via
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Cameron via @ 2025-05-06 16:53 UTC (permalink / raw)
To: Fan Ni; +Cc: anisa.su887, qemu-devel, dave, linux-cxl, Anisa Su
On Fri, 2 May 2025 08:57:36 -0700
Fan Ni <nifan.cxl@gmail.com> wrote:
> On Fri, May 02, 2025 at 10:01:55AM +0100, Jonathan Cameron wrote:
> > On Thu, 1 May 2025 20:21:56 +0000
> > Fan Ni <nifan.cxl@gmail.com> wrote:
> >
> > > On Thu, Apr 24, 2025 at 11:42:59AM +0100, Jonathan Cameron wrote:
> > > > On Mon, 17 Mar 2025 16:31:30 +0000
> > > > anisa.su887@gmail.com wrote:
> > > >
> > > > > From: Anisa Su <anisa.su@samsung.com>
> > > > >
> > > > > Add dsmas_flags field to DC Region struct in preparation for next
> > > > > command, which returns the dsmas flags in the response.
> > > > >
> > > > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > > > ---
> > > > > hw/mem/cxl_type3.c | 2 ++
> > > > > include/hw/cxl/cxl_device.h | 1 +
> > > > > 2 files changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > > > index 731497ebda..452a0c101a 100644
> > > > > --- a/hw/mem/cxl_type3.c
> > > > > +++ b/hw/mem/cxl_type3.c
> > > > > @@ -237,6 +237,8 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> > > > > ct3d->dc.regions[i].len,
> > > > > false, true, region_base);
> > > > > ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1;
> > > > > + CDATDsmas *dsmas = (CDATDsmas *) table[cur_ent + CT3_CDAT_DSMAS];
> > > > > + ct3d->dc.regions[i].dsmas_flags = dsmas->flags;
> > > >
> > > Hi Jonathan,
> > > Thanks for the feedback.
> > > > This is relying to much on the ordering of creating fields in
> > > > ct3_build_cdat_entries_for_mr().
> > > I am not sure whether I understand this clearly.
> > > In current qemu implemtation, each mr (ram,pmem or dc region) will have the
> > > whole set of cdat table entries (dsmas, dslbis0-3, etc), so as long as we point
> > > to the right table entry, we can get the table correctly.
> > > What do you mean "the ordering of creating fields"?
> >
> > It is an implementation detail only that the first bit of that table is
> > the DSMAS entry. I think we shouldn't rely on that.
> >
> > > >
> > > > I'd rather you just stored the information flags is built from in CXLDCRegion
> > > > and then built the field that is wonderfully called 'Note' in the DC region
> > I got distracted by the spec oddity :)
> >
> > > This sentence is kind of broken for me, not totally clear what you are
> > > suggesting :-(. Can you explain more?
> > > Are you suggesting not directly take dsmas->flags as dsmas_flags, but
> > > use bit op to generate the value used in Table 7-66 in cxl spec 3.2?
> >
> > No. Just store the various bools etc that become dsmas->flags in the
> > CXLDCRegion structure directly rather than reading back from dsmas->flags.
> > Probably as explicit bools etc not a single value.
> >
> > Then pass those in to ct3_build_cdat_entries_for_mr() . Mostly they overlap
> > with current true / false parameters that are hard coded.
>
> OK. Since some flags are not support yet, can we hard coded them for now?
Sure. Add some breadcrumbs / comments for later though if that makes sense.
>
> Fan
> >
> >
> > > > configuration in 6.2 spec. I've sent a mail to see if we can clean that
> > > 6.2 spec???
> > > > 'what is the field called' question for future spec releases.
> > > >
> > > > Whilst the flag definitions cross refer the CDAT spec, the actual locations
> > > > of those flags matches, but doesn't cross refer so maybe in the future
> > > > we will have other flags in here and locations might not match.
> > > For the flags stored in dsmas table, do we expect there can be more than those
> > > defined in Table 7-66 in spec 3.2?
> >
> > Not for now. Though I'm sure something will come along at some point.
> > The comment is about there being particular reason the flag locations should match
> > between CDAT and what we report via the commands being added here. The definitions
> > of individual bits cross refer between specs, the register as a whole does not.
> >
> > Jonathan
> >
> > >
> > > Fan
> > >
> > > >
> > > > >
> > > > > cur_ent += CT3_CDAT_NUM_ENTRIES;
> > > > > region_base += ct3d->dc.regions[i].len;
> > > > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > > > > index bebed04085..81b826f570 100644
> > > > > --- a/include/hw/cxl/cxl_device.h
> > > > > +++ b/include/hw/cxl/cxl_device.h
> > > > > @@ -609,6 +609,7 @@ typedef struct CXLDCRegion {
> > > > > uint8_t flags;
> > > > > unsigned long *blk_bitmap;
> > > > > uint64_t supported_blk_size_bitmask;
> > > > > + uint8_t dsmas_flags;
> > > > > } CXLDCRegion;
> > > > >
> > > > > typedef struct CXLSetFeatureInfo {
> > > >
> >
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 8/9] cxl-mailbox-utils: 0x5604 - FMAPI Initiate DC Add
2025-05-05 16:40 ` Anisa Su
@ 2025-05-06 16:55 ` Jonathan Cameron via
0 siblings, 0 replies; 34+ messages in thread
From: Jonathan Cameron via @ 2025-05-06 16:55 UTC (permalink / raw)
To: Anisa Su; +Cc: qemu-devel, nifan.cxl, dave, linux-cxl
On Mon, 5 May 2025 16:40:18 +0000
Anisa Su <anisa.su887@gmail.com> wrote:
> On Thu, Apr 24, 2025 at 12:19:59PM +0100, Jonathan Cameron wrote:
> > On Mon, 17 Mar 2025 16:31:35 +0000
> > anisa.su887@gmail.com wrote:
> >
> > > From: Anisa Su <anisa.su@samsung.com>
> > >
> > > FM DCD Management command 0x5604 implemented per CXL r3.2 Spec Section 7.6.7.6.5
> > >
> > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > ---
> ...
> > > + /* Create event record and insert to event log */
> > > + cxl_mbox_dc_event_create_record_hdr(ct3d, &event_rec.hdr);
> > > + event_rec.type = DC_EVENT_ADD_CAPACITY;
> > > + /* FIXME: for now, validity flag is cleared */
> >
> > This stuff is probably all valid. If we can return remaining extents though we might
> > as well.
> >
> > > + event_rec.validity_flags = 0;
> > > + /* FIXME: Currently only support 1 host */
> > > + event_rec.host_id = 0;
> > > + /* updated_region_id only valid for DC_EVENT_REGION_CONFIG_UPDATED */
> > > + event_rec.updated_region_id = 0;
> >
> > The event_rec is zeroed anyway so probably just don't set this at all
> > and no need for the comment.
> >
> > > + for (i = 0; i < in->ext_count; i++) {
> >
> > Why can't we combine this with the earlier loop and avoid the
> > need for separate storage of extents in event_rec_exts?
> >
> I discussed with Fan and for add specifically, we will need 2 loops
> because the pending list is of type CXLDCExtentGroupList. We must use the
> first loop to create a CXLDCExtentGroup from all of the extents and
> append the entire thing to the pending list before triggering any interrupts for
> event records.
> This is necessary to preserve the ordering/grouping in order for the
> memdev_add_rsp command to know what to remove from the pending list if
> no extents were accepted.
Ah. That makes sense. Thanks!
> But the storage of extents in event_rec_exts is unnecessary and for
> release, we only need 1 loop.
>
> Thanks,
> Anisa
> > > + memcpy(&event_rec.dynamic_capacity_extent,
> > > + &event_rec_exts[i],
> > > + sizeof(CXLDCExtentRaw));
> > > +
> > > + event_rec.flags = 0;
> > > + if (i < in->ext_count - 1) {
> > > + /* Set "More" flag */
> > > + event_rec.flags |= BIT(0);
> > > + }
> > > +
> > > + if (cxl_event_insert(&ct3d->cxl_dstate,
> > > + CXL_EVENT_TYPE_DYNAMIC_CAP,
> > > + (CXLEventRecordRaw *)&event_rec)) {
> > > + cxl_event_irq_assert(ct3d);
> > > + }
> > > + }
> > > +
> > > + return CXL_MBOX_SUCCESS;
> > > + default:
> > > + qemu_log_mask(LOG_UNIMP,
> > > + "CXL extent selection policy not supported.\n");
> > > + return CXL_MBOX_INVALID_INPUT;
> > > + }
> > > +
> > > + return CXL_MBOX_SUCCESS;
> > > +}
> > > +
> > > static const struct cxl_cmd cxl_cmd_set[256][256] = {
> > > [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> > > cmd_infostat_bg_op_abort, 0, 0 },
> > > @@ -3804,6 +3970,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> > > CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> > > [FMAPI_DCD_MGMT][GET_DC_REGION_EXTENT_LIST] = { "GET_DC_REGION_EXTENT_LIST",
> > > cmd_fm_get_dc_region_extent_list, 12, 0},
> > > + [FMAPI_DCD_MGMT][INITIATE_DC_ADD] = { "INIT_DC_ADD",
> > > + cmd_fm_initiate_dc_add, ~0,
> > > + (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
> > > + CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
> > > + CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> > > + CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> > > + CXL_MBOX_IMMEDIATE_DATA_CHANGE)},
> > > };
> > >
> > > /*
> > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > index b742b2bb8d..ccc619fe10 100644
> > > --- a/hw/mem/cxl_type3.c
> > > +++ b/hw/mem/cxl_type3.c
> > > @@ -1982,8 +1982,8 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> > > * the list.
> > > * Return value: return true if has overlaps; otherwise, return false
> > > */
> > > -static bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> > > - uint64_t dpa, uint64_t len)
> > > +bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> > > + uint64_t dpa, uint64_t len)
> > > {
> > > CXLDCExtent *ent;
> > > Range range1, range2;
> > > @@ -2028,8 +2028,8 @@ bool cxl_extents_contains_dpa_range(CXLDCExtentList *list,
> > > return false;
> > > }
> > >
> > > -static bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> > > - uint64_t dpa, uint64_t len)
> > > +bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> > > + uint64_t dpa, uint64_t len)
> > > {
> > > CXLDCExtentGroup *group;
> > >
> > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > > index 217003a29d..1d5831a0b6 100644
> > > --- a/include/hw/cxl/cxl_device.h
> > > +++ b/include/hw/cxl/cxl_device.h
> > > @@ -809,4 +809,8 @@ bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> > > void cxl_assign_event_header(CXLEventRecordHdr *hdr,
> > > const QemuUUID *uuid, uint32_t flags,
> > > uint8_t length, uint64_t timestamp);
> > > +bool cxl_extents_overlaps_dpa_range(CXLDCExtentList *list,
> > > + uint64_t dpa, uint64_t len);
> > > +bool cxl_extent_groups_overlaps_dpa_range(CXLDCExtentGroupList *list,
> > > + uint64_t dpa, uint64_t len);
> > > #endif
> >
>
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2025-05-06 17:11 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-17 16:31 [PATCH 0/9] CXL: FMAPI DCD Management Commands 0x5600-0x5605 anisa.su887
2025-03-17 16:31 ` [PATCH 1/9] cxl/type3: Add supported block sizes bitmask to CXLDCRegion struct anisa.su887
2025-04-24 10:11 ` Jonathan Cameron via
2025-04-29 17:56 ` Anisa Su
2025-03-17 16:31 ` [PATCH 2/9] cxl-mailbox-utils: 0x5600 - FMAPI Get DCD Info anisa.su887
2025-03-18 15:56 ` Jonathan Cameron via
2025-03-31 19:38 ` Anisa Su
2025-04-14 16:52 ` Anisa Su
2025-04-24 10:21 ` Jonathan Cameron via
2025-04-16 21:25 ` Anisa Su
2025-04-24 10:30 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 3/9] cxl/type3: Add dsmas_flags to CXLDCRegion struct anisa.su887
2025-04-24 10:42 ` Jonathan Cameron via
2025-05-01 20:21 ` Fan Ni
2025-05-02 9:01 ` Jonathan Cameron via
2025-05-02 15:57 ` Fan Ni
2025-05-06 16:53 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 4/9] cxl-mailbox-utils: 0x5601 - FMAPI Get Host Region Config anisa.su887
2025-04-24 10:53 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 5/9] cxl_events.h: move definition for dynamic_capacity_uuid and enum for DC event types anisa.su887
2025-04-24 10:55 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 6/9] cxl-mailbox-utils: 0x5602 - FMAPI Set DC Region Config anisa.su887
2025-04-16 21:33 ` Anisa Su
2025-04-24 11:05 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 7/9] cxl-mailbox-utils: 0x5603 - FMAPI Get DC Region Extent Lists anisa.su887
2025-04-24 11:10 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 8/9] cxl-mailbox-utils: 0x5604 - FMAPI Initiate DC Add anisa.su887
2025-04-24 11:19 ` Jonathan Cameron via
2025-04-28 20:41 ` Fan Ni
2025-05-05 16:40 ` Anisa Su
2025-05-06 16:55 ` Jonathan Cameron via
2025-03-17 16:31 ` [PATCH 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release anisa.su887
2025-04-24 11:23 ` Jonathan Cameron via
2025-04-28 20:44 ` Fan Ni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).