* [PATCH v4 00/10] Enabling DCD emulation support in Qemu
@ 2024-02-21 18:15 nifan.cxl
2024-02-21 18:15 ` [PATCH v4 01/10] hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output payload of identify memory device command nifan.cxl
` (12 more replies)
0 siblings, 13 replies; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:15 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
v3[1]->v4:
The code is rebased on mainstream QEMU with the following patch series:
hw/cxl/mailbox: change CCI cmd set structure to be a member, not a reference
hw/cxl/mailbox: interface to add CCI commands to an existing CCI
Main changes include:
1. Updated the specification references to align with cxl spec r3.1.
2. Add extra elements to get dc region configuration output payload and
procecced accordingly in mailbox command 4800h.
3. Removed the unwanted space.
4. Refactored ct3_build_cdat_entries_for_mr and extract it as a separate patch.
5. Updated cxl_create_dc_regions function to derive region len from host
backend size.
6. Changed the logic for creating DC regions when host backend and address
space processing is introduced, now cxl_create_dc_regions is called only
when host backend exists.
7. Updated the name of the definitions related to DC extents for consistency.
7. Updated dynamic capacity event record definition to align with spec r3.1.
9. Changed the dynamic capacity request process logic, for release request,
extra checks are done against the pending list to remove the extent yet added.
10. Changed the return value of cxl_create_dc_regions so the return can be used
to Remove the extent for the list if needed.
11. offset and size in the qmp interface are changed to be byte-wise while the
original is MiB-wise.
12. Fixed bugs in handling bitmap for dpa range existence.
13. NOTE: in previous version DC is set to non-volatile, while in this version
we change it to volatile per Jonathan's suggestion.
14. Updated the doc in qapi/cxl.json.
Thank Jonathan for the detailed review of the last version[1].
The code is tested with Ira's last kernel DCD patch set [2] with some minor
bug fixes[3]. Tested operations include:
1. create DC region;
2. Add/release DC extents;
3. convert DC capacity into system RAM (no real read/write to DCD tested);
v3:
[1] https://lore.kernel.org/linux-cxl/20231107180907.553451-1-nifan.cxl@gmail.com/T/#t
[2] https://github.com/weiny2/linux-kernel/tree/dcd-v3-2023-10-30
[3] https://github.com/moking/linux-dcd/commit/9d24fa6e5d39f934623220953caecc080f93e964
Fan Ni (10):
hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output
payload of identify memory device command
hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative
and mailbox command support
include/hw/cxl/cxl_device: Rename mem_size as static_mem_size for
type3 memory devices
hw/mem/cxl_type3: Add support to create DC regions to type3 memory
devices
hw/mem/cxl-type3: Refactor ct3_build_cdat_entries_for_mr to take mr
size insead of mr as argument
hw/mem/cxl_type3: Add host backend and address space handling for DC
regions
hw/mem/cxl_type3: Add DC extent list representative and get DC extent
list mailbox support
hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release
dynamic capacity response
hw/cxl/events: Add qmp interfaces to add/release dynamic capacity
extents
hw/mem/cxl_type3: Add dpa range validation for accesses to DC regions
hw/cxl/cxl-mailbox-utils.c | 507 +++++++++++++++++++++++++++++++-
hw/mem/cxl_type3.c | 559 +++++++++++++++++++++++++++++++++---
hw/mem/cxl_type3_stubs.c | 14 +
include/hw/cxl/cxl_device.h | 61 +++-
include/hw/cxl/cxl_events.h | 18 ++
qapi/cxl.json | 61 +++-
6 files changed, 1174 insertions(+), 46 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 01/10] hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output payload of identify memory device command
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
@ 2024-02-21 18:15 ` nifan.cxl
2024-02-21 18:15 ` [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support nifan.cxl
` (11 subsequent siblings)
12 siblings, 0 replies; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:15 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
Based on CXL spec r3.1 Table 8-127 (Identify Memory Device Output
Payload), dynamic capacity event log size should be part of
output of the Identify command.
Add dc_event_log_size to the output payload for the host to get the info.
Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 4bcd727f4c..ba1d9901df 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -21,6 +21,7 @@
#include "sysemu/hostmem.h"
#define CXL_CAPACITY_MULTIPLIER (256 * MiB)
+#define CXL_DC_EVENT_LOG_SIZE 8
/*
* How to add a new command, example. The command set FOO, with cmd BAR.
@@ -780,8 +781,9 @@ static CXLRetCode cmd_identify_memory_device(const struct cxl_cmd *cmd,
uint16_t inject_poison_limit;
uint8_t poison_caps;
uint8_t qos_telemetry_caps;
+ uint16_t dc_event_log_size;
} QEMU_PACKED *id;
- QEMU_BUILD_BUG_ON(sizeof(*id) != 0x43);
+ QEMU_BUILD_BUG_ON(sizeof(*id) != 0x45);
CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
CXLType3Class *cvc = CXL_TYPE3_GET_CLASS(ct3d);
CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
@@ -807,6 +809,7 @@ static CXLRetCode cmd_identify_memory_device(const struct cxl_cmd *cmd,
st24_le_p(id->poison_list_max_mer, 256);
/* No limit - so limited by main poison record limit */
stw_le_p(&id->inject_poison_limit, 0);
+ stw_le_p(&id->dc_event_log_size, CXL_DC_EVENT_LOG_SIZE);
*len_out = sizeof(*id);
return CXL_MBOX_SUCCESS;
--
2.43.0
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
2024-02-21 18:15 ` [PATCH v4 01/10] hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output payload of identify memory device command nifan.cxl
@ 2024-02-21 18:15 ` nifan.cxl
2024-02-26 17:33 ` Jonathan Cameron via
2024-03-04 12:40 ` Jørgen Hansen
2024-02-21 18:15 ` [PATCH v4 03/10] include/hw/cxl/cxl_device: Rename mem_size as static_mem_size for type3 memory devices nifan.cxl
` (10 subsequent siblings)
12 siblings, 2 replies; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:15 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
Per cxl spec r3.1, add dynamic capacity region representative based on
Table 8-165 and extend the cxl type3 device definition to include dc region
information. Also, based on info in 8.2.9.9.9.1, add 'Get Dynamic Capacity
Configuration' mailbox support.
Note: decode_len of a dc region is aligned to 256*MiB, divided by
256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
mailbox command.
Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 110 ++++++++++++++++++++++++++++++++++++
include/hw/cxl/cxl_device.h | 16 ++++++
2 files changed, 126 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index ba1d9901df..88e3b733e3 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -22,6 +22,7 @@
#define CXL_CAPACITY_MULTIPLIER (256 * MiB)
#define CXL_DC_EVENT_LOG_SIZE 8
+#define CXL_SPEC_AFTER_R30
/*
* How to add a new command, example. The command set FOO, with cmd BAR.
@@ -80,6 +81,8 @@ enum {
#define GET_POISON_LIST 0x0
#define INJECT_POISON 0x1
#define CLEAR_POISON 0x2
+ DCD_CONFIG = 0x48,
+ #define GET_DC_CONFIG 0x0
PHYSICAL_SWITCH = 0x51,
#define IDENTIFY_SWITCH_DEVICE 0x0
#define GET_PHYSICAL_PORT_STATE 0x1
@@ -1238,6 +1241,103 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+/*
+ * CXL r3.1 section 8.2.9.9.9.1: Get Dynamic Capacity Configuration
+ * (Opcode: 4800h)
+ */
+static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ struct get_dyn_cap_config_in_pl {
+ uint8_t region_cnt;
+ uint8_t start_region_id;
+ } QEMU_PACKED;
+
+ struct get_dyn_cap_config_out_pl {
+ uint8_t num_regions;
+ uint8_t regions_returned;
+ uint8_t rsvd1[6];
+ struct {
+ uint64_t base;
+ uint64_t decode_len;
+ uint64_t region_len;
+ uint64_t block_size;
+ uint32_t dsmadhandle;
+ uint8_t flags;
+ uint8_t rsvd2[3];
+ } QEMU_PACKED records[];
+ /*
+ * if cxl spec version >= 3.1, extra output payload as defined
+ * in struct get_dyn_cap_config_out_pl_extra comes here.
+ */
+ } QEMU_PACKED;
+
+ struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
+ struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
+ uint16_t record_count = 0;
+ uint16_t i;
+ uint16_t out_pl_len;
+ uint8_t start_region_id = in->start_region_id;
+#ifdef CXL_SPEC_AFTER_R30
+ struct get_dyn_cap_config_out_pl_extra {
+ uint32_t num_extents_supported;
+ uint32_t num_extents_available;
+ uint32_t num_tags_supported;
+ uint32_t num_tags_available;
+ } QEMU_PACKED;
+ struct get_dyn_cap_config_out_pl_extra *extra_out;
+#endif
+
+ if (start_region_id >= ct3d->dc.num_regions) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
+ in->region_cnt);
+
+ out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
+#ifdef CXL_SPEC_AFTER_R30
+ extra_out = (struct get_dyn_cap_config_out_pl_extra *)(payload_out +
+ out_pl_len);
+ out_pl_len += sizeof(struct get_dyn_cap_config_out_pl_extra);
+#endif
+ assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
+
+ out->num_regions = ct3d->dc.num_regions;
+#ifdef CXL_SPEC_AFTER_R30
+ out->regions_returned = record_count;
+#endif
+ for (i = 0; i < record_count; i++) {
+ stq_le_p(&out->records[i].base,
+ ct3d->dc.regions[start_region_id + i].base);
+ stq_le_p(&out->records[i].decode_len,
+ ct3d->dc.regions[start_region_id + i].decode_len /
+ CXL_CAPACITY_MULTIPLIER);
+ stq_le_p(&out->records[i].region_len,
+ ct3d->dc.regions[start_region_id + i].len);
+ stq_le_p(&out->records[i].block_size,
+ ct3d->dc.regions[start_region_id + i].block_size);
+ stl_le_p(&out->records[i].dsmadhandle,
+ ct3d->dc.regions[start_region_id + i].dsmadhandle);
+ out->records[i].flags = ct3d->dc.regions[start_region_id + i].flags;
+ }
+#ifdef CXL_SPEC_AFTER_R30
+ /*FIXME: need to set valid values in the future*/
+ stq_le_p(&extra_out->num_extents_supported, 0);
+ stq_le_p(&extra_out->num_extents_available, 0);
+ stq_le_p(&extra_out->num_tags_supported, 0);
+ stq_le_p(&extra_out->num_tags_available, 0);
+#endif
+
+ *len_out = out_pl_len;
+ return CXL_MBOX_SUCCESS;
+}
+
#define IMMEDIATE_CONFIG_CHANGE (1 << 1)
#define IMMEDIATE_DATA_CHANGE (1 << 2)
#define IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -1282,6 +1382,11 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
cmd_media_clear_poison, 72, 0 },
};
+static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
+ [DCD_CONFIG][GET_DC_CONFIG] = { "DCD_GET_DC_CONFIG",
+ cmd_dcd_get_dyn_cap_config, 2, 0 },
+};
+
static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
[INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 },
[INFOSTAT][BACKGROUND_OPERATION_STATUS] = { "BACKGROUND_OPERATION_STATUS",
@@ -1487,7 +1592,12 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
{
+ CXLType3Dev *ct3d = CXL_TYPE3(d);
+
cxl_copy_cci_commands(cci, cxl_cmd_set);
+ if (ct3d->dc.num_regions) {
+ cxl_copy_cci_commands(cci, cxl_cmd_set_dcd);
+ }
cci->d = d;
/* No separation for PCI MB as protocol handled in PCI device */
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 3cf3077afa..6df7fecdf1 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -422,6 +422,17 @@ typedef struct CXLPoison {
typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
#define CXL_POISON_LIST_LIMIT 256
+#define DCD_MAX_REGION_NUM 8
+
+typedef struct CXLDCDRegion {
+ uint64_t base;
+ uint64_t decode_len; /* aligned to 256*MiB */
+ uint64_t len;
+ uint64_t block_size;
+ uint32_t dsmadhandle;
+ uint8_t flags;
+} CXLDCDRegion;
+
struct CXLType3Dev {
/* Private */
PCIDevice parent_obj;
@@ -454,6 +465,11 @@ struct CXLType3Dev {
unsigned int poison_list_cnt;
bool poison_list_overflowed;
uint64_t poison_list_overflow_ts;
+
+ struct dynamic_capacity {
+ uint8_t num_regions; /* 0-8 regions */
+ CXLDCDRegion regions[DCD_MAX_REGION_NUM];
+ } dc;
};
#define TYPE_CXL_TYPE3 "cxl-type3"
--
2.43.0
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v4 03/10] include/hw/cxl/cxl_device: Rename mem_size as static_mem_size for type3 memory devices
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
2024-02-21 18:15 ` [PATCH v4 01/10] hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output payload of identify memory device command nifan.cxl
2024-02-21 18:15 ` [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support nifan.cxl
@ 2024-02-21 18:15 ` nifan.cxl
2024-02-21 18:15 ` [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to " nifan.cxl
` (9 subsequent siblings)
12 siblings, 0 replies; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:15 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
Rename mem_size as static_mem_size for type3 memdev to cover static RAM and
pmem capacity, preparing for the introduction of dynamic capacity to support
dynamic capacity devices.
Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 4 ++--
hw/mem/cxl_type3.c | 8 ++++----
include/hw/cxl/cxl_device.h | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 88e3b733e3..7d2b74c9c5 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -802,7 +802,7 @@ static CXLRetCode cmd_identify_memory_device(const struct cxl_cmd *cmd,
snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
stq_le_p(&id->total_capacity,
- cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER);
+ cxl_dstate->static_mem_size / CXL_CAPACITY_MULTIPLIER);
stq_le_p(&id->persistent_capacity,
cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
stq_le_p(&id->volatile_capacity,
@@ -1178,7 +1178,7 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
struct clear_poison_pl *in = (void *)payload_in;
dpa = ldq_le_p(&in->dpa);
- if (dpa + CXL_CACHE_LINE_SIZE > cxl_dstate->mem_size) {
+ if (dpa + CXL_CACHE_LINE_SIZE > cxl_dstate->static_mem_size) {
return CXL_MBOX_INVALID_PA;
}
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index e8801805b9..244d2b5fd5 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -608,7 +608,7 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
}
address_space_init(&ct3d->hostvmem_as, vmr, v_name);
ct3d->cxl_dstate.vmem_size = memory_region_size(vmr);
- ct3d->cxl_dstate.mem_size += memory_region_size(vmr);
+ ct3d->cxl_dstate.static_mem_size += memory_region_size(vmr);
g_free(v_name);
}
@@ -631,7 +631,7 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
}
address_space_init(&ct3d->hostpmem_as, pmr, p_name);
ct3d->cxl_dstate.pmem_size = memory_region_size(pmr);
- ct3d->cxl_dstate.mem_size += memory_region_size(pmr);
+ ct3d->cxl_dstate.static_mem_size += memory_region_size(pmr);
g_free(p_name);
}
@@ -837,7 +837,7 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
return -EINVAL;
}
- if (*dpa_offset > ct3d->cxl_dstate.mem_size) {
+ if (*dpa_offset > ct3d->cxl_dstate.static_mem_size) {
return -EINVAL;
}
@@ -1010,7 +1010,7 @@ static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
return false;
}
- if (dpa_offset + CXL_CACHE_LINE_SIZE > ct3d->cxl_dstate.mem_size) {
+ if (dpa_offset + CXL_CACHE_LINE_SIZE > ct3d->cxl_dstate.static_mem_size) {
return false;
}
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 6df7fecdf1..80188db670 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -234,7 +234,7 @@ typedef struct cxl_device_state {
} timestamp;
/* memory region size, HDM */
- uint64_t mem_size;
+ uint64_t static_mem_size;
uint64_t pmem_size;
uint64_t vmem_size;
--
2.43.0
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to type3 memory devices
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
` (2 preceding siblings ...)
2024-02-21 18:15 ` [PATCH v4 03/10] include/hw/cxl/cxl_device: Rename mem_size as static_mem_size for type3 memory devices nifan.cxl
@ 2024-02-21 18:15 ` nifan.cxl
2024-02-26 17:38 ` Jonathan Cameron via
2024-03-04 13:10 ` Jørgen Hansen
2024-02-21 18:15 ` [PATCH v4 05/10] hw/mem/cxl-type3: Refactor ct3_build_cdat_entries_for_mr to take mr size insead of mr as argument nifan.cxl
` (8 subsequent siblings)
12 siblings, 2 replies; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:15 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
With the change, when setting up memory for type3 memory device, we can
create DC regions.
A property 'num-dc-regions' is added to ct3_props to allow users to pass the
number of DC regions to create. To make it easier, other region parameters
like region base, length, and block size are hard coded. If needed,
these parameters can be added easily.
With the change, we can create DC regions with proper kernel side
support like below:
region=$(cat /sys/bus/cxl/devices/decoder0.0/create_dc_region)
echo $region > /sys/bus/cxl/devices/decoder0.0/create_dc_region
echo 256 > /sys/bus/cxl/devices/$region/interleave_granularity
echo 1 > /sys/bus/cxl/devices/$region/interleave_ways
echo "dc0" >/sys/bus/cxl/devices/decoder2.0/mode
echo 0x40000000 >/sys/bus/cxl/devices/decoder2.0/dpa_size
echo 0x40000000 > /sys/bus/cxl/devices/$region/size
echo "decoder2.0" > /sys/bus/cxl/devices/$region/target0
echo 1 > /sys/bus/cxl/devices/$region/commit
echo $region > /sys/bus/cxl/drivers/cxl_region/bind
However, we cannot really read/write to the DC regions due to lack of
1. host backend and address space setup for DC regions;
2. mailbox command support for adding/releasing DC extents.
Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
hw/mem/cxl_type3.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 244d2b5fd5..c61cd2b5ac 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -567,6 +567,40 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
}
}
+/*
+ * TODO: dc region configuration will be updated once host backend and address
+ * space support is added for DCD.
+ */
+static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
+{
+ int i;
+ uint64_t region_base = 0;
+ uint64_t region_len = 2 * GiB;
+ uint64_t decode_len = 2 * GiB;
+ uint64_t blk_size = 2 * MiB;
+ CXLDCDRegion *region;
+
+ if (ct3d->hostvmem) {
+ region_base += ct3d->hostvmem->size;
+ }
+ if (ct3d->hostpmem) {
+ region_base += ct3d->hostpmem->size;
+ }
+ for (i = 0; i < ct3d->dc.num_regions; i++) {
+ region = &ct3d->dc.regions[i];
+ region->base = region_base;
+ region->decode_len = decode_len;
+ region->len = region_len;
+ region->block_size = blk_size;
+ /* dsmad_handle is set when creating cdat table entries */
+ region->flags = 0;
+
+ region_base += region->len;
+ }
+
+ return true;
+}
+
static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
{
DeviceState *ds = DEVICE(ct3d);
@@ -635,6 +669,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
g_free(p_name);
}
+ if (!cxl_create_dc_regions(ct3d, errp)) {
+ error_setg(errp, "setup DC regions failed");
+ return false;
+ }
+
return true;
}
@@ -930,6 +969,7 @@ static Property ct3_props[] = {
HostMemoryBackend *),
DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL),
DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename),
+ DEFINE_PROP_UINT8("num-dc-regions", CXLType3Dev, dc.num_regions, 0),
DEFINE_PROP_END_OF_LIST(),
};
--
2.43.0
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v4 05/10] hw/mem/cxl-type3: Refactor ct3_build_cdat_entries_for_mr to take mr size insead of mr as argument
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
` (3 preceding siblings ...)
2024-02-21 18:15 ` [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to " nifan.cxl
@ 2024-02-21 18:15 ` nifan.cxl
2024-02-21 18:15 ` [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions nifan.cxl
` (7 subsequent siblings)
12 siblings, 0 replies; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:15 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
The function ct3_build_cdat_entries_for_mr only uses size of the passed
memory region argument, refactor the function definition to make the passed
arguments more specific.
Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
hw/mem/cxl_type3.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index c61cd2b5ac..6e5f908fb1 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -43,7 +43,7 @@ enum {
};
static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
- int dsmad_handle, MemoryRegion *mr,
+ int dsmad_handle, uint64_t size,
bool is_pmem, uint64_t dpa_base)
{
g_autofree CDATDsmas *dsmas = NULL;
@@ -62,7 +62,7 @@ static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
.DSMADhandle = dsmad_handle,
.flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0,
.DPA_base = dpa_base,
- .DPA_length = memory_region_size(mr),
+ .DPA_length = size,
};
/* For now, no memory side cache, plausiblish numbers */
@@ -131,7 +131,7 @@ static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
*/
.EFI_memory_type_attr = is_pmem ? 2 : 1,
.DPA_offset = 0,
- .DPA_length = memory_region_size(mr),
+ .DPA_length = size,
};
/* Header always at start of structure */
@@ -148,6 +148,7 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
g_autofree CDATSubHeader **table = NULL;
CXLType3Dev *ct3d = priv;
MemoryRegion *volatile_mr = NULL, *nonvolatile_mr = NULL;
+ uint64_t vmr_size = 0, pmr_size = 0;
int dsmad_handle = 0;
int cur_ent = 0;
int len = 0;
@@ -162,6 +163,7 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
return -EINVAL;
}
len += CT3_CDAT_NUM_ENTRIES;
+ vmr_size = memory_region_size(volatile_mr);
}
if (ct3d->hostpmem) {
@@ -170,21 +172,22 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
return -EINVAL;
}
len += CT3_CDAT_NUM_ENTRIES;
+ pmr_size = memory_region_size(nonvolatile_mr);
}
table = g_malloc0(len * sizeof(*table));
/* Now fill them in */
if (volatile_mr) {
- ct3_build_cdat_entries_for_mr(table, dsmad_handle++, volatile_mr,
+ ct3_build_cdat_entries_for_mr(table, dsmad_handle++, vmr_size,
false, 0);
cur_ent = CT3_CDAT_NUM_ENTRIES;
}
if (nonvolatile_mr) {
- uint64_t base = volatile_mr ? memory_region_size(volatile_mr) : 0;
+ uint64_t base = vmr_size;
ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
- nonvolatile_mr, true, base);
+ pmr_size, true, base);
cur_ent += CT3_CDAT_NUM_ENTRIES;
}
assert(len == cur_ent);
--
2.43.0
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
` (4 preceding siblings ...)
2024-02-21 18:15 ` [PATCH v4 05/10] hw/mem/cxl-type3: Refactor ct3_build_cdat_entries_for_mr to take mr size insead of mr as argument nifan.cxl
@ 2024-02-21 18:15 ` nifan.cxl
2024-02-26 17:45 ` Jonathan Cameron via
2024-02-21 18:16 ` [PATCH v4 07/10] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support nifan.cxl
` (6 subsequent siblings)
12 siblings, 1 reply; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:15 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
Add (file/memory backed) host backend, all the dynamic capacity regions
will share a single, large enough host backend. Set up address space for
DC regions to support read/write operations to dynamic capacity for DCD.
With the change, following supports are added:
1. Add a new property to type3 device "volatile-dc-memdev" to point to host
memory backend for dynamic capacity. Currently, all dc regions share one
one host backend.
2. Add namespace for dynamic capacity for read/write support;
3. Create cdat entries for each dynamic capacity region;
4. Fix dvsec range registers to include DC regions.
Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 16 ++-
hw/mem/cxl_type3.c | 188 ++++++++++++++++++++++++++++--------
include/hw/cxl/cxl_device.h | 4 +
3 files changed, 165 insertions(+), 43 deletions(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 7d2b74c9c5..f95e417683 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -621,7 +621,8 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
size_t *len_out,
CXLCCI *cci)
{
- CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate;
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
struct {
uint8_t slots_supported;
uint8_t slot_info;
@@ -635,7 +636,8 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
QEMU_BUILD_BUG_ON(sizeof(*fw_info) != 0x50);
if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
- (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {
+ (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER) ||
+ (ct3d->dc.total_capacity < CXL_CAPACITY_MULTIPLIER)) {
return CXL_MBOX_INTERNAL_ERROR;
}
@@ -792,7 +794,8 @@ static CXLRetCode cmd_identify_memory_device(const struct cxl_cmd *cmd,
CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) ||
- (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) {
+ (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER)) ||
+ (!QEMU_IS_ALIGNED(ct3d->dc.total_capacity, CXL_CAPACITY_MULTIPLIER))) {
return CXL_MBOX_INTERNAL_ERROR;
}
@@ -834,9 +837,11 @@ static CXLRetCode cmd_ccls_get_partition_info(const struct cxl_cmd *cmd,
uint64_t next_pmem;
} QEMU_PACKED *part_info = (void *)payload_out;
QEMU_BUILD_BUG_ON(sizeof(*part_info) != 0x20);
+ CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) ||
- (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) {
+ (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER)) ||
+ (!QEMU_IS_ALIGNED(ct3d->dc.total_capacity, CXL_CAPACITY_MULTIPLIER))) {
return CXL_MBOX_INTERNAL_ERROR;
}
@@ -1178,7 +1183,8 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
struct clear_poison_pl *in = (void *)payload_in;
dpa = ldq_le_p(&in->dpa);
- if (dpa + CXL_CACHE_LINE_SIZE > cxl_dstate->static_mem_size) {
+ if (dpa + CXL_CACHE_LINE_SIZE > cxl_dstate->static_mem_size +
+ ct3d->dc.total_capacity) {
return CXL_MBOX_INVALID_PA;
}
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 6e5f908fb1..b966fa4f10 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -30,6 +30,7 @@
#include "hw/pci/msix.h"
#define DWORD_BYTE 4
+#define CXL_CAPACITY_MULTIPLIER (256 * MiB)
/* Default CDAT entries for a memory region */
enum {
@@ -44,7 +45,8 @@ enum {
static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
int dsmad_handle, uint64_t size,
- bool is_pmem, uint64_t dpa_base)
+ bool is_pmem, bool is_dynamic,
+ uint64_t dpa_base)
{
g_autofree CDATDsmas *dsmas = NULL;
g_autofree CDATDslbis *dslbis0 = NULL;
@@ -60,7 +62,8 @@ static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
.length = sizeof(*dsmas),
},
.DSMADhandle = dsmad_handle,
- .flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0,
+ .flags = (is_pmem ? CDAT_DSMAS_FLAG_NV : 0) |
+ (is_dynamic ? CDAT_DSMAS_FLAG_DYNAMIC_CAP : 0),
.DPA_base = dpa_base,
.DPA_length = size,
};
@@ -148,12 +151,13 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
g_autofree CDATSubHeader **table = NULL;
CXLType3Dev *ct3d = priv;
MemoryRegion *volatile_mr = NULL, *nonvolatile_mr = NULL;
+ MemoryRegion *dc_mr = NULL;
uint64_t vmr_size = 0, pmr_size = 0;
int dsmad_handle = 0;
int cur_ent = 0;
int len = 0;
- if (!ct3d->hostpmem && !ct3d->hostvmem) {
+ if (!ct3d->hostpmem && !ct3d->hostvmem && !ct3d->dc.num_regions) {
return 0;
}
@@ -175,21 +179,51 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
pmr_size = memory_region_size(nonvolatile_mr);
}
+ if (ct3d->dc.num_regions) {
+ if (ct3d->dc.host_dc) {
+ dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
+ if (!dc_mr) {
+ return -EINVAL;
+ }
+ len += CT3_CDAT_NUM_ENTRIES * ct3d->dc.num_regions;
+ } else {
+ return -EINVAL;
+ }
+ }
+
table = g_malloc0(len * sizeof(*table));
/* Now fill them in */
if (volatile_mr) {
ct3_build_cdat_entries_for_mr(table, dsmad_handle++, vmr_size,
- false, 0);
+ false, false, 0);
cur_ent = CT3_CDAT_NUM_ENTRIES;
}
if (nonvolatile_mr) {
uint64_t base = vmr_size;
ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
- pmr_size, true, base);
+ pmr_size, true, false, base);
cur_ent += CT3_CDAT_NUM_ENTRIES;
}
+
+ if (dc_mr) {
+ int i;
+ uint64_t region_base = vmr_size + pmr_size;
+
+ /* FIXME: Currently we assume the dynamic capacity to be volatile. */
+ for (i = 0; i < ct3d->dc.num_regions; i++) {
+ ct3_build_cdat_entries_for_mr(&(table[cur_ent]),
+ dsmad_handle++,
+ ct3d->dc.regions[i].len,
+ false, true, region_base);
+ ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1;
+
+ cur_ent += CT3_CDAT_NUM_ENTRIES;
+ region_base += ct3d->dc.regions[i].len;
+ }
+ }
+
assert(len == cur_ent);
*cdat_table = g_steal_pointer(&table);
@@ -299,11 +333,24 @@ static void build_dvsecs(CXLType3Dev *ct3d)
range2_size_hi = ct3d->hostpmem->size >> 32;
range2_size_lo = (2 << 5) | (2 << 2) | 0x3 |
(ct3d->hostpmem->size & 0xF0000000);
+ } else if (ct3d->dc.host_dc) {
+ range2_size_hi = ct3d->dc.host_dc->size >> 32;
+ range2_size_lo = (2 << 5) | (2 << 2) | 0x3 |
+ (ct3d->dc.host_dc->size & 0xF0000000);
}
- } else {
+ } else if (ct3d->hostpmem) {
range1_size_hi = ct3d->hostpmem->size >> 32;
range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
(ct3d->hostpmem->size & 0xF0000000);
+ if (ct3d->dc.host_dc) {
+ range2_size_hi = ct3d->dc.host_dc->size >> 32;
+ range2_size_lo = (2 << 5) | (2 << 2) | 0x3 |
+ (ct3d->dc.host_dc->size & 0xF0000000);
+ }
+ } else {
+ range1_size_hi = ct3d->dc.host_dc->size >> 32;
+ range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
+ (ct3d->dc.host_dc->size & 0xF0000000);
}
dvsec = (uint8_t *)&(CXLDVSECDevice){
@@ -570,19 +617,32 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
}
}
-/*
- * TODO: dc region configuration will be updated once host backend and address
- * space support is added for DCD.
- */
static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
{
int i;
+ MemoryRegion *dc_mr;
+ uint64_t dc_size = 0;
uint64_t region_base = 0;
- uint64_t region_len = 2 * GiB;
- uint64_t decode_len = 2 * GiB;
+ uint64_t region_len;
+ uint64_t decode_len;
uint64_t blk_size = 2 * MiB;
CXLDCDRegion *region;
+ dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
+ dc_size = memory_region_size(dc_mr);
+ region_len = DIV_ROUND_UP(dc_size, ct3d->dc.num_regions);
+
+ if (region_len * ct3d->dc.num_regions > dc_size) {
+ error_setg(errp, "host backend size must be multiples of region len");
+ return false;
+ }
+ if (region_len % CXL_CAPACITY_MULTIPLIER != 0) {
+ error_setg(errp, "DC region size is unaligned to %lx",
+ CXL_CAPACITY_MULTIPLIER);
+ return false;
+ }
+ decode_len = region_len;
+
if (ct3d->hostvmem) {
region_base += ct3d->hostvmem->size;
}
@@ -599,6 +659,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
region->flags = 0;
region_base += region->len;
+ ct3d->dc.total_capacity += region->len;
}
return true;
@@ -608,7 +669,8 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
{
DeviceState *ds = DEVICE(ct3d);
- if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem) {
+ if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem
+ && !ct3d->dc.num_regions) {
error_setg(errp, "at least one memdev property must be set");
return false;
} else if (ct3d->hostmem && ct3d->hostpmem) {
@@ -672,9 +734,38 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
g_free(p_name);
}
- if (!cxl_create_dc_regions(ct3d, errp)) {
- error_setg(errp, "setup DC regions failed");
- return false;
+ ct3d->dc.total_capacity = 0;
+ if (ct3d->dc.num_regions) {
+ MemoryRegion *dc_mr;
+ char *dc_name;
+
+ if (!ct3d->dc.host_dc) {
+ error_setg(errp, "dynamic capacity must have a backing device");
+ return false;
+ }
+
+ dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
+ if (!dc_mr) {
+ error_setg(errp, "dynamic capacity must have a backing device");
+ return false;
+ }
+
+ /* FIXME: set dc as volatile for now */
+ memory_region_set_nonvolatile(dc_mr, false);
+ memory_region_set_enabled(dc_mr, true);
+ host_memory_backend_set_mapped(ct3d->dc.host_dc, true);
+ if (ds->id) {
+ dc_name = g_strdup_printf("cxl-dcd-dpa-dc-space:%s", ds->id);
+ } else {
+ dc_name = g_strdup("cxl-dcd-dpa-dc-space");
+ }
+ address_space_init(&ct3d->dc.host_dc_as, dc_mr, dc_name);
+ g_free(dc_name);
+
+ if (!cxl_create_dc_regions(ct3d, errp)) {
+ error_setg(errp, "setup DC regions failed");
+ return false;
+ }
}
return true;
@@ -766,6 +857,9 @@ err_release_cdat:
err_free_special_ops:
g_free(regs->special_ops);
err_address_space_free:
+ if (ct3d->dc.host_dc) {
+ address_space_destroy(&ct3d->dc.host_dc_as);
+ }
if (ct3d->hostpmem) {
address_space_destroy(&ct3d->hostpmem_as);
}
@@ -784,6 +878,9 @@ static void ct3_exit(PCIDevice *pci_dev)
pcie_aer_exit(pci_dev);
cxl_doe_cdat_release(cxl_cstate);
g_free(regs->special_ops);
+ if (ct3d->dc.host_dc) {
+ address_space_destroy(&ct3d->dc.host_dc_as);
+ }
if (ct3d->hostpmem) {
address_space_destroy(&ct3d->hostpmem_as);
}
@@ -862,16 +959,24 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
AddressSpace **as,
uint64_t *dpa_offset)
{
- MemoryRegion *vmr = NULL, *pmr = NULL;
+ MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL;
+ uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0;
if (ct3d->hostvmem) {
vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+ vmr_size = memory_region_size(vmr);
}
if (ct3d->hostpmem) {
pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+ pmr_size = memory_region_size(pmr);
+ }
+ if (ct3d->dc.host_dc) {
+ dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
+ /* Do we want dc_size to be dc_mr->size or not?? */
+ dc_size = ct3d->dc.total_capacity;
}
- if (!vmr && !pmr) {
+ if (!vmr && !pmr && !dc_mr) {
return -ENODEV;
}
@@ -879,19 +984,18 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
return -EINVAL;
}
- if (*dpa_offset > ct3d->cxl_dstate.static_mem_size) {
+ if (*dpa_offset >= vmr_size + pmr_size + dc_size) {
return -EINVAL;
}
- if (vmr) {
- if (*dpa_offset < memory_region_size(vmr)) {
- *as = &ct3d->hostvmem_as;
- } else {
- *as = &ct3d->hostpmem_as;
- *dpa_offset -= memory_region_size(vmr);
- }
- } else {
+ if (*dpa_offset < vmr_size) {
+ *as = &ct3d->hostvmem_as;
+ } else if (*dpa_offset < vmr_size + pmr_size) {
*as = &ct3d->hostpmem_as;
+ *dpa_offset -= vmr_size;
+ } else {
+ *as = &ct3d->dc.host_dc_as;
+ *dpa_offset -= (vmr_size + pmr_size);
}
return 0;
@@ -973,6 +1077,8 @@ static Property ct3_props[] = {
DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL),
DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename),
DEFINE_PROP_UINT8("num-dc-regions", CXLType3Dev, dc.num_regions, 0),
+ DEFINE_PROP_LINK("volatile-dc-memdev", CXLType3Dev, dc.host_dc,
+ TYPE_MEMORY_BACKEND, HostMemoryBackend *),
DEFINE_PROP_END_OF_LIST(),
};
@@ -1039,33 +1145,39 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size,
static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
{
- MemoryRegion *vmr = NULL, *pmr = NULL;
+ MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL;
AddressSpace *as;
+ uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0;
if (ct3d->hostvmem) {
vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+ vmr_size = memory_region_size(vmr);
}
if (ct3d->hostpmem) {
pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+ pmr_size = memory_region_size(pmr);
}
+ if (ct3d->dc.host_dc) {
+ dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
+ dc_size = ct3d->dc.total_capacity;
+ }
- if (!vmr && !pmr) {
+ if (!vmr && !pmr && !dc_mr) {
return false;
}
- if (dpa_offset + CXL_CACHE_LINE_SIZE > ct3d->cxl_dstate.static_mem_size) {
+ if (dpa_offset + CXL_CACHE_LINE_SIZE > vmr_size + pmr_size + dc_size) {
return false;
}
- if (vmr) {
- if (dpa_offset < memory_region_size(vmr)) {
- as = &ct3d->hostvmem_as;
- } else {
- as = &ct3d->hostpmem_as;
- dpa_offset -= memory_region_size(vmr);
- }
- } else {
+ if (dpa_offset < vmr_size) {
+ as = &ct3d->hostvmem_as;
+ } else if (dpa_offset < vmr_size + pmr_size) {
as = &ct3d->hostpmem_as;
+ dpa_offset -= vmr_size;
+ } else {
+ as = &ct3d->dc.host_dc_as;
+ dpa_offset -= (vmr_size + pmr_size);
}
address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, &data,
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 80188db670..2f244da9a1 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -467,6 +467,10 @@ struct CXLType3Dev {
uint64_t poison_list_overflow_ts;
struct dynamic_capacity {
+ HostMemoryBackend *host_dc;
+ AddressSpace host_dc_as;
+ uint64_t total_capacity; /* 256M aligned */
+
uint8_t num_regions; /* 0-8 regions */
CXLDCDRegion regions[DCD_MAX_REGION_NUM];
} dc;
--
2.43.0
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v4 07/10] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
` (5 preceding siblings ...)
2024-02-21 18:15 ` [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions nifan.cxl
@ 2024-02-21 18:16 ` nifan.cxl
2024-02-23 7:16 ` Wonjae Lee
2024-02-26 17:48 ` Jonathan Cameron via
2024-02-21 18:16 ` [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response nifan.cxl
` (5 subsequent siblings)
12 siblings, 2 replies; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:16 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
Add dynamic capacity extent list representative to the definition of
CXLType3Dev and add get DC extent list mailbox command per
CXL.spec.3.1:.8.2.9.9.9.2.
Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 71 +++++++++++++++++++++++++++++++++++++
hw/mem/cxl_type3.c | 1 +
include/hw/cxl/cxl_device.h | 23 ++++++++++++
3 files changed, 95 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index f95e417683..dae7fe00ed 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -83,6 +83,7 @@ enum {
#define CLEAR_POISON 0x2
DCD_CONFIG = 0x48,
#define GET_DC_CONFIG 0x0
+ #define GET_DYN_CAP_EXT_LIST 0x1
PHYSICAL_SWITCH = 0x51,
#define IDENTIFY_SWITCH_DEVICE 0x0
#define GET_PHYSICAL_PORT_STATE 0x1
@@ -1344,6 +1345,73 @@ static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+/*
+ * CXL r3.1 section 8.2.9.9.9.2:
+ * Get Dynamic Capacity Extent List (Opcode 4801h)
+ */
+static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ struct get_dyn_cap_ext_list_in_pl {
+ uint32_t extent_cnt;
+ uint32_t start_extent_id;
+ } QEMU_PACKED;
+
+ struct get_dyn_cap_ext_list_out_pl {
+ uint32_t count;
+ uint32_t total_extents;
+ uint32_t generation_num;
+ uint8_t rsvd[4];
+ CXLDCExtentRaw records[];
+ } QEMU_PACKED;
+
+ struct get_dyn_cap_ext_list_in_pl *in = (void *)payload_in;
+ struct get_dyn_cap_ext_list_out_pl *out = (void *)payload_out;
+ uint16_t record_count = 0, i = 0, record_done = 0;
+ CXLDCExtentList *extent_list = &ct3d->dc.extents;
+ CXLDCExtent *ent;
+ uint16_t out_pl_len;
+ uint32_t start_extent_id = in->start_extent_id;
+
+ if (start_extent_id > ct3d->dc.total_extent_count) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ record_count = MIN(in->extent_cnt,
+ ct3d->dc.total_extent_count - start_extent_id);
+
+ out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
+ assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
+
+ stl_le_p(&out->count, record_count);
+ stl_le_p(&out->total_extents, ct3d->dc.total_extent_count);
+ stl_le_p(&out->generation_num, ct3d->dc.ext_list_gen_seq);
+
+ if (record_count > 0) {
+ QTAILQ_FOREACH(ent, extent_list, node) {
+ if (i++ < start_extent_id) {
+ continue;
+ }
+ stq_le_p(&out->records[record_done].start_dpa, ent->start_dpa);
+ stq_le_p(&out->records[record_done].len, ent->len);
+ memcpy(&out->records[record_done].tag, ent->tag, 0x10);
+ stw_le_p(&out->records[record_done].shared_seq, ent->shared_seq);
+ record_done++;
+ if (record_done == record_count) {
+ break;
+ }
+ }
+ }
+
+ *len_out = out_pl_len;
+ return CXL_MBOX_SUCCESS;
+}
+
#define IMMEDIATE_CONFIG_CHANGE (1 << 1)
#define IMMEDIATE_DATA_CHANGE (1 << 2)
#define IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -1391,6 +1459,9 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
[DCD_CONFIG][GET_DC_CONFIG] = { "DCD_GET_DC_CONFIG",
cmd_dcd_get_dyn_cap_config, 2, 0 },
+ [DCD_CONFIG][GET_DYN_CAP_EXT_LIST] = {
+ "DCD_GET_DYNAMIC_CAPACITY_EXTENT_LIST", cmd_dcd_get_dyn_cap_ext_list,
+ 8, 0 },
};
static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index b966fa4f10..f4edada303 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -661,6 +661,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
region_base += region->len;
ct3d->dc.total_capacity += region->len;
}
+ QTAILQ_INIT(&ct3d->dc.extents);
return true;
}
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 2f244da9a1..12a6fb47a9 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -424,6 +424,25 @@ typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
#define DCD_MAX_REGION_NUM 8
+typedef struct CXLDCExtentRaw {
+ uint64_t start_dpa;
+ uint64_t len;
+ uint8_t tag[0x10];
+ uint16_t shared_seq;
+ uint8_t rsvd[0x6];
+} QEMU_PACKED CXLDCExtentRaw;
+
+typedef struct CXLDCExtent {
+ uint64_t start_dpa;
+ uint64_t len;
+ uint8_t tag[0x10];
+ uint16_t shared_seq;
+ uint8_t rsvd[0x6];
+
+ QTAILQ_ENTRY(CXLDCExtent) node;
+} CXLDCExtent;
+typedef QTAILQ_HEAD(, CXLDCExtent) CXLDCExtentList;
+
typedef struct CXLDCDRegion {
uint64_t base;
uint64_t decode_len; /* aligned to 256*MiB */
@@ -470,6 +489,10 @@ struct CXLType3Dev {
HostMemoryBackend *host_dc;
AddressSpace host_dc_as;
uint64_t total_capacity; /* 256M aligned */
+ CXLDCExtentList extents;
+
+ uint32_t total_extent_count;
+ uint32_t ext_list_gen_seq;
uint8_t num_regions; /* 0-8 regions */
CXLDCDRegion regions[DCD_MAX_REGION_NUM];
--
2.43.0
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
` (6 preceding siblings ...)
2024-02-21 18:16 ` [PATCH v4 07/10] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support nifan.cxl
@ 2024-02-21 18:16 ` nifan.cxl
2024-02-23 9:10 ` Wonjae Lee
2024-02-26 18:04 ` Jonathan Cameron via
2024-02-21 18:16 ` [PATCH v4 09/10] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents nifan.cxl
` (4 subsequent siblings)
12 siblings, 2 replies; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:16 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
Per CXL spec 3.1, two mailbox commands are implemented:
Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 288 ++++++++++++++++++++++++++++++++++++
include/hw/cxl/cxl_device.h | 2 +
2 files changed, 290 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index dae7fe00ed..65ed28f700 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -84,6 +84,8 @@ enum {
DCD_CONFIG = 0x48,
#define GET_DC_CONFIG 0x0
#define GET_DYN_CAP_EXT_LIST 0x1
+ #define ADD_DYN_CAP_RSP 0x2
+ #define RELEASE_DYN_CAP 0x3
PHYSICAL_SWITCH = 0x51,
#define IDENTIFY_SWITCH_DEVICE 0x0
#define GET_PHYSICAL_PORT_STATE 0x1
@@ -1412,6 +1414,286 @@ static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(const struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+/*
+ * Check whether any bit between addr[nr, nr+size) is set,
+ * return true if any bit is set, otherwise return false
+ */
+static bool test_any_bits_set(const unsigned long *addr, int nr, int size)
+{
+ unsigned long res = find_next_bit(addr, size + nr, nr);
+
+ return res < nr + size;
+}
+
+CXLDCDRegion *cxl_find_dc_region(CXLType3Dev *ct3d, uint64_t dpa, uint64_t len)
+{
+ int i;
+ CXLDCDRegion *region = &ct3d->dc.regions[0];
+
+ if (dpa < region->base ||
+ dpa >= region->base + ct3d->dc.total_capacity) {
+ return NULL;
+ }
+
+ /*
+ * CXL r3.1 section 9.13.3: Dynamic Capacity Device (DCD)
+ *
+ * Regions are used in increasing-DPA order, with Region 0 being used for
+ * the lowest DPA of Dynamic Capacity and Region 7 for the highest DPA.
+ * So check from the last region to find where the dpa belongs. Extents that
+ * cross multiple regions are not allowed.
+ */
+ for (i = ct3d->dc.num_regions - 1; i >= 0; i--) {
+ region = &ct3d->dc.regions[i];
+ if (dpa >= region->base) {
+ if (dpa + len > region->base + region->len) {
+ return NULL;
+ }
+ return region;
+ }
+ }
+
+ return NULL;
+}
+
+static void cxl_insert_extent_to_extent_list(CXLDCExtentList *list,
+ uint64_t dpa,
+ uint64_t len,
+ uint8_t *tag,
+ uint16_t shared_seq)
+{
+ CXLDCExtent *extent;
+
+ extent = g_new0(CXLDCExtent, 1);
+ extent->start_dpa = dpa;
+ extent->len = len;
+ if (tag) {
+ memcpy(extent->tag, tag, 0x10);
+ }
+ extent->shared_seq = shared_seq;
+
+ QTAILQ_INSERT_TAIL(list, extent, node);
+}
+
+static void cxl_remove_extent_from_extent_list(CXLDCExtentList *list,
+ CXLDCExtent *extent)
+{
+ QTAILQ_REMOVE(list, extent, node);
+ g_free(extent);
+}
+
+/*
+ * CXL r3.1 Table 8-168: Add Dynamic Capacity Response Input Payload
+ * CXL r3.1 Table 8-170: Release Dynamic Capacity Input Payload
+ */
+typedef struct CXLUpdateDCExtentListInPl {
+ uint32_t num_entries_updated;
+ uint8_t flags;
+ uint8_t rsvd[3];
+ /* CXL r3.1 Table 8-169: Updated Extent List */
+ struct {
+ uint64_t start_dpa;
+ uint64_t len;
+ uint8_t rsvd[8];
+ } QEMU_PACKED updated_entries[];
+} QEMU_PACKED CXLUpdateDCExtentListInPl;
+
+/*
+ * For the extents in the extent list to operate, check whether they are valid
+ * 1. The extent should be in the range of a valid DC region;
+ * 2. The extent should not cross multiple regions;
+ * 3. The start DPA and the length of the extent should align with the block
+ * size of the region;
+ * 4. The address range of multiple extents in the list should not overlap.
+ */
+static CXLRetCode cxl_detect_malformed_extent_list(CXLType3Dev *ct3d,
+ const CXLUpdateDCExtentListInPl *in)
+{
+ uint64_t min_block_size = UINT64_MAX;
+ CXLDCDRegion *region = &ct3d->dc.regions[0];
+ CXLDCDRegion *lastregion = &ct3d->dc.regions[ct3d->dc.num_regions - 1];
+ g_autofree unsigned long *blk_bitmap = NULL;
+ uint64_t dpa, len;
+ uint32_t i;
+
+ for (i = 0; i < ct3d->dc.num_regions; i++) {
+ region = &ct3d->dc.regions[i];
+ min_block_size = MIN(min_block_size, region->block_size);
+ }
+
+ blk_bitmap = bitmap_new((lastregion->base + lastregion->len -
+ ct3d->dc.regions[0].base) / min_block_size);
+
+ for (i = 0; i < in->num_entries_updated; i++) {
+ dpa = in->updated_entries[i].start_dpa;
+ len = in->updated_entries[i].len;
+
+ region = cxl_find_dc_region(ct3d, dpa, len);
+ if (!region) {
+ return CXL_MBOX_INVALID_PA;
+ }
+
+ dpa -= ct3d->dc.regions[0].base;
+ if (dpa % region->block_size || len % region->block_size) {
+ return CXL_MBOX_INVALID_EXTENT_LIST;
+ }
+ /* the dpa range already covered by some other extents in the list */
+ if (test_any_bits_set(blk_bitmap, dpa / min_block_size,
+ len / min_block_size)) {
+ return CXL_MBOX_INVALID_EXTENT_LIST;
+ }
+ bitmap_set(blk_bitmap, dpa / min_block_size, len / min_block_size);
+ }
+
+ return CXL_MBOX_SUCCESS;
+}
+
+/*
+ * CXL r3.1 section 8.2.9.9.9.3: Add Dynamic Capacity Response (opcode 4802h)
+ * An extent is added to the extent list and becomes usable only after the
+ * response is processed successfully
+ */
+static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ CXLUpdateDCExtentListInPl *in = (void *)payload_in;
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ CXLDCExtentList *extent_list = &ct3d->dc.extents;
+ CXLDCExtent *ent;
+ uint32_t i;
+ uint64_t dpa, len;
+ CXLRetCode ret;
+
+ if (in->num_entries_updated == 0) {
+ return CXL_MBOX_SUCCESS;
+ }
+
+ ret = cxl_detect_malformed_extent_list(ct3d, in);
+ if (ret != CXL_MBOX_SUCCESS) {
+ return ret;
+ }
+
+ for (i = 0; i < in->num_entries_updated; i++) {
+ dpa = in->updated_entries[i].start_dpa;
+ len = in->updated_entries[i].len;
+
+ /*
+ * Check if the DPA range of the to-be-added extent overlaps with
+ * existing extent list maintained by the device.
+ */
+ QTAILQ_FOREACH(ent, extent_list, node) {
+ if (ent->start_dpa <= dpa &&
+ dpa + len <= ent->start_dpa + ent->len) {
+ return CXL_MBOX_INVALID_PA;
+ /* Overlapping one end of the other */
+ } else if ((dpa < ent->start_dpa + ent->len &&
+ dpa + len > ent->start_dpa + ent->len) ||
+ (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) {
+ return CXL_MBOX_INVALID_PA;
+ }
+ }
+
+ /*
+ * TODO: we will add a pending extent list based on event log record
+ * and verify the input response; also, the "More" flag is not
+ * considered at the moment.
+ */
+
+ cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
+ ct3d->dc.total_extent_count += 1;
+ }
+
+ return CXL_MBOX_SUCCESS;
+}
+
+/*
+ * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (opcode 4803h)
+ */
+static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ CXLUpdateDCExtentListInPl *in = (void *)payload_in;
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+ CXLDCExtentList *extent_list = &ct3d->dc.extents;
+ CXLDCExtent *ent;
+ uint32_t i;
+ uint64_t dpa, len;
+ CXLRetCode ret;
+
+ if (in->num_entries_updated == 0) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ ret = cxl_detect_malformed_extent_list(ct3d, in);
+ if (ret != CXL_MBOX_SUCCESS) {
+ return ret;
+ }
+
+ for (i = 0; i < in->num_entries_updated; i++) {
+ bool found = false;
+
+ dpa = in->updated_entries[i].start_dpa;
+ len = in->updated_entries[i].len;
+
+ QTAILQ_FOREACH(ent, extent_list, node) {
+ if (ent->start_dpa <= dpa &&
+ dpa + len <= ent->start_dpa + ent->len) {
+ /*
+ * If an incoming extent covers a portion of an extent
+ * in the device extent list, remove only the overlapping
+ * portion, meaning
+ * 1. the portions that are not covered by the incoming
+ * extent at both end of the original extent will become
+ * new extents and inserted to the extent list; and
+ * 2. the original extent is removed from the extent list;
+ * 3. dc extent count is updated accordingly.
+ */
+ uint64_t ent_start_dpa = ent->start_dpa;
+ uint64_t ent_len = ent->len;
+ uint64_t len1 = dpa - ent_start_dpa;
+ uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
+
+ found = true;
+ cxl_remove_extent_from_extent_list(extent_list, ent);
+ ct3d->dc.total_extent_count -= 1;
+
+ if (len1) {
+ cxl_insert_extent_to_extent_list(extent_list,
+ ent_start_dpa, len1,
+ NULL, 0);
+ ct3d->dc.total_extent_count += 1;
+ }
+ if (len2) {
+ cxl_insert_extent_to_extent_list(extent_list, dpa + len,
+ len2, NULL, 0);
+ ct3d->dc.total_extent_count += 1;
+ }
+ break;
+ /*Currently we reject the attempt to remove a superset*/
+ } else if ((dpa < ent->start_dpa + ent->len &&
+ dpa + len > ent->start_dpa + ent->len) ||
+ (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) {
+ return CXL_MBOX_INVALID_EXTENT_LIST;
+ }
+ }
+
+ if (!found) {
+ /* Try to remove a non-existing extent */
+ return CXL_MBOX_INVALID_PA;
+ }
+ }
+
+ return CXL_MBOX_SUCCESS;
+}
+
#define IMMEDIATE_CONFIG_CHANGE (1 << 1)
#define IMMEDIATE_DATA_CHANGE (1 << 2)
#define IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -1462,6 +1744,12 @@ static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
[DCD_CONFIG][GET_DYN_CAP_EXT_LIST] = {
"DCD_GET_DYNAMIC_CAPACITY_EXTENT_LIST", cmd_dcd_get_dyn_cap_ext_list,
8, 0 },
+ [DCD_CONFIG][ADD_DYN_CAP_RSP] = {
+ "ADD_DCD_DYNAMIC_CAPACITY_RESPONSE", cmd_dcd_add_dyn_cap_rsp,
+ ~0, IMMEDIATE_DATA_CHANGE },
+ [DCD_CONFIG][RELEASE_DYN_CAP] = {
+ "RELEASE_DCD_DYNAMIC_CAPACITY", cmd_dcd_release_dyn_cap,
+ ~0, IMMEDIATE_DATA_CHANGE },
};
static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 12a6fb47a9..6178416cbb 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -548,4 +548,6 @@ void cxl_event_irq_assert(CXLType3Dev *ct3d);
void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d);
+CXLDCDRegion *cxl_find_dc_region(CXLType3Dev *ct3d, uint64_t dpa, uint64_t len);
+
#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v4 09/10] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
` (7 preceding siblings ...)
2024-02-21 18:16 ` [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response nifan.cxl
@ 2024-02-21 18:16 ` nifan.cxl
2024-02-23 12:16 ` Wonjae Lee
2024-02-26 18:10 ` Jonathan Cameron via
2024-02-21 18:16 ` [PATCH v4 10/10] hw/mem/cxl_type3: Add dpa range validation for accesses to DC regions nifan.cxl
` (3 subsequent siblings)
12 siblings, 2 replies; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:16 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
Since fabric manager emulation is not supported yet, the change implements
the functions to add/release dynamic capacity extents as QMP interfaces.
Note: we skips any FM issued extent release request if the exact extent
does not exist in the extent list of the device. We will loose the
restriction later once we have partial release support in the kernel.
1. Add dynamic capacity extents:
For example, the command to add two continuous extents (each 128MiB long)
to region 0 (starting at DPA offset 0) looks like below:
{ "execute": "qmp_capabilities" }
{ "execute": "cxl-add-dynamic-capacity",
"arguments": {
"path": "/machine/peripheral/cxl-dcd0",
"region-id": 0,
"extents": [
{
"dpa": 0,
"len": 134217728
},
{
"dpa": 134217728,
"len": 134217728
}
]
}
}
2. Release dynamic capacity extents:
For example, the command to release an extent of size 128MiB from region 0
(DPA offset 128MiB) look like below:
{ "execute": "cxl-release-dynamic-capacity",
"arguments": {
"path": "/machine/peripheral/cxl-dcd0",
"region-id": 0,
"extents": [
{
"dpa": 134217728,
"len": 134217728
}
]
}
}
Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 26 +++-
hw/mem/cxl_type3.c | 249 +++++++++++++++++++++++++++++++++++-
hw/mem/cxl_type3_stubs.c | 14 ++
include/hw/cxl/cxl_device.h | 7 +
include/hw/cxl/cxl_events.h | 18 +++
qapi/cxl.json | 61 ++++++++-
6 files changed, 366 insertions(+), 9 deletions(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 65ed28f700..34c4ebbd12 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1418,7 +1418,7 @@ static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(const struct cxl_cmd *cmd,
* Check whether any bit between addr[nr, nr+size) is set,
* return true if any bit is set, otherwise return false
*/
-static bool test_any_bits_set(const unsigned long *addr, int nr, int size)
+bool test_any_bits_set(const unsigned long *addr, int nr, int size)
{
unsigned long res = find_next_bit(addr, size + nr, nr);
@@ -1456,7 +1456,7 @@ CXLDCDRegion *cxl_find_dc_region(CXLType3Dev *ct3d, uint64_t dpa, uint64_t len)
return NULL;
}
-static void cxl_insert_extent_to_extent_list(CXLDCExtentList *list,
+void cxl_insert_extent_to_extent_list(CXLDCExtentList *list,
uint64_t dpa,
uint64_t len,
uint8_t *tag,
@@ -1597,16 +1597,28 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
}
}
- /*
- * TODO: we will add a pending extent list based on event log record
- * and verify the input response; also, the "More" flag is not
- * considered at the moment.
- */
+ QTAILQ_FOREACH(ent, &ct3d->dc.extents_pending_to_add, node) {
+ if (ent->start_dpa <= dpa &&
+ dpa + len <= ent->start_dpa + ent->len) {
+ break;
+ }
+ }
+ if (!ent) {
+ return CXL_MBOX_INVALID_PA;
+ }
+
+ cxl_remove_extent_from_extent_list(&ct3d->dc.extents_pending_to_add,
+ ent);
cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
ct3d->dc.total_extent_count += 1;
}
+ /*
+ * TODO: extents_pending_to_add needs to be cleared so the extents not
+ * accepted can be reclaimed base on spec r3.1: 8.2.9.9.9.3
+ */
+
return CXL_MBOX_SUCCESS;
}
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index f4edada303..b8c4273e99 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -662,6 +662,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
ct3d->dc.total_capacity += region->len;
}
QTAILQ_INIT(&ct3d->dc.extents);
+ QTAILQ_INIT(&ct3d->dc.extents_pending_to_add);
return true;
}
@@ -1424,7 +1425,8 @@ static int ct3d_qmp_cxl_event_log_enc(CxlEventLog log)
return CXL_EVENT_TYPE_FAIL;
case CXL_EVENT_LOG_FATAL:
return CXL_EVENT_TYPE_FATAL;
-/* DCD not yet supported */
+ case CXL_EVENT_LOG_DYNCAP:
+ return CXL_EVENT_TYPE_DYNAMIC_CAP;
default:
return -EINVAL;
}
@@ -1675,6 +1677,251 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
}
}
+/* CXL r3.1 Table 8-50: Dynanic 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 exact extent exists in the list
+ * Return value: the extent pointer in the list; else null
+ */
+static CXLDCExtent *cxl_dc_extent_exists(CXLDCExtentList *list,
+ CXLDCExtentRaw *ext)
+{
+ CXLDCExtent *ent;
+
+ if (!ext || !list) {
+ return NULL;
+ }
+
+ QTAILQ_FOREACH(ent, list, node) {
+ if (ent->start_dpa != ext->start_dpa) {
+ continue;
+ }
+
+ /*Found exact extent*/
+ return ent->len == ext->len ? ent : NULL;
+ }
+
+ return NULL;
+}
+
+/*
+ * The main function to process dynamic capacity event. Currently DC extents
+ * add/release requests are processed.
+ */
+static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
+ CXLDCEventType type, uint16_t hid,
+ uint8_t rid,
+ CXLDCExtentRecordList *records,
+ Error **errp)
+{
+ Object *obj;
+ CXLEventDynamicCapacity dCap = {};
+ CXLEventRecordHdr *hdr = &dCap.hdr;
+ CXLType3Dev *dcd;
+ uint8_t flags = 1 << CXL_EVENT_TYPE_INFO;
+ uint32_t num_extents = 0;
+ CXLDCExtentRecordList *list;
+ g_autofree CXLDCExtentRaw *extents = NULL;
+ uint8_t enc_log;
+ uint64_t offset, len, block_size;
+ int i;
+ int rc;
+ g_autofree unsigned long *blk_bitmap = NULL;
+
+ obj = object_resolve_path(path, NULL);
+ if (!obj) {
+ error_setg(errp, "Unable to resolve path");
+ return;
+ }
+ if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
+ error_setg(errp, "Path not point to a valid CXL type3 device");
+ return;
+ }
+
+ dcd = CXL_TYPE3(obj);
+ if (!dcd->dc.num_regions) {
+ error_setg(errp, "No dynamic capacity support from the device");
+ return;
+ }
+
+ rc = ct3d_qmp_cxl_event_log_enc(log);
+ if (rc < 0) {
+ error_setg(errp, "Unhandled error log type");
+ return;
+ }
+ enc_log = rc;
+
+ if (rid >= dcd->dc.num_regions) {
+ error_setg(errp, "region id is too large");
+ return;
+ }
+ block_size = dcd->dc.regions[rid].block_size;
+
+ /* Sanity check and count the extents */
+ list = records;
+ while (list) {
+ offset = list->value->offset;
+ len = list->value->len;
+
+ if (len == 0) {
+ error_setg(errp, "extent with 0 length is not allowed");
+ return;
+ }
+
+ if (offset % block_size || len % block_size) {
+ error_setg(errp, "dpa or len is not aligned to region block size");
+ return;
+ }
+
+ if (offset + len > dcd->dc.regions[rid].len) {
+ error_setg(errp, "extent range is beyond the region end");
+ return;
+ }
+
+ num_extents++;
+ list = list->next;
+ }
+ if (num_extents == 0) {
+ error_setg(errp, "No extents found in the command");
+ return;
+ }
+
+ blk_bitmap = bitmap_new(dcd->dc.regions[rid].len / block_size);
+
+ /* Create Extent list for event being passed to host */
+ i = 0;
+ list = records;
+ extents = g_new0(CXLDCExtentRaw, num_extents);
+ while (list) {
+ CXLDCExtent *ent;
+ bool skip_extent = false;
+
+ offset = list->value->offset;
+ len = list->value->len;
+
+ extents[i].start_dpa = offset + dcd->dc.regions[rid].base;
+ extents[i].len = len;
+ memset(extents[i].tag, 0, 0x10);
+ extents[i].shared_seq = 0;
+
+ if (type == DC_EVENT_RELEASE_CAPACITY ||
+ type == DC_EVENT_FORCED_RELEASE_CAPACITY) {
+ /*
+ * if the extent is still pending to be added to the host,
+ * remove it from the pending extent list, so later when the add
+ * response for the extent arrives, the device can reject the
+ * extent as it is not in the pending list.
+ */
+ ent = cxl_dc_extent_exists(&dcd->dc.extents_pending_to_add,
+ &extents[i]);
+ if (ent) {
+ QTAILQ_REMOVE(&dcd->dc.extents_pending_to_add, ent, node);
+ g_free(ent);
+ skip_extent = true;
+ } else if (!cxl_dc_extent_exists(&dcd->dc.extents, &extents[i])) {
+ /* If the exact extent is not in the accepted list, skip */
+ skip_extent = true;
+ }
+ }
+
+ /* No duplicate or overlapped extents are allowed */
+ if (test_any_bits_set(blk_bitmap, offset / block_size,
+ len / block_size)) {
+ error_setg(errp, "duplicate or overlapped extents are detected");
+ return;
+ }
+ bitmap_set(blk_bitmap, offset / block_size, len / block_size);
+
+ list = list->next;
+ if (!skip_extent) {
+ i++;
+ }
+ }
+ num_extents = i;
+
+ switch (type) {
+ case DC_EVENT_ADD_CAPACITY:
+ break;
+ default:
+ break;
+ }
+ /*
+ * CXL r3.1 section 8.2.9.2.1.6: Dynamic Capacity Event Record
+ *
+ * All Dynamic Capacity event records shall set the Event Record Severity
+ * field in the Common Event Record Format to Informational Event. All
+ * Dynamic Capacity related events shall be logged in the Dynamic Capacity
+ * Event Log.
+ */
+ cxl_assign_event_header(hdr, &dynamic_capacity_uuid, flags, sizeof(dCap),
+ cxl_device_get_timestamp(&dcd->cxl_dstate));
+
+ dCap.type = type;
+ /* FIXME: for now, validaity flag is cleared */
+ dCap.validity_flags = 0;
+ stw_le_p(&dCap.host_id, hid);
+ /* only valid for DC_REGION_CONFIG_UPDATED event */
+ dCap.updated_region_id = 0;
+ /*
+ * FIXME: for now, "More" flag is cleared as there is only one extent for
+ * each record
+ */
+ dCap.flags = 0;
+
+ /*
+ * For current implementation, each DC event record only associates with
+ * one extent, so the "More" flag does not need to be set.
+ */
+ for (i = 0; i < num_extents; i++) {
+ memcpy(&dCap.dynamic_capacity_extent, &extents[i],
+ sizeof(CXLDCExtentRaw));
+
+ if (type == DC_EVENT_ADD_CAPACITY) {
+ cxl_insert_extent_to_extent_list(&dcd->dc.extents_pending_to_add,
+ extents[i].start_dpa,
+ extents[i].len,
+ extents[i].tag,
+ extents[i].shared_seq);
+ }
+
+ if (cxl_event_insert(&dcd->cxl_dstate, enc_log,
+ (CXLEventRecordRaw *)&dCap)) {
+ cxl_event_irq_assert(dcd);
+ }
+ }
+}
+
+void qmp_cxl_add_dynamic_capacity(const char *path, uint8_t region_id,
+ CXLDCExtentRecordList *records,
+ Error **errp)
+{
+ qmp_cxl_process_dynamic_capacity(path, CXL_EVENT_LOG_DYNCAP,
+ DC_EVENT_ADD_CAPACITY, 0,
+ region_id, records, errp);
+}
+
+void qmp_cxl_release_dynamic_capacity(const char *path, uint8_t region_id,
+ CXLDCExtentRecordList *records,
+ Error **errp)
+{
+ qmp_cxl_process_dynamic_capacity(path, CXL_EVENT_LOG_DYNCAP,
+ DC_EVENT_RELEASE_CAPACITY, 0,
+ region_id, records, errp);
+}
+
static void ct3_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
index 3e1851e32b..d913b11b4d 100644
--- a/hw/mem/cxl_type3_stubs.c
+++ b/hw/mem/cxl_type3_stubs.c
@@ -67,3 +67,17 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
{
error_setg(errp, "CXL Type 3 support is not compiled in");
}
+
+void qmp_cxl_add_dynamic_capacity(const char *path, uint8_t region_id,
+ CXLDCExtentRecordList *records,
+ Error **errp)
+{
+ error_setg(errp, "CXL Type 3 support is not compiled in");
+}
+
+void qmp_cxl_release_dynamic_capacity(const char *path, uint8_t region_id,
+ CXLDCExtentRecordList *records,
+ Error **errp)
+{
+ error_setg(errp, "CXL Type 3 support is not compiled in");
+}
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 6178416cbb..1d31164bd3 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -490,6 +490,7 @@ struct CXLType3Dev {
AddressSpace host_dc_as;
uint64_t total_capacity; /* 256M aligned */
CXLDCExtentList extents;
+ CXLDCExtentList extents_pending_to_add;
uint32_t total_extent_count;
uint32_t ext_list_gen_seq;
@@ -550,4 +551,10 @@ void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d);
CXLDCDRegion *cxl_find_dc_region(CXLType3Dev *ct3d, uint64_t dpa, uint64_t len);
+void cxl_insert_extent_to_extent_list(CXLDCExtentList *list,
+ uint64_t dpa,
+ uint64_t len,
+ uint8_t *tag,
+ uint16_t shared_seq);
+bool test_any_bits_set(const unsigned long *addr, int nr, int size);
#endif
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index 5170b8dbf8..38cadaa0f3 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -166,4 +166,22 @@ typedef struct CXLEventMemoryModule {
uint8_t reserved[0x3d];
} QEMU_PACKED CXLEventMemoryModule;
+/*
+ * CXL r3.1 section Table 8-50: Dynamic Capacity Event Record
+ * All fields little endian.
+ */
+typedef struct CXLEventDynamicCapacity {
+ CXLEventRecordHdr hdr;
+ uint8_t type;
+ uint8_t validity_flags;
+ uint16_t host_id;
+ uint8_t updated_region_id;
+ uint8_t flags;
+ uint8_t reserved2[2];
+ uint8_t dynamic_capacity_extent[0x28]; /* defined in cxl_device.h */
+ uint8_t reserved[0x18];
+ uint32_t extents_avail;
+ uint32_t tags_avail;
+} QEMU_PACKED CXLEventDynamicCapacity;
+
#endif /* CXL_EVENTS_H */
diff --git a/qapi/cxl.json b/qapi/cxl.json
index 8cc4c72fa9..19927629c7 100644
--- a/qapi/cxl.json
+++ b/qapi/cxl.json
@@ -19,13 +19,16 @@
#
# @fatal: Fatal Event Log
#
+# @dyncap: Dynamic Capacity Event Log
+#
# Since: 8.1
##
{ 'enum': 'CxlEventLog',
'data': ['informational',
'warning',
'failure',
- 'fatal']
+ 'fatal',
+ 'dyncap']
}
##
@@ -361,3 +364,59 @@
##
{'command': 'cxl-inject-correctable-error',
'data': {'path': 'str', 'type': 'CxlCorErrorType'}}
+
+##
+# @CXLDCExtentRecord:
+#
+# Record of a single extent to add/release
+#
+# @offset: offset to the start of the region where the extent to be operated
+# @len: length of the extent
+#
+# Since: 9.0
+##
+{ 'struct': 'CXLDCExtentRecord',
+ 'data': {
+ 'offset':'uint64',
+ 'len': 'uint64'
+ }
+}
+
+##
+# @cxl-add-dynamic-capacity:
+#
+# Command to start add dynamic capacity extents flow. The device will
+# have to ackowledged the acceptance of the extents before they are usable.
+#
+# @path: CXL DCD canonical QOM path
+# @region-id: id of the region where the extent to add/release
+# @extents: Extents to add
+#
+# Since : 9.0
+##
+{ 'command': 'cxl-add-dynamic-capacity',
+ 'data': { 'path': 'str',
+ 'region-id': 'uint8',
+ 'extents': [ 'CXLDCExtentRecord' ]
+ }
+}
+
+##
+# @cxl-release-dynamic-capacity:
+#
+# Command to start release dynamic capacity extents flow. The host will
+# need to respond to indicate that it has released the capacity before it
+# is made unavailable for read and write and can be re-added.
+#
+# @path: CXL DCD canonical QOM path
+# @region-id: id of the region where the extent to add/release
+# @extents: Extents to release
+#
+# Since : 9.0
+##
+{ 'command': 'cxl-release-dynamic-capacity',
+ 'data': { 'path': 'str',
+ 'region-id': 'uint8',
+ 'extents': [ 'CXLDCExtentRecord' ]
+ }
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v4 10/10] hw/mem/cxl_type3: Add dpa range validation for accesses to DC regions
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
` (8 preceding siblings ...)
2024-02-21 18:16 ` [PATCH v4 09/10] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents nifan.cxl
@ 2024-02-21 18:16 ` nifan.cxl
[not found] ` <CGME20240221182126epcas2p1b684f9239e4262f17ff484939658a382@epcms2p1>
` (2 subsequent siblings)
12 siblings, 0 replies; 44+ messages in thread
From: nifan.cxl @ 2024-02-21 18:16 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, nifan.cxl,
jim.harris, Fan Ni
From: Fan Ni <fan.ni@samsung.com>
Not all dpa range in the DC regions is valid to access until an extent
covering the range has been added. Add a bitmap for each region to
record whether a DC block in the region has been backed by DC extent.
For the bitmap, a bit in the bitmap represents a DC block. When a DC
extent is added, all the bits of the blocks in the extent will be set,
which will be cleared when the extent is released.
Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
hw/cxl/cxl-mailbox-utils.c | 3 ++
hw/mem/cxl_type3.c | 82 +++++++++++++++++++++++++++++++++++++
include/hw/cxl/cxl_device.h | 7 ++++
3 files changed, 92 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 34c4ebbd12..fd3be2f9cf 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1676,17 +1676,20 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
found = true;
cxl_remove_extent_from_extent_list(extent_list, ent);
ct3d->dc.total_extent_count -= 1;
+ ct3_clear_region_block_backed(ct3d, ent_start_dpa, ent_len);
if (len1) {
cxl_insert_extent_to_extent_list(extent_list,
ent_start_dpa, len1,
NULL, 0);
ct3d->dc.total_extent_count += 1;
+ ct3_set_region_block_backed(ct3d, ent_start_dpa, len1);
}
if (len2) {
cxl_insert_extent_to_extent_list(extent_list, dpa + len,
len2, NULL, 0);
ct3d->dc.total_extent_count += 1;
+ ct3_set_region_block_backed(ct3d, dpa + len, len2);
}
break;
/*Currently we reject the attempt to remove a superset*/
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index b8c4273e99..a56906db25 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -660,6 +660,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
region_base += region->len;
ct3d->dc.total_capacity += region->len;
+ region->blk_bitmap = bitmap_new(region->len / region->block_size);
}
QTAILQ_INIT(&ct3d->dc.extents);
QTAILQ_INIT(&ct3d->dc.extents_pending_to_add);
@@ -667,6 +668,17 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
return true;
}
+static void cxl_destroy_dc_regions(CXLType3Dev *ct3d)
+{
+ int i;
+ struct CXLDCDRegion *region;
+
+ for (i = 0; i < ct3d->dc.num_regions; i++) {
+ region = &ct3d->dc.regions[i];
+ g_free(region->blk_bitmap);
+ }
+}
+
static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
{
DeviceState *ds = DEVICE(ct3d);
@@ -860,6 +872,7 @@ err_free_special_ops:
g_free(regs->special_ops);
err_address_space_free:
if (ct3d->dc.host_dc) {
+ cxl_destroy_dc_regions(ct3d);
address_space_destroy(&ct3d->dc.host_dc_as);
}
if (ct3d->hostpmem) {
@@ -881,6 +894,7 @@ static void ct3_exit(PCIDevice *pci_dev)
cxl_doe_cdat_release(cxl_cstate);
g_free(regs->special_ops);
if (ct3d->dc.host_dc) {
+ cxl_destroy_dc_regions(ct3d);
address_space_destroy(&ct3d->dc.host_dc_as);
}
if (ct3d->hostpmem) {
@@ -891,6 +905,70 @@ static void ct3_exit(PCIDevice *pci_dev)
}
}
+/*
+ * Mark the DPA range [dpa, dap + len) to be backed and accessible. This
+ * happens when a DC extent is added and accepted by the host.
+ */
+void ct3_set_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+ uint64_t len)
+{
+ CXLDCDRegion *region;
+
+ region = cxl_find_dc_region(ct3d, dpa, len);
+ if (!region) {
+ return;
+ }
+
+ bitmap_set(region->blk_bitmap, (dpa - region->base) / region->block_size,
+ len / region->block_size);
+}
+
+/*
+ * Check whether the DPA range [dpa, dpa + len) is backed with DC extents.
+ * Used when validating read/write to dc regions
+ */
+bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+ uint64_t len)
+{
+ CXLDCDRegion *region;
+ uint64_t nbits;
+ long nr;
+
+ region = cxl_find_dc_region(ct3d, dpa, len);
+ if (!region) {
+ return false;
+ }
+
+ nr = (dpa - region->base) / region->block_size;
+ nbits = DIV_ROUND_UP(len, region->block_size);
+ /*
+ * if bits between [dpa, dpa + len) are all 1s, meaning the DPA range is
+ * backed with DC extents, return true; else return false.
+ */
+ return find_next_zero_bit(region->blk_bitmap, nr + nbits, nr) == nr + nbits;
+}
+
+/*
+ * Mark the DPA range [dpa, dap + len) to be unbacked and inaccessible. This
+ * happens when a dc extent is released by the host.
+ */
+void ct3_clear_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+ uint64_t len)
+{
+ CXLDCDRegion *region;
+ uint64_t nbits;
+ long nr;
+
+ region = cxl_find_dc_region(ct3d, dpa, len);
+ if (!region) {
+ return;
+ }
+
+ nr = (dpa - region->base) / region->block_size;
+ nbits = len / region->block_size;
+ bitmap_clear(region->blk_bitmap, nr, nbits);
+}
+
static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
{
int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
@@ -996,6 +1074,10 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
*as = &ct3d->hostpmem_as;
*dpa_offset -= vmr_size;
} else {
+ if (!ct3_test_region_block_backed(ct3d, *dpa_offset, size)) {
+ return -ENODEV;
+ }
+
*as = &ct3d->dc.host_dc_as;
*dpa_offset -= (vmr_size + pmr_size);
}
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 1d31164bd3..10f0389b50 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -450,6 +450,7 @@ typedef struct CXLDCDRegion {
uint64_t block_size;
uint32_t dsmadhandle;
uint8_t flags;
+ unsigned long *blk_bitmap;
} CXLDCDRegion;
struct CXLType3Dev {
@@ -557,4 +558,10 @@ void cxl_insert_extent_to_extent_list(CXLDCExtentList *list,
uint8_t *tag,
uint16_t shared_seq);
bool test_any_bits_set(const unsigned long *addr, int nr, int size);
+void ct3_set_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+ uint64_t len);
+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);
#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 44+ messages in thread
* RE: [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support
[not found] ` <CGME20240221182126epcas2p1b684f9239e4262f17ff484939658a382@epcms2p1>
@ 2024-02-22 7:45 ` Wonjae Lee
2024-02-22 16:54 ` fan
0 siblings, 1 reply; 44+ messages in thread
From: Wonjae Lee @ 2024-02-22 7:45 UTC (permalink / raw)
To: nifan.cxl, qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, Adam Manzanares, dave, nmtadam.samsung,
Jim Harris, Fan Ni, KyungSan Kim, Hojin Nam
On Wed, Feb 21, 2024 at 10:15:55AM -0800, nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Per cxl spec r3.1, add dynamic capacity region representative based on
> Table 8-165 and extend the cxl type3 device definition to include dc region
> information. Also, based on info in 8.2.9.9.9.1, add 'Get Dynamic Capacity
> Configuration' mailbox support.
>
> Note: decode_len of a dc region is aligned to 256*MiB, divided by
> 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> mailbox command.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 110 ++++++++++++++++++++++++++++++++++++
> include/hw/cxl/cxl_device.h | 16 ++++++
> 2 files changed, 126 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index ba1d9901df..88e3b733e3 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -22,6 +22,7 @@
>
[snip]
> +#ifdef CXL_SPEC_AFTER_R30
> + /*FIXME: need to set valid values in the future*/
> + stq_le_p(&extra_out->num_extents_supported, 0);
> + stq_le_p(&extra_out->num_extents_available, 0);
> + stq_le_p(&extra_out->num_tags_supported, 0);
> + stq_le_p(&extra_out->num_tags_available, 0);
Hello,
It's a trivial comment and doesn't have any impact yet, but shouldn't it
be stl_le_p()?
Thanks,
Wonjae
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions
[not found] ` <CGME20240221182137epcas2p276d22514caaa9412d0119ded6f9a63d4@epcms2p3>
@ 2024-02-22 9:22 ` Wonjae Lee
2024-02-22 16:56 ` fan
0 siblings, 1 reply; 44+ messages in thread
From: Wonjae Lee @ 2024-02-22 9:22 UTC (permalink / raw)
To: nifan.cxl, qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, Adam Manzanares, dave, nmtadam.samsung,
Jim Harris, Fan Ni
On Wed, Feb 21, 2024 at 10:15:59AM -0800, nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Add (file/memory backed) host backend, all the dynamic capacity regions
> will share a single, large enough host backend. Set up address space for
> DC regions to support read/write operations to dynamic capacity for DCD.
>
> With the change, following supports are added:
> 1. Add a new property to type3 device "volatile-dc-memdev" to point to host
> memory backend for dynamic capacity. Currently, all dc regions share one
> one host backend.
Hello, I know that it's too minor comment, but 'one' was used twice.
Thanks,
Wonjae
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support
2024-02-22 7:45 ` [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support Wonjae Lee
@ 2024-02-22 16:54 ` fan
0 siblings, 0 replies; 44+ messages in thread
From: fan @ 2024-02-22 16:54 UTC (permalink / raw)
To: Wonjae Lee
Cc: nifan.cxl, qemu-devel, jonathan.cameron, linux-cxl,
gregory.price, ira.weiny, dan.j.williams, Adam Manzanares, dave,
nmtadam.samsung, Jim Harris, Fan Ni, KyungSan Kim, Hojin Nam
On Thu, Feb 22, 2024 at 04:45:25PM +0900, Wonjae Lee wrote:
> On Wed, Feb 21, 2024 at 10:15:55AM -0800, nifan.cxl@gmail.com wrote:
> > From: Fan Ni <fan.ni@samsung.com>
> >
> > Per cxl spec r3.1, add dynamic capacity region representative based on
> > Table 8-165 and extend the cxl type3 device definition to include dc region
> > information. Also, based on info in 8.2.9.9.9.1, add 'Get Dynamic Capacity
> > Configuration' mailbox support.
> >
> > Note: decode_len of a dc region is aligned to 256*MiB, divided by
> > 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> > mailbox command.
> >
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 110 ++++++++++++++++++++++++++++++++++++
> > include/hw/cxl/cxl_device.h | 16 ++++++
> > 2 files changed, 126 insertions(+)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index ba1d9901df..88e3b733e3 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -22,6 +22,7 @@
> >
>
> [snip]
>
> > +#ifdef CXL_SPEC_AFTER_R30
> > + /*FIXME: need to set valid values in the future*/
> > + stq_le_p(&extra_out->num_extents_supported, 0);
> > + stq_le_p(&extra_out->num_extents_available, 0);
> > + stq_le_p(&extra_out->num_tags_supported, 0);
> > + stq_le_p(&extra_out->num_tags_available, 0);
>
> Hello,
>
> It's a trivial comment and doesn't have any impact yet, but shouldn't it
> be stl_le_p()?
>
> Thanks,
> Wonjae
Hi Wonjae,
Thanks for the review. You are right, will fix in the next version.
Fan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions
2024-02-22 9:22 ` [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions Wonjae Lee
@ 2024-02-22 16:56 ` fan
0 siblings, 0 replies; 44+ messages in thread
From: fan @ 2024-02-22 16:56 UTC (permalink / raw)
To: Wonjae Lee
Cc: nifan.cxl, qemu-devel, jonathan.cameron, linux-cxl,
gregory.price, ira.weiny, dan.j.williams, Adam Manzanares, dave,
nmtadam.samsung, Jim Harris, Fan Ni
On Thu, Feb 22, 2024 at 06:22:52PM +0900, Wonjae Lee wrote:
> On Wed, Feb 21, 2024 at 10:15:59AM -0800, nifan.cxl@gmail.com wrote:
> > From: Fan Ni <fan.ni@samsung.com>
> >
> > Add (file/memory backed) host backend, all the dynamic capacity regions
> > will share a single, large enough host backend. Set up address space for
> > DC regions to support read/write operations to dynamic capacity for DCD.
> >
> > With the change, following supports are added:
> > 1. Add a new property to type3 device "volatile-dc-memdev" to point to host
> > memory backend for dynamic capacity. Currently, all dc regions share one
> > one host backend.
>
> Hello, I know that it's too minor comment, but 'one' was used twice.
Good catch. Thanks.
Fan
>
> Thanks,
> Wonjae
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 07/10] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support
2024-02-21 18:16 ` [PATCH v4 07/10] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support nifan.cxl
@ 2024-02-23 7:16 ` Wonjae Lee
2024-02-23 16:56 ` fan
2024-02-26 17:48 ` Jonathan Cameron via
1 sibling, 1 reply; 44+ messages in thread
From: Wonjae Lee @ 2024-02-23 7:16 UTC (permalink / raw)
To: nifan.cxl, qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, jim.harris,
Fan Ni
On 2024-02-22 오전 3:16, nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Add dynamic capacity extent list representative to the definition of
> CXLType3Dev and add get DC extent list mailbox command per
> CXL.spec.3.1:.8.2.9.9.9.2.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 71 +++++++++++++++++++++++++++++++++++++
> hw/mem/cxl_type3.c | 1 +
> include/hw/cxl/cxl_device.h | 23 ++++++++++++
> 3 files changed, 95 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index f95e417683..dae7fe00ed 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -83,6 +83,7 @@ enum {
> #define CLEAR_POISON 0x2
> DCD_CONFIG = 0x48,
> #define GET_DC_CONFIG 0x0
> + #define GET_DYN_CAP_EXT_LIST 0x1
> PHYSICAL_SWITCH = 0x51,
> #define IDENTIFY_SWITCH_DEVICE 0x0
> #define GET_PHYSICAL_PORT_STATE 0x1
> @@ -1344,6 +1345,73 @@ static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * CXL r3.1 section 8.2.9.9.9.2:
> + * Get Dynamic Capacity Extent List (Opcode 4801h)
> + */
> +static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + struct get_dyn_cap_ext_list_in_pl {
> + uint32_t extent_cnt;
> + uint32_t start_extent_id;
> + } QEMU_PACKED;
> +
> + struct get_dyn_cap_ext_list_out_pl {
> + uint32_t count;
> + uint32_t total_extents;
> + uint32_t generation_num;
> + uint8_t rsvd[4];
> + CXLDCExtentRaw records[];
> + } QEMU_PACKED;
> +
> + struct get_dyn_cap_ext_list_in_pl *in = (void *)payload_in;
> + struct get_dyn_cap_ext_list_out_pl *out = (void *)payload_out;
> + uint16_t record_count = 0, i = 0, record_done = 0;
> + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> + CXLDCExtent *ent;
> + uint16_t out_pl_len;
> + uint32_t start_extent_id = in->start_extent_id;
> +
> + if (start_extent_id > ct3d->dc.total_extent_count) {
Hello,
Shouldn't it be >= rather than >?
(I accidentally replied to v3 with the same comment above, so please
ignore that.)
Thanks,
Wonjae
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
2024-02-21 18:16 ` [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response nifan.cxl
@ 2024-02-23 9:10 ` Wonjae Lee
2024-02-26 18:04 ` Jonathan Cameron via
1 sibling, 0 replies; 44+ messages in thread
From: Wonjae Lee @ 2024-02-23 9:10 UTC (permalink / raw)
To: nifan.cxl, qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, jim.harris,
Fan Ni
On 2024-02-22 오전 3:16, nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Per CXL spec 3.1, two mailbox commands are implemented:
> Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 288 ++++++++++++++++++++++++++++++++++++
> include/hw/cxl/cxl_device.h | 2 +
> 2 files changed, 290 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index dae7fe00ed..65ed28f700 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
[snip]
> +/*
> + * CXL r3.1 Table 8-168: Add Dynamic Capacity Response Input Payload
> + * CXL r3.1 Table 8-170: Release Dynamic Capacity Input Payload
> + */
> +typedef struct CXLUpdateDCExtentListInPl {
> + uint32_t num_entries_updated;
> + uint8_t flags;
> + uint8_t rsvd[3];
> + /* CXL r3.1 Table 8-169: Updated Extent List */
I'm not sure why this was changed, but in r3.1 the table is simply
renamed to "Updated Extent"
...
> +/*
> + * CXL r3.1 section 8.2.9.9.9.3: Add Dynamic Capacity Response (opcode 4802h)
It's too trivial, but it's written as "Opcode" in other comments.
: Opcode 4802h
...
> +/*
> + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (opcode 4803h)
And here, too
: Opcode 4803h
...
> @@ -1462,6 +1744,12 @@ static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
> [DCD_CONFIG][GET_DYN_CAP_EXT_LIST] = {
> "DCD_GET_DYNAMIC_CAPACITY_EXTENT_LIST", cmd_dcd_get_dyn_cap_ext_list,
> 8, 0 },
> + [DCD_CONFIG][ADD_DYN_CAP_RSP] = {
> + "ADD_DCD_DYNAMIC_CAPACITY_RESPONSE", cmd_dcd_add_dyn_cap_rsp,
> + ~0, IMMEDIATE_DATA_CHANGE },
> + [DCD_CONFIG][RELEASE_DYN_CAP] = {
> + "RELEASE_DCD_DYNAMIC_CAPACITY", cmd_dcd_release_dyn_cap,
> + ~0, IMMEDIATE_DATA_CHANGE },
For consistency, how about setting the names of commands as follows?
: DCD_ADD_DYNAMIC_CAPACITY_RESPONSE
: DCD_RELEASE_DYNAMIC_CAPACITY
Thanks,
Wonjae
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 09/10] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
2024-02-21 18:16 ` [PATCH v4 09/10] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents nifan.cxl
@ 2024-02-23 12:16 ` Wonjae Lee
2024-02-26 18:10 ` Jonathan Cameron via
1 sibling, 0 replies; 44+ messages in thread
From: Wonjae Lee @ 2024-02-23 12:16 UTC (permalink / raw)
To: nifan.cxl, qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, jim.harris,
Fan Ni
On 2024-02-22 오전 3:16, nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Since fabric manager emulation is not supported yet, the change implements
> the functions to add/release dynamic capacity extents as QMP interfaces.
>
> Note: we skips any FM issued extent release request if the exact extent
> does not exist in the extent list of the device. We will loose the
> restriction later once we have partial release support in the kernel.
>
[snip]
> +/* CXL r3.1 Table 8-50: Dynanic Capacity Event Record */
It needs a spell check. Dynamic.
...
> + switch (type) {
> + case DC_EVENT_ADD_CAPACITY:
> + break;
> + default:
> + break;
> + }
Perhaps this switch statement has become unnecessary in the revision to
patch v4?
...
> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index 8cc4c72fa9..19927629c7 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
[snip]
> +##
> +# @cxl-add-dynamic-capacity:
> +#
> +# Command to start add dynamic capacity extents flow. The device will
> +# have to ackowledged the acceptance of the extents before they are usable.
> +#
> +# @path: CXL DCD canonical QOM path
> +# @region-id: id of the region where the extent to add/release
How about just using "add" instead of "add/release"?
...
> +##
> +# @cxl-release-dynamic-capacity:
> +#
> +# Command to start release dynamic capacity extents flow. The host will
> +# need to respond to indicate that it has released the capacity before it
> +# is made unavailable for read and write and can be re-added.
> +#
> +# @path: CXL DCD canonical QOM path
> +# @region-id: id of the region where the extent to add/release
And here. Only "release"?
Thanks,
Wonjae
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 07/10] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support
2024-02-23 7:16 ` Wonjae Lee
@ 2024-02-23 16:56 ` fan
0 siblings, 0 replies; 44+ messages in thread
From: fan @ 2024-02-23 16:56 UTC (permalink / raw)
To: Wonjae Lee
Cc: nifan.cxl, qemu-devel, jonathan.cameron, linux-cxl,
gregory.price, ira.weiny, dan.j.williams, a.manzanares, dave,
nmtadam.samsung, jim.harris, Fan Ni
On Fri, Feb 23, 2024 at 04:16:53PM +0900, Wonjae Lee wrote:
> On 2024-02-22 오전 3:16, nifan.cxl@gmail.com wrote:
> > From: Fan Ni <fan.ni@samsung.com>
> >
> > Add dynamic capacity extent list representative to the definition of
> > CXLType3Dev and add get DC extent list mailbox command per
> > CXL.spec.3.1:.8.2.9.9.9.2.
> >
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 71 +++++++++++++++++++++++++++++++++++++
> > hw/mem/cxl_type3.c | 1 +
> > include/hw/cxl/cxl_device.h | 23 ++++++++++++
> > 3 files changed, 95 insertions(+)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index f95e417683..dae7fe00ed 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -83,6 +83,7 @@ enum {
> > #define CLEAR_POISON 0x2
> > DCD_CONFIG = 0x48,
> > #define GET_DC_CONFIG 0x0
> > + #define GET_DYN_CAP_EXT_LIST 0x1
> > PHYSICAL_SWITCH = 0x51,
> > #define IDENTIFY_SWITCH_DEVICE 0x0
> > #define GET_PHYSICAL_PORT_STATE 0x1
> > @@ -1344,6 +1345,73 @@ static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> > return CXL_MBOX_SUCCESS;
> > }
> > +/*
> > + * CXL r3.1 section 8.2.9.9.9.2:
> > + * Get Dynamic Capacity Extent List (Opcode 4801h)
> > + */
> > +static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(const struct cxl_cmd *cmd,
> > + uint8_t *payload_in,
> > + size_t len_in,
> > + uint8_t *payload_out,
> > + size_t *len_out,
> > + CXLCCI *cci)
> > +{
> > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > + struct get_dyn_cap_ext_list_in_pl {
> > + uint32_t extent_cnt;
> > + uint32_t start_extent_id;
> > + } QEMU_PACKED;
> > +
> > + struct get_dyn_cap_ext_list_out_pl {
> > + uint32_t count;
> > + uint32_t total_extents;
> > + uint32_t generation_num;
> > + uint8_t rsvd[4];
> > + CXLDCExtentRaw records[];
> > + } QEMU_PACKED;
> > +
> > + struct get_dyn_cap_ext_list_in_pl *in = (void *)payload_in;
> > + struct get_dyn_cap_ext_list_out_pl *out = (void *)payload_out;
> > + uint16_t record_count = 0, i = 0, record_done = 0;
> > + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> > + CXLDCExtent *ent;
> > + uint16_t out_pl_len;
> > + uint32_t start_extent_id = in->start_extent_id;
> > +
> > + if (start_extent_id > ct3d->dc.total_extent_count) {
>
> Hello,
>
> Shouldn't it be >= rather than >?
>
> (I accidentally replied to v3 with the same comment above, so please ignore
> that.)
The spec says "Greater than", so I will keep it as it is until there is
more clarification about this. When start_extent_id ==
total_extent_count, return 0 extents.
Fan
>
> Thanks,
> Wonjae
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 00/10] Enabling DCD emulation support in Qemu
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
` (11 preceding siblings ...)
[not found] ` <CGME20240221182137epcas2p276d22514caaa9412d0119ded6f9a63d4@epcms2p3>
@ 2024-02-23 18:05 ` fan
12 siblings, 0 replies; 44+ messages in thread
From: fan @ 2024-02-23 18:05 UTC (permalink / raw)
To: nifan.cxl, wj28.lee
Cc: qemu-devel, jonathan.cameron, linux-cxl, gregory.price,
ira.weiny, dan.j.williams, a.manzanares, dave, nmtadam.samsung,
jim.harris, Fan Ni
On Wed, Feb 21, 2024 at 10:15:53AM -0800, nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> v3[1]->v4:
>
> The code is rebased on mainstream QEMU with the following patch series:
>
> hw/cxl/mailbox: change CCI cmd set structure to be a member, not a reference
> hw/cxl/mailbox: interface to add CCI commands to an existing CCI
>
> Main changes include:
>
> 1. Updated the specification references to align with cxl spec r3.1.
> 2. Add extra elements to get dc region configuration output payload and
> procecced accordingly in mailbox command 4800h.
> 3. Removed the unwanted space.
> 4. Refactored ct3_build_cdat_entries_for_mr and extract it as a separate patch.
> 5. Updated cxl_create_dc_regions function to derive region len from host
> backend size.
> 6. Changed the logic for creating DC regions when host backend and address
> space processing is introduced, now cxl_create_dc_regions is called only
> when host backend exists.
> 7. Updated the name of the definitions related to DC extents for consistency.
> 7. Updated dynamic capacity event record definition to align with spec r3.1.
> 9. Changed the dynamic capacity request process logic, for release request,
> extra checks are done against the pending list to remove the extent yet added.
> 10. Changed the return value of cxl_create_dc_regions so the return can be used
> to Remove the extent for the list if needed.
> 11. offset and size in the qmp interface are changed to be byte-wise while the
> original is MiB-wise.
> 12. Fixed bugs in handling bitmap for dpa range existence.
> 13. NOTE: in previous version DC is set to non-volatile, while in this version
> we change it to volatile per Jonathan's suggestion.
> 14. Updated the doc in qapi/cxl.json.
>
> Thank Jonathan for the detailed review of the last version[1].
>
> The code is tested with Ira's last kernel DCD patch set [2] with some minor
> bug fixes[3]. Tested operations include:
> 1. create DC region;
> 2. Add/release DC extents;
> 3. convert DC capacity into system RAM (no real read/write to DCD tested);
>
>
> v3:
> [1] https://lore.kernel.org/linux-cxl/20231107180907.553451-1-nifan.cxl@gmail.com/T/#t
> [2] https://github.com/weiny2/linux-kernel/tree/dcd-v3-2023-10-30
> [3] https://github.com/moking/linux-dcd/commit/9d24fa6e5d39f934623220953caecc080f93e964
>
> Fan Ni (10):
> hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output
> payload of identify memory device command
> hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative
> and mailbox command support
> include/hw/cxl/cxl_device: Rename mem_size as static_mem_size for
> type3 memory devices
> hw/mem/cxl_type3: Add support to create DC regions to type3 memory
> devices
> hw/mem/cxl-type3: Refactor ct3_build_cdat_entries_for_mr to take mr
> size insead of mr as argument
> hw/mem/cxl_type3: Add host backend and address space handling for DC
> regions
> hw/mem/cxl_type3: Add DC extent list representative and get DC extent
> list mailbox support
> hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release
> dynamic capacity response
> hw/cxl/events: Add qmp interfaces to add/release dynamic capacity
> extents
> hw/mem/cxl_type3: Add dpa range validation for accesses to DC regions
>
> hw/cxl/cxl-mailbox-utils.c | 507 +++++++++++++++++++++++++++++++-
> hw/mem/cxl_type3.c | 559 +++++++++++++++++++++++++++++++++---
> hw/mem/cxl_type3_stubs.c | 14 +
> include/hw/cxl/cxl_device.h | 61 +++-
> include/hw/cxl/cxl_events.h | 18 ++
> qapi/cxl.json | 61 +++-
> 6 files changed, 1174 insertions(+), 46 deletions(-)
>
> --
> 2.43.0
>
Hi,
I fixed some issues mentioned by Wonjae Lee (wj28.lee@gmail.com) in his
review comments and pushed the changes to my local tree:
https://github.com/moking/qemu/tree/dcd-v4
This is no functional changes to the code, mainly fixes include:
1. Did spell check for the patchset and fixed 2 typos;
2. Fixed a misuse of stq_le_p and replaced it with stl_le_p in
cmd_dcd_get_dyn_cap_config;
3. Removed unnecessary switch case in patch 9;
4. Capitalized ""opcode" to "Opcode" in two code comments;
5. Updated one reference text to the spec;
6. Minor text change in qapi/cxl.json.
Thanks Wonjae for the careful review.
I will wait until next week to send out v5 to see if there are further
comments to come in.
Fan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support
2024-02-21 18:15 ` [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support nifan.cxl
@ 2024-02-26 17:33 ` Jonathan Cameron via
2024-03-04 12:40 ` Jørgen Hansen
1 sibling, 0 replies; 44+ messages in thread
From: Jonathan Cameron @ 2024-02-26 17:33 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:15:55 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Per cxl spec r3.1, add dynamic capacity region representative based on
> Table 8-165 and extend the cxl type3 device definition to include dc region
> information. Also, based on info in 8.2.9.9.9.1, add 'Get Dynamic Capacity
> Configuration' mailbox support.
>
> Note: decode_len of a dc region is aligned to 256*MiB, divided by
> 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> mailbox command.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Hi Fan,
A few comments inline.
Jonathan
> ---
> hw/cxl/cxl-mailbox-utils.c | 110 ++++++++++++++++++++++++++++++++++++
> include/hw/cxl/cxl_device.h | 16 ++++++
> 2 files changed, 126 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index ba1d9901df..88e3b733e3 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -22,6 +22,7 @@
>
> #define CXL_CAPACITY_MULTIPLIER (256 * MiB)
> #define CXL_DC_EVENT_LOG_SIZE 8
> +#define CXL_SPEC_AFTER_R30
As below. Drop this. Kernel code needs to be able to cope with newer specs
than it understands anyway so should be fine with the larger records (otherwise
it's buggy and needs fixing!)
>
> /*
> * How to add a new command, example. The command set FOO, with cmd BAR.
> @@ -80,6 +81,8 @@ enum {
> #define GET_POISON_LIST 0x0
> #define INJECT_POISON 0x1
> #define CLEAR_POISON 0x2
> + DCD_CONFIG = 0x48,
> + #define GET_DC_CONFIG 0x0
> PHYSICAL_SWITCH = 0x51,
> #define IDENTIFY_SWITCH_DEVICE 0x0
> #define GET_PHYSICAL_PORT_STATE 0x1
> @@ -1238,6 +1241,103 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * CXL r3.1 section 8.2.9.9.9.1: Get Dynamic Capacity Configuration
> + * (Opcode: 4800h)
> + */
> +static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + struct get_dyn_cap_config_in_pl {
Type not needed - see below.
> + uint8_t region_cnt;
> + uint8_t start_region_id;
> + } QEMU_PACKED;
} QEMU_PACKED *in;
> +
> + struct get_dyn_cap_config_out_pl {
Don't think giving this a type is necessary - see below.
> + uint8_t num_regions;
> + uint8_t regions_returned;
> + uint8_t rsvd1[6];
> + struct {
> + uint64_t base;
> + uint64_t decode_len;
> + uint64_t region_len;
> + uint64_t block_size;
> + uint32_t dsmadhandle;
> + uint8_t flags;
> + uint8_t rsvd2[3];
> + } QEMU_PACKED records[];
> + /*
> + * if cxl spec version >= 3.1, extra output payload as defined
> + * in struct get_dyn_cap_config_out_pl_extra comes here.
> + */
> + } QEMU_PACKED;
} QEMU_PACKED *out;
> +
> + struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
> + struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
We've (mostly) use the (void *) casting where we haven't given the structures
a type. I think I'd prefer we kept to that style for consistency.
There is an argument we should have given all these types
for readability reasons and to avoid casting via void * but
we have gone this way now - with the exception of
the poison list - oops.
> + uint16_t record_count = 0;
> + uint16_t i;
> + uint16_t out_pl_len;
> + uint8_t start_region_id = in->start_region_id;
> +#ifdef CXL_SPEC_AFTER_R30
Handy for testing, but I'd drop the ifdef for the final
version. We don't need to support old specs.
> + struct get_dyn_cap_config_out_pl_extra {
> + uint32_t num_extents_supported;
> + uint32_t num_extents_available;
> + uint32_t num_tags_supported;
> + uint32_t num_tags_available;
> + } QEMU_PACKED;
> + struct get_dyn_cap_config_out_pl_extra *extra_out;
As above, anonymous structure should work ok.
> +#endif
> +
> + if (start_region_id >= ct3d->dc.num_regions) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
> + in->region_cnt);
> +
> + out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> +#ifdef CXL_SPEC_AFTER_R30
> + extra_out = (struct get_dyn_cap_config_out_pl_extra *)(payload_out +
> + out_pl_len);
> + out_pl_len += sizeof(struct get_dyn_cap_config_out_pl_extra);
> +#endif
> + assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> +
> + out->num_regions = ct3d->dc.num_regions;
> +#ifdef CXL_SPEC_AFTER_R30
> + out->regions_returned = record_count;
> +#endif
> + for (i = 0; i < record_count; i++) {
> + stq_le_p(&out->records[i].base,
> + ct3d->dc.regions[start_region_id + i].base);
Qemu allows a couple of style choices, but don't think this matches any of them.
https://elixir.bootlin.com/qemu/latest/source/docs/devel/style.rst#L79
My preference is immediately after opening bracket.
> + stq_le_p(&out->records[i].decode_len,
> + ct3d->dc.regions[start_region_id + i].decode_len /
> + CXL_CAPACITY_MULTIPLIER);
> + stq_le_p(&out->records[i].region_len,
> + ct3d->dc.regions[start_region_id + i].len);
> + stq_le_p(&out->records[i].block_size,
> + ct3d->dc.regions[start_region_id + i].block_size);
> + stl_le_p(&out->records[i].dsmadhandle,
> + ct3d->dc.regions[start_region_id + i].dsmadhandle);
> + out->records[i].flags = ct3d->dc.regions[start_region_id + i].flags;
> + }
> +#ifdef CXL_SPEC_AFTER_R30
> + /*FIXME: need to set valid values in the future*/
We need to do that before upstreaming..
For tags, 0 is fine for now.
For extents allow say 512 and count how many are in lists
I guess then subtract that.
> + stq_le_p(&extra_out->num_extents_supported, 0);
> + stq_le_p(&extra_out->num_extents_available, 0);
> + stq_le_p(&extra_out->num_tags_supported, 0);
> + stq_le_p(&extra_out->num_tags_available, 0);
> +#endif
> +
> + *len_out = out_pl_len;
> + return CXL_MBOX_SUCCESS;
> +}
> +
> #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> #define IMMEDIATE_DATA_CHANGE (1 << 2)
> #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -1282,6 +1382,11 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
> cmd_media_clear_poison, 72, 0 },
> };
>
> +static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
> + [DCD_CONFIG][GET_DC_CONFIG] = { "DCD_GET_DC_CONFIG",
> + cmd_dcd_get_dyn_cap_config, 2, 0 },
> +};
> +
> static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 },
> [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { "BACKGROUND_OPERATION_STATUS",
> @@ -1487,7 +1592,12 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
>
> void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
> {
> + CXLType3Dev *ct3d = CXL_TYPE3(d);
> +
> cxl_copy_cci_commands(cci, cxl_cmd_set);
> + if (ct3d->dc.num_regions) {
> + cxl_copy_cci_commands(cci, cxl_cmd_set_dcd);
> + }
> cci->d = d;
>
> /* No separation for PCI MB as protocol handled in PCI device */
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 3cf3077afa..6df7fecdf1 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -422,6 +422,17 @@ typedef struct CXLPoison {
> typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> #define CXL_POISON_LIST_LIMIT 256
>
> +#define DCD_MAX_REGION_NUM 8
Really trivial but call this
DCD_MAX_NUM_REGION because it's the maximum number of
regions. The maximum region number is 7 (as zero indexed).
> +
> +typedef struct CXLDCDRegion {
> + uint64_t base;
Probably makes sense to call out that bas also aligned to 256 MiB
as not immediately clear the comment applies to both.
Easiest will be to duplicate it.
> + uint64_t decode_len; /* aligned to 256*MiB */
> + uint64_t len;
> + uint64_t block_size;
> + uint32_t dsmadhandle;
> + uint8_t flags;
> +} CXLDCDRegion;
> +
> struct CXLType3Dev {
> /* Private */
> PCIDevice parent_obj;
> @@ -454,6 +465,11 @@ struct CXLType3Dev {
> unsigned int poison_list_cnt;
> bool poison_list_overflowed;
> uint64_t poison_list_overflow_ts;
> +
> + struct dynamic_capacity {
> + uint8_t num_regions; /* 0-8 regions */
> + CXLDCDRegion regions[DCD_MAX_REGION_NUM];
> + } dc;
> };
>
> #define TYPE_CXL_TYPE3 "cxl-type3"
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support
@ 2024-02-26 17:33 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron via @ 2024-02-26 17:33 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:15:55 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Per cxl spec r3.1, add dynamic capacity region representative based on
> Table 8-165 and extend the cxl type3 device definition to include dc region
> information. Also, based on info in 8.2.9.9.9.1, add 'Get Dynamic Capacity
> Configuration' mailbox support.
>
> Note: decode_len of a dc region is aligned to 256*MiB, divided by
> 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> mailbox command.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Hi Fan,
A few comments inline.
Jonathan
> ---
> hw/cxl/cxl-mailbox-utils.c | 110 ++++++++++++++++++++++++++++++++++++
> include/hw/cxl/cxl_device.h | 16 ++++++
> 2 files changed, 126 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index ba1d9901df..88e3b733e3 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -22,6 +22,7 @@
>
> #define CXL_CAPACITY_MULTIPLIER (256 * MiB)
> #define CXL_DC_EVENT_LOG_SIZE 8
> +#define CXL_SPEC_AFTER_R30
As below. Drop this. Kernel code needs to be able to cope with newer specs
than it understands anyway so should be fine with the larger records (otherwise
it's buggy and needs fixing!)
>
> /*
> * How to add a new command, example. The command set FOO, with cmd BAR.
> @@ -80,6 +81,8 @@ enum {
> #define GET_POISON_LIST 0x0
> #define INJECT_POISON 0x1
> #define CLEAR_POISON 0x2
> + DCD_CONFIG = 0x48,
> + #define GET_DC_CONFIG 0x0
> PHYSICAL_SWITCH = 0x51,
> #define IDENTIFY_SWITCH_DEVICE 0x0
> #define GET_PHYSICAL_PORT_STATE 0x1
> @@ -1238,6 +1241,103 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * CXL r3.1 section 8.2.9.9.9.1: Get Dynamic Capacity Configuration
> + * (Opcode: 4800h)
> + */
> +static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + struct get_dyn_cap_config_in_pl {
Type not needed - see below.
> + uint8_t region_cnt;
> + uint8_t start_region_id;
> + } QEMU_PACKED;
} QEMU_PACKED *in;
> +
> + struct get_dyn_cap_config_out_pl {
Don't think giving this a type is necessary - see below.
> + uint8_t num_regions;
> + uint8_t regions_returned;
> + uint8_t rsvd1[6];
> + struct {
> + uint64_t base;
> + uint64_t decode_len;
> + uint64_t region_len;
> + uint64_t block_size;
> + uint32_t dsmadhandle;
> + uint8_t flags;
> + uint8_t rsvd2[3];
> + } QEMU_PACKED records[];
> + /*
> + * if cxl spec version >= 3.1, extra output payload as defined
> + * in struct get_dyn_cap_config_out_pl_extra comes here.
> + */
> + } QEMU_PACKED;
} QEMU_PACKED *out;
> +
> + struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
> + struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
We've (mostly) use the (void *) casting where we haven't given the structures
a type. I think I'd prefer we kept to that style for consistency.
There is an argument we should have given all these types
for readability reasons and to avoid casting via void * but
we have gone this way now - with the exception of
the poison list - oops.
> + uint16_t record_count = 0;
> + uint16_t i;
> + uint16_t out_pl_len;
> + uint8_t start_region_id = in->start_region_id;
> +#ifdef CXL_SPEC_AFTER_R30
Handy for testing, but I'd drop the ifdef for the final
version. We don't need to support old specs.
> + struct get_dyn_cap_config_out_pl_extra {
> + uint32_t num_extents_supported;
> + uint32_t num_extents_available;
> + uint32_t num_tags_supported;
> + uint32_t num_tags_available;
> + } QEMU_PACKED;
> + struct get_dyn_cap_config_out_pl_extra *extra_out;
As above, anonymous structure should work ok.
> +#endif
> +
> + if (start_region_id >= ct3d->dc.num_regions) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
> + in->region_cnt);
> +
> + out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> +#ifdef CXL_SPEC_AFTER_R30
> + extra_out = (struct get_dyn_cap_config_out_pl_extra *)(payload_out +
> + out_pl_len);
> + out_pl_len += sizeof(struct get_dyn_cap_config_out_pl_extra);
> +#endif
> + assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> +
> + out->num_regions = ct3d->dc.num_regions;
> +#ifdef CXL_SPEC_AFTER_R30
> + out->regions_returned = record_count;
> +#endif
> + for (i = 0; i < record_count; i++) {
> + stq_le_p(&out->records[i].base,
> + ct3d->dc.regions[start_region_id + i].base);
Qemu allows a couple of style choices, but don't think this matches any of them.
https://elixir.bootlin.com/qemu/latest/source/docs/devel/style.rst#L79
My preference is immediately after opening bracket.
> + stq_le_p(&out->records[i].decode_len,
> + ct3d->dc.regions[start_region_id + i].decode_len /
> + CXL_CAPACITY_MULTIPLIER);
> + stq_le_p(&out->records[i].region_len,
> + ct3d->dc.regions[start_region_id + i].len);
> + stq_le_p(&out->records[i].block_size,
> + ct3d->dc.regions[start_region_id + i].block_size);
> + stl_le_p(&out->records[i].dsmadhandle,
> + ct3d->dc.regions[start_region_id + i].dsmadhandle);
> + out->records[i].flags = ct3d->dc.regions[start_region_id + i].flags;
> + }
> +#ifdef CXL_SPEC_AFTER_R30
> + /*FIXME: need to set valid values in the future*/
We need to do that before upstreaming..
For tags, 0 is fine for now.
For extents allow say 512 and count how many are in lists
I guess then subtract that.
> + stq_le_p(&extra_out->num_extents_supported, 0);
> + stq_le_p(&extra_out->num_extents_available, 0);
> + stq_le_p(&extra_out->num_tags_supported, 0);
> + stq_le_p(&extra_out->num_tags_available, 0);
> +#endif
> +
> + *len_out = out_pl_len;
> + return CXL_MBOX_SUCCESS;
> +}
> +
> #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> #define IMMEDIATE_DATA_CHANGE (1 << 2)
> #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -1282,6 +1382,11 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
> cmd_media_clear_poison, 72, 0 },
> };
>
> +static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
> + [DCD_CONFIG][GET_DC_CONFIG] = { "DCD_GET_DC_CONFIG",
> + cmd_dcd_get_dyn_cap_config, 2, 0 },
> +};
> +
> static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 },
> [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { "BACKGROUND_OPERATION_STATUS",
> @@ -1487,7 +1592,12 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
>
> void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
> {
> + CXLType3Dev *ct3d = CXL_TYPE3(d);
> +
> cxl_copy_cci_commands(cci, cxl_cmd_set);
> + if (ct3d->dc.num_regions) {
> + cxl_copy_cci_commands(cci, cxl_cmd_set_dcd);
> + }
> cci->d = d;
>
> /* No separation for PCI MB as protocol handled in PCI device */
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 3cf3077afa..6df7fecdf1 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -422,6 +422,17 @@ typedef struct CXLPoison {
> typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> #define CXL_POISON_LIST_LIMIT 256
>
> +#define DCD_MAX_REGION_NUM 8
Really trivial but call this
DCD_MAX_NUM_REGION because it's the maximum number of
regions. The maximum region number is 7 (as zero indexed).
> +
> +typedef struct CXLDCDRegion {
> + uint64_t base;
Probably makes sense to call out that bas also aligned to 256 MiB
as not immediately clear the comment applies to both.
Easiest will be to duplicate it.
> + uint64_t decode_len; /* aligned to 256*MiB */
> + uint64_t len;
> + uint64_t block_size;
> + uint32_t dsmadhandle;
> + uint8_t flags;
> +} CXLDCDRegion;
> +
> struct CXLType3Dev {
> /* Private */
> PCIDevice parent_obj;
> @@ -454,6 +465,11 @@ struct CXLType3Dev {
> unsigned int poison_list_cnt;
> bool poison_list_overflowed;
> uint64_t poison_list_overflow_ts;
> +
> + struct dynamic_capacity {
> + uint8_t num_regions; /* 0-8 regions */
> + CXLDCDRegion regions[DCD_MAX_REGION_NUM];
> + } dc;
> };
>
> #define TYPE_CXL_TYPE3 "cxl-type3"
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to type3 memory devices
2024-02-21 18:15 ` [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to " nifan.cxl
@ 2024-02-26 17:38 ` Jonathan Cameron via
2024-03-04 13:10 ` Jørgen Hansen
1 sibling, 0 replies; 44+ messages in thread
From: Jonathan Cameron @ 2024-02-26 17:38 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:15:57 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> With the change, when setting up memory for type3 memory device, we can
> create DC regions.
> A property 'num-dc-regions' is added to ct3_props to allow users to pass the
> number of DC regions to create. To make it easier, other region parameters
> like region base, length, and block size are hard coded. If needed,
> these parameters can be added easily.
>
> With the change, we can create DC regions with proper kernel side
> support like below:
>
> region=$(cat /sys/bus/cxl/devices/decoder0.0/create_dc_region)
> echo $region > /sys/bus/cxl/devices/decoder0.0/create_dc_region
> echo 256 > /sys/bus/cxl/devices/$region/interleave_granularity
> echo 1 > /sys/bus/cxl/devices/$region/interleave_ways
>
> echo "dc0" >/sys/bus/cxl/devices/decoder2.0/mode
> echo 0x40000000 >/sys/bus/cxl/devices/decoder2.0/dpa_size
>
> echo 0x40000000 > /sys/bus/cxl/devices/$region/size
> echo "decoder2.0" > /sys/bus/cxl/devices/$region/target0
> echo 1 > /sys/bus/cxl/devices/$region/commit
> echo $region > /sys/bus/cxl/drivers/cxl_region/bind
>
I'd be tempted to delete the rest of this description.
> However, we cannot really read/write to the DC regions due to lack of
"However, we cannot yet read/write ..."
> 1. host backend and address space setup for DC regions;
> 2. mailbox command support for adding/releasing DC extents.
"This will be added later in the series."
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Without the above additions it sounds like this patch series only partly
does the job :)
A forwards reference is good, or just don't mention it.
No guarantee of code doing anything useful, beyond not crashing
at mid point of a series.
Jonathan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to type3 memory devices
@ 2024-02-26 17:38 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron via @ 2024-02-26 17:38 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:15:57 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> With the change, when setting up memory for type3 memory device, we can
> create DC regions.
> A property 'num-dc-regions' is added to ct3_props to allow users to pass the
> number of DC regions to create. To make it easier, other region parameters
> like region base, length, and block size are hard coded. If needed,
> these parameters can be added easily.
>
> With the change, we can create DC regions with proper kernel side
> support like below:
>
> region=$(cat /sys/bus/cxl/devices/decoder0.0/create_dc_region)
> echo $region > /sys/bus/cxl/devices/decoder0.0/create_dc_region
> echo 256 > /sys/bus/cxl/devices/$region/interleave_granularity
> echo 1 > /sys/bus/cxl/devices/$region/interleave_ways
>
> echo "dc0" >/sys/bus/cxl/devices/decoder2.0/mode
> echo 0x40000000 >/sys/bus/cxl/devices/decoder2.0/dpa_size
>
> echo 0x40000000 > /sys/bus/cxl/devices/$region/size
> echo "decoder2.0" > /sys/bus/cxl/devices/$region/target0
> echo 1 > /sys/bus/cxl/devices/$region/commit
> echo $region > /sys/bus/cxl/drivers/cxl_region/bind
>
I'd be tempted to delete the rest of this description.
> However, we cannot really read/write to the DC regions due to lack of
"However, we cannot yet read/write ..."
> 1. host backend and address space setup for DC regions;
> 2. mailbox command support for adding/releasing DC extents.
"This will be added later in the series."
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Without the above additions it sounds like this patch series only partly
does the job :)
A forwards reference is good, or just don't mention it.
No guarantee of code doing anything useful, beyond not crashing
at mid point of a series.
Jonathan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions
2024-02-21 18:15 ` [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions nifan.cxl
@ 2024-02-26 17:45 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron @ 2024-02-26 17:45 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:15:59 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Add (file/memory backed) host backend, all the dynamic capacity regions
> will share a single, large enough host backend. Set up address space for
> DC regions to support read/write operations to dynamic capacity for DCD.
>
> With the change, following supports are added:
> 1. Add a new property to type3 device "volatile-dc-memdev" to point to host
> memory backend for dynamic capacity. Currently, all dc regions share one
> one host backend.
> 2. Add namespace for dynamic capacity for read/write support;
> 3. Create cdat entries for each dynamic capacity region;
> 4. Fix dvsec range registers to include DC regions.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Only comment on this one from me is beware of FIXME wording for
features we haven't implemented yet. Makes people thing code isn't
good to go, when in reality we may or may not care about implementing
that configurability in the future!
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 6e5f908fb1..b966fa4f10 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> +
> + if (dc_mr) {
> + int i;
> + uint64_t region_base = vmr_size + pmr_size;
> +
> + /* FIXME: Currently we assume the dynamic capacity to be volatile. */
As below. TODO: Allow for non volatile dynamic capacity.
> + for (i = 0; i < ct3d->dc.num_regions; i++) {
> + ct3_build_cdat_entries_for_mr(&(table[cur_ent]),
> + dsmad_handle++,
> + ct3d->dc.regions[i].len,
> + false, true, region_base);
> + ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1;
> +
> + cur_ent += CT3_CDAT_NUM_ENTRIES;
> + region_base += ct3d->dc.regions[i].len;
> + }
> + }
> +
> +
> + /* FIXME: set dc as volatile for now */
Not sure it's a fixme, more of a TODO to add control of this later.
Fixme sounds broken, whereas it's a missing feature only.
> + memory_region_set_nonvolatile(dc_mr, false);
> + memory_region_set_enabled(dc_mr, true);
> + host_memory_backend_set_mapped(ct3d->dc.host_dc, true);
> + if (ds->id) {
> + dc_name = g_strdup_printf("cxl-dcd-dpa-dc-space:%s", ds->id);
> + } else {
> + dc_name = g_strdup("cxl-dcd-dpa-dc-space");
> + }
> + address_space_init(&ct3d->dc.host_dc_as, dc_mr, dc_name);
> + g_free(dc_name);
> +
> + if (!cxl_create_dc_regions(ct3d, errp)) {
> + error_setg(errp, "setup DC regions failed");
> + return false;
> + }
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions
@ 2024-02-26 17:45 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron via @ 2024-02-26 17:45 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:15:59 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Add (file/memory backed) host backend, all the dynamic capacity regions
> will share a single, large enough host backend. Set up address space for
> DC regions to support read/write operations to dynamic capacity for DCD.
>
> With the change, following supports are added:
> 1. Add a new property to type3 device "volatile-dc-memdev" to point to host
> memory backend for dynamic capacity. Currently, all dc regions share one
> one host backend.
> 2. Add namespace for dynamic capacity for read/write support;
> 3. Create cdat entries for each dynamic capacity region;
> 4. Fix dvsec range registers to include DC regions.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Only comment on this one from me is beware of FIXME wording for
features we haven't implemented yet. Makes people thing code isn't
good to go, when in reality we may or may not care about implementing
that configurability in the future!
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 6e5f908fb1..b966fa4f10 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> +
> + if (dc_mr) {
> + int i;
> + uint64_t region_base = vmr_size + pmr_size;
> +
> + /* FIXME: Currently we assume the dynamic capacity to be volatile. */
As below. TODO: Allow for non volatile dynamic capacity.
> + for (i = 0; i < ct3d->dc.num_regions; i++) {
> + ct3_build_cdat_entries_for_mr(&(table[cur_ent]),
> + dsmad_handle++,
> + ct3d->dc.regions[i].len,
> + false, true, region_base);
> + ct3d->dc.regions[i].dsmadhandle = dsmad_handle - 1;
> +
> + cur_ent += CT3_CDAT_NUM_ENTRIES;
> + region_base += ct3d->dc.regions[i].len;
> + }
> + }
> +
> +
> + /* FIXME: set dc as volatile for now */
Not sure it's a fixme, more of a TODO to add control of this later.
Fixme sounds broken, whereas it's a missing feature only.
> + memory_region_set_nonvolatile(dc_mr, false);
> + memory_region_set_enabled(dc_mr, true);
> + host_memory_backend_set_mapped(ct3d->dc.host_dc, true);
> + if (ds->id) {
> + dc_name = g_strdup_printf("cxl-dcd-dpa-dc-space:%s", ds->id);
> + } else {
> + dc_name = g_strdup("cxl-dcd-dpa-dc-space");
> + }
> + address_space_init(&ct3d->dc.host_dc_as, dc_mr, dc_name);
> + g_free(dc_name);
> +
> + if (!cxl_create_dc_regions(ct3d, errp)) {
> + error_setg(errp, "setup DC regions failed");
> + return false;
> + }
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 07/10] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support
2024-02-21 18:16 ` [PATCH v4 07/10] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support nifan.cxl
@ 2024-02-26 17:48 ` Jonathan Cameron via
2024-02-26 17:48 ` Jonathan Cameron via
1 sibling, 0 replies; 44+ messages in thread
From: Jonathan Cameron @ 2024-02-26 17:48 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:16:00 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Add dynamic capacity extent list representative to the definition of
> CXLType3Dev and add get DC extent list mailbox command per
> CXL.spec.3.1:.8.2.9.9.9.2.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Follow on from earlier comment on my preference for anonymous
structure types when we only use them in one place.
> +/*
> + * CXL r3.1 section 8.2.9.9.9.2:
> + * Get Dynamic Capacity Extent List (Opcode 4801h)
> + */
> +static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + struct get_dyn_cap_ext_list_in_pl {
> + uint32_t extent_cnt;
> + uint32_t start_extent_id;
> + } QEMU_PACKED;
> +
> + struct get_dyn_cap_ext_list_out_pl {
> + uint32_t count;
> + uint32_t total_extents;
> + uint32_t generation_num;
> + uint8_t rsvd[4];
> + CXLDCExtentRaw records[];
> + } QEMU_PACKED;
> +
> + struct get_dyn_cap_ext_list_in_pl *in = (void *)payload_in;
> + struct get_dyn_cap_ext_list_out_pl *out = (void *)payload_out;
As for earlier patches, I think anonymous struct types are fine for
these and lead to shorter code.
> + uint16_t record_count = 0, i = 0, record_done = 0;
> + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> + CXLDCExtent *ent;
> + uint16_t out_pl_len;
> + uint32_t start_extent_id = in->start_extent_id;
> +
> + if (start_extent_id > ct3d->dc.total_extent_count) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + record_count = MIN(in->extent_cnt,
> + ct3d->dc.total_extent_count - start_extent_id);
> +
> + out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> + assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> +
> + stl_le_p(&out->count, record_count);
> + stl_le_p(&out->total_extents, ct3d->dc.total_extent_count);
> + stl_le_p(&out->generation_num, ct3d->dc.ext_list_gen_seq);
> +
> + if (record_count > 0) {
> + QTAILQ_FOREACH(ent, extent_list, node) {
> + if (i++ < start_extent_id) {
> + continue;
> + }
> + stq_le_p(&out->records[record_done].start_dpa, ent->start_dpa);
> + stq_le_p(&out->records[record_done].len, ent->len);
> + memcpy(&out->records[record_done].tag, ent->tag, 0x10);
> + stw_le_p(&out->records[record_done].shared_seq, ent->shared_seq);
> + record_done++;
> + if (record_done == record_count) {
> + break;
> + }
> + }
> + }
> +
> + *len_out = out_pl_len;
> + return CXL_MBOX_SUCCESS;
> +}
> +
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 07/10] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support
@ 2024-02-26 17:48 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron via @ 2024-02-26 17:48 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:16:00 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Add dynamic capacity extent list representative to the definition of
> CXLType3Dev and add get DC extent list mailbox command per
> CXL.spec.3.1:.8.2.9.9.9.2.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Follow on from earlier comment on my preference for anonymous
structure types when we only use them in one place.
> +/*
> + * CXL r3.1 section 8.2.9.9.9.2:
> + * Get Dynamic Capacity Extent List (Opcode 4801h)
> + */
> +static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + struct get_dyn_cap_ext_list_in_pl {
> + uint32_t extent_cnt;
> + uint32_t start_extent_id;
> + } QEMU_PACKED;
> +
> + struct get_dyn_cap_ext_list_out_pl {
> + uint32_t count;
> + uint32_t total_extents;
> + uint32_t generation_num;
> + uint8_t rsvd[4];
> + CXLDCExtentRaw records[];
> + } QEMU_PACKED;
> +
> + struct get_dyn_cap_ext_list_in_pl *in = (void *)payload_in;
> + struct get_dyn_cap_ext_list_out_pl *out = (void *)payload_out;
As for earlier patches, I think anonymous struct types are fine for
these and lead to shorter code.
> + uint16_t record_count = 0, i = 0, record_done = 0;
> + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> + CXLDCExtent *ent;
> + uint16_t out_pl_len;
> + uint32_t start_extent_id = in->start_extent_id;
> +
> + if (start_extent_id > ct3d->dc.total_extent_count) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + record_count = MIN(in->extent_cnt,
> + ct3d->dc.total_extent_count - start_extent_id);
> +
> + out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> + assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> +
> + stl_le_p(&out->count, record_count);
> + stl_le_p(&out->total_extents, ct3d->dc.total_extent_count);
> + stl_le_p(&out->generation_num, ct3d->dc.ext_list_gen_seq);
> +
> + if (record_count > 0) {
> + QTAILQ_FOREACH(ent, extent_list, node) {
> + if (i++ < start_extent_id) {
> + continue;
> + }
> + stq_le_p(&out->records[record_done].start_dpa, ent->start_dpa);
> + stq_le_p(&out->records[record_done].len, ent->len);
> + memcpy(&out->records[record_done].tag, ent->tag, 0x10);
> + stw_le_p(&out->records[record_done].shared_seq, ent->shared_seq);
> + record_done++;
> + if (record_done == record_count) {
> + break;
> + }
> + }
> + }
> +
> + *len_out = out_pl_len;
> + return CXL_MBOX_SUCCESS;
> +}
> +
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
2024-02-21 18:16 ` [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response nifan.cxl
@ 2024-02-26 18:04 ` Jonathan Cameron via
2024-02-26 18:04 ` Jonathan Cameron via
1 sibling, 0 replies; 44+ messages in thread
From: Jonathan Cameron @ 2024-02-26 18:04 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:16:01 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Per CXL spec 3.1, two mailbox commands are implemented:
> Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Hi Fan,
Comments on this are all about corner cases. If we can I think we need
to cover a few more. Linux won't hit them (I think) so it will be
a bit of a pain to test but maybe raw commands enabled and some
userspace code will let us exercise the corner cases?
Jonathan
> +
> +/*
> + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (opcode 4803h)
> + */
> +static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> + CXLDCExtent *ent;
> + uint32_t i;
> + uint64_t dpa, len;
> + CXLRetCode ret;
> +
> + if (in->num_entries_updated == 0) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + ret = cxl_detect_malformed_extent_list(ct3d, in);
> + if (ret != CXL_MBOX_SUCCESS) {
> + return ret;
> + }
> +
> + for (i = 0; i < in->num_entries_updated; i++) {
> + bool found = false;
> +
> + dpa = in->updated_entries[i].start_dpa;
> + len = in->updated_entries[i].len;
> +
> + QTAILQ_FOREACH(ent, extent_list, node) {
> + if (ent->start_dpa <= dpa &&
> + dpa + len <= ent->start_dpa + ent->len) {
> + /*
> + * If an incoming extent covers a portion of an extent
> + * in the device extent list, remove only the overlapping
> + * portion, meaning
> + * 1. the portions that are not covered by the incoming
> + * extent at both end of the original extent will become
> + * new extents and inserted to the extent list; and
> + * 2. the original extent is removed from the extent list;
> + * 3. dc extent count is updated accordingly.
> + */
> + uint64_t ent_start_dpa = ent->start_dpa;
> + uint64_t ent_len = ent->len;
> + uint64_t len1 = dpa - ent_start_dpa;
> + uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
> +
> + found = true;
> + cxl_remove_extent_from_extent_list(extent_list, ent);
> + ct3d->dc.total_extent_count -= 1;
> +
> + if (len1) {
> + cxl_insert_extent_to_extent_list(extent_list,
> + ent_start_dpa, len1,
> + NULL, 0);
> + ct3d->dc.total_extent_count += 1;
> + }
> + if (len2) {
> + cxl_insert_extent_to_extent_list(extent_list, dpa + len,
> + len2, NULL, 0);
> + ct3d->dc.total_extent_count += 1;
There is a non zero chance that we'll overflow however many extents we claim
to support. So we need to check that and fail the remove if it happens.
Could ignore this for now though as that value is (I think!) conservative
to allow for complex extent list tracking implementations. Succeeding
when a naive solution would fail due to running out of extents that it can
manage is not (I think) a bug.
> + }
> + break;
> + /*Currently we reject the attempt to remove a superset*/
Space after /* and before */
I think we need to fix this. Linux isn't going to do it any time soon, but
I think it's allowed to allocate two extents next to each other then free them
in one go. Isn't this case easy to do or are there awkward corners?
If it's sufficiently nasty (maybe because only part of extent provided exists?)
then maybe we can leave it for now.
I worry about something like
| EXTENT TO FREE |
| Exists | gap | Exists |
Where we have to check for gap before removing anything?
Does the spec address this? Not that I can find.
I think the implication is we have to do a validation pass, then a free
pass after we know whole of requested extent is valid.
Nasty to test if nothing else :( Would look much like your check
on malformed extent lists.
> + } else if ((dpa < ent->start_dpa + ent->len &&
> + dpa + len > ent->start_dpa + ent->len) ||
> + (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) {
> + return CXL_MBOX_INVALID_EXTENT_LIST;
> + }
> + }
> +
> + if (!found) {
> + /* Try to remove a non-existing extent */
> + return CXL_MBOX_INVALID_PA;
> + }
> + }
> +
> + return CXL_MBOX_SUCCESS;
> +}
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
@ 2024-02-26 18:04 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron via @ 2024-02-26 18:04 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:16:01 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Per CXL spec 3.1, two mailbox commands are implemented:
> Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Hi Fan,
Comments on this are all about corner cases. If we can I think we need
to cover a few more. Linux won't hit them (I think) so it will be
a bit of a pain to test but maybe raw commands enabled and some
userspace code will let us exercise the corner cases?
Jonathan
> +
> +/*
> + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (opcode 4803h)
> + */
> +static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> + CXLDCExtent *ent;
> + uint32_t i;
> + uint64_t dpa, len;
> + CXLRetCode ret;
> +
> + if (in->num_entries_updated == 0) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + ret = cxl_detect_malformed_extent_list(ct3d, in);
> + if (ret != CXL_MBOX_SUCCESS) {
> + return ret;
> + }
> +
> + for (i = 0; i < in->num_entries_updated; i++) {
> + bool found = false;
> +
> + dpa = in->updated_entries[i].start_dpa;
> + len = in->updated_entries[i].len;
> +
> + QTAILQ_FOREACH(ent, extent_list, node) {
> + if (ent->start_dpa <= dpa &&
> + dpa + len <= ent->start_dpa + ent->len) {
> + /*
> + * If an incoming extent covers a portion of an extent
> + * in the device extent list, remove only the overlapping
> + * portion, meaning
> + * 1. the portions that are not covered by the incoming
> + * extent at both end of the original extent will become
> + * new extents and inserted to the extent list; and
> + * 2. the original extent is removed from the extent list;
> + * 3. dc extent count is updated accordingly.
> + */
> + uint64_t ent_start_dpa = ent->start_dpa;
> + uint64_t ent_len = ent->len;
> + uint64_t len1 = dpa - ent_start_dpa;
> + uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
> +
> + found = true;
> + cxl_remove_extent_from_extent_list(extent_list, ent);
> + ct3d->dc.total_extent_count -= 1;
> +
> + if (len1) {
> + cxl_insert_extent_to_extent_list(extent_list,
> + ent_start_dpa, len1,
> + NULL, 0);
> + ct3d->dc.total_extent_count += 1;
> + }
> + if (len2) {
> + cxl_insert_extent_to_extent_list(extent_list, dpa + len,
> + len2, NULL, 0);
> + ct3d->dc.total_extent_count += 1;
There is a non zero chance that we'll overflow however many extents we claim
to support. So we need to check that and fail the remove if it happens.
Could ignore this for now though as that value is (I think!) conservative
to allow for complex extent list tracking implementations. Succeeding
when a naive solution would fail due to running out of extents that it can
manage is not (I think) a bug.
> + }
> + break;
> + /*Currently we reject the attempt to remove a superset*/
Space after /* and before */
I think we need to fix this. Linux isn't going to do it any time soon, but
I think it's allowed to allocate two extents next to each other then free them
in one go. Isn't this case easy to do or are there awkward corners?
If it's sufficiently nasty (maybe because only part of extent provided exists?)
then maybe we can leave it for now.
I worry about something like
| EXTENT TO FREE |
| Exists | gap | Exists |
Where we have to check for gap before removing anything?
Does the spec address this? Not that I can find.
I think the implication is we have to do a validation pass, then a free
pass after we know whole of requested extent is valid.
Nasty to test if nothing else :( Would look much like your check
on malformed extent lists.
> + } else if ((dpa < ent->start_dpa + ent->len &&
> + dpa + len > ent->start_dpa + ent->len) ||
> + (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) {
> + return CXL_MBOX_INVALID_EXTENT_LIST;
> + }
> + }
> +
> + if (!found) {
> + /* Try to remove a non-existing extent */
> + return CXL_MBOX_INVALID_PA;
> + }
> + }
> +
> + return CXL_MBOX_SUCCESS;
> +}
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 09/10] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
2024-02-21 18:16 ` [PATCH v4 09/10] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents nifan.cxl
@ 2024-02-26 18:10 ` Jonathan Cameron via
2024-02-26 18:10 ` Jonathan Cameron via
1 sibling, 0 replies; 44+ messages in thread
From: Jonathan Cameron @ 2024-02-26 18:10 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:16:02 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Since fabric manager emulation is not supported yet, the change implements
> the functions to add/release dynamic capacity extents as QMP interfaces.
>
> Note: we skips any FM issued extent release request if the exact extent
> does not exist in the extent list of the device. We will loose the
> restriction later once we have partial release support in the kernel.
>
> 1. Add dynamic capacity extents:
>
> For example, the command to add two continuous extents (each 128MiB long)
> to region 0 (starting at DPA offset 0) looks like below:
>
> { "execute": "qmp_capabilities" }
>
> { "execute": "cxl-add-dynamic-capacity",
> "arguments": {
> "path": "/machine/peripheral/cxl-dcd0",
> "region-id": 0,
> "extents": [
> {
> "dpa": 0,
> "len": 134217728
> },
> {
> "dpa": 134217728,
> "len": 134217728
> }
> ]
> }
> }
>
> 2. Release dynamic capacity extents:
>
> For example, the command to release an extent of size 128MiB from region 0
> (DPA offset 128MiB) look like below:
>
> { "execute": "cxl-release-dynamic-capacity",
> "arguments": {
> "path": "/machine/peripheral/cxl-dcd0",
> "region-id": 0,
> "extents": [
> {
> "dpa": 134217728,
> "len": 134217728
> }
> ]
> }
> }
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
A few things inline. I don't understand one of the comments.
> ---
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index f4edada303..b8c4273e99 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> +/*
> + * Check whether the exact extent exists in the list
> + * Return value: the extent pointer in the list; else null
> + */
> +static CXLDCExtent *cxl_dc_extent_exists(CXLDCExtentList *list,
> + CXLDCExtentRaw *ext)
> +{
> + CXLDCExtent *ent;
> +
> + if (!ext || !list) {
> + return NULL;
> + }
> +
> + QTAILQ_FOREACH(ent, list, node) {
> + if (ent->start_dpa != ext->start_dpa) {
> + continue;
> + }
> +
> + /*Found exact extent*/
Spacing /* Found .. extent */
> + return ent->len == ext->len ? ent : NULL;
> + }
> +
> + return NULL;
> +}
> +
> +/*
> + * The main function to process dynamic capacity event. Currently DC extents
> + * add/release requests are processed.
> + */
> +static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
> + CXLDCEventType type, uint16_t hid,
> + uint8_t rid,
> + CXLDCExtentRecordList *records,
> + Error **errp)
> +{
> + Object *obj;
> + CXLEventDynamicCapacity dCap = {};
> + CXLEventRecordHdr *hdr = &dCap.hdr;
> + CXLType3Dev *dcd;
> + uint8_t flags = 1 << CXL_EVENT_TYPE_INFO;
> + uint32_t num_extents = 0;
> + CXLDCExtentRecordList *list;
> + g_autofree CXLDCExtentRaw *extents = NULL;
> + uint8_t enc_log;
> + uint64_t offset, len, block_size;
> + int i;
> + int rc;
> + g_autofree unsigned long *blk_bitmap = NULL;
> +
> + obj = object_resolve_path(path, NULL);
> + if (!obj) {
> + error_setg(errp, "Unable to resolve path");
> + return;
> + }
> + if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
> + error_setg(errp, "Path not point to a valid CXL type3 device");
> + return;
> + }
> +
> + dcd = CXL_TYPE3(obj);
> + if (!dcd->dc.num_regions) {
> + error_setg(errp, "No dynamic capacity support from the device");
> + return;
> + }
> +
> + rc = ct3d_qmp_cxl_event_log_enc(log);
> + if (rc < 0) {
> + error_setg(errp, "Unhandled error log type");
> + return;
> + }
> + enc_log = rc;
> +
> + if (rid >= dcd->dc.num_regions) {
> + error_setg(errp, "region id is too large");
> + return;
> + }
> + block_size = dcd->dc.regions[rid].block_size;
> +
> + /* Sanity check and count the extents */
> + list = records;
> + while (list) {
> + offset = list->value->offset;
> + len = list->value->len;
> +
> + if (len == 0) {
> + error_setg(errp, "extent with 0 length is not allowed");
> + return;
> + }
> +
> + if (offset % block_size || len % block_size) {
> + error_setg(errp, "dpa or len is not aligned to region block size");
> + return;
> + }
> +
> + if (offset + len > dcd->dc.regions[rid].len) {
> + error_setg(errp, "extent range is beyond the region end");
> + return;
> + }
> +
> + num_extents++;
> + list = list->next;
> + }
> + if (num_extents == 0) {
> + error_setg(errp, "No extents found in the command");
> + return;
> + }
> +
> + blk_bitmap = bitmap_new(dcd->dc.regions[rid].len / block_size);
> +
> + /* Create Extent list for event being passed to host */
> + i = 0;
> + list = records;
> + extents = g_new0(CXLDCExtentRaw, num_extents);
> + while (list) {
> + CXLDCExtent *ent;
> + bool skip_extent = false;
> +
> + offset = list->value->offset;
> + len = list->value->len;
> +
> + extents[i].start_dpa = offset + dcd->dc.regions[rid].base;
> + extents[i].len = len;
> + memset(extents[i].tag, 0, 0x10);
> + extents[i].shared_seq = 0;
> +
> + if (type == DC_EVENT_RELEASE_CAPACITY ||
> + type == DC_EVENT_FORCED_RELEASE_CAPACITY) {
> + /*
> + * if the extent is still pending to be added to the host,
> + * remove it from the pending extent list, so later when the add
> + * response for the extent arrives, the device can reject the
> + * extent as it is not in the pending list.
> + */
> + ent = cxl_dc_extent_exists(&dcd->dc.extents_pending_to_add,
> + &extents[i]);
> + if (ent) {
> + QTAILQ_REMOVE(&dcd->dc.extents_pending_to_add, ent, node);
> + g_free(ent);
> + skip_extent = true;
> + } else if (!cxl_dc_extent_exists(&dcd->dc.extents, &extents[i])) {
> + /* If the exact extent is not in the accepted list, skip */
> + skip_extent = true;
> + }
> + }
> +
> + /* No duplicate or overlapped extents are allowed */
> + if (test_any_bits_set(blk_bitmap, offset / block_size,
> + len / block_size)) {
> + error_setg(errp, "duplicate or overlapped extents are detected");
> + return;
> + }
> + bitmap_set(blk_bitmap, offset / block_size, len / block_size);
> +
> + list = list->next;
> + if (!skip_extent) {
> + i++;
> + }
> + }
> + num_extents = i;
> +
> + switch (type) {
> + case DC_EVENT_ADD_CAPACITY:
> + break;
> + default:
> + break;
> + }
> + /*
> + * CXL r3.1 section 8.2.9.2.1.6: Dynamic Capacity Event Record
> + *
> + * All Dynamic Capacity event records shall set the Event Record Severity
> + * field in the Common Event Record Format to Informational Event. All
> + * Dynamic Capacity related events shall be logged in the Dynamic Capacity
> + * Event Log.
> + */
> + cxl_assign_event_header(hdr, &dynamic_capacity_uuid, flags, sizeof(dCap),
> + cxl_device_get_timestamp(&dcd->cxl_dstate));
> +
> + dCap.type = type;
> + /* FIXME: for now, validaity flag is cleared */
spelling
> + dCap.validity_flags = 0;
> + stw_le_p(&dCap.host_id, hid);
> + /* only valid for DC_REGION_CONFIG_UPDATED event */
> + dCap.updated_region_id = 0;
> + /*
> + * FIXME: for now, "More" flag is cleared as there is only one extent for
> + * each record
This need more info. If they have the same tag then should set more on the
records.
> + */
> + dCap.flags = 0;
> +
> + /*
> + * For current implementation, each DC event record only associates with
> + * one extent, so the "More" flag does not need to be set.
> + */
> + for (i = 0; i < num_extents; i++) {
> + memcpy(&dCap.dynamic_capacity_extent, &extents[i],
> + sizeof(CXLDCExtentRaw));
> +
> + if (type == DC_EVENT_ADD_CAPACITY) {
> + cxl_insert_extent_to_extent_list(&dcd->dc.extents_pending_to_add,
> + extents[i].start_dpa,
> + extents[i].len,
> + extents[i].tag,
> + extents[i].shared_seq);
> + }
> +
> + if (cxl_event_insert(&dcd->cxl_dstate, enc_log,
> + (CXLEventRecordRaw *)&dCap)) {
> + cxl_event_irq_assert(dcd);
> + }
> + }
> +}
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 09/10] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
@ 2024-02-26 18:10 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron via @ 2024-02-26 18:10 UTC (permalink / raw)
To: nifan.cxl
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Wed, 21 Feb 2024 10:16:02 -0800
nifan.cxl@gmail.com wrote:
> From: Fan Ni <fan.ni@samsung.com>
>
> Since fabric manager emulation is not supported yet, the change implements
> the functions to add/release dynamic capacity extents as QMP interfaces.
>
> Note: we skips any FM issued extent release request if the exact extent
> does not exist in the extent list of the device. We will loose the
> restriction later once we have partial release support in the kernel.
>
> 1. Add dynamic capacity extents:
>
> For example, the command to add two continuous extents (each 128MiB long)
> to region 0 (starting at DPA offset 0) looks like below:
>
> { "execute": "qmp_capabilities" }
>
> { "execute": "cxl-add-dynamic-capacity",
> "arguments": {
> "path": "/machine/peripheral/cxl-dcd0",
> "region-id": 0,
> "extents": [
> {
> "dpa": 0,
> "len": 134217728
> },
> {
> "dpa": 134217728,
> "len": 134217728
> }
> ]
> }
> }
>
> 2. Release dynamic capacity extents:
>
> For example, the command to release an extent of size 128MiB from region 0
> (DPA offset 128MiB) look like below:
>
> { "execute": "cxl-release-dynamic-capacity",
> "arguments": {
> "path": "/machine/peripheral/cxl-dcd0",
> "region-id": 0,
> "extents": [
> {
> "dpa": 134217728,
> "len": 134217728
> }
> ]
> }
> }
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
A few things inline. I don't understand one of the comments.
> ---
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index f4edada303..b8c4273e99 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> +/*
> + * Check whether the exact extent exists in the list
> + * Return value: the extent pointer in the list; else null
> + */
> +static CXLDCExtent *cxl_dc_extent_exists(CXLDCExtentList *list,
> + CXLDCExtentRaw *ext)
> +{
> + CXLDCExtent *ent;
> +
> + if (!ext || !list) {
> + return NULL;
> + }
> +
> + QTAILQ_FOREACH(ent, list, node) {
> + if (ent->start_dpa != ext->start_dpa) {
> + continue;
> + }
> +
> + /*Found exact extent*/
Spacing /* Found .. extent */
> + return ent->len == ext->len ? ent : NULL;
> + }
> +
> + return NULL;
> +}
> +
> +/*
> + * The main function to process dynamic capacity event. Currently DC extents
> + * add/release requests are processed.
> + */
> +static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
> + CXLDCEventType type, uint16_t hid,
> + uint8_t rid,
> + CXLDCExtentRecordList *records,
> + Error **errp)
> +{
> + Object *obj;
> + CXLEventDynamicCapacity dCap = {};
> + CXLEventRecordHdr *hdr = &dCap.hdr;
> + CXLType3Dev *dcd;
> + uint8_t flags = 1 << CXL_EVENT_TYPE_INFO;
> + uint32_t num_extents = 0;
> + CXLDCExtentRecordList *list;
> + g_autofree CXLDCExtentRaw *extents = NULL;
> + uint8_t enc_log;
> + uint64_t offset, len, block_size;
> + int i;
> + int rc;
> + g_autofree unsigned long *blk_bitmap = NULL;
> +
> + obj = object_resolve_path(path, NULL);
> + if (!obj) {
> + error_setg(errp, "Unable to resolve path");
> + return;
> + }
> + if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
> + error_setg(errp, "Path not point to a valid CXL type3 device");
> + return;
> + }
> +
> + dcd = CXL_TYPE3(obj);
> + if (!dcd->dc.num_regions) {
> + error_setg(errp, "No dynamic capacity support from the device");
> + return;
> + }
> +
> + rc = ct3d_qmp_cxl_event_log_enc(log);
> + if (rc < 0) {
> + error_setg(errp, "Unhandled error log type");
> + return;
> + }
> + enc_log = rc;
> +
> + if (rid >= dcd->dc.num_regions) {
> + error_setg(errp, "region id is too large");
> + return;
> + }
> + block_size = dcd->dc.regions[rid].block_size;
> +
> + /* Sanity check and count the extents */
> + list = records;
> + while (list) {
> + offset = list->value->offset;
> + len = list->value->len;
> +
> + if (len == 0) {
> + error_setg(errp, "extent with 0 length is not allowed");
> + return;
> + }
> +
> + if (offset % block_size || len % block_size) {
> + error_setg(errp, "dpa or len is not aligned to region block size");
> + return;
> + }
> +
> + if (offset + len > dcd->dc.regions[rid].len) {
> + error_setg(errp, "extent range is beyond the region end");
> + return;
> + }
> +
> + num_extents++;
> + list = list->next;
> + }
> + if (num_extents == 0) {
> + error_setg(errp, "No extents found in the command");
> + return;
> + }
> +
> + blk_bitmap = bitmap_new(dcd->dc.regions[rid].len / block_size);
> +
> + /* Create Extent list for event being passed to host */
> + i = 0;
> + list = records;
> + extents = g_new0(CXLDCExtentRaw, num_extents);
> + while (list) {
> + CXLDCExtent *ent;
> + bool skip_extent = false;
> +
> + offset = list->value->offset;
> + len = list->value->len;
> +
> + extents[i].start_dpa = offset + dcd->dc.regions[rid].base;
> + extents[i].len = len;
> + memset(extents[i].tag, 0, 0x10);
> + extents[i].shared_seq = 0;
> +
> + if (type == DC_EVENT_RELEASE_CAPACITY ||
> + type == DC_EVENT_FORCED_RELEASE_CAPACITY) {
> + /*
> + * if the extent is still pending to be added to the host,
> + * remove it from the pending extent list, so later when the add
> + * response for the extent arrives, the device can reject the
> + * extent as it is not in the pending list.
> + */
> + ent = cxl_dc_extent_exists(&dcd->dc.extents_pending_to_add,
> + &extents[i]);
> + if (ent) {
> + QTAILQ_REMOVE(&dcd->dc.extents_pending_to_add, ent, node);
> + g_free(ent);
> + skip_extent = true;
> + } else if (!cxl_dc_extent_exists(&dcd->dc.extents, &extents[i])) {
> + /* If the exact extent is not in the accepted list, skip */
> + skip_extent = true;
> + }
> + }
> +
> + /* No duplicate or overlapped extents are allowed */
> + if (test_any_bits_set(blk_bitmap, offset / block_size,
> + len / block_size)) {
> + error_setg(errp, "duplicate or overlapped extents are detected");
> + return;
> + }
> + bitmap_set(blk_bitmap, offset / block_size, len / block_size);
> +
> + list = list->next;
> + if (!skip_extent) {
> + i++;
> + }
> + }
> + num_extents = i;
> +
> + switch (type) {
> + case DC_EVENT_ADD_CAPACITY:
> + break;
> + default:
> + break;
> + }
> + /*
> + * CXL r3.1 section 8.2.9.2.1.6: Dynamic Capacity Event Record
> + *
> + * All Dynamic Capacity event records shall set the Event Record Severity
> + * field in the Common Event Record Format to Informational Event. All
> + * Dynamic Capacity related events shall be logged in the Dynamic Capacity
> + * Event Log.
> + */
> + cxl_assign_event_header(hdr, &dynamic_capacity_uuid, flags, sizeof(dCap),
> + cxl_device_get_timestamp(&dcd->cxl_dstate));
> +
> + dCap.type = type;
> + /* FIXME: for now, validaity flag is cleared */
spelling
> + dCap.validity_flags = 0;
> + stw_le_p(&dCap.host_id, hid);
> + /* only valid for DC_REGION_CONFIG_UPDATED event */
> + dCap.updated_region_id = 0;
> + /*
> + * FIXME: for now, "More" flag is cleared as there is only one extent for
> + * each record
This need more info. If they have the same tag then should set more on the
records.
> + */
> + dCap.flags = 0;
> +
> + /*
> + * For current implementation, each DC event record only associates with
> + * one extent, so the "More" flag does not need to be set.
> + */
> + for (i = 0; i < num_extents; i++) {
> + memcpy(&dCap.dynamic_capacity_extent, &extents[i],
> + sizeof(CXLDCExtentRaw));
> +
> + if (type == DC_EVENT_ADD_CAPACITY) {
> + cxl_insert_extent_to_extent_list(&dcd->dc.extents_pending_to_add,
> + extents[i].start_dpa,
> + extents[i].len,
> + extents[i].tag,
> + extents[i].shared_seq);
> + }
> +
> + if (cxl_event_insert(&dcd->cxl_dstate, enc_log,
> + (CXLEventRecordRaw *)&dCap)) {
> + cxl_event_irq_assert(dcd);
> + }
> + }
> +}
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support
2024-02-26 17:33 ` Jonathan Cameron via
(?)
@ 2024-02-26 19:16 ` fan
-1 siblings, 0 replies; 44+ messages in thread
From: fan @ 2024-02-26 19:16 UTC (permalink / raw)
To: Jonathan Cameron
Cc: nifan.cxl, qemu-devel, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, jim.harris,
Fan Ni
On Mon, Feb 26, 2024 at 05:33:17PM +0000, Jonathan Cameron wrote:
> On Wed, 21 Feb 2024 10:15:55 -0800
> nifan.cxl@gmail.com wrote:
>
> > From: Fan Ni <fan.ni@samsung.com>
> >
> > Per cxl spec r3.1, add dynamic capacity region representative based on
> > Table 8-165 and extend the cxl type3 device definition to include dc region
> > information. Also, based on info in 8.2.9.9.9.1, add 'Get Dynamic Capacity
> > Configuration' mailbox support.
> >
> > Note: decode_len of a dc region is aligned to 256*MiB, divided by
> > 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> > mailbox command.
> >
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> Hi Fan,
>
> A few comments inline.
>
> Jonathan
>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 110 ++++++++++++++++++++++++++++++++++++
> > include/hw/cxl/cxl_device.h | 16 ++++++
> > 2 files changed, 126 insertions(+)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index ba1d9901df..88e3b733e3 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -22,6 +22,7 @@
> >
> > #define CXL_CAPACITY_MULTIPLIER (256 * MiB)
> > #define CXL_DC_EVENT_LOG_SIZE 8
> > +#define CXL_SPEC_AFTER_R30
> As below. Drop this. Kernel code needs to be able to cope with newer specs
> than it understands anyway so should be fine with the larger records (otherwise
> it's buggy and needs fixing!)
Will remove it.
>
> >
> > /*
> > * How to add a new command, example. The command set FOO, with cmd BAR.
> > @@ -80,6 +81,8 @@ enum {
> > #define GET_POISON_LIST 0x0
> > #define INJECT_POISON 0x1
> > #define CLEAR_POISON 0x2
> > + DCD_CONFIG = 0x48,
> > + #define GET_DC_CONFIG 0x0
> > PHYSICAL_SWITCH = 0x51,
> > #define IDENTIFY_SWITCH_DEVICE 0x0
> > #define GET_PHYSICAL_PORT_STATE 0x1
> > @@ -1238,6 +1241,103 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
> > return CXL_MBOX_SUCCESS;
> > }
> >
> > +/*
> > + * CXL r3.1 section 8.2.9.9.9.1: Get Dynamic Capacity Configuration
> > + * (Opcode: 4800h)
> > + */
> > +static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> > + uint8_t *payload_in,
> > + size_t len_in,
> > + uint8_t *payload_out,
> > + size_t *len_out,
> > + CXLCCI *cci)
> > +{
> > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > + struct get_dyn_cap_config_in_pl {
> Type not needed - see below.
> > + uint8_t region_cnt;
> > + uint8_t start_region_id;
> > + } QEMU_PACKED;
> } QEMU_PACKED *in;
Get your point, it seems I followed the wrong example like
cmd_media_clear_poison and cmd_inject_poison :-<
Fan
>
> > +
> > + struct get_dyn_cap_config_out_pl {
> Don't think giving this a type is necessary - see below.
> > + uint8_t num_regions;
> > + uint8_t regions_returned;
> > + uint8_t rsvd1[6];
> > + struct {
> > + uint64_t base;
> > + uint64_t decode_len;
> > + uint64_t region_len;
> > + uint64_t block_size;
> > + uint32_t dsmadhandle;
> > + uint8_t flags;
> > + uint8_t rsvd2[3];
> > + } QEMU_PACKED records[];
> > + /*
> > + * if cxl spec version >= 3.1, extra output payload as defined
> > + * in struct get_dyn_cap_config_out_pl_extra comes here.
> > + */
> > + } QEMU_PACKED;
> } QEMU_PACKED *out;
> > +
> > + struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
> > + struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
>
> We've (mostly) use the (void *) casting where we haven't given the structures
> a type. I think I'd prefer we kept to that style for consistency.
>
> There is an argument we should have given all these types
> for readability reasons and to avoid casting via void * but
> we have gone this way now - with the exception of
> the poison list - oops.
>
> > + uint16_t record_count = 0;
> > + uint16_t i;
> > + uint16_t out_pl_len;
> > + uint8_t start_region_id = in->start_region_id;
> > +#ifdef CXL_SPEC_AFTER_R30
>
> Handy for testing, but I'd drop the ifdef for the final
> version. We don't need to support old specs.
>
> > + struct get_dyn_cap_config_out_pl_extra {
> > + uint32_t num_extents_supported;
> > + uint32_t num_extents_available;
> > + uint32_t num_tags_supported;
> > + uint32_t num_tags_available;
> > + } QEMU_PACKED;
> > + struct get_dyn_cap_config_out_pl_extra *extra_out;
> As above, anonymous structure should work ok.
> > +#endif
> > +
> > + if (start_region_id >= ct3d->dc.num_regions) {
> > + return CXL_MBOX_INVALID_INPUT;
> > + }
> > +
> > + record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
> > + in->region_cnt);
> > +
> > + out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> > +#ifdef CXL_SPEC_AFTER_R30
> > + extra_out = (struct get_dyn_cap_config_out_pl_extra *)(payload_out +
> > + out_pl_len);
> > + out_pl_len += sizeof(struct get_dyn_cap_config_out_pl_extra);
> > +#endif
> > + assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> > +
> > + out->num_regions = ct3d->dc.num_regions;
> > +#ifdef CXL_SPEC_AFTER_R30
> > + out->regions_returned = record_count;
> > +#endif
> > + for (i = 0; i < record_count; i++) {
> > + stq_le_p(&out->records[i].base,
> > + ct3d->dc.regions[start_region_id + i].base);
> Qemu allows a couple of style choices, but don't think this matches any of them.
> https://elixir.bootlin.com/qemu/latest/source/docs/devel/style.rst#L79
>
> My preference is immediately after opening bracket.
>
>
> > + stq_le_p(&out->records[i].decode_len,
> > + ct3d->dc.regions[start_region_id + i].decode_len /
> > + CXL_CAPACITY_MULTIPLIER);
> > + stq_le_p(&out->records[i].region_len,
> > + ct3d->dc.regions[start_region_id + i].len);
> > + stq_le_p(&out->records[i].block_size,
> > + ct3d->dc.regions[start_region_id + i].block_size);
> > + stl_le_p(&out->records[i].dsmadhandle,
> > + ct3d->dc.regions[start_region_id + i].dsmadhandle);
> > + out->records[i].flags = ct3d->dc.regions[start_region_id + i].flags;
> > + }
> > +#ifdef CXL_SPEC_AFTER_R30
> > + /*FIXME: need to set valid values in the future*/
>
> We need to do that before upstreaming..
> For tags, 0 is fine for now.
>
> For extents allow say 512 and count how many are in lists
> I guess then subtract that.
>
> > + stq_le_p(&extra_out->num_extents_supported, 0);
> > + stq_le_p(&extra_out->num_extents_available, 0);
> > + stq_le_p(&extra_out->num_tags_supported, 0);
> > + stq_le_p(&extra_out->num_tags_available, 0);
> > +#endif
> > +
> > + *len_out = out_pl_len;
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
> > #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> > #define IMMEDIATE_DATA_CHANGE (1 << 2)
> > #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> > @@ -1282,6 +1382,11 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
> > cmd_media_clear_poison, 72, 0 },
> > };
> >
> > +static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
> > + [DCD_CONFIG][GET_DC_CONFIG] = { "DCD_GET_DC_CONFIG",
> > + cmd_dcd_get_dyn_cap_config, 2, 0 },
> > +};
> > +
> > static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> > [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 },
> > [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { "BACKGROUND_OPERATION_STATUS",
> > @@ -1487,7 +1592,12 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> >
> > void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
> > {
> > + CXLType3Dev *ct3d = CXL_TYPE3(d);
> > +
> > cxl_copy_cci_commands(cci, cxl_cmd_set);
> > + if (ct3d->dc.num_regions) {
> > + cxl_copy_cci_commands(cci, cxl_cmd_set_dcd);
> > + }
> > cci->d = d;
> >
> > /* No separation for PCI MB as protocol handled in PCI device */
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 3cf3077afa..6df7fecdf1 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -422,6 +422,17 @@ typedef struct CXLPoison {
> > typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> > #define CXL_POISON_LIST_LIMIT 256
> >
> > +#define DCD_MAX_REGION_NUM 8
> Really trivial but call this
>
> DCD_MAX_NUM_REGION because it's the maximum number of
> regions. The maximum region number is 7 (as zero indexed).
>
> > +
> > +typedef struct CXLDCDRegion {
> > + uint64_t base;
> Probably makes sense to call out that bas also aligned to 256 MiB
> as not immediately clear the comment applies to both.
> Easiest will be to duplicate it.
> > + uint64_t decode_len; /* aligned to 256*MiB */
> > + uint64_t len;
> > + uint64_t block_size;
> > + uint32_t dsmadhandle;
> > + uint8_t flags;
> > +} CXLDCDRegion;
> > +
> > struct CXLType3Dev {
> > /* Private */
> > PCIDevice parent_obj;
> > @@ -454,6 +465,11 @@ struct CXLType3Dev {
> > unsigned int poison_list_cnt;
> > bool poison_list_overflowed;
> > uint64_t poison_list_overflow_ts;
> > +
> > + struct dynamic_capacity {
> > + uint8_t num_regions; /* 0-8 regions */
> > + CXLDCDRegion regions[DCD_MAX_REGION_NUM];
> > + } dc;
> > };
> >
> > #define TYPE_CXL_TYPE3 "cxl-type3"
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
2024-02-26 18:04 ` Jonathan Cameron via
(?)
@ 2024-02-27 1:01 ` fan
2024-02-27 10:39 ` Jonathan Cameron via
-1 siblings, 1 reply; 44+ messages in thread
From: fan @ 2024-02-27 1:01 UTC (permalink / raw)
To: Jonathan Cameron
Cc: nifan.cxl, qemu-devel, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, jim.harris,
Fan Ni
On Mon, Feb 26, 2024 at 06:04:17PM +0000, Jonathan Cameron wrote:
> On Wed, 21 Feb 2024 10:16:01 -0800
> nifan.cxl@gmail.com wrote:
>
> > From: Fan Ni <fan.ni@samsung.com>
> >
> > Per CXL spec 3.1, two mailbox commands are implemented:
> > Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> > Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
> >
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
>
> Hi Fan,
>
> Comments on this are all about corner cases. If we can I think we need
> to cover a few more. Linux won't hit them (I think) so it will be
> a bit of a pain to test but maybe raw commands enabled and some
> userspace code will let us exercise the corner cases?
>
> Jonathan
>
>
>
> > +
> > +/*
> > + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (opcode 4803h)
> > + */
> > +static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> > + uint8_t *payload_in,
> > + size_t len_in,
> > + uint8_t *payload_out,
> > + size_t *len_out,
> > + CXLCCI *cci)
> > +{
> > + CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> > + CXLDCExtent *ent;
> > + uint32_t i;
> > + uint64_t dpa, len;
> > + CXLRetCode ret;
> > +
> > + if (in->num_entries_updated == 0) {
> > + return CXL_MBOX_INVALID_INPUT;
> > + }
> > +
> > + ret = cxl_detect_malformed_extent_list(ct3d, in);
> > + if (ret != CXL_MBOX_SUCCESS) {
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < in->num_entries_updated; i++) {
> > + bool found = false;
> > +
> > + dpa = in->updated_entries[i].start_dpa;
> > + len = in->updated_entries[i].len;
> > +
> > + QTAILQ_FOREACH(ent, extent_list, node) {
> > + if (ent->start_dpa <= dpa &&
> > + dpa + len <= ent->start_dpa + ent->len) {
> > + /*
> > + * If an incoming extent covers a portion of an extent
> > + * in the device extent list, remove only the overlapping
> > + * portion, meaning
> > + * 1. the portions that are not covered by the incoming
> > + * extent at both end of the original extent will become
> > + * new extents and inserted to the extent list; and
> > + * 2. the original extent is removed from the extent list;
> > + * 3. dc extent count is updated accordingly.
> > + */
> > + uint64_t ent_start_dpa = ent->start_dpa;
> > + uint64_t ent_len = ent->len;
> > + uint64_t len1 = dpa - ent_start_dpa;
> > + uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
> > +
> > + found = true;
> > + cxl_remove_extent_from_extent_list(extent_list, ent);
> > + ct3d->dc.total_extent_count -= 1;
> > +
> > + if (len1) {
> > + cxl_insert_extent_to_extent_list(extent_list,
> > + ent_start_dpa, len1,
> > + NULL, 0);
> > + ct3d->dc.total_extent_count += 1;
> > + }
> > + if (len2) {
> > + cxl_insert_extent_to_extent_list(extent_list, dpa + len,
> > + len2, NULL, 0);
> > + ct3d->dc.total_extent_count += 1;
>
> There is a non zero chance that we'll overflow however many extents we claim
> to support. So we need to check that and fail the remove if it happens.
> Could ignore this for now though as that value is (I think!) conservative
> to allow for complex extent list tracking implementations. Succeeding
> when a naive solution would fail due to running out of extents that it can
> manage is not (I think) a bug.
Yeah. spec r3.1 mentioned about the overflow issue that adding/releasing
extent requests can raise. We should fail the operation if running out of
extents and report resource exhausted.
>
> > + }
> > + break;
> > + /*Currently we reject the attempt to remove a superset*/
>
> Space after /* and before */
>
> I think we need to fix this. Linux isn't going to do it any time soon, but
> I think it's allowed to allocate two extents next to each other then free them
> in one go. Isn't this case easy to do or are there awkward corners?
> If it's sufficiently nasty (maybe because only part of extent provided exists?)
> then maybe we can leave it for now.
>
> I worry about something like
>
> | EXTENT TO FREE |
> | Exists | gap | Exists |
> Where we have to check for gap before removing anything?
> Does the spec address this? Not that I can find.
> I think the implication is we have to do a validation pass, then a free
> pass after we know whole of requested extent is valid.
> Nasty to test if nothing else :( Would look much like your check
> on malformed extent lists.
>
I cannot find anything specific to this in the specification either.
Since we have already detected the case where the extent range across
multiple regions, the only case we need to capture here is one/multiple
portions of an extents getting released and causing extent overflow.
I think we can handle it after we introduce the bitmaps (PATCH 10) which
indicates DPA ranges mapped by valid extents in the device.
With that, The release workflow would be
1) detecting malformed extent lists; if passed
2) do cxl_detect_extent_overflow {
delta = 0;
make a copy of the bitmap as bitmap_copy;
for each extent in the updated_extent_list; do
if (extent range not fully set in the bitmap_copy)
return error;
else {
if gap at the front based on the bitmap_copy:
delta += 1;
if gap at the end based on the bitmap_copy:
delta += 1;
delta -= 1;
// NOTE: current_extent_count will not be updated in the
// loop since delta will track the whole loop
if (delta + current_extent_count > max_extent_count)
return resource exhausted;
update bitmap_copy to clear the range covered by the extent
under consideration;
}
done
}; if pass
3. do real release: in the pass, we will not need to detect extent
errors;
Does the above solution sound reasonable? If so, do we want to go this
way? do we need to introduce the bitmap earlier in the series?
Thanks,
Fan
>
> > + } else if ((dpa < ent->start_dpa + ent->len &&
> > + dpa + len > ent->start_dpa + ent->len) ||
> > + (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) {
> > + return CXL_MBOX_INVALID_EXTENT_LIST;
> > + }
> > + }
> > +
> > + if (!found) {
> > + /* Try to remove a non-existing extent */
> > + return CXL_MBOX_INVALID_PA;
> > + }
> > + }
> > +
> > + return CXL_MBOX_SUCCESS;
> > +}
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
2024-02-26 18:04 ` Jonathan Cameron via
(?)
(?)
@ 2024-02-27 1:06 ` fan
-1 siblings, 0 replies; 44+ messages in thread
From: fan @ 2024-02-27 1:06 UTC (permalink / raw)
To: Jonathan Cameron
Cc: nifan.cxl, qemu-devel, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, jim.harris,
Fan Ni
On Mon, Feb 26, 2024 at 06:04:17PM +0000, Jonathan Cameron wrote:
> On Wed, 21 Feb 2024 10:16:01 -0800
> nifan.cxl@gmail.com wrote:
>
> > From: Fan Ni <fan.ni@samsung.com>
> >
> > Per CXL spec 3.1, two mailbox commands are implemented:
> > Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> > Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
> >
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
>
> Hi Fan,
>
> Comments on this are all about corner cases. If we can I think we need
> to cover a few more. Linux won't hit them (I think) so it will be
> a bit of a pain to test but maybe raw commands enabled and some
> userspace code will let us exercise the corner cases?
>
> Jonathan
>
>
>
> > +
> > +/*
> > + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (opcode 4803h)
> > + */
> > +static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> > + uint8_t *payload_in,
> > + size_t len_in,
> > + uint8_t *payload_out,
> > + size_t *len_out,
> > + CXLCCI *cci)
> > +{
> > + CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> > + CXLDCExtent *ent;
> > + uint32_t i;
> > + uint64_t dpa, len;
> > + CXLRetCode ret;
> > +
> > + if (in->num_entries_updated == 0) {
> > + return CXL_MBOX_INVALID_INPUT;
> > + }
> > +
> > + ret = cxl_detect_malformed_extent_list(ct3d, in);
> > + if (ret != CXL_MBOX_SUCCESS) {
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < in->num_entries_updated; i++) {
> > + bool found = false;
> > +
> > + dpa = in->updated_entries[i].start_dpa;
> > + len = in->updated_entries[i].len;
> > +
> > + QTAILQ_FOREACH(ent, extent_list, node) {
> > + if (ent->start_dpa <= dpa &&
> > + dpa + len <= ent->start_dpa + ent->len) {
> > + /*
> > + * If an incoming extent covers a portion of an extent
> > + * in the device extent list, remove only the overlapping
> > + * portion, meaning
> > + * 1. the portions that are not covered by the incoming
> > + * extent at both end of the original extent will become
> > + * new extents and inserted to the extent list; and
> > + * 2. the original extent is removed from the extent list;
> > + * 3. dc extent count is updated accordingly.
> > + */
> > + uint64_t ent_start_dpa = ent->start_dpa;
> > + uint64_t ent_len = ent->len;
> > + uint64_t len1 = dpa - ent_start_dpa;
> > + uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
> > +
> > + found = true;
> > + cxl_remove_extent_from_extent_list(extent_list, ent);
> > + ct3d->dc.total_extent_count -= 1;
> > +
> > + if (len1) {
> > + cxl_insert_extent_to_extent_list(extent_list,
> > + ent_start_dpa, len1,
> > + NULL, 0);
> > + ct3d->dc.total_extent_count += 1;
> > + }
> > + if (len2) {
> > + cxl_insert_extent_to_extent_list(extent_list, dpa + len,
> > + len2, NULL, 0);
> > + ct3d->dc.total_extent_count += 1;
>
> There is a non zero chance that we'll overflow however many extents we claim
> to support. So we need to check that and fail the remove if it happens.
> Could ignore this for now though as that value is (I think!) conservative
> to allow for complex extent list tracking implementations. Succeeding
> when a naive solution would fail due to running out of extents that it can
> manage is not (I think) a bug.
>
> > + }
> > + break;
> > + /*Currently we reject the attempt to remove a superset*/
>
> Space after /* and before */
>
> I think we need to fix this. Linux isn't going to do it any time soon, but
> I think it's allowed to allocate two extents next to each other then free them
> in one go. Isn't this case easy to do or are there awkward corners?
If we use the bitmap (indicating each range is filled by valid extents)
in PATCH 10, it should not be that difficult to do.
Fan
> If it's sufficiently nasty (maybe because only part of extent provided exists?)
> then maybe we can leave it for now.
>
> I worry about something like
>
> | EXTENT TO FREE |
> | Exists | gap | Exists |
> Where we have to check for gap before removing anything?
> Does the spec address this? Not that I can find.
> I think the implication is we have to do a validation pass, then a free
> pass after we know whole of requested extent is valid.
> Nasty to test if nothing else :( Would look much like your check
> on malformed extent lists.
>
>
> > + } else if ((dpa < ent->start_dpa + ent->len &&
> > + dpa + len > ent->start_dpa + ent->len) ||
> > + (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) {
> > + return CXL_MBOX_INVALID_EXTENT_LIST;
> > + }
> > + }
> > +
> > + if (!found) {
> > + /* Try to remove a non-existing extent */
> > + return CXL_MBOX_INVALID_PA;
> > + }
> > + }
> > +
> > + return CXL_MBOX_SUCCESS;
> > +}
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
2024-02-27 1:01 ` fan
@ 2024-02-27 10:39 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron @ 2024-02-27 10:39 UTC (permalink / raw)
To: fan
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Mon, 26 Feb 2024 17:01:22 -0800
fan <nifan.cxl@gmail.com> wrote:
> On Mon, Feb 26, 2024 at 06:04:17PM +0000, Jonathan Cameron wrote:
> > On Wed, 21 Feb 2024 10:16:01 -0800
> > nifan.cxl@gmail.com wrote:
> >
> > > From: Fan Ni <fan.ni@samsung.com>
> > >
> > > Per CXL spec 3.1, two mailbox commands are implemented:
> > > Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> > > Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
> > >
> > > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> >
> > Hi Fan,
> >
> > Comments on this are all about corner cases. If we can I think we need
> > to cover a few more. Linux won't hit them (I think) so it will be
> > a bit of a pain to test but maybe raw commands enabled and some
> > userspace code will let us exercise the corner cases?
> >
> > Jonathan
> >
> >
> >
> > > +
> > > +/*
> > > + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (opcode 4803h)
> > > + */
> > > +static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> > > + uint8_t *payload_in,
> > > + size_t len_in,
> > > + uint8_t *payload_out,
> > > + size_t *len_out,
> > > + CXLCCI *cci)
> > > +{
> > > + CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> > > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> > > + CXLDCExtent *ent;
> > > + uint32_t i;
> > > + uint64_t dpa, len;
> > > + CXLRetCode ret;
> > > +
> > > + if (in->num_entries_updated == 0) {
> > > + return CXL_MBOX_INVALID_INPUT;
> > > + }
> > > +
> > > + ret = cxl_detect_malformed_extent_list(ct3d, in);
> > > + if (ret != CXL_MBOX_SUCCESS) {
> > > + return ret;
> > > + }
> > > +
> > > + for (i = 0; i < in->num_entries_updated; i++) {
> > > + bool found = false;
> > > +
> > > + dpa = in->updated_entries[i].start_dpa;
> > > + len = in->updated_entries[i].len;
> > > +
> > > + QTAILQ_FOREACH(ent, extent_list, node) {
> > > + if (ent->start_dpa <= dpa &&
> > > + dpa + len <= ent->start_dpa + ent->len) {
> > > + /*
> > > + * If an incoming extent covers a portion of an extent
> > > + * in the device extent list, remove only the overlapping
> > > + * portion, meaning
> > > + * 1. the portions that are not covered by the incoming
> > > + * extent at both end of the original extent will become
> > > + * new extents and inserted to the extent list; and
> > > + * 2. the original extent is removed from the extent list;
> > > + * 3. dc extent count is updated accordingly.
> > > + */
> > > + uint64_t ent_start_dpa = ent->start_dpa;
> > > + uint64_t ent_len = ent->len;
> > > + uint64_t len1 = dpa - ent_start_dpa;
> > > + uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
> > > +
> > > + found = true;
> > > + cxl_remove_extent_from_extent_list(extent_list, ent);
> > > + ct3d->dc.total_extent_count -= 1;
> > > +
> > > + if (len1) {
> > > + cxl_insert_extent_to_extent_list(extent_list,
> > > + ent_start_dpa, len1,
> > > + NULL, 0);
> > > + ct3d->dc.total_extent_count += 1;
> > > + }
> > > + if (len2) {
> > > + cxl_insert_extent_to_extent_list(extent_list, dpa + len,
> > > + len2, NULL, 0);
> > > + ct3d->dc.total_extent_count += 1;
> >
> > There is a non zero chance that we'll overflow however many extents we claim
> > to support. So we need to check that and fail the remove if it happens.
> > Could ignore this for now though as that value is (I think!) conservative
> > to allow for complex extent list tracking implementations. Succeeding
> > when a naive solution would fail due to running out of extents that it can
> > manage is not (I think) a bug.
>
> Yeah. spec r3.1 mentioned about the overflow issue that adding/releasing
> extent requests can raise. We should fail the operation if running out of
> extents and report resource exhausted.
>
> >
> > > + }
> > > + break;
> > > + /*Currently we reject the attempt to remove a superset*/
> >
> > Space after /* and before */
> >
> > I think we need to fix this. Linux isn't going to do it any time soon, but
> > I think it's allowed to allocate two extents next to each other then free them
> > in one go. Isn't this case easy to do or are there awkward corners?
> > If it's sufficiently nasty (maybe because only part of extent provided exists?)
> > then maybe we can leave it for now.
> >
> > I worry about something like
> >
> > | EXTENT TO FREE |
> > | Exists | gap | Exists |
> > Where we have to check for gap before removing anything?
> > Does the spec address this? Not that I can find.
> > I think the implication is we have to do a validation pass, then a free
> > pass after we know whole of requested extent is valid.
> > Nasty to test if nothing else :( Would look much like your check
> > on malformed extent lists.
> >
>
> I cannot find anything specific to this in the specification either.
> Since we have already detected the case where the extent range across
> multiple regions, the only case we need to capture here is one/multiple
> portions of an extents getting released and causing extent overflow.
> I think we can handle it after we introduce the bitmaps (PATCH 10) which
> indicates DPA ranges mapped by valid extents in the device.
>
> With that, The release workflow would be
>
> 1) detecting malformed extent lists; if passed
> 2) do cxl_detect_extent_overflow {
> delta = 0;
> make a copy of the bitmap as bitmap_copy;
> for each extent in the updated_extent_list; do
> if (extent range not fully set in the bitmap_copy)
> return error;
> else {
> if gap at the front based on the bitmap_copy:
> delta += 1;
> if gap at the end based on the bitmap_copy:
> delta += 1;
> delta -= 1;
> // NOTE: current_extent_count will not be updated in the
> // loop since delta will track the whole loop
> if (delta + current_extent_count > max_extent_count)
> return resource exhausted;
> update bitmap_copy to clear the range covered by the extent
> under consideration;
> }
> done
>
> }; if pass
> 3. do real release: in the pass, we will not need to detect extent
> errors;
>
> Does the above solution sound reasonable? If so, do we want to go this
> way? do we need to introduce the bitmap earlier in the series?
Yes, something along these lines should work nicely.
Jonathan
>
> Thanks,
> Fan
>
>
>
> >
> > > + } else if ((dpa < ent->start_dpa + ent->len &&
> > > + dpa + len > ent->start_dpa + ent->len) ||
> > > + (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) {
> > > + return CXL_MBOX_INVALID_EXTENT_LIST;
> > > + }
> > > + }
> > > +
> > > + if (!found) {
> > > + /* Try to remove a non-existing extent */
> > > + return CXL_MBOX_INVALID_PA;
> > > + }
> > > + }
> > > +
> > > + return CXL_MBOX_SUCCESS;
> > > +}
> >
> >
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
@ 2024-02-27 10:39 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron via @ 2024-02-27 10:39 UTC (permalink / raw)
To: fan
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
On Mon, 26 Feb 2024 17:01:22 -0800
fan <nifan.cxl@gmail.com> wrote:
> On Mon, Feb 26, 2024 at 06:04:17PM +0000, Jonathan Cameron wrote:
> > On Wed, 21 Feb 2024 10:16:01 -0800
> > nifan.cxl@gmail.com wrote:
> >
> > > From: Fan Ni <fan.ni@samsung.com>
> > >
> > > Per CXL spec 3.1, two mailbox commands are implemented:
> > > Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> > > Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
> > >
> > > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> >
> > Hi Fan,
> >
> > Comments on this are all about corner cases. If we can I think we need
> > to cover a few more. Linux won't hit them (I think) so it will be
> > a bit of a pain to test but maybe raw commands enabled and some
> > userspace code will let us exercise the corner cases?
> >
> > Jonathan
> >
> >
> >
> > > +
> > > +/*
> > > + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (opcode 4803h)
> > > + */
> > > +static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> > > + uint8_t *payload_in,
> > > + size_t len_in,
> > > + uint8_t *payload_out,
> > > + size_t *len_out,
> > > + CXLCCI *cci)
> > > +{
> > > + CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> > > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> > > + CXLDCExtent *ent;
> > > + uint32_t i;
> > > + uint64_t dpa, len;
> > > + CXLRetCode ret;
> > > +
> > > + if (in->num_entries_updated == 0) {
> > > + return CXL_MBOX_INVALID_INPUT;
> > > + }
> > > +
> > > + ret = cxl_detect_malformed_extent_list(ct3d, in);
> > > + if (ret != CXL_MBOX_SUCCESS) {
> > > + return ret;
> > > + }
> > > +
> > > + for (i = 0; i < in->num_entries_updated; i++) {
> > > + bool found = false;
> > > +
> > > + dpa = in->updated_entries[i].start_dpa;
> > > + len = in->updated_entries[i].len;
> > > +
> > > + QTAILQ_FOREACH(ent, extent_list, node) {
> > > + if (ent->start_dpa <= dpa &&
> > > + dpa + len <= ent->start_dpa + ent->len) {
> > > + /*
> > > + * If an incoming extent covers a portion of an extent
> > > + * in the device extent list, remove only the overlapping
> > > + * portion, meaning
> > > + * 1. the portions that are not covered by the incoming
> > > + * extent at both end of the original extent will become
> > > + * new extents and inserted to the extent list; and
> > > + * 2. the original extent is removed from the extent list;
> > > + * 3. dc extent count is updated accordingly.
> > > + */
> > > + uint64_t ent_start_dpa = ent->start_dpa;
> > > + uint64_t ent_len = ent->len;
> > > + uint64_t len1 = dpa - ent_start_dpa;
> > > + uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
> > > +
> > > + found = true;
> > > + cxl_remove_extent_from_extent_list(extent_list, ent);
> > > + ct3d->dc.total_extent_count -= 1;
> > > +
> > > + if (len1) {
> > > + cxl_insert_extent_to_extent_list(extent_list,
> > > + ent_start_dpa, len1,
> > > + NULL, 0);
> > > + ct3d->dc.total_extent_count += 1;
> > > + }
> > > + if (len2) {
> > > + cxl_insert_extent_to_extent_list(extent_list, dpa + len,
> > > + len2, NULL, 0);
> > > + ct3d->dc.total_extent_count += 1;
> >
> > There is a non zero chance that we'll overflow however many extents we claim
> > to support. So we need to check that and fail the remove if it happens.
> > Could ignore this for now though as that value is (I think!) conservative
> > to allow for complex extent list tracking implementations. Succeeding
> > when a naive solution would fail due to running out of extents that it can
> > manage is not (I think) a bug.
>
> Yeah. spec r3.1 mentioned about the overflow issue that adding/releasing
> extent requests can raise. We should fail the operation if running out of
> extents and report resource exhausted.
>
> >
> > > + }
> > > + break;
> > > + /*Currently we reject the attempt to remove a superset*/
> >
> > Space after /* and before */
> >
> > I think we need to fix this. Linux isn't going to do it any time soon, but
> > I think it's allowed to allocate two extents next to each other then free them
> > in one go. Isn't this case easy to do or are there awkward corners?
> > If it's sufficiently nasty (maybe because only part of extent provided exists?)
> > then maybe we can leave it for now.
> >
> > I worry about something like
> >
> > | EXTENT TO FREE |
> > | Exists | gap | Exists |
> > Where we have to check for gap before removing anything?
> > Does the spec address this? Not that I can find.
> > I think the implication is we have to do a validation pass, then a free
> > pass after we know whole of requested extent is valid.
> > Nasty to test if nothing else :( Would look much like your check
> > on malformed extent lists.
> >
>
> I cannot find anything specific to this in the specification either.
> Since we have already detected the case where the extent range across
> multiple regions, the only case we need to capture here is one/multiple
> portions of an extents getting released and causing extent overflow.
> I think we can handle it after we introduce the bitmaps (PATCH 10) which
> indicates DPA ranges mapped by valid extents in the device.
>
> With that, The release workflow would be
>
> 1) detecting malformed extent lists; if passed
> 2) do cxl_detect_extent_overflow {
> delta = 0;
> make a copy of the bitmap as bitmap_copy;
> for each extent in the updated_extent_list; do
> if (extent range not fully set in the bitmap_copy)
> return error;
> else {
> if gap at the front based on the bitmap_copy:
> delta += 1;
> if gap at the end based on the bitmap_copy:
> delta += 1;
> delta -= 1;
> // NOTE: current_extent_count will not be updated in the
> // loop since delta will track the whole loop
> if (delta + current_extent_count > max_extent_count)
> return resource exhausted;
> update bitmap_copy to clear the range covered by the extent
> under consideration;
> }
> done
>
> }; if pass
> 3. do real release: in the pass, we will not need to detect extent
> errors;
>
> Does the above solution sound reasonable? If so, do we want to go this
> way? do we need to introduce the bitmap earlier in the series?
Yes, something along these lines should work nicely.
Jonathan
>
> Thanks,
> Fan
>
>
>
> >
> > > + } else if ((dpa < ent->start_dpa + ent->len &&
> > > + dpa + len > ent->start_dpa + ent->len) ||
> > > + (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) {
> > > + return CXL_MBOX_INVALID_EXTENT_LIST;
> > > + }
> > > + }
> > > +
> > > + if (!found) {
> > > + /* Try to remove a non-existing extent */
> > > + return CXL_MBOX_INVALID_PA;
> > > + }
> > > + }
> > > +
> > > + return CXL_MBOX_SUCCESS;
> > > +}
> >
> >
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
2024-02-27 10:39 ` Jonathan Cameron via
(?)
@ 2024-03-01 3:56 ` fan
2024-03-06 14:58 ` Jonathan Cameron via
-1 siblings, 1 reply; 44+ messages in thread
From: fan @ 2024-03-01 3:56 UTC (permalink / raw)
To: Jonathan Cameron
Cc: fan, qemu-devel, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, jim.harris,
Fan Ni
On Tue, Feb 27, 2024 at 10:39:09AM +0000, Jonathan Cameron wrote:
> On Mon, 26 Feb 2024 17:01:22 -0800
> fan <nifan.cxl@gmail.com> wrote:
>
> > On Mon, Feb 26, 2024 at 06:04:17PM +0000, Jonathan Cameron wrote:
> > > On Wed, 21 Feb 2024 10:16:01 -0800
> > > nifan.cxl@gmail.com wrote:
> > >
> > > > From: Fan Ni <fan.ni@samsung.com>
> > > >
> > > > Per CXL spec 3.1, two mailbox commands are implemented:
> > > > Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> > > > Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
> > > >
> > > > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > >
> > > Hi Fan,
> > >
> > > Comments on this are all about corner cases. If we can I think we need
> > > to cover a few more. Linux won't hit them (I think) so it will be
> > > a bit of a pain to test but maybe raw commands enabled and some
> > > userspace code will let us exercise the corner cases?
> > >
> > > Jonathan
> > >
> > >
> > >
> > > > +
> > > > +/*
> > > > + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (opcode 4803h)
> > > > + */
> > > > +static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> > > > + uint8_t *payload_in,
> > > > + size_t len_in,
> > > > + uint8_t *payload_out,
> > > > + size_t *len_out,
> > > > + CXLCCI *cci)
> > > > +{
> > > > + CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> > > > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > > + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> > > > + CXLDCExtent *ent;
> > > > + uint32_t i;
> > > > + uint64_t dpa, len;
> > > > + CXLRetCode ret;
> > > > +
> > > > + if (in->num_entries_updated == 0) {
> > > > + return CXL_MBOX_INVALID_INPUT;
> > > > + }
> > > > +
> > > > + ret = cxl_detect_malformed_extent_list(ct3d, in);
> > > > + if (ret != CXL_MBOX_SUCCESS) {
> > > > + return ret;
> > > > + }
> > > > +
> > > > + for (i = 0; i < in->num_entries_updated; i++) {
> > > > + bool found = false;
> > > > +
> > > > + dpa = in->updated_entries[i].start_dpa;
> > > > + len = in->updated_entries[i].len;
> > > > +
> > > > + QTAILQ_FOREACH(ent, extent_list, node) {
> > > > + if (ent->start_dpa <= dpa &&
> > > > + dpa + len <= ent->start_dpa + ent->len) {
> > > > + /*
> > > > + * If an incoming extent covers a portion of an extent
> > > > + * in the device extent list, remove only the overlapping
> > > > + * portion, meaning
> > > > + * 1. the portions that are not covered by the incoming
> > > > + * extent at both end of the original extent will become
> > > > + * new extents and inserted to the extent list; and
> > > > + * 2. the original extent is removed from the extent list;
> > > > + * 3. dc extent count is updated accordingly.
> > > > + */
> > > > + uint64_t ent_start_dpa = ent->start_dpa;
> > > > + uint64_t ent_len = ent->len;
> > > > + uint64_t len1 = dpa - ent_start_dpa;
> > > > + uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
> > > > +
> > > > + found = true;
> > > > + cxl_remove_extent_from_extent_list(extent_list, ent);
> > > > + ct3d->dc.total_extent_count -= 1;
> > > > +
> > > > + if (len1) {
> > > > + cxl_insert_extent_to_extent_list(extent_list,
> > > > + ent_start_dpa, len1,
> > > > + NULL, 0);
> > > > + ct3d->dc.total_extent_count += 1;
> > > > + }
> > > > + if (len2) {
> > > > + cxl_insert_extent_to_extent_list(extent_list, dpa + len,
> > > > + len2, NULL, 0);
> > > > + ct3d->dc.total_extent_count += 1;
> > >
> > > There is a non zero chance that we'll overflow however many extents we claim
> > > to support. So we need to check that and fail the remove if it happens.
> > > Could ignore this for now though as that value is (I think!) conservative
> > > to allow for complex extent list tracking implementations. Succeeding
> > > when a naive solution would fail due to running out of extents that it can
> > > manage is not (I think) a bug.
> >
> > Yeah. spec r3.1 mentioned about the overflow issue that adding/releasing
> > extent requests can raise. We should fail the operation if running out of
> > extents and report resource exhausted.
> >
> > >
> > > > + }
> > > > + break;
> > > > + /*Currently we reject the attempt to remove a superset*/
> > >
> > > Space after /* and before */
> > >
> > > I think we need to fix this. Linux isn't going to do it any time soon, but
> > > I think it's allowed to allocate two extents next to each other then free them
> > > in one go. Isn't this case easy to do or are there awkward corners?
> > > If it's sufficiently nasty (maybe because only part of extent provided exists?)
> > > then maybe we can leave it for now.
> > >
> > > I worry about something like
> > >
> > > | EXTENT TO FREE |
> > > | Exists | gap | Exists |
> > > Where we have to check for gap before removing anything?
> > > Does the spec address this? Not that I can find.
> > > I think the implication is we have to do a validation pass, then a free
> > > pass after we know whole of requested extent is valid.
> > > Nasty to test if nothing else :( Would look much like your check
> > > on malformed extent lists.
> > >
> >
> > I cannot find anything specific to this in the specification either.
> > Since we have already detected the case where the extent range across
> > multiple regions, the only case we need to capture here is one/multiple
> > portions of an extents getting released and causing extent overflow.
> > I think we can handle it after we introduce the bitmaps (PATCH 10) which
> > indicates DPA ranges mapped by valid extents in the device.
> >
> > With that, The release workflow would be
> >
> > 1) detecting malformed extent lists; if passed
> > 2) do cxl_detect_extent_overflow {
> > delta = 0;
> > make a copy of the bitmap as bitmap_copy;
> > for each extent in the updated_extent_list; do
> > if (extent range not fully set in the bitmap_copy)
> > return error;
> > else {
> > if gap at the front based on the bitmap_copy:
> > delta += 1;
> > if gap at the end based on the bitmap_copy:
> > delta += 1;
> > delta -= 1;
> > // NOTE: current_extent_count will not be updated in the
> > // loop since delta will track the whole loop
> > if (delta + current_extent_count > max_extent_count)
> > return resource exhausted;
> > update bitmap_copy to clear the range covered by the extent
> > under consideration;
> > }
> > done
> >
> > }; if pass
> > 3. do real release: in the pass, we will not need to detect extent
> > errors;
> >
> > Does the above solution sound reasonable? If so, do we want to go this
> > way? do we need to introduce the bitmap earlier in the series?
>
> Yes, something along these lines should work nicely.
>
> Jonathan
Hi Jonathan,
I updated the code based on your feedback and now we can process extent
release request more flexible.
We can now support superset release (actually it can do even more,
as long as the DPA range is coverd by accepted extents, we can release).
I have run following tests and the code works as expected,
1. Add multiple extents, and removing them one by one, passed;
2. Superset release: add multiple extents with continuous DPA ranges, and
remove all of them with a single release request with an extent covering the
whole DPA range, passed;
3. Partial extent release: add a large extent and release only part of it,
passed;
4. Partial+superset release: add multiple extents,and release it with some
leftover with one request with an extent. For example, add extents [0-128M]
and [128M-256M], release [64M-256M]. Passed;
5. Release extent not aligned to block size, failed as expected;
6. Extents have overlaps, fail the request as expected;
7. Extent has uncovered DPA range, skip the extent as expected;
The only limitation is that for superset release case, if we find
part of its DPA range is still pending to add, while the other is
accepted, we reject it through QMP interface.
The latest code is https://github.com/moking/qemu/tree/dcd-v5.
The main changes are in the last three commits.
Btw, in the last commit, I introduce new QMP interfaces to print out
accepted and pending-to-add list in the device to a file "/tmp/qmp.txt",
do we want it? If yes, I can polish it a little bit, otherwise I will
keep it for my own test purpose.
I will test more and send out v5 if the above looks reasonable to you.
Fan
>
>
> >
> > Thanks,
> > Fan
> >
> >
> >
> > >
> > > > + } else if ((dpa < ent->start_dpa + ent->len &&
> > > > + dpa + len > ent->start_dpa + ent->len) ||
> > > > + (dpa < ent->start_dpa && dpa + len > ent->start_dpa)) {
> > > > + return CXL_MBOX_INVALID_EXTENT_LIST;
> > > > + }
> > > > + }
> > > > +
> > > > + if (!found) {
> > > > + /* Try to remove a non-existing extent */
> > > > + return CXL_MBOX_INVALID_PA;
> > > > + }
> > > > + }
> > > > +
> > > > + return CXL_MBOX_SUCCESS;
> > > > +}
> > >
> > >
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support
2024-02-21 18:15 ` [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support nifan.cxl
2024-02-26 17:33 ` Jonathan Cameron via
@ 2024-03-04 12:40 ` Jørgen Hansen
2024-03-04 17:35 ` fan
1 sibling, 1 reply; 44+ messages in thread
From: Jørgen Hansen @ 2024-03-04 12:40 UTC (permalink / raw)
To: nifan.cxl, qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, jim.harris,
Fan Ni
On 2/21/24 19:15, nifan.cxl@gmail.com wrote:
> CAUTION: This email originated from outside of Western Digital. Do not click on links or open attachments unless you recognize the sender and know that the content is safe.
>
>
> From: Fan Ni <fan.ni@samsung.com>
>
> Per cxl spec r3.1, add dynamic capacity region representative based on
> Table 8-165 and extend the cxl type3 device definition to include dc region
> information. Also, based on info in 8.2.9.9.9.1, add 'Get Dynamic Capacity
> Configuration' mailbox support.
>
> Note: decode_len of a dc region is aligned to 256*MiB, divided by
> 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> mailbox command.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 110 ++++++++++++++++++++++++++++++++++++
> include/hw/cxl/cxl_device.h | 16 ++++++
> 2 files changed, 126 insertions(+)
>
[snip]
>
> /* No separation for PCI MB as protocol handled in PCI device */
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 3cf3077afa..6df7fecdf1 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -422,6 +422,17 @@ typedef struct CXLPoison {
> typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> #define CXL_POISON_LIST_LIMIT 256
>
> +#define DCD_MAX_REGION_NUM 8
> +
> +typedef struct CXLDCDRegion {
> + uint64_t base;
> + uint64_t decode_len; /* aligned to 256*MiB */
> + uint64_t len;
> + uint64_t block_size;
> + uint32_t dsmadhandle;
> + uint8_t flags;
> +} CXLDCDRegion;
> +
To be consistent with the naming of extents, and functions defined later
on, shouldn't this be CXLDCRegion (DC instead of DCD)?
> struct CXLType3Dev {
> /* Private */
> PCIDevice parent_obj;
> @@ -454,6 +465,11 @@ struct CXLType3Dev {
> unsigned int poison_list_cnt;
> bool poison_list_overflowed;
> uint64_t poison_list_overflow_ts;
> +
> + struct dynamic_capacity {
> + uint8_t num_regions; /* 0-8 regions */
> + CXLDCDRegion regions[DCD_MAX_REGION_NUM];
> + } dc;
> };
>
> #define TYPE_CXL_TYPE3 "cxl-type3"
> --
> 2.43.0
>
>
Thanks,
Jorgen
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to type3 memory devices
2024-02-21 18:15 ` [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to " nifan.cxl
2024-02-26 17:38 ` Jonathan Cameron via
@ 2024-03-04 13:10 ` Jørgen Hansen
2024-03-04 17:31 ` fan
1 sibling, 1 reply; 44+ messages in thread
From: Jørgen Hansen @ 2024-03-04 13:10 UTC (permalink / raw)
To: nifan.cxl, qemu-devel
Cc: jonathan.cameron, linux-cxl, gregory.price, ira.weiny,
dan.j.williams, a.manzanares, dave, nmtadam.samsung, jim.harris,
Fan Ni
On 2/21/24 19:15, nifan.cxl@gmail.com wrote:
> CAUTION: This email originated from outside of Western Digital. Do not click on links or open attachments unless you recognize the sender and know that the content is safe.
>
>
> From: Fan Ni <fan.ni@samsung.com>
>
> With the change, when setting up memory for type3 memory device, we can
> create DC regions.
> A property 'num-dc-regions' is added to ct3_props to allow users to pass the
> number of DC regions to create. To make it easier, other region parameters
> like region base, length, and block size are hard coded. If needed,
> these parameters can be added easily.
>
> With the change, we can create DC regions with proper kernel side
> support like below:
>
> region=$(cat /sys/bus/cxl/devices/decoder0.0/create_dc_region)
> echo $region > /sys/bus/cxl/devices/decoder0.0/create_dc_region
> echo 256 > /sys/bus/cxl/devices/$region/interleave_granularity
> echo 1 > /sys/bus/cxl/devices/$region/interleave_ways
>
> echo "dc0" >/sys/bus/cxl/devices/decoder2.0/mode
> echo 0x40000000 >/sys/bus/cxl/devices/decoder2.0/dpa_size
>
> echo 0x40000000 > /sys/bus/cxl/devices/$region/size
> echo "decoder2.0" > /sys/bus/cxl/devices/$region/target0
> echo 1 > /sys/bus/cxl/devices/$region/commit
> echo $region > /sys/bus/cxl/drivers/cxl_region/bind
>
> However, we cannot really read/write to the DC regions due to lack of
> 1. host backend and address space setup for DC regions;
> 2. mailbox command support for adding/releasing DC extents.
>
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> ---
> hw/mem/cxl_type3.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 244d2b5fd5..c61cd2b5ac 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -567,6 +567,40 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> }
> }
>
> +/*
> + * TODO: dc region configuration will be updated once host backend and address
> + * space support is added for DCD.
> + */
> +static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
> +{
> + int i;
> + uint64_t region_base = 0;
> + uint64_t region_len = 2 * GiB;
> + uint64_t decode_len = 2 * GiB;
> + uint64_t blk_size = 2 * MiB;
> + CXLDCDRegion *region;
> +
> + if (ct3d->hostvmem) {
> + region_base += ct3d->hostvmem->size;
> + }
> + if (ct3d->hostpmem) {
> + region_base += ct3d->hostpmem->size;
> + }
You could use memory_region_size() here instead of accessing field directly.
Also, it doesn't look like there is any enforcement of hostvmem and
hostpmem being a multiple of 256MB, so region_base isn't necessarily
aligned to 256MB either. This probably should be fixed separately for
vmem and pmem, such that they check the size similar to what you do, as
the existing code also assumes that they are multiples of 256MB.
> + for (i = 0; i < ct3d->dc.num_regions; i++) {
> + region = &ct3d->dc.regions[i];
> + region->base = region_base;
> + region->decode_len = decode_len;
> + region->len = region_len;
> + region->block_size = blk_size;
> + /* dsmad_handle is set when creating cdat table entries */
> + region->flags = 0;
> +
> + region_base += region->len;
> + }
> +
> + return true;
> +}
> +
> static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
> {
> DeviceState *ds = DEVICE(ct3d);
> @@ -635,6 +669,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
> g_free(p_name);
> }
>
> + if (!cxl_create_dc_regions(ct3d, errp)) {
> + error_setg(errp, "setup DC regions failed");
> + return false;
> + }
> +
> return true;
> }
>
> @@ -930,6 +969,7 @@ static Property ct3_props[] = {
> HostMemoryBackend *),
> DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL),
> DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename),
> + DEFINE_PROP_UINT8("num-dc-regions", CXLType3Dev, dc.num_regions, 0),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> --
> 2.43.0
>
>
Thanks,
Jorgen
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to type3 memory devices
2024-03-04 13:10 ` Jørgen Hansen
@ 2024-03-04 17:31 ` fan
0 siblings, 0 replies; 44+ messages in thread
From: fan @ 2024-03-04 17:31 UTC (permalink / raw)
To: Jørgen Hansen
Cc: nifan.cxl, qemu-devel, jonathan.cameron, linux-cxl,
gregory.price, ira.weiny, dan.j.williams, a.manzanares, dave,
nmtadam.samsung, jim.harris, Fan Ni
On Mon, Mar 04, 2024 at 01:10:37PM +0000, Jørgen Hansen wrote:
> On 2/21/24 19:15, nifan.cxl@gmail.com wrote:
> > CAUTION: This email originated from outside of Western Digital. Do not click on links or open attachments unless you recognize the sender and know that the content is safe.
> >
> >
> > From: Fan Ni <fan.ni@samsung.com>
> >
> > With the change, when setting up memory for type3 memory device, we can
> > create DC regions.
> > A property 'num-dc-regions' is added to ct3_props to allow users to pass the
> > number of DC regions to create. To make it easier, other region parameters
> > like region base, length, and block size are hard coded. If needed,
> > these parameters can be added easily.
> >
> > With the change, we can create DC regions with proper kernel side
> > support like below:
> >
> > region=$(cat /sys/bus/cxl/devices/decoder0.0/create_dc_region)
> > echo $region > /sys/bus/cxl/devices/decoder0.0/create_dc_region
> > echo 256 > /sys/bus/cxl/devices/$region/interleave_granularity
> > echo 1 > /sys/bus/cxl/devices/$region/interleave_ways
> >
> > echo "dc0" >/sys/bus/cxl/devices/decoder2.0/mode
> > echo 0x40000000 >/sys/bus/cxl/devices/decoder2.0/dpa_size
> >
> > echo 0x40000000 > /sys/bus/cxl/devices/$region/size
> > echo "decoder2.0" > /sys/bus/cxl/devices/$region/target0
> > echo 1 > /sys/bus/cxl/devices/$region/commit
> > echo $region > /sys/bus/cxl/drivers/cxl_region/bind
> >
> > However, we cannot really read/write to the DC regions due to lack of
> > 1. host backend and address space setup for DC regions;
> > 2. mailbox command support for adding/releasing DC extents.
> >
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> > hw/mem/cxl_type3.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 244d2b5fd5..c61cd2b5ac 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -567,6 +567,40 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> > }
> > }
> >
> > +/*
> > + * TODO: dc region configuration will be updated once host backend and address
> > + * space support is added for DCD.
> > + */
> > +static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
> > +{
> > + int i;
> > + uint64_t region_base = 0;
> > + uint64_t region_len = 2 * GiB;
> > + uint64_t decode_len = 2 * GiB;
> > + uint64_t blk_size = 2 * MiB;
> > + CXLDCDRegion *region;
> > +
> > + if (ct3d->hostvmem) {
> > + region_base += ct3d->hostvmem->size;
> > + }
> > + if (ct3d->hostpmem) {
> > + region_base += ct3d->hostpmem->size;
> > + }
>
> You could use memory_region_size() here instead of accessing field directly.
>
> Also, it doesn't look like there is any enforcement of hostvmem and
> hostpmem being a multiple of 256MB, so region_base isn't necessarily
> aligned to 256MB either. This probably should be fixed separately for
> vmem and pmem, such that they check the size similar to what you do, as
> the existing code also assumes that they are multiples of 256MB.
Yeah. I will update the code and add check here to make sure the region
base is aligned with 256MB.
Fan
>
> > + for (i = 0; i < ct3d->dc.num_regions; i++) {
> > + region = &ct3d->dc.regions[i];
> > + region->base = region_base;
> > + region->decode_len = decode_len;
> > + region->len = region_len;
> > + region->block_size = blk_size;
> > + /* dsmad_handle is set when creating cdat table entries */
> > + region->flags = 0;
> > +
> > + region_base += region->len;
> > + }
> > +
> > + return true;
> > +}
> > +
> > static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
> > {
> > DeviceState *ds = DEVICE(ct3d);
> > @@ -635,6 +669,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
> > g_free(p_name);
> > }
> >
> > + if (!cxl_create_dc_regions(ct3d, errp)) {
> > + error_setg(errp, "setup DC regions failed");
> > + return false;
> > + }
> > +
> > return true;
> > }
> >
> > @@ -930,6 +969,7 @@ static Property ct3_props[] = {
> > HostMemoryBackend *),
> > DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL),
> > DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename),
> > + DEFINE_PROP_UINT8("num-dc-regions", CXLType3Dev, dc.num_regions, 0),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > --
> > 2.43.0
> >
> >
>
> Thanks,
> Jorgen
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support
2024-03-04 12:40 ` Jørgen Hansen
@ 2024-03-04 17:35 ` fan
0 siblings, 0 replies; 44+ messages in thread
From: fan @ 2024-03-04 17:35 UTC (permalink / raw)
To: Jørgen Hansen
Cc: nifan.cxl, qemu-devel, jonathan.cameron, linux-cxl,
gregory.price, ira.weiny, dan.j.williams, a.manzanares, dave,
nmtadam.samsung, jim.harris, Fan Ni
On Mon, Mar 04, 2024 at 12:40:18PM +0000, Jørgen Hansen wrote:
> On 2/21/24 19:15, nifan.cxl@gmail.com wrote:
> > CAUTION: This email originated from outside of Western Digital. Do not click on links or open attachments unless you recognize the sender and know that the content is safe.
> >
> >
> > From: Fan Ni <fan.ni@samsung.com>
> >
> > Per cxl spec r3.1, add dynamic capacity region representative based on
> > Table 8-165 and extend the cxl type3 device definition to include dc region
> > information. Also, based on info in 8.2.9.9.9.1, add 'Get Dynamic Capacity
> > Configuration' mailbox support.
> >
> > Note: decode_len of a dc region is aligned to 256*MiB, divided by
> > 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> > mailbox command.
> >
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 110 ++++++++++++++++++++++++++++++++++++
> > include/hw/cxl/cxl_device.h | 16 ++++++
> > 2 files changed, 126 insertions(+)
> >
>
> [snip]
>
> >
> > /* No separation for PCI MB as protocol handled in PCI device */
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 3cf3077afa..6df7fecdf1 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -422,6 +422,17 @@ typedef struct CXLPoison {
> > typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> > #define CXL_POISON_LIST_LIMIT 256
> >
> > +#define DCD_MAX_REGION_NUM 8
> > +
> > +typedef struct CXLDCDRegion {
> > + uint64_t base;
> > + uint64_t decode_len; /* aligned to 256*MiB */
> > + uint64_t len;
> > + uint64_t block_size;
> > + uint32_t dsmadhandle;
> > + uint8_t flags;
> > +} CXLDCDRegion;
> > +
>
> To be consistent with the naming of extents, and functions defined later
> on, shouldn't this be CXLDCRegion (DC instead of DCD)?
>
I was also thinking about it and a bit lazy to make the change until
someone points out. Now I have a good reason to make the change :-).
Thanks.
Fan
> > struct CXLType3Dev {
> > /* Private */
> > PCIDevice parent_obj;
> > @@ -454,6 +465,11 @@ struct CXLType3Dev {
> > unsigned int poison_list_cnt;
> > bool poison_list_overflowed;
> > uint64_t poison_list_overflow_ts;
> > +
> > + struct dynamic_capacity {
> > + uint8_t num_regions; /* 0-8 regions */
> > + CXLDCDRegion regions[DCD_MAX_REGION_NUM];
> > + } dc;
> > };
> >
> > #define TYPE_CXL_TYPE3 "cxl-type3"
> > --
> > 2.43.0
> >
> >
>
> Thanks,
> Jorgen
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
2024-03-01 3:56 ` fan
@ 2024-03-06 14:58 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron @ 2024-03-06 14:58 UTC (permalink / raw)
To: fan
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
...
> > > I cannot find anything specific to this in the specification either.
> > > Since we have already detected the case where the extent range across
> > > multiple regions, the only case we need to capture here is one/multiple
> > > portions of an extents getting released and causing extent overflow.
> > > I think we can handle it after we introduce the bitmaps (PATCH 10) which
> > > indicates DPA ranges mapped by valid extents in the device.
> > >
> > > With that, The release workflow would be
> > >
> > > 1) detecting malformed extent lists; if passed
> > > 2) do cxl_detect_extent_overflow {
> > > delta = 0;
> > > make a copy of the bitmap as bitmap_copy;
> > > for each extent in the updated_extent_list; do
> > > if (extent range not fully set in the bitmap_copy)
> > > return error;
> > > else {
> > > if gap at the front based on the bitmap_copy:
> > > delta += 1;
> > > if gap at the end based on the bitmap_copy:
> > > delta += 1;
> > > delta -= 1;
> > > // NOTE: current_extent_count will not be updated in the
> > > // loop since delta will track the whole loop
> > > if (delta + current_extent_count > max_extent_count)
> > > return resource exhausted;
> > > update bitmap_copy to clear the range covered by the extent
> > > under consideration;
> > > }
> > > done
> > >
> > > }; if pass
> > > 3. do real release: in the pass, we will not need to detect extent
> > > errors;
> > >
> > > Does the above solution sound reasonable? If so, do we want to go this
> > > way? do we need to introduce the bitmap earlier in the series?
> >
> > Yes, something along these lines should work nicely.
> >
> > Jonathan
>
> Hi Jonathan,
> I updated the code based on your feedback and now we can process extent
> release request more flexible.
Excellent!
> We can now support superset release (actually it can do even more,
> as long as the DPA range is coverd by accepted extents, we can release).
>
> I have run following tests and the code works as expected,
> 1. Add multiple extents, and removing them one by one, passed;
> 2. Superset release: add multiple extents with continuous DPA ranges, and
> remove all of them with a single release request with an extent covering the
> whole DPA range, passed;
> 3. Partial extent release: add a large extent and release only part of it,
> passed;
> 4. Partial+superset release: add multiple extents,and release it with some
> leftover with one request with an extent. For example, add extents [0-128M]
> and [128M-256M], release [64M-256M]. Passed;
> 5. Release extent not aligned to block size, failed as expected;
> 6. Extents have overlaps, fail the request as expected;
> 7. Extent has uncovered DPA range, skip the extent as expected;
>
> The only limitation is that for superset release case, if we find
> part of its DPA range is still pending to add, while the other is
> accepted, we reject it through QMP interface.
I think that is a reasonable limitation as we don't expect people
to do that crazy on QMP side. Maybe long term we'll want a
'release all' type command (I'm thinking virtualized device usecases)
but we can deal with that later.
>
> The latest code is https://github.com/moking/qemu/tree/dcd-v5.
>
> The main changes are in the last three commits.
> Btw, in the last commit, I introduce new QMP interfaces to print out
> accepted and pending-to-add list in the device to a file "/tmp/qmp.txt",
> do we want it? If yes, I can polish it a little bit, otherwise I will
> keep it for my own test purpose.
Ah. I missed this mail and replied directly. That needs a rethink
as the thread has concluded I think. I'll carry it on my tree, but not
look to upstream it.
>
> I will test more and send out v5 if the above looks reasonable to you.
>
Sorry for slow reply - I'm a bit behind with mailing lists.
Great you sent it out in the meantime.
Jonathan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
@ 2024-03-06 14:58 ` Jonathan Cameron via
0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron via @ 2024-03-06 14:58 UTC (permalink / raw)
To: fan
Cc: qemu-devel, linux-cxl, gregory.price, ira.weiny, dan.j.williams,
a.manzanares, dave, nmtadam.samsung, jim.harris, Fan Ni
...
> > > I cannot find anything specific to this in the specification either.
> > > Since we have already detected the case where the extent range across
> > > multiple regions, the only case we need to capture here is one/multiple
> > > portions of an extents getting released and causing extent overflow.
> > > I think we can handle it after we introduce the bitmaps (PATCH 10) which
> > > indicates DPA ranges mapped by valid extents in the device.
> > >
> > > With that, The release workflow would be
> > >
> > > 1) detecting malformed extent lists; if passed
> > > 2) do cxl_detect_extent_overflow {
> > > delta = 0;
> > > make a copy of the bitmap as bitmap_copy;
> > > for each extent in the updated_extent_list; do
> > > if (extent range not fully set in the bitmap_copy)
> > > return error;
> > > else {
> > > if gap at the front based on the bitmap_copy:
> > > delta += 1;
> > > if gap at the end based on the bitmap_copy:
> > > delta += 1;
> > > delta -= 1;
> > > // NOTE: current_extent_count will not be updated in the
> > > // loop since delta will track the whole loop
> > > if (delta + current_extent_count > max_extent_count)
> > > return resource exhausted;
> > > update bitmap_copy to clear the range covered by the extent
> > > under consideration;
> > > }
> > > done
> > >
> > > }; if pass
> > > 3. do real release: in the pass, we will not need to detect extent
> > > errors;
> > >
> > > Does the above solution sound reasonable? If so, do we want to go this
> > > way? do we need to introduce the bitmap earlier in the series?
> >
> > Yes, something along these lines should work nicely.
> >
> > Jonathan
>
> Hi Jonathan,
> I updated the code based on your feedback and now we can process extent
> release request more flexible.
Excellent!
> We can now support superset release (actually it can do even more,
> as long as the DPA range is coverd by accepted extents, we can release).
>
> I have run following tests and the code works as expected,
> 1. Add multiple extents, and removing them one by one, passed;
> 2. Superset release: add multiple extents with continuous DPA ranges, and
> remove all of them with a single release request with an extent covering the
> whole DPA range, passed;
> 3. Partial extent release: add a large extent and release only part of it,
> passed;
> 4. Partial+superset release: add multiple extents,and release it with some
> leftover with one request with an extent. For example, add extents [0-128M]
> and [128M-256M], release [64M-256M]. Passed;
> 5. Release extent not aligned to block size, failed as expected;
> 6. Extents have overlaps, fail the request as expected;
> 7. Extent has uncovered DPA range, skip the extent as expected;
>
> The only limitation is that for superset release case, if we find
> part of its DPA range is still pending to add, while the other is
> accepted, we reject it through QMP interface.
I think that is a reasonable limitation as we don't expect people
to do that crazy on QMP side. Maybe long term we'll want a
'release all' type command (I'm thinking virtualized device usecases)
but we can deal with that later.
>
> The latest code is https://github.com/moking/qemu/tree/dcd-v5.
>
> The main changes are in the last three commits.
> Btw, in the last commit, I introduce new QMP interfaces to print out
> accepted and pending-to-add list in the device to a file "/tmp/qmp.txt",
> do we want it? If yes, I can polish it a little bit, otherwise I will
> keep it for my own test purpose.
Ah. I missed this mail and replied directly. That needs a rethink
as the thread has concluded I think. I'll carry it on my tree, but not
look to upstream it.
>
> I will test more and send out v5 if the above looks reasonable to you.
>
Sorry for slow reply - I'm a bit behind with mailing lists.
Great you sent it out in the meantime.
Jonathan
^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2024-03-06 14:59 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-21 18:15 [PATCH v4 00/10] Enabling DCD emulation support in Qemu nifan.cxl
2024-02-21 18:15 ` [PATCH v4 01/10] hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output payload of identify memory device command nifan.cxl
2024-02-21 18:15 ` [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support nifan.cxl
2024-02-26 17:33 ` Jonathan Cameron
2024-02-26 17:33 ` Jonathan Cameron via
2024-02-26 19:16 ` fan
2024-03-04 12:40 ` Jørgen Hansen
2024-03-04 17:35 ` fan
2024-02-21 18:15 ` [PATCH v4 03/10] include/hw/cxl/cxl_device: Rename mem_size as static_mem_size for type3 memory devices nifan.cxl
2024-02-21 18:15 ` [PATCH v4 04/10] hw/mem/cxl_type3: Add support to create DC regions to " nifan.cxl
2024-02-26 17:38 ` Jonathan Cameron
2024-02-26 17:38 ` Jonathan Cameron via
2024-03-04 13:10 ` Jørgen Hansen
2024-03-04 17:31 ` fan
2024-02-21 18:15 ` [PATCH v4 05/10] hw/mem/cxl-type3: Refactor ct3_build_cdat_entries_for_mr to take mr size insead of mr as argument nifan.cxl
2024-02-21 18:15 ` [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions nifan.cxl
2024-02-26 17:45 ` Jonathan Cameron
2024-02-26 17:45 ` Jonathan Cameron via
2024-02-21 18:16 ` [PATCH v4 07/10] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support nifan.cxl
2024-02-23 7:16 ` Wonjae Lee
2024-02-23 16:56 ` fan
2024-02-26 17:48 ` Jonathan Cameron
2024-02-26 17:48 ` Jonathan Cameron via
2024-02-21 18:16 ` [PATCH v4 08/10] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response nifan.cxl
2024-02-23 9:10 ` Wonjae Lee
2024-02-26 18:04 ` Jonathan Cameron
2024-02-26 18:04 ` Jonathan Cameron via
2024-02-27 1:01 ` fan
2024-02-27 10:39 ` Jonathan Cameron
2024-02-27 10:39 ` Jonathan Cameron via
2024-03-01 3:56 ` fan
2024-03-06 14:58 ` Jonathan Cameron
2024-03-06 14:58 ` Jonathan Cameron via
2024-02-27 1:06 ` fan
2024-02-21 18:16 ` [PATCH v4 09/10] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents nifan.cxl
2024-02-23 12:16 ` Wonjae Lee
2024-02-26 18:10 ` Jonathan Cameron
2024-02-26 18:10 ` Jonathan Cameron via
2024-02-21 18:16 ` [PATCH v4 10/10] hw/mem/cxl_type3: Add dpa range validation for accesses to DC regions nifan.cxl
[not found] ` <CGME20240221182126epcas2p1b684f9239e4262f17ff484939658a382@epcms2p1>
2024-02-22 7:45 ` [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support Wonjae Lee
2024-02-22 16:54 ` fan
[not found] ` <CGME20240221182137epcas2p276d22514caaa9412d0119ded6f9a63d4@epcms2p3>
2024-02-22 9:22 ` [PATCH v4 06/10] hw/mem/cxl_type3: Add host backend and address space handling for DC regions Wonjae Lee
2024-02-22 16:56 ` fan
2024-02-23 18:05 ` [PATCH v4 00/10] Enabling DCD emulation support in Qemu fan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.