* [PATCH 0/4] CXL: SK hynix Niagara MHSLD Device
@ 2023-07-21 16:35 Gregory Price
2023-07-21 16:35 ` [PATCH 1/4] cxl/mailbox: change CCI cmd set structure to be a member, not a refernce Gregory Price
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Gregory Price @ 2023-07-21 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, junhee.ryu, kwangjin.ko,
Gregory Price
Base repo: https://gitlab.com/jic23/qemu
Base branch: cxl-2023-07-17
This patch set includes an emulation of the SK hynix Niagara MHSLD
platform with custom CCI commands that allow for isolation of memory
blocks between attached hosts.
There are 4 total patches in this set:
1 & 2: Modifications to the CCI interface to allow the addition of
custom CCI commands to an existing CCI command set.
3: Minimum MHD cci support for a type-3 device
4: The SK hynix Niagara Device
The changes to the core device and cci interface are very limited,
and this provides a clean way for developers to add custom CCI commands
to a device while retaining the possiblity to upstream the model.
This device allows hosts to request memory blocks directly from the
device, rather than requiring full the DCD command set. As a matter of
simplicity, this is beneficial to for testing and applications of
dynamic memory pooling on top of the 1.1 and 2.0 specification.
Note that these CCI commands are not servicable without the kernel
allowing raw CXL commands to be passed through the mailbox driver,
so users should enable `CONFIG_CXL_MEM_RAW_COMMANDS=y` on the kernel
of their QEMU instance.
Signed-off-by: Gregory Price <gregory.price@memverge.com>
Gregory Price (4):
cxl/mailbox: change CCI cmd set structure to be a member, not a
refernce
cxl/mailbox: interface to add CCI commands to an existing CCI
cxl/type3: minimum MHD cci support
cxl/vendor: SK hynix Niagara Multi-Headed SLD Device
hw/cxl/Kconfig | 4 +
hw/cxl/cxl-mailbox-utils.c | 90 +++-
hw/cxl/meson.build | 2 +
hw/cxl/vendor/meson.build | 1 +
hw/cxl/vendor/skhynix/.gitignore | 1 +
hw/cxl/vendor/skhynix/init_niagara.c | 99 +++++
hw/cxl/vendor/skhynix/meson.build | 1 +
hw/cxl/vendor/skhynix/skhynix_niagara.c | 521 ++++++++++++++++++++++++
hw/mem/cxl_type3.c | 67 +++
include/hw/cxl/cxl_device.h | 18 +-
tools/cxl/cxl_mhd_init.c | 63 +++
tools/cxl/meson.build | 3 +
tools/meson.build | 1 +
13 files changed, 866 insertions(+), 5 deletions(-)
create mode 100644 hw/cxl/vendor/meson.build
create mode 100644 hw/cxl/vendor/skhynix/.gitignore
create mode 100644 hw/cxl/vendor/skhynix/init_niagara.c
create mode 100644 hw/cxl/vendor/skhynix/meson.build
create mode 100644 hw/cxl/vendor/skhynix/skhynix_niagara.c
create mode 100644 tools/cxl/cxl_mhd_init.c
create mode 100644 tools/cxl/meson.build
--
2.39.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] cxl/mailbox: change CCI cmd set structure to be a member, not a refernce
2023-07-21 16:35 [PATCH 0/4] CXL: SK hynix Niagara MHSLD Device Gregory Price
@ 2023-07-21 16:35 ` Gregory Price
2023-08-04 14:56 ` Jonathan Cameron via
2023-07-21 16:35 ` [PATCH 2/4] cxl/mailbox: interface to add CCI commands to an existing CCI Gregory Price
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Gregory Price @ 2023-07-21 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, junhee.ryu, kwangjin.ko,
Gregory Price
This allows devices to have fully customized CCIs, along with complex
devices where wrapper devices can override or add additional CCI
commands without having to replicate full command structures or
pollute a base device with every command that might ever be used.
Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
hw/cxl/cxl-mailbox-utils.c | 18 ++++++++++++++----
include/hw/cxl/cxl_device.h | 2 +-
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 2819914e8d..ddee3f1718 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1373,9 +1373,19 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
bg_timercb, cci);
}
+static void cxl_copy_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmds)[256]) {
+ for (int set = 0; set < 256; set++) {
+ for (int cmd = 0; cmd < 256; cmd++) {
+ if (cxl_cmds[set][cmd].handler) {
+ cci->cxl_cmd_set[set][cmd] = cxl_cmds[set][cmd];
+ }
+ }
+ }
+}
+
void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, DeviceState *d, size_t payload_max)
{
- cci->cxl_cmd_set = cxl_cmd_set_sw;
+ cxl_copy_cci_commands(cci, cxl_cmd_set_sw);
cci->d = d;
cci->intf = intf;
cxl_init_cci(cci, payload_max);
@@ -1383,7 +1393,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, DeviceState *d
void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
{
- cci->cxl_cmd_set = cxl_cmd_set;
+ cxl_copy_cci_commands(cci, cxl_cmd_set);
cci->d = d;
/* No separation for PCI MB as protocol handled in PCI device */
@@ -1398,7 +1408,7 @@ static const struct cxl_cmd cxl_cmd_set_t3_mctp[256][256] = {
void cxl_initialize_t3_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf,
size_t payload_max)
{
- cci->cxl_cmd_set = cxl_cmd_set_t3_mctp;
+ cxl_copy_cci_commands(cci, cxl_cmd_set_t3_mctp);
cci->d = d;
cci->intf = intf;
cxl_init_cci(cci, payload_max);
@@ -1414,7 +1424,7 @@ static const struct cxl_cmd cxl_cmd_set_usp_mctp[256][256] = {
void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf, size_t payload_max)
{
- cci->cxl_cmd_set = cxl_cmd_set_usp_mctp;
+ cxl_copy_cci_commands(cci, cxl_cmd_set_usp_mctp);
cci->d = d;
cci->intf = intf;
cxl_init_cci(cci, payload_max);
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index c68981b618..9a3c8b2dfa 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -163,7 +163,7 @@ typedef struct CXLEventLog {
} CXLEventLog;
typedef struct CXLCCI {
- const struct cxl_cmd (*cxl_cmd_set)[256];
+ struct cxl_cmd cxl_cmd_set[256][256];
struct cel_log {
uint16_t opcode;
uint16_t effect;
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] cxl/mailbox: interface to add CCI commands to an existing CCI
2023-07-21 16:35 [PATCH 0/4] CXL: SK hynix Niagara MHSLD Device Gregory Price
2023-07-21 16:35 ` [PATCH 1/4] cxl/mailbox: change CCI cmd set structure to be a member, not a refernce Gregory Price
@ 2023-07-21 16:35 ` Gregory Price
2023-08-04 15:14 ` Jonathan Cameron via
2023-07-21 16:35 ` [PATCH 3/4] cxl/type3: minimum MHD cci support Gregory Price
2023-07-21 16:35 ` [PATCH 4/4] cxl/vendor: SK hynix Niagara Multi-Headed SLD Device Gregory Price
3 siblings, 1 reply; 13+ messages in thread
From: Gregory Price @ 2023-07-21 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, junhee.ryu, kwangjin.ko,
Gregory Price
This enables wrapper devices to customize the base device's CCI
(for example, with custom commands outside the specification)
without the need to change the base device.
The also enabled the base device to dispatch those commands without
requiring additional driver support.
Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
hw/cxl/cxl-mailbox-utils.c | 19 +++++++++++++++++++
include/hw/cxl/cxl_device.h | 2 ++
2 files changed, 21 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index ddee3f1718..cad0cd0adb 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1383,6 +1383,25 @@ static void cxl_copy_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmds)[
}
}
+void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256], size_t payload_max)
+{
+ cci->payload_max = payload_max > cci->payload_max ? payload_max : cci->payload_max;
+ for (int set = 0; set < 256; set++) {
+ for (int cmd = 0; cmd < 256; cmd++) {
+ if (cxl_cmd_set[set][cmd].handler) {
+ const struct cxl_cmd *c = &cxl_cmd_set[set][cmd];
+ cci->cxl_cmd_set[set][cmd] = *c;
+ struct cel_log *log =
+ &cci->cel_log[cci->cel_size];
+
+ log->opcode = (set << 8) | cmd;
+ log->effect = c->effect;
+ cci->cel_size++;
+ }
+ }
+ }
+}
+
void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, DeviceState *d, size_t payload_max)
{
cxl_copy_cci_commands(cci, cxl_cmd_set_sw);
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 9a3c8b2dfa..abc8405cc5 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -297,6 +297,8 @@ void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max);
void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
DeviceState *d, size_t payload_max);
void cxl_init_cci(CXLCCI *cci, size_t payload_max);
+void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
+ size_t payload_max);
int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
size_t len_in, uint8_t *pl_in,
size_t *len_out, uint8_t *pl_out,
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] cxl/type3: minimum MHD cci support
2023-07-21 16:35 [PATCH 0/4] CXL: SK hynix Niagara MHSLD Device Gregory Price
2023-07-21 16:35 ` [PATCH 1/4] cxl/mailbox: change CCI cmd set structure to be a member, not a refernce Gregory Price
2023-07-21 16:35 ` [PATCH 2/4] cxl/mailbox: interface to add CCI commands to an existing CCI Gregory Price
@ 2023-07-21 16:35 ` Gregory Price
2023-08-07 14:56 ` Jonathan Cameron via
2023-07-21 16:35 ` [PATCH 4/4] cxl/vendor: SK hynix Niagara Multi-Headed SLD Device Gregory Price
3 siblings, 1 reply; 13+ messages in thread
From: Gregory Price @ 2023-07-21 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, junhee.ryu, kwangjin.ko,
Gregory Price
Implement the MHD GET_INFO cci command and add a shared memory
region to the type3 device to host the information.
Add a helper program to initialize this shared memory region.
Add a function pointer to type3 devices for future work that
will allow an mhd device to provide a hook to validate whether
a memory access is valid or not.
For now, limit the number of LD's to the number of heads. Later,
this limitation will need to be lifted for MH-MLDs.
Intended use case:
1. Create the shared memory region
2. Format the shared memory region
3. Launch QEMU with `is_mhd=true,mhd_head=N,mhd_shmid=$shmid`
shmid=`ipcmk -M 4096 | grep -o -E '[0-9]+' | head -1`
cxl_mhd_init 4 $shmid
qemu-system-x86_64 \
-nographic \
-accel kvm \
-drive file=./mhd.qcow2,format=qcow2,index=0,media=disk,id=hd \
-m 4G,slots=4,maxmem=8G \
-smp 4 \
-machine type=q35,cxl=on,hmat=on \
-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 \
-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,port=0,slot=0 \
-object memory-backend-file,id=mem0,mem-path=/tmp/mem0,size=4G,share=true \
-device cxl-type3,bus=rp0,volatile-memdev=mem0,id=cxl-mem0,sn=66666,is_mhd=true,mhd_head=0,mhd_shmid=$shmid \
-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G
Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
hw/cxl/cxl-mailbox-utils.c | 53 +++++++++++++++++++++++++++++
hw/mem/cxl_type3.c | 67 +++++++++++++++++++++++++++++++++++++
include/hw/cxl/cxl_device.h | 14 ++++++++
tools/cxl/cxl_mhd_init.c | 63 ++++++++++++++++++++++++++++++++++
tools/cxl/meson.build | 3 ++
tools/meson.build | 1 +
6 files changed, 201 insertions(+)
create mode 100644 tools/cxl/cxl_mhd_init.c
create mode 100644 tools/cxl/meson.build
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index cad0cd0adb..57b8da4376 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -84,6 +84,8 @@ enum {
#define GET_PHYSICAL_PORT_STATE 0x1
TUNNEL = 0x53,
#define MANAGEMENT_COMMAND 0x0
+ MHD = 0x55,
+ #define GET_MHD_INFO 0x0
};
/* CCI Message Format CXL r3.0 Figure 7-19 */
@@ -1155,6 +1157,56 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+static CXLRetCode cmd_mhd_get_info(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 {
+ uint8_t start_ld;
+ uint8_t ldmap_len;
+ } QEMU_PACKED *input = (void *)payload_in;
+
+ struct {
+ uint8_t nr_lds;
+ uint8_t nr_heads;
+ uint16_t resv1;
+ uint8_t start_ld;
+ uint8_t ldmap_len;
+ uint16_t resv2;
+ uint8_t ldmap[];
+ } QEMU_PACKED *output = (void *)payload_out;
+
+ uint8_t start_ld = input->start_ld;
+ uint8_t ldmap_len = input->ldmap_len;
+ uint8_t i;
+
+ if (!ct3d->is_mhd) {
+ return CXL_MBOX_UNSUPPORTED;
+ }
+
+ if (start_ld >= ct3d->mhd_state->nr_lds) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ output->nr_lds = ct3d->mhd_state->nr_lds;
+ output->nr_heads = ct3d->mhd_state->nr_heads;
+ output->resv1 = 0;
+ output->start_ld = start_ld;
+ output->resv2 = 0;
+
+ for (i = 0; i < ldmap_len && (start_ld + i) < output->nr_lds; i++) {
+ output->ldmap[i] = ct3d->mhd_state->ldmap[start_ld + i];
+ }
+ output->ldmap_len = i;
+
+ *len_out = sizeof(*output) + output->ldmap_len;
+ return CXL_MBOX_SUCCESS;
+}
+
#define IMMEDIATE_CONFIG_CHANGE (1 << 1)
#define IMMEDIATE_DATA_CHANGE (1 << 2)
#define IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -1195,6 +1247,7 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
cmd_media_inject_poison, 8, 0 },
[MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON",
cmd_media_clear_poison, 72, 0 },
+ [MHD][GET_MHD_INFO] = {"GET_MULTI_HEADED_INFO", cmd_mhd_get_info, 2, 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 efb7dece80..c8eb3aa67d 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -18,6 +18,7 @@
#include "hw/cxl/cxl.h"
#include "hw/pci/msix.h"
#include "hw/pci/spdm.h"
+#include <sys/shm.h>
#define DWORD_BYTE 4
@@ -794,6 +795,48 @@ static DOEProtocol doe_spdm_prot[] = {
{ }
};
+static bool cxl_setup_mhd(CXLType3Dev *ct3d, Error **errp)
+{
+ if (!ct3d->is_mhd) {
+ ct3d->mhd_access_valid = NULL;
+ return true;
+ } else if (ct3d->is_mhd &&
+ (!ct3d->mhd_shmid || (ct3d->mhd_head == ~(0)))) {
+ error_setg(errp, "is_mhd requires mhd_shmid and mhd_head settings");
+ return false;
+ } else if (!ct3d->is_mhd &&
+ (ct3d->mhd_shmid || (ct3d->mhd_head == ~(0)))) {
+ error_setg(errp, "(is_mhd,mhd_head,mhd_shmid) invalid");
+ return false;
+ }
+
+ if (ct3d->mhd_head >= 32) {
+ error_setg(errp, "MHD Head ID must be between 0-31");
+ return false;
+ }
+
+ ct3d->mhd_state = shmat(ct3d->mhd_shmid, NULL, 0);
+ if (ct3d->mhd_state == (void*)-1) {
+ ct3d->mhd_state = NULL;
+ error_setg(errp, "Unable to attach MHD State. Check ipcs is valid");
+ return false;
+ }
+
+ /* For now, limit the number of heads to the number of LD's (SLD) */
+ if (ct3d->mhd_state->nr_heads <= ct3d->mhd_head) {
+ error_setg(errp, "Invalid head ID for multiheaded device.");
+ return false;
+ }
+
+ if (ct3d->mhd_state->nr_lds <= ct3d->mhd_head) {
+ error_setg(errp, "MHD Shared state does not have sufficient lds.");
+ return false;
+ }
+
+ ct3d->mhd_state->ldmap[ct3d->mhd_head] = ct3d->mhd_head;
+ return true;
+}
+
static void ct3_realize(PCIDevice *pci_dev, Error **errp)
{
CXLType3Dev *ct3d = CXL_TYPE3(pci_dev);
@@ -806,6 +849,10 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
QTAILQ_INIT(&ct3d->error_list);
+ if (!cxl_setup_mhd(ct3d, errp)) {
+ return;
+ }
+
if (!cxl_setup_memory(ct3d, errp)) {
return;
}
@@ -910,6 +957,9 @@ static void ct3_exit(PCIDevice *pci_dev)
if (ct3d->hostvmem) {
address_space_destroy(&ct3d->hostvmem_as);
}
+ if (ct3d->mhd_state) {
+ shmdt(ct3d->mhd_state);
+ }
}
static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
@@ -1006,6 +1056,7 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
unsigned size, MemTxAttrs attrs)
{
+ CXLType3Dev *ct3d = CXL_TYPE3(d);
uint64_t dpa_offset = 0;
AddressSpace *as = NULL;
int res;
@@ -1016,16 +1067,23 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
return MEMTX_ERROR;
}
+ if (ct3d->is_mhd && ct3d->mhd_access_valid) {
+ if (!ct3d->mhd_access_valid(ct3d, dpa_offset, size))
+ return MEMTX_ERROR;
+ }
+
if (sanitize_running(&CXL_TYPE3(d)->cci)) {
qemu_guest_getrandom_nofail(data, size);
return MEMTX_OK;
}
+
return address_space_read(as, dpa_offset, attrs, data, size);
}
MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
unsigned size, MemTxAttrs attrs)
{
+ CXLType3Dev *ct3d = CXL_TYPE3(d);
uint64_t dpa_offset = 0;
AddressSpace *as = NULL;
int res;
@@ -1035,6 +1093,12 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
if (res) {
return MEMTX_ERROR;
}
+
+ if (ct3d->is_mhd && ct3d->mhd_access_valid) {
+ if (!ct3d->mhd_access_valid(ct3d, dpa_offset, size))
+ return MEMTX_ERROR;
+ }
+
if (sanitize_running(&CXL_TYPE3(d)->cci)) {
return MEMTX_OK;
}
@@ -1067,6 +1131,9 @@ static Property ct3_props[] = {
DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL),
DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename),
DEFINE_PROP_UINT16("spdm", CXLType3Dev, spdm_port, 0),
+ DEFINE_PROP_BOOL("is_mhd", CXLType3Dev, is_mhd, false),
+ DEFINE_PROP_UINT32("mhd_head", CXLType3Dev, mhd_head, 0),
+ DEFINE_PROP_UINT32("mhd_shmid", CXLType3Dev, mhd_shmid, 0),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index abc8405cc5..b545c5b6f3 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -408,6 +408,12 @@ typedef struct CXLPoison {
typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
#define CXL_POISON_LIST_LIMIT 256
+struct CXLMHD_SharedState {
+ uint8_t nr_heads;
+ uint8_t nr_lds;
+ uint8_t ldmap[];
+};
+
struct CXLType3Dev {
/* Private */
PCIDevice parent_obj;
@@ -442,6 +448,14 @@ struct CXLType3Dev {
unsigned int poison_list_cnt;
bool poison_list_overflowed;
uint64_t poison_list_overflow_ts;
+
+ /* Multi-headed Device */
+ bool is_mhd;
+ uint32_t mhd_head;
+ uint32_t mhd_shmid;
+ struct CXLMHD_SharedState *mhd_state;
+ bool (*mhd_access_valid)(CXLType3Dev* ct3d, uint64_t addr,
+ unsigned int size);
};
#define TYPE_CXL_TYPE3 "cxl-type3"
diff --git a/tools/cxl/cxl_mhd_init.c b/tools/cxl/cxl_mhd_init.c
new file mode 100644
index 0000000000..1303aa9494
--- /dev/null
+++ b/tools/cxl/cxl_mhd_init.c
@@ -0,0 +1,63 @@
+#include <signal.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ipc.h>
+#include <sys/shm.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+struct mhd_state {
+ uint8_t nr_heads;
+ uint8_t nr_lds;
+ uint8_t ldmap[];
+};
+
+int main(int argc, char *argv[]) {
+ int shmid = 0;
+ uint32_t heads = 0;
+ struct mhd_state* mhd_state = 0;
+ uint8_t i;
+
+ if (argc != 3) {
+ printf("usage: cxl_mhd_init <heads> <shmid>\n"
+ "\theads : number of heads on the device\n"
+ "\tshmid : /tmp/mytoken.tmp\n");
+ return -1;
+ }
+
+ // must have at least 1 head
+ heads = (uint32_t)atoi(argv[1]);
+ if (heads == 0 || heads > 32) {
+ printf("bad heads argument (1-32)\n");
+ return -1;
+ }
+
+ shmid = (uint32_t)atoi(argv[2]);
+ if (shmid== 0) {
+ printf("bad shmid argument\n");
+ return -1;
+ }
+
+ mhd_state = shmat(shmid, NULL, 0);
+ if (mhd_state == (void*)-1) {
+ printf("Unable to attach to shared memory\n");
+ return -1;
+ }
+
+ // Initialize the mhd_state
+ size_t mhd_state_size = sizeof(struct mhd_state) + (sizeof(uint8_t) * heads);
+ memset(mhd_state, 0, mhd_state_size);
+ mhd_state->nr_heads = heads;
+ mhd_state->nr_lds = heads;
+
+ // Head ID == LD ID for now
+ for (i = 0; i < heads; i++)
+ mhd_state->ldmap[i] = i;
+
+ printf("mhd initialized\n");
+ shmdt(mhd_state);
+ return 0;
+}
diff --git a/tools/cxl/meson.build b/tools/cxl/meson.build
new file mode 100644
index 0000000000..218658fe69
--- /dev/null
+++ b/tools/cxl/meson.build
@@ -0,0 +1,3 @@
+executable('cxl_mhd_init', files('cxl_mhd_init.c'),
+ install: true,
+ install_dir: get_option('libexecdir'))
diff --git a/tools/meson.build b/tools/meson.build
index e69de29bb2..91a1d788cb 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -0,0 +1 @@
+subdir('cxl')
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] cxl/vendor: SK hynix Niagara Multi-Headed SLD Device
2023-07-21 16:35 [PATCH 0/4] CXL: SK hynix Niagara MHSLD Device Gregory Price
` (2 preceding siblings ...)
2023-07-21 16:35 ` [PATCH 3/4] cxl/type3: minimum MHD cci support Gregory Price
@ 2023-07-21 16:35 ` Gregory Price
2023-08-07 15:36 ` Jonathan Cameron via
3 siblings, 1 reply; 13+ messages in thread
From: Gregory Price @ 2023-07-21 16:35 UTC (permalink / raw)
To: qemu-devel
Cc: jonathan.cameron, linux-cxl, junhee.ryu, kwangjin.ko,
Gregory Price
Create a new device to emulate the SK hynix Niagara MHSLD platform.
This device has custom CCI commands that allow for applying isolation
to each memory block between hosts. This enables an early form of
dynamic capacity, whereby the NUMA node maps the entire region, but
the host is responsible for asking the device which memory blocks
are allocated to it, and therefore may be onlined.
To instantiate, wrap a cxl-type3 mhd in a cxl-skh-niagara like so:
-device cxl-type3,bus=rp0,volatile-memdev=mem0,id=cxl-mem0,sn=66666,is_mhd=true,mhd_head=0,mhd_shmid=15
-device cxl-skh-niagara,target=cxl-mem0
The linux kernel will require raw CXL commands enabled to allow for
passing through of Niagara CXL commands via the CCI mailbox.
The Niagara MH-SLD has a slightly different shared memory region
than the base MHD, so an additional tool ('init_niagara') is located
in the vendor subdirectory. Utilize this in place of cxl_mhd_init.
usage: init_niagara <heads> <sections> <section_size> <shmid>
heads : number of heads on the device
sections : number of sections
section_size : size of a section in 128mb increments
shmid : shmid produced by ipcmk
Example:
$shmid1=ipcmk -M 131072
./init_niagara 4 32 1 $shmid1
Signed-off-by: Gregory Price <gregory.price@memverge.com>
Signed-off-by: Junhee Ryu <junhee.ryu@sk.com>
Signed-off-by: Kwangjin Ko <kwangjin.ko@sk.com>
---
hw/cxl/Kconfig | 4 +
hw/cxl/meson.build | 2 +
hw/cxl/vendor/meson.build | 1 +
hw/cxl/vendor/skhynix/.gitignore | 1 +
hw/cxl/vendor/skhynix/init_niagara.c | 99 +++++
hw/cxl/vendor/skhynix/meson.build | 1 +
hw/cxl/vendor/skhynix/skhynix_niagara.c | 521 ++++++++++++++++++++++++
7 files changed, 629 insertions(+)
create mode 100644 hw/cxl/vendor/meson.build
create mode 100644 hw/cxl/vendor/skhynix/.gitignore
create mode 100644 hw/cxl/vendor/skhynix/init_niagara.c
create mode 100644 hw/cxl/vendor/skhynix/meson.build
create mode 100644 hw/cxl/vendor/skhynix/skhynix_niagara.c
diff --git a/hw/cxl/Kconfig b/hw/cxl/Kconfig
index c9b2e46bac..dd6c54b54d 100644
--- a/hw/cxl/Kconfig
+++ b/hw/cxl/Kconfig
@@ -2,5 +2,9 @@ config CXL
bool
default y if PCI_EXPRESS
+config CXL_VENDOR
+ bool
+ default y
+
config I2C_MCTP_CXL
bool
diff --git a/hw/cxl/meson.build b/hw/cxl/meson.build
index 1393821fc4..e8c8c1355a 100644
--- a/hw/cxl/meson.build
+++ b/hw/cxl/meson.build
@@ -15,3 +15,5 @@ system_ss.add(when: 'CONFIG_CXL',
system_ss.add(when: 'CONFIG_I2C_MCTP_CXL', if_true: files('i2c_mctp_cxl.c'))
system_ss.add(when: 'CONFIG_ALL', if_true: files('cxl-host-stubs.c'))
+
+subdir('vendor')
diff --git a/hw/cxl/vendor/meson.build b/hw/cxl/vendor/meson.build
new file mode 100644
index 0000000000..12db8991f1
--- /dev/null
+++ b/hw/cxl/vendor/meson.build
@@ -0,0 +1 @@
+subdir('skhynix')
diff --git a/hw/cxl/vendor/skhynix/.gitignore b/hw/cxl/vendor/skhynix/.gitignore
new file mode 100644
index 0000000000..6d96de38ea
--- /dev/null
+++ b/hw/cxl/vendor/skhynix/.gitignore
@@ -0,0 +1 @@
+init_niagara
diff --git a/hw/cxl/vendor/skhynix/init_niagara.c b/hw/cxl/vendor/skhynix/init_niagara.c
new file mode 100644
index 0000000000..28612339e0
--- /dev/null
+++ b/hw/cxl/vendor/skhynix/init_niagara.c
@@ -0,0 +1,99 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Copyright (c) 2023 MemVerge Inc.
+ * Copyright (c) 2023 SK hynix Inc.
+ *
+ * Reference list:
+ * From www.computeexpresslink.org
+ * Compute Express Link (CXL) Specification revision 3.0 Version 1.0
+ */
+
+#include <signal.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ipc.h>
+#include <sys/shm.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+struct niagara_state {
+ uint8_t nr_heads;
+ uint8_t nr_lds;
+ uint8_t ldmap[65536];
+ uint32_t total_sections;
+ uint32_t free_sections;
+ uint32_t section_size;
+ uint32_t sections[];
+};
+
+int main(int argc, char *argv[]) {
+ int shmid = 0;
+ uint32_t sections = 0;
+ uint32_t section_size = 0;
+ uint32_t heads = 0;
+ struct niagara_state* niagara_state = 0;
+ uint8_t i;
+
+ if (argc != 5) {
+ printf("usage: init_niagara <heads> <sections> <section_size> <shmid>\n"
+ "\theads : number of heads on the device\n"
+ "\tsections : number of sections\n"
+ "\tsection_size : size of a section in 128mb increments\n"
+ "\tshmid : /tmp/mytoken.tmp\n\n"
+ "It is recommended your shared memory region is at least 128kb\n");
+ return -1;
+ }
+
+ // must have at least 1 head
+ heads = (uint32_t)atoi(argv[1]);
+ if (heads == 0 || heads > 32) {
+ printf("bad heads argument (1-32)\n");
+ return -1;
+ }
+
+ // Get number of sections
+ sections = (uint32_t)atoi(argv[2]);
+ if (sections == 0) {
+ printf("bad sections argument\n");
+ return -1;
+ }
+
+ section_size = (uint32_t)atoi(argv[3]);
+ if (sections == 0) {
+ printf("bad section size argument\n");
+ return -1;
+ }
+
+ shmid = (uint32_t)atoi(argv[4]);
+ if (shmid== 0) {
+ printf("bad shmid argument\n");
+ return -1;
+ }
+
+ niagara_state = shmat(shmid, NULL, 0);
+ if (niagara_state == (void*)-1) {
+ printf("Unable to attach to shared memory\n");
+ return -1;
+ }
+
+ // Initialize the niagara_state
+ size_t niagara_state_size = sizeof(struct niagara_state) + (sizeof(uint32_t) * sections);
+ memset(niagara_state, 0, niagara_state_size);
+ niagara_state->nr_heads = heads;
+ niagara_state->nr_lds = heads;
+ niagara_state->total_sections = sections;
+ niagara_state->free_sections = sections;
+ niagara_state->section_size = section_size;
+
+ memset(&niagara_state->ldmap, '\xff', sizeof(niagara_state->ldmap));
+ for (i = 0; i < heads; i++)
+ niagara_state->ldmap[i] = i;
+
+ printf("niagara initialized\n");
+ shmdt(niagara_state);
+ return 0;
+}
diff --git a/hw/cxl/vendor/skhynix/meson.build b/hw/cxl/vendor/skhynix/meson.build
new file mode 100644
index 0000000000..4e57db65f1
--- /dev/null
+++ b/hw/cxl/vendor/skhynix/meson.build
@@ -0,0 +1 @@
+system_ss.add(when: 'CONFIG_CXL_VENDOR', if_true: files('skhynix_niagara.c',))
diff --git a/hw/cxl/vendor/skhynix/skhynix_niagara.c b/hw/cxl/vendor/skhynix/skhynix_niagara.c
new file mode 100644
index 0000000000..1224978585
--- /dev/null
+++ b/hw/cxl/vendor/skhynix/skhynix_niagara.c
@@ -0,0 +1,521 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Copyright (c) 2023 MemVerge Inc.
+ * Copyright (c) 2023 SK hynix Inc.
+ *
+ * Reference list:
+ * From www.computeexpresslink.org
+ * Compute Express Link (CXL) Specification revision 3.0 Version 1.0
+ */
+
+#include "qemu/osdep.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "hw/cxl/cxl.h"
+#include "hw/cxl/cxl_device.h"
+#include "hw/pci/pcie.h"
+#include "hw/pci/pcie_port.h"
+#include "hw/qdev-properties.h"
+
+#define MIN_MEMBLK_SIZE (1024*1024*128)
+
+/*
+ * The shared state cannot have 2 variable sized regions
+ * so we have to max out the ldmap.
+*/
+typedef struct Niagara_Shared_State Niagara_Shared_State;
+struct Niagara_Shared_State {
+ uint8_t nr_heads;
+ uint8_t nr_lds;
+ uint8_t ldmap[65536];
+ uint32_t total_sections;
+ uint32_t free_sections;
+ uint32_t section_size;
+ uint32_t sections[];
+};
+
+#define IMMEDIATE_CONFIG_CHANGE (1 << 1)
+#define IMMEDIATE_DATA_CHANGE (1 << 2)
+#define IMMEDIATE_POLICY_CHANGE (1 << 3)
+#define IMMEDIATE_LOG_CHANGE (1 << 4)
+#define SECURITY_STATE_CHANGE (1 << 5)
+#define BACKGROUND_OPERATION (1 << 6)
+
+enum {
+ NIAGARA = 0xC0
+ #define GET_SECTION_STATUS 0x0
+ #define SET_SECTION_ALLOC 0x1
+ #define SET_SECTION_RELEASE 0x2
+ #define SET_SECTION_SIZE 0x3
+ #define MOVE_DATA 0x4
+ #define GET_SECTION_MAP 0x5
+ #define CLEAR_SECTION 0x99
+};
+
+static CXLRetCode cmd_niagara_get_section_status(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);
+ Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
+ struct {
+ uint32_t total_section_count;
+ uint32_t free_section_count;
+ } QEMU_PACKED *output = (void *)payload_out;
+
+ if (!ct3d->is_mhd)
+ return CXL_MBOX_UNSUPPORTED;
+
+ output->total_section_count = niagara_state->total_sections;
+ output->free_section_count = niagara_state->free_sections;
+
+ *len_out = sizeof(*output);
+
+ return CXL_MBOX_SUCCESS;
+}
+
+#define MHD_SECTION_ALLOC_POLICY_ALL_OR_NOTHING 0
+#define MHD_SECTION_ALLOC_POLICY_BEST_EFFORT 1
+#define MHD_SECTION_ALLOC_POLICY_MANUAL 2
+static CXLRetCode cmd_niagara_set_section_alloc(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);
+ Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
+ struct {
+ uint8_t policy;
+ uint8_t reserved1[3];
+ uint32_t section_count;
+ uint8_t reserved2[4];
+ uint32_t extent_count;
+ struct {
+ uint32_t start_section_id;
+ uint32_t section_count;
+ uint8_t reserved[8];
+ } extents[];
+ } QEMU_PACKED *input = (void *)payload_in;
+ struct {
+ uint32_t section_count;
+ uint32_t extent_count;
+ struct {
+ uint32_t start_section_id;
+ uint32_t section_count;
+ uint8_t reserved[8];
+ } extents[];
+ } QEMU_PACKED *output = (void *)payload_out;
+
+ uint8_t policy = input->policy;
+ uint32_t count = input->section_count;
+ uint32_t i = 0;
+
+ if (count == 0 || count > niagara_state->total_sections) {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ if (input->policy == MHD_SECTION_ALLOC_POLICY_MANUAL) {
+ /* iterate input extents and count total sections for validation */
+ uint32_t ttl_sec = 0;
+ for (i = 0; i < input->extent_count; i++) {
+ uint32_t start = input->extents[i].start_section_id;
+ uint32_t end = start + input->extents[i].section_count;
+ if ((start >= niagara_state->total_sections) || (end > niagara_state->total_sections))
+ return CXL_MBOX_INVALID_INPUT;
+ ttl_sec += input->extents[i].section_count;
+ }
+ if (ttl_sec != input->section_count)
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ uint32_t *section_ids = malloc(count*sizeof(uint32_t));
+ uint32_t *sections = &niagara_state->sections[0];
+ uint32_t allocated = 0;
+
+ if (input->policy & MHD_SECTION_ALLOC_POLICY_MANUAL) {
+ uint32_t cur_extent = 0;
+ for (cur_extent = 0; cur_extent < input->extent_count; cur_extent++) {
+ uint32_t start_section = input->extents[cur_extent].start_section_id;
+ uint32_t count = input->extents[cur_extent].section_count;
+ uint32_t cur_section;
+ for (cur_section = input->extents[cur_extent].start_section_id;
+ cur_section < (start_section + count);
+ cur_section++) {
+ uint32_t *section = §ions[cur_section];
+ uint32_t old_value = __sync_fetch_and_or(section, (1 << ct3d->mhd_head));
+ /* if the old value wasn't 0, this section was already claimed
+ * if it was owned by use already, just continue and don't count it
+ */
+ if (old_value & (1 << ct3d->mhd_head)) {
+ continue;
+ } else if (old_value != 0) {
+ __sync_fetch_and_and(section, ~(1 << ct3d->mhd_head));
+ continue;
+ }
+ __sync_fetch_and_sub(&niagara_state->free_sections, 1);
+ section_ids[allocated++] = i;
+ }
+ }
+ } else {
+ /* Iterate the the section list and allocate free sections */
+ for (i = 0; (i < niagara_state->total_sections) && (allocated != count); i++) {
+ uint32_t old_value = __sync_fetch_and_or(§ions[i], (1 << ct3d->mhd_head));
+ /* if the old value wasn't 0, this section was already claimed
+ * if it was owned by use already, just continue and don't count it
+ */
+ if (old_value & (1 << ct3d->mhd_head)) {
+ continue;
+ } else if (old_value != 0) {
+ __sync_fetch_and_and(§ions[i], ~(1 << ct3d->mhd_head));
+ continue;
+ }
+ __sync_fetch_and_sub(&niagara_state->free_sections, 1);
+ section_ids[allocated++] = i;
+ }
+ }
+
+ if ((policy & MHD_SECTION_ALLOC_POLICY_ALL_OR_NOTHING) &&
+ (allocated != count)) {
+ goto all_or_nothing_fail;
+ }
+
+ /* Build the output */
+ output->section_count = allocated;
+ uint32_t extents = 0;
+ uint32_t previous = 0;
+ for (i=0; i < allocated; i++) {
+ if (i == 0) {
+ /* start the first extent */
+ output->extents[extents].start_section_id = section_ids[i];
+ output->extents[extents].section_count = 1;
+ extents++;
+ }
+ else if (section_ids[i] == (previous+1)) {
+ /* increment the current extent */
+ output->extents[extents-1].section_count++;
+ }
+ else {
+ /* start a new extent */
+ output->extents[extents].start_section_id = section_ids[i];
+ output->extents[extents].section_count = 1;
+ extents++;
+ }
+ previous = section_ids[i];
+ }
+ output->extent_count = extents;
+
+ *len_out = (8+(16*extents));
+
+ free(section_ids);
+ return CXL_MBOX_SUCCESS;
+all_or_nothing_fail:
+ /* free any successfully allocated sections */
+ for (i = 0; i < allocated; i++) {
+ __sync_fetch_and_and(§ions[i], ~(1 << ct3d->mhd_head));
+ __sync_fetch_and_add(&niagara_state->free_sections, 1);
+ }
+ free(section_ids);
+ return CXL_MBOX_INTERNAL_ERROR;
+}
+
+#define MHD_SECTION_RELEASE_POLICY_NONE 0
+#define MHD_SECTION_RELEASE_POLICY_CLEARING 1
+#define MHD_SECTION_RELEASE_POLICY_RANDOMIZING 2
+static CXLRetCode cmd_niagara_set_section_release(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);
+ Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
+ struct {
+ uint32_t extent_count;
+ uint8_t policy;
+ uint8_t reserved[3];
+ struct {
+ uint32_t start_section_id;
+ uint32_t section_count;
+ uint8_t reserved[8];
+ } extents[];
+ } QEMU_PACKED *input = (void *)payload_in;
+ uint32_t i, j;
+
+ uint32_t* sections = &niagara_state->sections[0];
+ for (i = 0; i < input->extent_count; i++) {
+ uint32_t start = input->extents[i].start_section_id;
+ for (j = 0; j < input->extents[i].section_count; j++) {
+ uint32_t old_val = __sync_fetch_and_and(§ions[start+j], ~(1 << ct3d->mhd_head));
+ if (old_val & (1 << ct3d->mhd_head))
+ __sync_fetch_and_add(&niagara_state->free_sections, 1);
+
+ // TODO: Policy
+ }
+ }
+
+ return CXL_MBOX_SUCCESS;
+}
+
+static CXLRetCode cmd_niagara_set_section_size(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);
+ Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
+ struct {
+ uint8_t section_unit;
+ uint8_t reserved[7];
+ } QEMU_PACKED *input = (void *)payload_in;
+ struct {
+ uint8_t section_unit;
+ uint8_t reserved[7];
+ } QEMU_PACKED *output = (void *)payload_out;
+
+ if (niagara_state->section_size == (1 << (input->section_unit - 1)))
+ goto set_section_size_success;
+
+ /* Check that there are no actively alloc'd sections */
+ if(niagara_state->free_sections != niagara_state->total_sections)
+ return CXL_MBOX_INTERNAL_ERROR;
+
+ uint32_t prev_section_size = niagara_state->section_size;
+ uint32_t prev_total_sections = niagara_state->total_sections;
+
+ niagara_state->section_size = (1 << (input->section_unit - 1));
+ niagara_state->total_sections = (prev_section_size * prev_total_sections) / niagara_state->section_size;
+ niagara_state->free_sections = niagara_state->total_sections;
+
+set_section_size_success:
+ output->section_unit = input->section_unit;
+ return CXL_MBOX_SUCCESS;
+}
+
+#define MHD_MOVE_DATA_POLICY_CLEARING 0
+#define MHD_MOVE_DATA_POLICY_NONE 1
+static CXLRetCode cmd_niagara_move_data(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ struct {
+ uint32_t extent_count;
+ uint8_t policy;
+ uint8_t reserved[3];
+ struct {
+ uint32_t source_section_id;
+ uint32_t source_data_offset;
+ uint32_t destination_section_id;
+ uint32_t destination_data_offset;
+ uint32_t data_length;
+ uint8_t reserved[4];
+ } extents;
+ } QEMU_PACKED *input = (void *)payload_in;
+
+ struct {
+ uint64_t task_id;
+ uint32_t bitset[];
+ } QEMU_PACKED *output = (void *)payload_out;
+
+ (void)input;
+ (void)output;
+
+ return CXL_MBOX_UNSUPPORTED;
+}
+
+static CXLRetCode cmd_niagara_clear_section(const struct cxl_cmd *cmd,
+ uint8_t *payload_in,
+ size_t len_in,
+ uint8_t *payload_out,
+ size_t *len_out,
+ CXLCCI *cci)
+{
+ return CXL_MBOX_UNSUPPORTED;
+}
+
+#define MHD_GSM_QUERY_FREE 0
+#define MHD_GSM_QUERY_ALLOCATED 1
+static CXLRetCode cmd_niagara_get_section_map(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);
+ Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
+ struct {
+ uint8_t query_type;
+ uint8_t reserved[7];
+ } QEMU_PACKED *input = (void *)payload_in;
+ struct {
+ uint32_t ttl_section_count;
+ uint32_t qry_section_count;
+ uint8_t bitset[];
+ } QEMU_PACKED *output = (void *)payload_out;
+
+ uint8_t query_type = input->query_type;
+ uint32_t i;
+
+ if ((query_type != MHD_GSM_QUERY_FREE) && (query_type != MHD_GSM_QUERY_ALLOCATED))
+ return CXL_MBOX_INVALID_INPUT;
+
+ output->ttl_section_count = niagara_state->total_sections;
+ output->qry_section_count = 0;
+ uint32_t bytes = (output->ttl_section_count/8);
+ if (output->ttl_section_count % 8)
+ bytes += 1;
+ for (i = 0; i < bytes; i++)
+ output->bitset[i] = 0x0;
+
+ /* Iterate the the section list and check the bits */
+ uint32_t* sections = &niagara_state->sections[0];
+ for (i = 0; (i < niagara_state->total_sections); i++) {
+ uint32_t section = sections[i];
+ if (((query_type == MHD_GSM_QUERY_FREE) && (!section)) ||
+ ((query_type == MHD_GSM_QUERY_ALLOCATED) && (section & (1 << ct3d->mhd_head)))) {
+ uint32_t byte = i / 8;
+ uint8_t bit = (1 << (i % 8));
+ output->bitset[byte] |= bit;
+ output->qry_section_count++;
+ }
+ }
+
+ *len_out = (8+bytes);
+ return CXL_MBOX_SUCCESS;
+}
+
+static bool mhdsld_access_valid(CXLType3Dev *ct3d, uint64_t dpa_offset, unsigned int size) {
+ Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
+ if (ct3d->mhd_state) {
+ uint32_t section = (dpa_offset / MIN_MEMBLK_SIZE);
+ if (!(niagara_state->sections[section] & (1 << ct3d->mhd_head))) {
+ return false;
+ }
+ }
+ return true;
+}
+
+static const struct cxl_cmd cxl_cmd_set_niagara[256][256] = {
+ [NIAGARA][GET_SECTION_STATUS] = { "GET_SECTION_STATUS",
+ cmd_niagara_get_section_status, 0, 0 },
+ [NIAGARA][SET_SECTION_ALLOC] = { "SET_SECTION_ALLOC",
+ cmd_niagara_set_section_alloc, ~0,
+ (IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE |
+ IMMEDIATE_POLICY_CHANGE | IMMEDIATE_LOG_CHANGE)
+ },
+ [NIAGARA][SET_SECTION_RELEASE] = { "SET_SECTION_RELEASE",
+ cmd_niagara_set_section_release, ~0,
+ (IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE |
+ IMMEDIATE_POLICY_CHANGE | IMMEDIATE_LOG_CHANGE)
+ },
+ [NIAGARA][SET_SECTION_SIZE] = { "SET_SECTION_SIZE",
+ cmd_niagara_set_section_size, 8,
+ (IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE |
+ IMMEDIATE_POLICY_CHANGE | IMMEDIATE_LOG_CHANGE)
+ },
+ [NIAGARA][MOVE_DATA] = { "MOVE_DATA",
+ cmd_niagara_move_data, ~0, IMMEDIATE_DATA_CHANGE },
+ [NIAGARA][GET_SECTION_MAP] = { "GET_SECTION_MAP",
+ cmd_niagara_get_section_map, 8 , IMMEDIATE_DATA_CHANGE },
+ [NIAGARA][CLEAR_SECTION] = { "CLEAR_SECTION",
+ cmd_niagara_clear_section, 0, IMMEDIATE_DATA_CHANGE },
+};
+
+enum cxl_dev_type {
+ cxl_type3,
+};
+
+struct CXL_Niagara_State {
+ PCIDevice parent_obj;
+ PCIDevice *target;
+ enum cxl_dev_type type;
+ CXLCCI *cci;
+};
+
+struct CXL_NiagaraClass {
+ PCIDeviceClass parent_class;
+};
+
+
+#define TYPE_CXL_Niagara "cxl-skh-niagara"
+OBJECT_DECLARE_TYPE(CXL_Niagara_State, CXL_NiagaraClass, CXL_Niagara)
+
+static Property cxl_niagara_props[] = {
+ DEFINE_PROP_LINK("target", CXL_Niagara_State,
+ target, TYPE_PCI_DEVICE, PCIDevice *),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void cxl_niagara_realize(DeviceState *d, Error **errp)
+{
+ CXL_Niagara_State *s = CXL_Niagara(d);
+
+ if (object_dynamic_cast(OBJECT(s->target), TYPE_CXL_TYPE3)) {
+ CXLType3Dev *ct3d = CXL_TYPE3(s->target);
+
+ if (!ct3d->is_mhd) {
+ error_setg(errp, "Niagara target must be a cxl-type3 mhd");
+ return;
+ }
+
+ s->type = cxl_type3;
+ s->cci = &ct3d->cci;
+
+ ct3d->mhd_access_valid = mhdsld_access_valid;
+ return;
+ }
+
+ error_setg(errp, "Unhandled target type for CXL Niagara MHSLD");
+}
+
+static void cxl_niagara_reset(DeviceState *d)
+{
+ CXL_Niagara_State *s = CXL_Niagara(d);
+
+ if (object_dynamic_cast(OBJECT(s->target), TYPE_CXL_TYPE3)) {
+ CXLType3Dev *ct3d = CXL_TYPE3(s->target);
+ cxl_add_cci_commands(&ct3d->cci, cxl_cmd_set_niagara, 512);
+ return;
+ }
+}
+
+static void cxl_niagara_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ dc->realize = cxl_niagara_realize;
+ dc->reset = cxl_niagara_reset;
+ device_class_set_props(dc, cxl_niagara_props);
+}
+
+static const TypeInfo cxl_niagara_info = {
+ .name = TYPE_CXL_Niagara,
+ .parent = TYPE_CXL_TYPE3,
+ .class_size = sizeof(struct CXL_NiagaraClass),
+ .class_init = cxl_niagara_class_init,
+ .instance_size = sizeof(CXL_Niagara_State),
+ .interfaces = (InterfaceInfo[]) {
+ { INTERFACE_CXL_DEVICE },
+ { INTERFACE_PCIE_DEVICE },
+ {}
+ },
+};
+
+static void cxl_niagara_register_types(void)
+{
+ type_register_static(&cxl_niagara_info);
+}
+
+type_init(cxl_niagara_register_types)
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] cxl/mailbox: change CCI cmd set structure to be a member, not a refernce
2023-07-21 16:35 ` [PATCH 1/4] cxl/mailbox: change CCI cmd set structure to be a member, not a refernce Gregory Price
@ 2023-08-04 14:56 ` Jonathan Cameron via
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron via @ 2023-08-04 14:56 UTC (permalink / raw)
To: Gregory Price
Cc: qemu-devel, linux-cxl, junhee.ryu, kwangjin.ko, Gregory Price
On Fri, 21 Jul 2023 12:35:04 -0400
Gregory Price <gourry.memverge@gmail.com> wrote:
Typo in patch title. reference
> This allows devices to have fully customized CCIs, along with complex
> devices where wrapper devices can override or add additional CCI
> commands without having to replicate full command structures or
> pollute a base device with every command that might ever be used.
>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
Hi Gregory,
This might give us a nice solution to a problem with rebasing Fan's
DCD set, so I'm reviewing it head of the later part of your series
and may pick it up in the meantime.
> ---
> hw/cxl/cxl-mailbox-utils.c | 18 ++++++++++++++----
> include/hw/cxl/cxl_device.h | 2 +-
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 2819914e8d..ddee3f1718 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1373,9 +1373,19 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
> bg_timercb, cci);
> }
>
> +static void cxl_copy_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmds)[256]) {
Bracket on next line.
> + for (int set = 0; set < 256; set++) {
> + for (int cmd = 0; cmd < 256; cmd++) {
> + if (cxl_cmds[set][cmd].handler) {
> + cci->cxl_cmd_set[set][cmd] = cxl_cmds[set][cmd];
> + }
> + }
> + }
> +}
> +
> void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, DeviceState *d, size_t payload_max)
> {
> - cci->cxl_cmd_set = cxl_cmd_set_sw;
> + cxl_copy_cci_commands(cci, cxl_cmd_set_sw);
> cci->d = d;
> cci->intf = intf;
> cxl_init_cci(cci, payload_max);
> @@ -1383,7 +1393,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, DeviceState *d
>
> void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
> {
> - cci->cxl_cmd_set = cxl_cmd_set;
> + cxl_copy_cci_commands(cci, cxl_cmd_set);
> cci->d = d;
>
> /* No separation for PCI MB as protocol handled in PCI device */
> @@ -1398,7 +1408,7 @@ static const struct cxl_cmd cxl_cmd_set_t3_mctp[256][256] = {
> void cxl_initialize_t3_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf,
> size_t payload_max)
> {
> - cci->cxl_cmd_set = cxl_cmd_set_t3_mctp;
> + cxl_copy_cci_commands(cci, cxl_cmd_set_t3_mctp);
> cci->d = d;
> cci->intf = intf;
> cxl_init_cci(cci, payload_max);
> @@ -1414,7 +1424,7 @@ static const struct cxl_cmd cxl_cmd_set_usp_mctp[256][256] = {
>
> void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf, size_t payload_max)
> {
> - cci->cxl_cmd_set = cxl_cmd_set_usp_mctp;
> + cxl_copy_cci_commands(cci, cxl_cmd_set_usp_mctp);
> cci->d = d;
> cci->intf = intf;
> cxl_init_cci(cci, payload_max);
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index c68981b618..9a3c8b2dfa 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -163,7 +163,7 @@ typedef struct CXLEventLog {
> } CXLEventLog;
>
> typedef struct CXLCCI {
> - const struct cxl_cmd (*cxl_cmd_set)[256];
> + struct cxl_cmd cxl_cmd_set[256][256];
> struct cel_log {
> uint16_t opcode;
> uint16_t effect;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] cxl/mailbox: interface to add CCI commands to an existing CCI
2023-07-21 16:35 ` [PATCH 2/4] cxl/mailbox: interface to add CCI commands to an existing CCI Gregory Price
@ 2023-08-04 15:14 ` Jonathan Cameron via
2023-08-04 16:41 ` Gregory Price
0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron via @ 2023-08-04 15:14 UTC (permalink / raw)
To: Gregory Price
Cc: qemu-devel, linux-cxl, junhee.ryu, kwangjin.ko, Gregory Price
On Fri, 21 Jul 2023 12:35:06 -0400
Gregory Price <gourry.memverge@gmail.com> wrote:
> This enables wrapper devices to customize the base device's CCI
> (for example, with custom commands outside the specification)
> without the need to change the base device.
>
> The also enabled the base device to dispatch those commands without
> requiring additional driver support.
>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 19 +++++++++++++++++++
> include/hw/cxl/cxl_device.h | 2 ++
> 2 files changed, 21 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index ddee3f1718..cad0cd0adb 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1383,6 +1383,25 @@ static void cxl_copy_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmds)[
> }
> }
>
> +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256], size_t payload_max)
> +{
> + cci->payload_max = payload_max > cci->payload_max ? payload_max : cci->payload_max;
> + for (int set = 0; set < 256; set++) {
> + for (int cmd = 0; cmd < 256; cmd++) {
> + if (cxl_cmd_set[set][cmd].handler) {
> + const struct cxl_cmd *c = &cxl_cmd_set[set][cmd];
> + cci->cxl_cmd_set[set][cmd] = *c;
Don't interleave definitions and code.
> + struct cel_log *log =
> + &cci->cel_log[cci->cel_size];
> +
> + log->opcode = (set << 8) | cmd;
> + log->effect = c->effect;
> + cci->cel_size++;
So my gut feeling on this is based on the large amount of overlapping code. I might queue it
as it stands, but I'd like to see this refactored.
1) Single copy routine used in all places that copie in any new entries to cci->cxl_cmd_set[][]
2) A single cel_log builder function to be called in normal path and after an update. Just rebuild
the whole thing rather than trying to append to it I think.
Something like (so far untested but I'll poke it with Fan's code in a few mins)
However this is all proving rather costly in space so maybe we need a better
representation for the sparse nature of cxl comamnds - a job for another day.
From 8ab48adfb2b481be0702b84a0d172a4f142b0df6 Mon Sep 17 00:00:00 2001
From: Gregory Price <gourry.memverge@gmail.com>
Date: Fri, 21 Jul 2023 12:35:06 -0400
Subject: [PATCH] cxl/mailbox: interface to add CCI commands to an existing CCI
This enables wrapper devices to customize the base device's CCI
(for example, with custom commands outside the specification)
without the need to change the base device.
The also enabled the base device to dispatch those commands without
requiring additional driver support.
Signed-off-by: Gregory Price <gregory.price@memverge.com>
Link: https://lore.kernel.org/r/20230721163505.1910-3-gregory.price@memverge.com
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
--
Heavily edited by Jonathan Cameron to increase code reuse
---
include/hw/cxl/cxl_device.h | 2 ++
hw/cxl/cxl-mailbox-utils.c | 21 +++++++++++++++++++--
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 0c9254dff9..2a3050fbad 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -297,6 +297,8 @@ void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max);
void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
DeviceState *d, size_t payload_max);
void cxl_init_cci(CXLCCI *cci, size_t payload_max);
+void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
+ size_t payload_max);
int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
size_t len_in, uint8_t *pl_in,
size_t *len_out, uint8_t *pl_out,
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 37703f254f..852e5a046b 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1353,9 +1353,9 @@ static void bg_timercb(void *opaque)
}
}
-void cxl_init_cci(CXLCCI *cci, size_t payload_max)
+static void cxl_rebuild_cel(CXLCCI *cci)
{
- cci->payload_max = payload_max;
+ cci->cel_size = 0; /* Reset for a fresh build */
for (int set = 0; set < 256; set++) {
for (int cmd = 0; cmd < 256; cmd++) {
if (cci->cxl_cmd_set[set][cmd].handler) {
@@ -1369,6 +1369,13 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
}
}
}
+}
+
+void cxl_init_cci(CXLCCI *cci, size_t payload_max)
+{
+ cci->payload_max = payload_max;
+ cxl_rebuild_cel(cci);
+
cci->bg.complete_pct = 0;
cci->bg.starttime = 0;
cci->bg.runtime = 0;
@@ -1387,10 +1394,19 @@ static void cxl_copy_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmds)[
}
}
+void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
+ size_t payload_max)
+{
+ cci->payload_max = payload_max > cci->payload_max ? payload_max : cci->payload_max;
+ cxl_copy_cci_commands(cci, cxl_cmd_set);
+ cxl_rebuild_cel(cci);
+}
+
void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
DeviceState *d, size_t payload_max)
{
cxl_copy_cci_commands(cci, cxl_cmd_set_sw);
+ cxl_rebuild_cel(cci);
cci->d = d;
cci->intf = intf;
cxl_init_cci(cci, payload_max);
@@ -1399,6 +1415,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
{
cxl_copy_cci_commands(cci, cxl_cmd_set);
+ cxl_rebuild_cel(cci);
cci->d = d;
/* No separation for PCI MB as protocol handled in PCI device */
--
2.39.2
> + }
> + }
> + }
> +}
> +
> void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, DeviceState *d, size_t payload_max)
> {
> cxl_copy_cci_commands(cci, cxl_cmd_set_sw);
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 9a3c8b2dfa..abc8405cc5 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -297,6 +297,8 @@ void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max);
> void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> DeviceState *d, size_t payload_max);
> void cxl_init_cci(CXLCCI *cci, size_t payload_max);
> +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
> + size_t payload_max);
> int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
> size_t len_in, uint8_t *pl_in,
> size_t *len_out, uint8_t *pl_out,
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] cxl/mailbox: interface to add CCI commands to an existing CCI
2023-08-04 15:14 ` Jonathan Cameron via
@ 2023-08-04 16:41 ` Gregory Price
2023-08-07 14:30 ` Jonathan Cameron via
0 siblings, 1 reply; 13+ messages in thread
From: Gregory Price @ 2023-08-04 16:41 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Gregory Price, qemu-devel, linux-cxl, junhee.ryu, kwangjin.ko
On Fri, Aug 04, 2023 at 04:14:14PM +0100, Jonathan Cameron wrote:
> On Fri, 21 Jul 2023 12:35:06 -0400
> Gregory Price <gourry.memverge@gmail.com> wrote:
>
> > This enables wrapper devices to customize the base device's CCI
> > (for example, with custom commands outside the specification)
> > without the need to change the base device.
> >
> > The also enabled the base device to dispatch those commands without
> > requiring additional driver support.
> >
> > Signed-off-by: Gregory Price <gregory.price@memverge.com>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 19 +++++++++++++++++++
> > include/hw/cxl/cxl_device.h | 2 ++
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index ddee3f1718..cad0cd0adb 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -1383,6 +1383,25 @@ static void cxl_copy_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmds)[
> > }
> > }
> >
> > +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256], size_t payload_max)
> > +{
> > + cci->payload_max = payload_max > cci->payload_max ? payload_max : cci->payload_max;
> > + for (int set = 0; set < 256; set++) {
> > + for (int cmd = 0; cmd < 256; cmd++) {
> > + if (cxl_cmd_set[set][cmd].handler) {
> > + const struct cxl_cmd *c = &cxl_cmd_set[set][cmd];
> > + cci->cxl_cmd_set[set][cmd] = *c;
> Don't interleave definitions and code.
>
> > + struct cel_log *log =
> > + &cci->cel_log[cci->cel_size];
> > +
> > + log->opcode = (set << 8) | cmd;
> > + log->effect = c->effect;
> > + cci->cel_size++;
>
> So my gut feeling on this is based on the large amount of overlapping code. I might queue it
> as it stands, but I'd like to see this refactored.
>
> 1) Single copy routine used in all places that copie in any new entries to cci->cxl_cmd_set[][]
> 2) A single cel_log builder function to be called in normal path and after an update. Just rebuild
> the whole thing rather than trying to append to it I think.
>
> Something like (so far untested but I'll poke it with Fan's code in a few mins)
>
> However this is all proving rather costly in space so maybe we need a better
> representation for the sparse nature of cxl comamnds - a job for another day.
I'd certainly considered the issue of space, but it seemed better to
blow up the size in one commit and then come back around and change the
structure out from under the work this unblocks. What we save in space
we sacrifice in complexity, but the structure seems simple enough that a
change shouldn't take a ton of scrutiny to get right.
One downside of the approach here is what happens when there's an
overlap and custom devices build up over time. As in - if i steal the
0xFF command group for my custom emulated MHMLD DCD Everything Super Device,
what happens if the spec finally comes around to defining 0xFF as a real
command set?
tl;dr: Should the copy function error on overlap detections?
Quick read-back through the spec, I don't see explicit carve-outs for
reserved command regions for custom sets, might be worth a discussion.
For now it shouldn't be an issue.
>
>
> From 8ab48adfb2b481be0702b84a0d172a4f142b0df6 Mon Sep 17 00:00:00 2001
> From: Gregory Price <gourry.memverge@gmail.com>
> Date: Fri, 21 Jul 2023 12:35:06 -0400
> Subject: [PATCH] cxl/mailbox: interface to add CCI commands to an existing CCI
>
> This enables wrapper devices to customize the base device's CCI
> (for example, with custom commands outside the specification)
> without the need to change the base device.
>
> The also enabled the base device to dispatch those commands without
> requiring additional driver support.
>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> Link: https://lore.kernel.org/r/20230721163505.1910-3-gregory.price@memverge.com
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> --
> Heavily edited by Jonathan Cameron to increase code reuse
> ---
> include/hw/cxl/cxl_device.h | 2 ++
> hw/cxl/cxl-mailbox-utils.c | 21 +++++++++++++++++++--
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 0c9254dff9..2a3050fbad 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -297,6 +297,8 @@ void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max);
> void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> DeviceState *d, size_t payload_max);
> void cxl_init_cci(CXLCCI *cci, size_t payload_max);
> +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
> + size_t payload_max);
> int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
> size_t len_in, uint8_t *pl_in,
> size_t *len_out, uint8_t *pl_out,
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 37703f254f..852e5a046b 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1353,9 +1353,9 @@ static void bg_timercb(void *opaque)
> }
> }
>
> -void cxl_init_cci(CXLCCI *cci, size_t payload_max)
> +static void cxl_rebuild_cel(CXLCCI *cci)
> {
> - cci->payload_max = payload_max;
> + cci->cel_size = 0; /* Reset for a fresh build */
> for (int set = 0; set < 256; set++) {
> for (int cmd = 0; cmd < 256; cmd++) {
> if (cci->cxl_cmd_set[set][cmd].handler) {
> @@ -1369,6 +1369,13 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
> }
> }
> }
> +}
> +
> +void cxl_init_cci(CXLCCI *cci, size_t payload_max)
> +{
> + cci->payload_max = payload_max;
> + cxl_rebuild_cel(cci);
> +
> cci->bg.complete_pct = 0;
> cci->bg.starttime = 0;
> cci->bg.runtime = 0;
> @@ -1387,10 +1394,19 @@ static void cxl_copy_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmds)[
> }
> }
>
> +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
> + size_t payload_max)
> +{
> + cci->payload_max = payload_max > cci->payload_max ? payload_max : cci->payload_max;
> + cxl_copy_cci_commands(cci, cxl_cmd_set);
> + cxl_rebuild_cel(cci);
> +}
> +
> void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> DeviceState *d, size_t payload_max)
> {
> cxl_copy_cci_commands(cci, cxl_cmd_set_sw);
> + cxl_rebuild_cel(cci);
> cci->d = d;
> cci->intf = intf;
> cxl_init_cci(cci, payload_max);
> @@ -1399,6 +1415,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
> {
> cxl_copy_cci_commands(cci, cxl_cmd_set);
> + cxl_rebuild_cel(cci);
> cci->d = d;
>
> /* No separation for PCI MB as protocol handled in PCI device */
> --
> 2.39.2
>
>
>
> > + }
> > + }
> > + }
> > +}
> > +
> > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, DeviceState *d, size_t payload_max)
> > {
> > cxl_copy_cci_commands(cci, cxl_cmd_set_sw);
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 9a3c8b2dfa..abc8405cc5 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -297,6 +297,8 @@ void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max);
> > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> > DeviceState *d, size_t payload_max);
> > void cxl_init_cci(CXLCCI *cci, size_t payload_max);
> > +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
> > + size_t payload_max);
> > int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
> > size_t len_in, uint8_t *pl_in,
> > size_t *len_out, uint8_t *pl_out,
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] cxl/mailbox: interface to add CCI commands to an existing CCI
2023-08-04 16:41 ` Gregory Price
@ 2023-08-07 14:30 ` Jonathan Cameron via
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron via @ 2023-08-07 14:30 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, qemu-devel, linux-cxl, junhee.ryu, kwangjin.ko
On Fri, 4 Aug 2023 12:41:26 -0400
Gregory Price <gregory.price@memverge.com> wrote:
> On Fri, Aug 04, 2023 at 04:14:14PM +0100, Jonathan Cameron wrote:
> > On Fri, 21 Jul 2023 12:35:06 -0400
> > Gregory Price <gourry.memverge@gmail.com> wrote:
> >
> > > This enables wrapper devices to customize the base device's CCI
> > > (for example, with custom commands outside the specification)
> > > without the need to change the base device.
> > >
> > > The also enabled the base device to dispatch those commands without
> > > requiring additional driver support.
> > >
> > > Signed-off-by: Gregory Price <gregory.price@memverge.com>
> > > ---
> > > hw/cxl/cxl-mailbox-utils.c | 19 +++++++++++++++++++
> > > include/hw/cxl/cxl_device.h | 2 ++
> > > 2 files changed, 21 insertions(+)
> > >
> > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > index ddee3f1718..cad0cd0adb 100644
> > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > @@ -1383,6 +1383,25 @@ static void cxl_copy_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmds)[
> > > }
> > > }
> > >
> > > +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256], size_t payload_max)
> > > +{
> > > + cci->payload_max = payload_max > cci->payload_max ? payload_max : cci->payload_max;
> > > + for (int set = 0; set < 256; set++) {
> > > + for (int cmd = 0; cmd < 256; cmd++) {
> > > + if (cxl_cmd_set[set][cmd].handler) {
> > > + const struct cxl_cmd *c = &cxl_cmd_set[set][cmd];
> > > + cci->cxl_cmd_set[set][cmd] = *c;
> > Don't interleave definitions and code.
> >
> > > + struct cel_log *log =
> > > + &cci->cel_log[cci->cel_size];
> > > +
> > > + log->opcode = (set << 8) | cmd;
> > > + log->effect = c->effect;
> > > + cci->cel_size++;
> >
> > So my gut feeling on this is based on the large amount of overlapping code. I might queue it
> > as it stands, but I'd like to see this refactored.
> >
> > 1) Single copy routine used in all places that copie in any new entries to cci->cxl_cmd_set[][]
> > 2) A single cel_log builder function to be called in normal path and after an update. Just rebuild
> > the whole thing rather than trying to append to it I think.
> >
> > Something like (so far untested but I'll poke it with Fan's code in a few mins)
> >
> > However this is all proving rather costly in space so maybe we need a better
> > representation for the sparse nature of cxl comamnds - a job for another day.
>
> I'd certainly considered the issue of space, but it seemed better to
> blow up the size in one commit and then come back around and change the
> structure out from under the work this unblocks. What we save in space
> we sacrifice in complexity, but the structure seems simple enough that a
> change shouldn't take a ton of scrutiny to get right.
Makes sense.
>
> One downside of the approach here is what happens when there's an
> overlap and custom devices build up over time. As in - if i steal the
> 0xFF command group for my custom emulated MHMLD DCD Everything Super Device,
> what happens if the spec finally comes around to defining 0xFF as a real
> command set?
We don't support your command unless it's in the vendor defined space.
Opcodes c000h-ffffh are all yours to do what you like with :)
>
> tl;dr: Should the copy function error on overlap detections?
Maybe.. If it's easy we definitely could do that even if it's considered a
bug if it occurs.
>
> Quick read-back through the spec, I don't see explicit carve-outs for
> reserved command regions for custom sets, might be worth a discussion.
8.2.9 Component Command Interface
"Opcodes C000h-FFFFh describe vendor specific commands."
Jonathan
>
> For now it shouldn't be an issue.
>
> >
> >
> > From 8ab48adfb2b481be0702b84a0d172a4f142b0df6 Mon Sep 17 00:00:00 2001
> > From: Gregory Price <gourry.memverge@gmail.com>
> > Date: Fri, 21 Jul 2023 12:35:06 -0400
> > Subject: [PATCH] cxl/mailbox: interface to add CCI commands to an existing CCI
> >
> > This enables wrapper devices to customize the base device's CCI
> > (for example, with custom commands outside the specification)
> > without the need to change the base device.
> >
> > The also enabled the base device to dispatch those commands without
> > requiring additional driver support.
> >
> > Signed-off-by: Gregory Price <gregory.price@memverge.com>
> > Link: https://lore.kernel.org/r/20230721163505.1910-3-gregory.price@memverge.com
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > --
> > Heavily edited by Jonathan Cameron to increase code reuse
> > ---
> > include/hw/cxl/cxl_device.h | 2 ++
> > hw/cxl/cxl-mailbox-utils.c | 21 +++++++++++++++++++--
> > 2 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 0c9254dff9..2a3050fbad 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -297,6 +297,8 @@ void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max);
> > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> > DeviceState *d, size_t payload_max);
> > void cxl_init_cci(CXLCCI *cci, size_t payload_max);
> > +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
> > + size_t payload_max);
> > int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
> > size_t len_in, uint8_t *pl_in,
> > size_t *len_out, uint8_t *pl_out,
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 37703f254f..852e5a046b 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -1353,9 +1353,9 @@ static void bg_timercb(void *opaque)
> > }
> > }
> >
> > -void cxl_init_cci(CXLCCI *cci, size_t payload_max)
> > +static void cxl_rebuild_cel(CXLCCI *cci)
> > {
> > - cci->payload_max = payload_max;
> > + cci->cel_size = 0; /* Reset for a fresh build */
> > for (int set = 0; set < 256; set++) {
> > for (int cmd = 0; cmd < 256; cmd++) {
> > if (cci->cxl_cmd_set[set][cmd].handler) {
> > @@ -1369,6 +1369,13 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
> > }
> > }
> > }
> > +}
> > +
> > +void cxl_init_cci(CXLCCI *cci, size_t payload_max)
> > +{
> > + cci->payload_max = payload_max;
> > + cxl_rebuild_cel(cci);
> > +
> > cci->bg.complete_pct = 0;
> > cci->bg.starttime = 0;
> > cci->bg.runtime = 0;
> > @@ -1387,10 +1394,19 @@ static void cxl_copy_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmds)[
> > }
> > }
> >
> > +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
> > + size_t payload_max)
> > +{
> > + cci->payload_max = payload_max > cci->payload_max ? payload_max : cci->payload_max;
> > + cxl_copy_cci_commands(cci, cxl_cmd_set);
> > + cxl_rebuild_cel(cci);
> > +}
> > +
> > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> > DeviceState *d, size_t payload_max)
> > {
> > cxl_copy_cci_commands(cci, cxl_cmd_set_sw);
> > + cxl_rebuild_cel(cci);
> > cci->d = d;
> > cci->intf = intf;
> > cxl_init_cci(cci, payload_max);
> > @@ -1399,6 +1415,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> > void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
> > {
> > cxl_copy_cci_commands(cci, cxl_cmd_set);
> > + cxl_rebuild_cel(cci);
> > cci->d = d;
> >
> > /* No separation for PCI MB as protocol handled in PCI device */
> > --
> > 2.39.2
> >
> >
> >
> > > + }
> > > + }
> > > + }
> > > +}
> > > +
> > > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, DeviceState *d, size_t payload_max)
> > > {
> > > cxl_copy_cci_commands(cci, cxl_cmd_set_sw);
> > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > > index 9a3c8b2dfa..abc8405cc5 100644
> > > --- a/include/hw/cxl/cxl_device.h
> > > +++ b/include/hw/cxl/cxl_device.h
> > > @@ -297,6 +297,8 @@ void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max);
> > > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
> > > DeviceState *d, size_t payload_max);
> > > void cxl_init_cci(CXLCCI *cci, size_t payload_max);
> > > +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
> > > + size_t payload_max);
> > > int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
> > > size_t len_in, uint8_t *pl_in,
> > > size_t *len_out, uint8_t *pl_out,
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] cxl/type3: minimum MHD cci support
2023-07-21 16:35 ` [PATCH 3/4] cxl/type3: minimum MHD cci support Gregory Price
@ 2023-08-07 14:56 ` Jonathan Cameron via
2023-08-31 16:59 ` Gregory Price
0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron via @ 2023-08-07 14:56 UTC (permalink / raw)
To: Gregory Price
Cc: qemu-devel, linux-cxl, junhee.ryu, kwangjin.ko, Gregory Price
On Fri, 21 Jul 2023 12:35:08 -0400
Gregory Price <gourry.memverge@gmail.com> wrote:
> Implement the MHD GET_INFO cci command and add a shared memory
> region to the type3 device to host the information.
>
> Add a helper program to initialize this shared memory region.
>
> Add a function pointer to type3 devices for future work that
> will allow an mhd device to provide a hook to validate whether
> a memory access is valid or not.
>
> For now, limit the number of LD's to the number of heads. Later,
> this limitation will need to be lifted for MH-MLDs.
>
> Intended use case:
>
> 1. Create the shared memory region
> 2. Format the shared memory region
> 3. Launch QEMU with `is_mhd=true,mhd_head=N,mhd_shmid=$shmid`
I'd definitely like some feedback from experienced QEMU folk on
how to model this sort of cross QEMU instance sharing.
My instinct is a socket would be more flexible as it lets us
potentially emulate the machines on multiple hosts (assuming they
can see some shared backend storage).
Anyhow, some superficial comments inline.
What you have here looks good if we think shared memory is the
way to do this! Some bits are good anyway of course :)
Jonathan
>
> shmid=`ipcmk -M 4096 | grep -o -E '[0-9]+' | head -1`
> cxl_mhd_init 4 $shmid
> qemu-system-x86_64 \
> -nographic \
> -accel kvm \
> -drive file=./mhd.qcow2,format=qcow2,index=0,media=disk,id=hd \
> -m 4G,slots=4,maxmem=8G \
> -smp 4 \
> -machine type=q35,cxl=on,hmat=on \
> -device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 \
> -device cxl-rp,id=rp0,bus=cxl.0,chassis=0,port=0,slot=0 \
> -object memory-backend-file,id=mem0,mem-path=/tmp/mem0,size=4G,share=true \
> -device cxl-type3,bus=rp0,volatile-memdev=mem0,id=cxl-mem0,sn=66666,is_mhd=true,mhd_head=0,mhd_shmid=$shmid \
> -M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G
>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 53 +++++++++++++++++++++++++++++
> hw/mem/cxl_type3.c | 67 +++++++++++++++++++++++++++++++++++++
> include/hw/cxl/cxl_device.h | 14 ++++++++
> tools/cxl/cxl_mhd_init.c | 63 ++++++++++++++++++++++++++++++++++
> tools/cxl/meson.build | 3 ++
> tools/meson.build | 1 +
> 6 files changed, 201 insertions(+)
> create mode 100644 tools/cxl/cxl_mhd_init.c
> create mode 100644 tools/cxl/meson.build
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index cad0cd0adb..57b8da4376 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -84,6 +84,8 @@ enum {
> #define GET_PHYSICAL_PORT_STATE 0x1
> TUNNEL = 0x53,
> #define MANAGEMENT_COMMAND 0x0
> + MHD = 0x55,
> + #define GET_MHD_INFO 0x0
> };
>
> /* CCI Message Format CXL r3.0 Figure 7-19 */
> @@ -1155,6 +1157,56 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +static CXLRetCode cmd_mhd_get_info(const struct cxl_cmd *cmd,
Reference would be good (for some reason it took me quite a bit
of typing related search terms in to actually find it in
CXL r3.0 section 7.6.7.5.1: Get Multi-Headed Info (Opcode 5500h)
(I'm trying to standardize on that format - just need to fix
all the existing references!)
> + 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 {
> + uint8_t start_ld;
> + uint8_t ldmap_len;
> + } QEMU_PACKED *input = (void *)payload_in;
> +
> + struct {
> + uint8_t nr_lds;
> + uint8_t nr_heads;
> + uint16_t resv1;
> + uint8_t start_ld;
> + uint8_t ldmap_len;
> + uint16_t resv2;
> + uint8_t ldmap[];
> + } QEMU_PACKED *output = (void *)payload_out;
> +
> + uint8_t start_ld = input->start_ld;
> + uint8_t ldmap_len = input->ldmap_len;
> + uint8_t i;
> +
> + if (!ct3d->is_mhd) {
> + return CXL_MBOX_UNSUPPORTED;
> + }
> +
> + if (start_ld >= ct3d->mhd_state->nr_lds) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + output->nr_lds = ct3d->mhd_state->nr_lds;
> + output->nr_heads = ct3d->mhd_state->nr_heads;
> + output->resv1 = 0;
> + output->start_ld = start_ld;
> + output->resv2 = 0;
> +
> + for (i = 0; i < ldmap_len && (start_ld + i) < output->nr_lds; i++) {
> + output->ldmap[i] = ct3d->mhd_state->ldmap[start_ld + i];
> + }
> + output->ldmap_len = i;
> +
> + *len_out = sizeof(*output) + output->ldmap_len;
> + return CXL_MBOX_SUCCESS;
> +}
> +
> #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> #define IMMEDIATE_DATA_CHANGE (1 << 2)
> #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -1195,6 +1247,7 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
> cmd_media_inject_poison, 8, 0 },
> [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON",
> cmd_media_clear_poison, 72, 0 },
> + [MHD][GET_MHD_INFO] = {"GET_MULTI_HEADED_INFO", cmd_mhd_get_info, 2, 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 efb7dece80..c8eb3aa67d 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -18,6 +18,7 @@
> #include "hw/cxl/cxl.h"
> #include "hw/pci/msix.h"
> #include "hw/pci/spdm.h"
> +#include <sys/shm.h>
>
> #define DWORD_BYTE 4
>
> @@ -794,6 +795,48 @@ static DOEProtocol doe_spdm_prot[] = {
> { }
> };
>
> +static bool cxl_setup_mhd(CXLType3Dev *ct3d, Error **errp)
> +{
> + if (!ct3d->is_mhd) {
> + ct3d->mhd_access_valid = NULL;
> + return true;
> + } else if (ct3d->is_mhd &&
else not needed if we returned in previous leg.
if (ct3d->is_mhd && ...
> + (!ct3d->mhd_shmid || (ct3d->mhd_head == ~(0)))) {
How does mhd_head equal that? Default is 0. I'm not sure there is a reason
to require it.
> + error_setg(errp, "is_mhd requires mhd_shmid and mhd_head settings");
> + return false;
> + } else if (!ct3d->is_mhd &&
same here
> + (ct3d->mhd_shmid || (ct3d->mhd_head == ~(0)))) {
How does mhd_head equal that? Default is 0.
> + error_setg(errp, "(is_mhd,mhd_head,mhd_shmid) invalid");
> + return false;
> + }
> +
> + if (ct3d->mhd_head >= 32) {
> + error_setg(errp, "MHD Head ID must be between 0-31");
> + return false;
> + }
> +
> + ct3d->mhd_state = shmat(ct3d->mhd_shmid, NULL, 0);
> + if (ct3d->mhd_state == (void*)-1) {
> + ct3d->mhd_state = NULL;
> + error_setg(errp, "Unable to attach MHD State. Check ipcs is valid");
> + return false;
> + }
> +
> + /* For now, limit the number of heads to the number of LD's (SLD) */
Feels backwards. Number of heads never going to be bigger than numbre of
LDs. Other way around is possible of course.
> + if (ct3d->mhd_state->nr_heads <= ct3d->mhd_head) {
mhd head needs to be out of range? Confused.
> + error_setg(errp, "Invalid head ID for multiheaded device.");
> + return false;
> + }
> +
> + if (ct3d->mhd_state->nr_lds <= ct3d->mhd_head) {
> + error_setg(errp, "MHD Shared state does not have sufficient lds.");
> + return false;
> + }
> +
> + ct3d->mhd_state->ldmap[ct3d->mhd_head] = ct3d->mhd_head;
> + return true;
> +}
> +
> static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> {
> CXLType3Dev *ct3d = CXL_TYPE3(pci_dev);
> @@ -806,6 +849,10 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>
> QTAILQ_INIT(&ct3d->error_list);
>
> + if (!cxl_setup_mhd(ct3d, errp)) {
> + return;
> + }
> +
> if (!cxl_setup_memory(ct3d, errp)) {
> return;
> }
> @@ -910,6 +957,9 @@ static void ct3_exit(PCIDevice *pci_dev)
> if (ct3d->hostvmem) {
> address_space_destroy(&ct3d->hostvmem_as);
> }
> + if (ct3d->mhd_state) {
> + shmdt(ct3d->mhd_state);
> + }
Reverse order of realize - so I think this wants to be earlier.
> }
>
> static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
> @@ -1006,6 +1056,7 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
> MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> unsigned size, MemTxAttrs attrs)
> {
> + CXLType3Dev *ct3d = CXL_TYPE3(d);
> uint64_t dpa_offset = 0;
> AddressSpace *as = NULL;
> int res;
> @@ -1016,16 +1067,23 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> return MEMTX_ERROR;
> }
>
> + if (ct3d->is_mhd && ct3d->mhd_access_valid) {
> + if (!ct3d->mhd_access_valid(ct3d, dpa_offset, size))
> + return MEMTX_ERROR;
Brackets for inner block.
Arguably could just add the ct3d->is_mhd check in the place where
mhd_access_valid is set and hence only need to check that here.
Maybe that makes it slightly harder to follow though.
Also could just unset is_mhd if mhd_access_valid not present..
> + }
> +
> if (sanitize_running(&CXL_TYPE3(d)->cci)) {
> qemu_guest_getrandom_nofail(data, size);
> return MEMTX_OK;
> }
> +
Reasonable change but not in this patch set.
> return address_space_read(as, dpa_offset, attrs, data, size);
> }
>
> MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
> unsigned size, MemTxAttrs attrs)
> {
> + CXLType3Dev *ct3d = CXL_TYPE3(d);
Use that in the other places (can see one below).
> uint64_t dpa_offset = 0;
> AddressSpace *as = NULL;
> int res;
> @@ -1035,6 +1093,12 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
> if (res) {
> return MEMTX_ERROR;
> }
> +
> + if (ct3d->is_mhd && ct3d->mhd_access_valid) {
> + if (!ct3d->mhd_access_valid(ct3d, dpa_offset, size))
Even one lines blocks in QEMU like this need {}
I keep forgetting that and getting it picked up in reviews.
> + return MEMTX_ERROR;
> + }
if (ct3d->is_mhd && ct3d->mhd_access_valid &&
!ctrd->mhd_access_valid(ct3d, dpa_offset, size);
> +
> if (sanitize_running(&CXL_TYPE3(d)->cci)) {
> return MEMTX_OK;
> }
> @@ -1067,6 +1131,9 @@ static Property ct3_props[] = {
> DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL),
> DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename),
> DEFINE_PROP_UINT16("spdm", CXLType3Dev, spdm_port, 0),
> + DEFINE_PROP_BOOL("is_mhd", CXLType3Dev, is_mhd, false),
> + DEFINE_PROP_UINT32("mhd_head", CXLType3Dev, mhd_head, 0),
"mhd-head" etc for naming. IIRC that's the conventional form for
QEMU parameters.
> + DEFINE_PROP_UINT32("mhd_shmid", CXLType3Dev, mhd_shmid, 0),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index abc8405cc5..b545c5b6f3 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -408,6 +408,12 @@ typedef struct CXLPoison {
> typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> #define CXL_POISON_LIST_LIMIT 256
>
> +struct CXLMHD_SharedState {
No underscores in QEMU types, and make it a typedef to
CXLMHDSharedState
> + uint8_t nr_heads;
> + uint8_t nr_lds;
> + uint8_t ldmap[];
> +};
> +
> struct CXLType3Dev {
> /* Private */
> PCIDevice parent_obj;
> @@ -442,6 +448,14 @@ struct CXLType3Dev {
> unsigned int poison_list_cnt;
> bool poison_list_overflowed;
> uint64_t poison_list_overflow_ts;
> +
> + /* Multi-headed Device */
> + bool is_mhd;
> + uint32_t mhd_head;
> + uint32_t mhd_shmid;
> + struct CXLMHD_SharedState *mhd_state;
> + bool (*mhd_access_valid)(CXLType3Dev* ct3d, uint64_t addr,
> + unsigned int size);
> };
>
> #define TYPE_CXL_TYPE3 "cxl-type3"
> diff --git a/tools/cxl/cxl_mhd_init.c b/tools/cxl/cxl_mhd_init.c
> new file mode 100644
> index 0000000000..1303aa9494
> --- /dev/null
> +++ b/tools/cxl/cxl_mhd_init.c
Good to have a license.
> @@ -0,0 +1,63 @@
> +#include <signal.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ipc.h>
> +#include <sys/shm.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +struct mhd_state {
> + uint8_t nr_heads;
> + uint8_t nr_lds;
> + uint8_t ldmap[];
> +};
> +
> +int main(int argc, char *argv[]) {
> + int shmid = 0;
> + uint32_t heads = 0;
> + struct mhd_state* mhd_state = 0;
> + uint8_t i;
> +
> + if (argc != 3) {
> + printf("usage: cxl_mhd_init <heads> <shmid>\n"
> + "\theads : number of heads on the device\n"
> + "\tshmid : /tmp/mytoken.tmp\n");
> + return -1;
> + }
> +
> + // must have at least 1 head
> + heads = (uint32_t)atoi(argv[1]);
> + if (heads == 0 || heads > 32) {
Nice to have a comment here on why 32 is the maximum
> + printf("bad heads argument (1-32)\n");
> + return -1;
> + }
> +
> + shmid = (uint32_t)atoi(argv[2]);
> + if (shmid== 0) {
> + printf("bad shmid argument\n");
> + return -1;
> + }
> +
> + mhd_state = shmat(shmid, NULL, 0);
> + if (mhd_state == (void*)-1) {
> + printf("Unable to attach to shared memory\n");
> + return -1;
> + }
> +
> + // Initialize the mhd_state
> + size_t mhd_state_size = sizeof(struct mhd_state) + (sizeof(uint8_t) * heads);
> + memset(mhd_state, 0, mhd_state_size);
> + mhd_state->nr_heads = heads;
> + mhd_state->nr_lds = heads;
> +
> + // Head ID == LD ID for now
Trivial but... C style comments for QEMU probably even standalone utils.
https://elixir.bootlin.com/qemu/latest/source/docs/devel/style.rst#L235
> + for (i = 0; i < heads; i++)
> + mhd_state->ldmap[i] = i;
> +
> + printf("mhd initialized\n");
> + shmdt(mhd_state);
> + return 0;
> +}
> diff --git a/tools/cxl/meson.build b/tools/cxl/meson.build
> new file mode 100644
> index 0000000000..218658fe69
> --- /dev/null
> +++ b/tools/cxl/meson.build
> @@ -0,0 +1,3 @@
> +executable('cxl_mhd_init', files('cxl_mhd_init.c'),
> + install: true,
> + install_dir: get_option('libexecdir'))
> diff --git a/tools/meson.build b/tools/meson.build
> index e69de29bb2..91a1d788cb 100644
> --- a/tools/meson.build
> +++ b/tools/meson.build
> @@ -0,0 +1 @@
> +subdir('cxl')
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] cxl/vendor: SK hynix Niagara Multi-Headed SLD Device
2023-07-21 16:35 ` [PATCH 4/4] cxl/vendor: SK hynix Niagara Multi-Headed SLD Device Gregory Price
@ 2023-08-07 15:36 ` Jonathan Cameron via
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron via @ 2023-08-07 15:36 UTC (permalink / raw)
To: Gregory Price
Cc: qemu-devel, linux-cxl, junhee.ryu, kwangjin.ko, Gregory Price
On Fri, 21 Jul 2023 12:35:09 -0400
Gregory Price <gourry.memverge@gmail.com> wrote:
> Create a new device to emulate the SK hynix Niagara MHSLD platform.
>
> This device has custom CCI commands that allow for applying isolation
> to each memory block between hosts. This enables an early form of
> dynamic capacity, whereby the NUMA node maps the entire region, but
> the host is responsible for asking the device which memory blocks
> are allocated to it, and therefore may be onlined.
>
> To instantiate, wrap a cxl-type3 mhd in a cxl-skh-niagara like so:
>
> -device cxl-type3,bus=rp0,volatile-memdev=mem0,id=cxl-mem0,sn=66666,is_mhd=true,mhd_head=0,mhd_shmid=15
> -device cxl-skh-niagara,target=cxl-mem0
Hmm. I'm not sure why the wrapping makes sense rather than inheriting form
the cxl-type3 device - with appropriate class provided overrides of functions
etc and relying on the qemu object model to allow you to cast up
and down the inheritance tree.
A few related bits inline on that.
>
> The linux kernel will require raw CXL commands enabled to allow for
> passing through of Niagara CXL commands via the CCI mailbox.
>
> The Niagara MH-SLD has a slightly different shared memory region
> than the base MHD, so an additional tool ('init_niagara') is located
> in the vendor subdirectory. Utilize this in place of cxl_mhd_init.
>
> usage: init_niagara <heads> <sections> <section_size> <shmid>
> heads : number of heads on the device
> sections : number of sections
> section_size : size of a section in 128mb increments
> shmid : shmid produced by ipcmk
>
> Example:
> $shmid1=ipcmk -M 131072
> ./init_niagara 4 32 1 $shmid1
>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> Signed-off-by: Junhee Ryu <junhee.ryu@sk.com>
> Signed-off-by: Kwangjin Ko <kwangjin.ko@sk.com>
I'm definitely fine with emulating real devices that go beyond what is
defined in the CXL spec, so great to see one being proposed.
Most comments are cleanup related.
> ---
> hw/cxl/Kconfig | 4 +
> hw/cxl/meson.build | 2 +
> hw/cxl/vendor/meson.build | 1 +
> hw/cxl/vendor/skhynix/.gitignore | 1 +
> hw/cxl/vendor/skhynix/init_niagara.c | 99 +++++
> hw/cxl/vendor/skhynix/meson.build | 1 +
> hw/cxl/vendor/skhynix/skhynix_niagara.c | 521 ++++++++++++++++++++++++
> 7 files changed, 629 insertions(+)
> create mode 100644 hw/cxl/vendor/meson.build
> create mode 100644 hw/cxl/vendor/skhynix/.gitignore
> create mode 100644 hw/cxl/vendor/skhynix/init_niagara.c
> create mode 100644 hw/cxl/vendor/skhynix/meson.build
> create mode 100644 hw/cxl/vendor/skhynix/skhynix_niagara.c
>
> diff --git a/hw/cxl/Kconfig b/hw/cxl/Kconfig
> index c9b2e46bac..dd6c54b54d 100644
> --- a/hw/cxl/Kconfig
> +++ b/hw/cxl/Kconfig
> @@ -2,5 +2,9 @@ config CXL
> bool
> default y if PCI_EXPRESS
>
> +config CXL_VENDOR
> + bool
> + default y
> +
> config I2C_MCTP_CXL
> bool
> diff --git a/hw/cxl/meson.build b/hw/cxl/meson.build
> index 1393821fc4..e8c8c1355a 100644
> --- a/hw/cxl/meson.build
> +++ b/hw/cxl/meson.build
> @@ -15,3 +15,5 @@ system_ss.add(when: 'CONFIG_CXL',
> system_ss.add(when: 'CONFIG_I2C_MCTP_CXL', if_true: files('i2c_mctp_cxl.c'))
>
> system_ss.add(when: 'CONFIG_ALL', if_true: files('cxl-host-stubs.c'))
> +
> +subdir('vendor')
> diff --git a/hw/cxl/vendor/meson.build b/hw/cxl/vendor/meson.build
> new file mode 100644
> index 0000000000..12db8991f1
> --- /dev/null
> +++ b/hw/cxl/vendor/meson.build
> @@ -0,0 +1 @@
> +subdir('skhynix')
> diff --git a/hw/cxl/vendor/skhynix/.gitignore b/hw/cxl/vendor/skhynix/.gitignore
> new file mode 100644
> index 0000000000..6d96de38ea
> --- /dev/null
> +++ b/hw/cxl/vendor/skhynix/.gitignore
> @@ -0,0 +1 @@
> +init_niagara
> diff --git a/hw/cxl/vendor/skhynix/init_niagara.c b/hw/cxl/vendor/skhynix/init_niagara.c
> new file mode 100644
> index 0000000000..28612339e0
> --- /dev/null
> +++ b/hw/cxl/vendor/skhynix/init_niagara.c
I'm not sure we should burry tooling all the way down here. Not much under tools though.
> @@ -0,0 +1,99 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright (c) 2023 MemVerge Inc.
> + * Copyright (c) 2023 SK hynix Inc.
> + *
> + * Reference list:
> + * From www.computeexpresslink.org
> + * Compute Express Link (CXL) Specification revision 3.0 Version 1.0
Not much CXL 3.0 in this file!
> + */
> +
> +#include <signal.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ipc.h>
> +#include <sys/shm.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +struct niagara_state {
> + uint8_t nr_heads;
> + uint8_t nr_lds;
> + uint8_t ldmap[65536];
> + uint32_t total_sections;
> + uint32_t free_sections;
> + uint32_t section_size;
> + uint32_t sections[];
> +};
> +
> +int main(int argc, char *argv[]) {
New line before {
It's in the qemu tree, so we should stick to qemu style mostly.
> + int shmid = 0;
> + uint32_t sections = 0;
> + uint32_t section_size = 0;
> + uint32_t heads = 0;
> + struct niagara_state* niagara_state = 0;
NULL
> + uint8_t i;
> +
> + if (argc != 5) {
> + printf("usage: init_niagara <heads> <sections> <section_size> <shmid>\n"
> + "\theads : number of heads on the device\n"
> + "\tsections : number of sections\n"
> + "\tsection_size : size of a section in 128mb increments\n"
> + "\tshmid : /tmp/mytoken.tmp\n\n"
> + "It is recommended your shared memory region is at least 128kb\n");
> + return -1;
> + }
> +
> + // must have at least 1 head
> + heads = (uint32_t)atoi(argv[1]);
> + if (heads == 0 || heads > 32) {
> + printf("bad heads argument (1-32)\n");
> + return -1;
> + }
> +
> + // Get number of sections
> + sections = (uint32_t)atoi(argv[2]);
> + if (sections == 0) {
> + printf("bad sections argument\n");
> + return -1;
> + }
> +
> + section_size = (uint32_t)atoi(argv[3]);
> + if (sections == 0) {
> + printf("bad section size argument\n");
> + return -1;
> + }
> +
> + shmid = (uint32_t)atoi(argv[4]);
> + if (shmid== 0) {
> + printf("bad shmid argument\n");
> + return -1;
> + }
> +
> + niagara_state = shmat(shmid, NULL, 0);
> + if (niagara_state == (void*)-1) {
> + printf("Unable to attach to shared memory\n");
> + return -1;
> + }
> +
> + // Initialize the niagara_state
> + size_t niagara_state_size = sizeof(struct niagara_state) + (sizeof(uint32_t) * sections);
> + memset(niagara_state, 0, niagara_state_size);
> + niagara_state->nr_heads = heads;
> + niagara_state->nr_lds = heads;
> + niagara_state->total_sections = sections;
> + niagara_state->free_sections = sections;
> + niagara_state->section_size = section_size;
> +
> + memset(&niagara_state->ldmap, '\xff', sizeof(niagara_state->ldmap));
> + for (i = 0; i < heads; i++)
> + niagara_state->ldmap[i] = i;
> +
> + printf("niagara initialized\n");
> + shmdt(niagara_state);
> + return 0;
> +}
> diff --git a/hw/cxl/vendor/skhynix/meson.build b/hw/cxl/vendor/skhynix/meson.build
> new file mode 100644
> index 0000000000..4e57db65f1
> --- /dev/null
> +++ b/hw/cxl/vendor/skhynix/meson.build
> @@ -0,0 +1 @@
> +system_ss.add(when: 'CONFIG_CXL_VENDOR', if_true: files('skhynix_niagara.c',))
> diff --git a/hw/cxl/vendor/skhynix/skhynix_niagara.c b/hw/cxl/vendor/skhynix/skhynix_niagara.c
> new file mode 100644
> index 0000000000..1224978585
> --- /dev/null
> +++ b/hw/cxl/vendor/skhynix/skhynix_niagara.c
> @@ -0,0 +1,521 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright (c) 2023 MemVerge Inc.
> + * Copyright (c) 2023 SK hynix Inc.
> + *
> + * Reference list:
> + * From www.computeexpresslink.org
> + * Compute Express Link (CXL) Specification revision 3.0 Version 1.0
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/irq.h"
> +#include "migration/vmstate.h"
> +#include "qapi/error.h"
> +#include "hw/cxl/cxl.h"
> +#include "hw/cxl/cxl_device.h"
> +#include "hw/pci/pcie.h"
> +#include "hw/pci/pcie_port.h"
> +#include "hw/qdev-properties.h"
> +
> +#define MIN_MEMBLK_SIZE (1024*1024*128)
> +
> +/*
> + * The shared state cannot have 2 variable sized regions
> + * so we have to max out the ldmap.
> +*/
> +typedef struct Niagara_Shared_State Niagara_Shared_State;
Qemu is all camel case for types and combined struct definitions
with typdefs
typedef struct NiagraSharedState {
uint8_t nr_heads;
uint8_t nr_lds;
uint8_t ldmap[65536];
uint32_t total_sections;
uint32_t free_sections;
uint32_t section_size;
uint32_t sections[];
} NiagraSharedstate;
> +struct Niagara_Shared_State {
> + uint8_t nr_heads;
> + uint8_t nr_lds;
> + uint8_t ldmap[65536];
> + uint32_t total_sections;
> + uint32_t free_sections;
> + uint32_t section_size;
> + uint32_t sections[];
> +};
> +
> +#define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> +#define IMMEDIATE_DATA_CHANGE (1 << 2)
> +#define IMMEDIATE_POLICY_CHANGE (1 << 3)
> +#define IMMEDIATE_LOG_CHANGE (1 << 4)
> +#define SECURITY_STATE_CHANGE (1 << 5)
> +#define BACKGROUND_OPERATION (1 << 6)
Lift these out of the core cxl code into a suitable header rather than
repeating. Fair enough not in first RFC though.
> +
> +enum {
> + NIAGARA = 0xC0
Good it's in the vendor defined space. I wondered after your
comment earlier on there not being one defined!
> + #define GET_SECTION_STATUS 0x0
> + #define SET_SECTION_ALLOC 0x1
> + #define SET_SECTION_RELEASE 0x2
> + #define SET_SECTION_SIZE 0x3
> + #define MOVE_DATA 0x4
> + #define GET_SECTION_MAP 0x5
> + #define CLEAR_SECTION 0x99
> +};
> +
> +static CXLRetCode cmd_niagara_get_section_status(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);
> + Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
> + struct {
> + uint32_t total_section_count;
> + uint32_t free_section_count;
> + } QEMU_PACKED *output = (void *)payload_out;
> +
> + if (!ct3d->is_mhd)
> + return CXL_MBOX_UNSUPPORTED;
> +
> + output->total_section_count = niagara_state->total_sections;
> + output->free_section_count = niagara_state->free_sections;
> +
> + *len_out = sizeof(*output);
> +
> + return CXL_MBOX_SUCCESS;
> +}
> +
> +#define MHD_SECTION_ALLOC_POLICY_ALL_OR_NOTHING 0
> +#define MHD_SECTION_ALLOC_POLICY_BEST_EFFORT 1
> +#define MHD_SECTION_ALLOC_POLICY_MANUAL 2
Even though they are buried down here, good to name these so they
are niagara specific.
> +static CXLRetCode cmd_niagara_set_section_alloc(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);
> + Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
> + struct {
> + uint8_t policy;
> + uint8_t reserved1[3];
> + uint32_t section_count;
> + uint8_t reserved2[4];
> + uint32_t extent_count;
> + struct {
> + uint32_t start_section_id;
> + uint32_t section_count;
> + uint8_t reserved[8];
> + } extents[];
> + } QEMU_PACKED *input = (void *)payload_in;
> + struct {
> + uint32_t section_count;
> + uint32_t extent_count;
> + struct {
> + uint32_t start_section_id;
> + uint32_t section_count;
> + uint8_t reserved[8];
> + } extents[];
> + } QEMU_PACKED *output = (void *)payload_out;
> +
> + uint8_t policy = input->policy;
> + uint32_t count = input->section_count;
> + uint32_t i = 0;
> +
> + if (count == 0 || count > niagara_state->total_sections) {
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + if (input->policy == MHD_SECTION_ALLOC_POLICY_MANUAL) {
> + /* iterate input extents and count total sections for validation */
> + uint32_t ttl_sec = 0;
> + for (i = 0; i < input->extent_count; i++) {
> + uint32_t start = input->extents[i].start_section_id;
> + uint32_t end = start + input->extents[i].section_count;
> + if ((start >= niagara_state->total_sections) || (end > niagara_state->total_sections))
> + return CXL_MBOX_INVALID_INPUT;
> + ttl_sec += input->extents[i].section_count;
> + }
> + if (ttl_sec != input->section_count)
Brackets needed.
> + return CXL_MBOX_INVALID_INPUT;
> + }
> +
> + uint32_t *section_ids = malloc(count*sizeof(uint32_t));
docs/devel/style.rst:
"
Declarations
============
Mixed declarations (interleaving statements and declarations within
blocks) are generally not allowed; declarations should be at the beginning
of blocks
"
This is looking like it might benefit from being split into multiple functions
as a lot going on in here!
> + uint32_t *sections = &niagara_state->sections[0];
> + uint32_t allocated = 0;
> +
> + if (input->policy & MHD_SECTION_ALLOC_POLICY_MANUAL) {
> + uint32_t cur_extent = 0;
No need to init. Also blank line after declarations
> + for (cur_extent = 0; cur_extent < input->extent_count; cur_extent++) {
> + uint32_t start_section = input->extents[cur_extent].start_section_id;
> + uint32_t count = input->extents[cur_extent].section_count;
> + uint32_t cur_section;
blank line here etc.
> + for (cur_section = input->extents[cur_extent].start_section_id;
> + cur_section < (start_section + count);
Brackets don't add anything.
> + cur_section++) {
> + uint32_t *section = §ions[cur_section];
> + uint32_t old_value = __sync_fetch_and_or(section, (1 << ct3d->mhd_head));
> + /* if the old value wasn't 0, this section was already claimed
Wrong comment syntax.
> + * if it was owned by use already, just continue and don't count it
> + */
> + if (old_value & (1 << ct3d->mhd_head)) {
> + continue;
You continued, no need for an else after it.
> + } else if (old_value != 0) {
> + __sync_fetch_and_and(section, ~(1 << ct3d->mhd_head));
> + continue;
> + }
> + __sync_fetch_and_sub(&niagara_state->free_sections, 1);
> + section_ids[allocated++] = i;
> + }
> + }
> + } else {
> + /* Iterate the the section list and allocate free sections */
> + for (i = 0; (i < niagara_state->total_sections) && (allocated != count); i++) {
> + uint32_t old_value = __sync_fetch_and_or(§ions[i], (1 << ct3d->mhd_head));
> + /* if the old value wasn't 0, this section was already claimed
As above
> + * if it was owned by use already, just continue and don't count it
> + */
> + if (old_value & (1 << ct3d->mhd_head)) {
> + continue;
Continued so no point in else
> + } else if (old_value != 0) {
> + __sync_fetch_and_and(§ions[i], ~(1 << ct3d->mhd_head));
> + continue;
> + }
> + __sync_fetch_and_sub(&niagara_state->free_sections, 1);
> + section_ids[allocated++] = i;
Quite a bit common with previous leg. Perhaps a utility function with that
stuff factored out?
> + }
> + }
> +
> + if ((policy & MHD_SECTION_ALLOC_POLICY_ALL_OR_NOTHING) &&
> + (allocated != count)) {
> + goto all_or_nothing_fail;
> + }
> +
> + /* Build the output */
> + output->section_count = allocated;
> + uint32_t extents = 0;
> + uint32_t previous = 0;
> + for (i=0; i < allocated; i++) {
> + if (i == 0) {
> + /* start the first extent */
> + output->extents[extents].start_section_id = section_ids[i];
> + output->extents[extents].section_count = 1;
> + extents++;
> + }
> + else if (section_ids[i] == (previous+1)) {
> + /* increment the current extent */
> + output->extents[extents-1].section_count++;
> + }
> + else {
> + /* start a new extent */
> + output->extents[extents].start_section_id = section_ids[i];
> + output->extents[extents].section_count = 1;
> + extents++;
> + }
> + previous = section_ids[i];
> + }
> + output->extent_count = extents;
> +
> + *len_out = (8+(16*extents));
> +
> + free(section_ids);
Looks like a g_autofree pointer would help.
> + return CXL_MBOX_SUCCESS;
> +all_or_nothing_fail:
> + /* free any successfully allocated sections */
> + for (i = 0; i < allocated; i++) {
> + __sync_fetch_and_and(§ions[i], ~(1 << ct3d->mhd_head));
> + __sync_fetch_and_add(&niagara_state->free_sections, 1);
> + }
> + free(section_ids);
> + return CXL_MBOX_INTERNAL_ERROR;
> +}
> +
> +#define MHD_SECTION_RELEASE_POLICY_NONE 0
> +#define MHD_SECTION_RELEASE_POLICY_CLEARING 1
> +#define MHD_SECTION_RELEASE_POLICY_RANDOMIZING 2
As above, I'd like these in a niagra specific namespace.
> +static CXLRetCode cmd_niagara_set_section_release(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);
> + Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
> + struct {
> + uint32_t extent_count;
> + uint8_t policy;
> + uint8_t reserved[3];
> + struct {
> + uint32_t start_section_id;
> + uint32_t section_count;
> + uint8_t reserved[8];
> + } extents[];
> + } QEMU_PACKED *input = (void *)payload_in;
> + uint32_t i, j;
> +
> + uint32_t* sections = &niagara_state->sections[0];
> + for (i = 0; i < input->extent_count; i++) {
> + uint32_t start = input->extents[i].start_section_id;
> + for (j = 0; j < input->extents[i].section_count; j++) {
> + uint32_t old_val = __sync_fetch_and_and(§ions[start+j], ~(1 << ct3d->mhd_head));
Fix up spacing around operators.
> + if (old_val & (1 << ct3d->mhd_head))
> + __sync_fetch_and_add(&niagara_state->free_sections, 1);
{}
> +
> + // TODO: Policy
> + }
> + }
> +
> + return CXL_MBOX_SUCCESS;
> +}
> +
> +static CXLRetCode cmd_niagara_set_section_size(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);
> + Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
My gut feeling for now would be to pull the mhd shared state into this driver state
as that will reduce impact on the main type 3 driver whilst we work out what this
all looks like.
> + struct {
> + uint8_t section_unit;
> + uint8_t reserved[7];
> + } QEMU_PACKED *input = (void *)payload_in;
> + struct {
> + uint8_t section_unit;
> + uint8_t reserved[7];
> + } QEMU_PACKED *output = (void *)payload_out;
> +
> + if (niagara_state->section_size == (1 << (input->section_unit - 1)))
> + goto set_section_size_success;
I'd just have the handling here to simplify flow. Or flip condition and indent the
rest.
{}
> +
> + /* Check that there are no actively alloc'd sections */
> + if(niagara_state->free_sections != niagara_state->total_sections)
> + return CXL_MBOX_INTERNAL_ERROR;
{}
> +
> + uint32_t prev_section_size = niagara_state->section_size;
> + uint32_t prev_total_sections = niagara_state->total_sections;
> +
> + niagara_state->section_size = (1 << (input->section_unit - 1));
> + niagara_state->total_sections = (prev_section_size * prev_total_sections) / niagara_state->section_size;
> + niagara_state->free_sections = niagara_state->total_sections;
> +
> +set_section_size_success:
> + output->section_unit = input->section_unit;
> + return CXL_MBOX_SUCCESS;
> +}
> +
> +#define MHD_MOVE_DATA_POLICY_CLEARING 0
> +#define MHD_MOVE_DATA_POLICY_NONE 1
> +static CXLRetCode cmd_niagara_move_data(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + struct {
> + uint32_t extent_count;
> + uint8_t policy;
> + uint8_t reserved[3];
> + struct {
> + uint32_t source_section_id;
> + uint32_t source_data_offset;
> + uint32_t destination_section_id;
> + uint32_t destination_data_offset;
> + uint32_t data_length;
> + uint8_t reserved[4];
> + } extents;
> + } QEMU_PACKED *input = (void *)payload_in;
> +
> + struct {
> + uint64_t task_id;
> + uint32_t bitset[];
> + } QEMU_PACKED *output = (void *)payload_out;
> +
> + (void)input;
> + (void)output;
Don't supply this one either if not implemented yet! Be nice to us
reviewers :)
> +
> + return CXL_MBOX_UNSUPPORTED;
> +}
> +
> +static CXLRetCode cmd_niagara_clear_section(const struct cxl_cmd *cmd,
> + uint8_t *payload_in,
> + size_t len_in,
> + uint8_t *payload_out,
> + size_t *len_out,
> + CXLCCI *cci)
> +{
> + return CXL_MBOX_UNSUPPORTED;
Umm. Don't supply it then! :)
> +}
> +
> +#define MHD_GSM_QUERY_FREE 0
> +#define MHD_GSM_QUERY_ALLOCATED 1
> +static CXLRetCode cmd_niagara_get_section_map(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);
> + Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
> + struct {
> + uint8_t query_type;
> + uint8_t reserved[7];
> + } QEMU_PACKED *input = (void *)payload_in;
> + struct {
> + uint32_t ttl_section_count;
> + uint32_t qry_section_count;
> + uint8_t bitset[];
> + } QEMU_PACKED *output = (void *)payload_out;
> +
> + uint8_t query_type = input->query_type;
> + uint32_t i;
> +
> + if ((query_type != MHD_GSM_QUERY_FREE) && (query_type != MHD_GSM_QUERY_ALLOCATED))
> + return CXL_MBOX_INVALID_INPUT;
> +
> + output->ttl_section_count = niagara_state->total_sections;
> + output->qry_section_count = 0;
> + uint32_t bytes = (output->ttl_section_count/8);
> + if (output->ttl_section_count % 8)
> + bytes += 1;
> + for (i = 0; i < bytes; i++)
> + output->bitset[i] = 0x0;
> +
> + /* Iterate the the section list and check the bits */
> + uint32_t* sections = &niagara_state->sections[0];
run scripts/checkpatch.pl on HEAD~3..HEAD or similar and fix every error and warning.
> + for (i = 0; (i < niagara_state->total_sections); i++) {
> + uint32_t section = sections[i];
> + if (((query_type == MHD_GSM_QUERY_FREE) && (!section)) ||
> + ((query_type == MHD_GSM_QUERY_ALLOCATED) && (section & (1 << ct3d->mhd_head)))) {
> + uint32_t byte = i / 8;
> + uint8_t bit = (1 << (i % 8));
> + output->bitset[byte] |= bit;
> + output->qry_section_count++;
> + }
> + }
> +
> + *len_out = (8+bytes);
Spaces around +
> + return CXL_MBOX_SUCCESS;
> +}
> +
> +static bool mhdsld_access_valid(CXLType3Dev *ct3d, uint64_t dpa_offset, unsigned int size) {
> + Niagara_Shared_State *niagara_state = (Niagara_Shared_State*)ct3d->mhd_state;
> + if (ct3d->mhd_state) {
> + uint32_t section = (dpa_offset / MIN_MEMBLK_SIZE);
> + if (!(niagara_state->sections[section] & (1 << ct3d->mhd_head))) {
> + return false;
> + }
> + }
> + return true;
> +}
> +
> +static const struct cxl_cmd cxl_cmd_set_niagara[256][256] = {
> + [NIAGARA][GET_SECTION_STATUS] = { "GET_SECTION_STATUS",
> + cmd_niagara_get_section_status, 0, 0 },
> + [NIAGARA][SET_SECTION_ALLOC] = { "SET_SECTION_ALLOC",
> + cmd_niagara_set_section_alloc, ~0,
> + (IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE |
> + IMMEDIATE_POLICY_CHANGE | IMMEDIATE_LOG_CHANGE)
> + },
> + [NIAGARA][SET_SECTION_RELEASE] = { "SET_SECTION_RELEASE",
> + cmd_niagara_set_section_release, ~0,
> + (IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE |
> + IMMEDIATE_POLICY_CHANGE | IMMEDIATE_LOG_CHANGE)
> + },
> + [NIAGARA][SET_SECTION_SIZE] = { "SET_SECTION_SIZE",
> + cmd_niagara_set_section_size, 8,
> + (IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE |
> + IMMEDIATE_POLICY_CHANGE | IMMEDIATE_LOG_CHANGE)
> + },
> + [NIAGARA][MOVE_DATA] = { "MOVE_DATA",
> + cmd_niagara_move_data, ~0, IMMEDIATE_DATA_CHANGE },
> + [NIAGARA][GET_SECTION_MAP] = { "GET_SECTION_MAP",
> + cmd_niagara_get_section_map, 8 , IMMEDIATE_DATA_CHANGE },
> + [NIAGARA][CLEAR_SECTION] = { "CLEAR_SECTION",
> + cmd_niagara_clear_section, 0, IMMEDIATE_DATA_CHANGE },
> +};
> +
> +enum cxl_dev_type {
> + cxl_type3,
> +};
> +
> +struct CXL_Niagara_State {
> + PCIDevice parent_obj;
Why not inherit from CXLType3Dev? Should end up simpler and
more standard which will help upstreaming.
> + PCIDevice *target;
> + enum cxl_dev_type type;
> + CXLCCI *cci;
> +};
> +
> +struct CXL_NiagaraClass {
> + PCIDeviceClass parent_class;
> +};
> +
One blank line only.
> +
> +#define TYPE_CXL_Niagara "cxl-skh-niagara"
> +OBJECT_DECLARE_TYPE(CXL_Niagara_State, CXL_NiagaraClass, CXL_Niagara)
> +
> +static Property cxl_niagara_props[] = {
> + DEFINE_PROP_LINK("target", CXL_Niagara_State,
> + target, TYPE_PCI_DEVICE, PCIDevice *),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void cxl_niagara_realize(DeviceState *d, Error **errp)
> +{
> + CXL_Niagara_State *s = CXL_Niagara(d);
> +
> + if (object_dynamic_cast(OBJECT(s->target), TYPE_CXL_TYPE3)) {
> + CXLType3Dev *ct3d = CXL_TYPE3(s->target);
> +
> + if (!ct3d->is_mhd) {
> + error_setg(errp, "Niagara target must be a cxl-type3 mhd");
> + return;
> + }
> +
> + s->type = cxl_type3;
> + s->cci = &ct3d->cci;
> +
> + ct3d->mhd_access_valid = mhdsld_access_valid;
> + return;
> + }
> +
> + error_setg(errp, "Unhandled target type for CXL Niagara MHSLD");
> +}
> +
> +static void cxl_niagara_reset(DeviceState *d)
> +{
> + CXL_Niagara_State *s = CXL_Niagara(d);
> +
> + if (object_dynamic_cast(OBJECT(s->target), TYPE_CXL_TYPE3)) {
> + CXLType3Dev *ct3d = CXL_TYPE3(s->target);
> + cxl_add_cci_commands(&ct3d->cci, cxl_cmd_set_niagara, 512);
> + return;
> + }
> +}
> +
> +static void cxl_niagara_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + dc->realize = cxl_niagara_realize;
> + dc->reset = cxl_niagara_reset;
> + device_class_set_props(dc, cxl_niagara_props);
> +}
> +
> +static const TypeInfo cxl_niagara_info = {
> + .name = TYPE_CXL_Niagara,
> + .parent = TYPE_CXL_TYPE3,
> + .class_size = sizeof(struct CXL_NiagaraClass),
> + .class_init = cxl_niagara_class_init,
> + .instance_size = sizeof(CXL_Niagara_State),
I'd embed the type3 in the state.
> + .interfaces = (InterfaceInfo[]) {
> + { INTERFACE_CXL_DEVICE },
> + { INTERFACE_PCIE_DEVICE },
> + {}
> + },
> +};
> +
> +static void cxl_niagara_register_types(void)
> +{
> + type_register_static(&cxl_niagara_info);
> +}
> +
> +type_init(cxl_niagara_register_types)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] cxl/type3: minimum MHD cci support
2023-08-07 14:56 ` Jonathan Cameron via
@ 2023-08-31 16:59 ` Gregory Price
2023-09-01 9:05 ` Jonathan Cameron via
0 siblings, 1 reply; 13+ messages in thread
From: Gregory Price @ 2023-08-31 16:59 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Gregory Price, qemu-devel, linux-cxl, junhee.ryu, kwangjin.ko
On Mon, Aug 07, 2023 at 03:56:09PM +0100, Jonathan Cameron wrote:
> On Fri, 21 Jul 2023 12:35:08 -0400
> Gregory Price <gourry.memverge@gmail.com> wrote:
>
> > Implement the MHD GET_INFO cci command and add a shared memory
> > region to the type3 device to host the information.
> >
> > Add a helper program to initialize this shared memory region.
> >
> > Add a function pointer to type3 devices for future work that
> > will allow an mhd device to provide a hook to validate whether
> > a memory access is valid or not.
> >
> > For now, limit the number of LD's to the number of heads. Later,
> > this limitation will need to be lifted for MH-MLDs.
> >
> > Intended use case:
> >
> > 1. Create the shared memory region
> > 2. Format the shared memory region
> > 3. Launch QEMU with `is_mhd=true,mhd_head=N,mhd_shmid=$shmid`
>
> I'd definitely like some feedback from experienced QEMU folk on
> how to model this sort of cross QEMU instance sharing.
>
> My instinct is a socket would be more flexible as it lets us
> potentially emulate the machines on multiple hosts (assuming they
> can see some shared backend storage).
>
I'd considered a socket, but after mulling it over, I realized the
operations I was trying to implement amounted to an atomic compare
and swap. I wanted to avoid locks and serialization if at all
possible to avoid introducing that into the hot-path of memory
accesses. Maybe there is a known pattern for this kind of
multi-instance QEMU coherency stuff, but that seemed the simplest path.
This obviously has the downside of limiting emulation of MHD system to
a local machine, but the use of common files/memory to back CXL memory
already does that (i think).
¯\_(ツ)_/¯ feedback welcome. I'll leave it for now unless there are
strong opinions.
> else not needed if we returned in previous leg.
>
> if (ct3d->is_mhd && ...
>
> > + (!ct3d->mhd_shmid || (ct3d->mhd_head == ~(0)))) {
>
> How does mhd_head equal that? Default is 0. I'm not sure there is a reason
> to require it.
>
Good catch, the default should be ~(0). Updated in the field
initialization section.
> > + /* For now, limit the number of heads to the number of LD's (SLD) */
>
> Feels backwards. Number of heads never going to be bigger than numbre of
> LDs. Other way around is possible of course.
>
> > + if (ct3d->mhd_state->nr_heads <= ct3d->mhd_head) {
>
> mhd head needs to be out of range? Confused.
>
Woops, yes this should be mhd_head >= nr_heads. Wording is backwards and
so is the code. fixed.
> > + if (ct3d->mhd_state) {
> > + shmdt(ct3d->mhd_state);
> > + }
>
> Reverse order of realize - so I think this wants to be earlier.
>
> > }
Just to be clear you *don't* want the reverse order, correct? If so
i'll swap it.
> >
> > + if (ct3d->is_mhd && ct3d->mhd_access_valid) {
> > + if (!ct3d->mhd_access_valid(ct3d, dpa_offset, size))
> > + return MEMTX_ERROR;
>
> Brackets for inner block.
> Arguably could just add the ct3d->is_mhd check in the place where
> mhd_access_valid is set and hence only need to check that here.
> Maybe that makes it slightly harder to follow though.
>
> Also could just unset is_mhd if mhd_access_valid not present..
>
I'm going to change the next patch to fully inherit CXLType3Dev into
Niagara as you suggested. I will have to see what falls out when i do
that, but my feeling was being more explicit when checking pointers is
better. If you prefer to to drop the is_mhd check i'm ok with that
simply because mhd_access_valid should always be NULL when is_mhd is
false. Either way the result is the same.
~Gregory
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] cxl/type3: minimum MHD cci support
2023-08-31 16:59 ` Gregory Price
@ 2023-09-01 9:05 ` Jonathan Cameron via
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron via @ 2023-09-01 9:05 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, qemu-devel, linux-cxl, junhee.ryu, kwangjin.ko
On Thu, 31 Aug 2023 12:59:24 -0400
Gregory Price <gregory.price@memverge.com> wrote:
> On Mon, Aug 07, 2023 at 03:56:09PM +0100, Jonathan Cameron wrote:
> > On Fri, 21 Jul 2023 12:35:08 -0400
> > Gregory Price <gourry.memverge@gmail.com> wrote:
> >
> > > Implement the MHD GET_INFO cci command and add a shared memory
> > > region to the type3 device to host the information.
> > >
> > > Add a helper program to initialize this shared memory region.
> > >
> > > Add a function pointer to type3 devices for future work that
> > > will allow an mhd device to provide a hook to validate whether
> > > a memory access is valid or not.
> > >
> > > For now, limit the number of LD's to the number of heads. Later,
> > > this limitation will need to be lifted for MH-MLDs.
> > >
> > > Intended use case:
> > >
> > > 1. Create the shared memory region
> > > 2. Format the shared memory region
> > > 3. Launch QEMU with `is_mhd=true,mhd_head=N,mhd_shmid=$shmid`
> >
> > I'd definitely like some feedback from experienced QEMU folk on
> > how to model this sort of cross QEMU instance sharing.
> >
> > My instinct is a socket would be more flexible as it lets us
> > potentially emulate the machines on multiple hosts (assuming they
> > can see some shared backend storage).
> >
>
> I'd considered a socket, but after mulling it over, I realized the
> operations I was trying to implement amounted to an atomic compare
> and swap. I wanted to avoid locks and serialization if at all
> possible to avoid introducing that into the hot-path of memory
> accesses. Maybe there is a known pattern for this kind of
> multi-instance QEMU coherency stuff, but that seemed the simplest path.
I'd definitely like some input from others on this if possible as I've
no idea if this problem has really been tackled in the past.
>
> This obviously has the downside of limiting emulation of MHD system to
> a local machine, but the use of common files/memory to back CXL memory
> already does that (i think).
Files should be fine I think on a suitable sharing aware file system.
>
> ¯\_(ツ)_/¯ feedback welcome. I'll leave it for now unless there are
> strong opinions.
>
> > else not needed if we returned in previous leg.
> >
> > if (ct3d->is_mhd && ...
> >
> > > + (!ct3d->mhd_shmid || (ct3d->mhd_head == ~(0)))) {
> >
> > How does mhd_head equal that? Default is 0. I'm not sure there is a reason
> > to require it.
> >
>
> Good catch, the default should be ~(0). Updated in the field
> initialization section.
>
> > > + /* For now, limit the number of heads to the number of LD's (SLD) */
> >
> > Feels backwards. Number of heads never going to be bigger than numbre of
> > LDs. Other way around is possible of course.
> >
> > > + if (ct3d->mhd_state->nr_heads <= ct3d->mhd_head) {
> >
> > mhd head needs to be out of range? Confused.
> >
>
> Woops, yes this should be mhd_head >= nr_heads. Wording is backwards and
> so is the code. fixed.
>
> > > + if (ct3d->mhd_state) {
> > > + shmdt(ct3d->mhd_state);
> > > + }
> >
> > Reverse order of realize - so I think this wants to be earlier.
> >
> > > }
>
> Just to be clear you *don't* want the reverse order, correct? If so
> i'll swap it.
I do want reverse order. Generally speaking it's much easier to
review precisely matching reverse order as then mostly the ordering is
fine in a way that it isn't in any other ordering.
>
> > >
> > > + if (ct3d->is_mhd && ct3d->mhd_access_valid) {
> > > + if (!ct3d->mhd_access_valid(ct3d, dpa_offset, size))
> > > + return MEMTX_ERROR;
> >
> > Brackets for inner block.
> > Arguably could just add the ct3d->is_mhd check in the place where
> > mhd_access_valid is set and hence only need to check that here.
> > Maybe that makes it slightly harder to follow though.
> >
> > Also could just unset is_mhd if mhd_access_valid not present..
> >
>
> I'm going to change the next patch to fully inherit CXLType3Dev into
> Niagara as you suggested. I will have to see what falls out when i do
> that, but my feeling was being more explicit when checking pointers is
> better. If you prefer to to drop the is_mhd check i'm ok with that
> simply because mhd_access_valid should always be NULL when is_mhd is
> false. Either way the result is the same.
Minimizing redundant checks is always good as long as a reviewer can
easily see they are redundant. I think that's true here.
Jonathan
>
> ~Gregory
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-09-01 9:11 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-21 16:35 [PATCH 0/4] CXL: SK hynix Niagara MHSLD Device Gregory Price
2023-07-21 16:35 ` [PATCH 1/4] cxl/mailbox: change CCI cmd set structure to be a member, not a refernce Gregory Price
2023-08-04 14:56 ` Jonathan Cameron via
2023-07-21 16:35 ` [PATCH 2/4] cxl/mailbox: interface to add CCI commands to an existing CCI Gregory Price
2023-08-04 15:14 ` Jonathan Cameron via
2023-08-04 16:41 ` Gregory Price
2023-08-07 14:30 ` Jonathan Cameron via
2023-07-21 16:35 ` [PATCH 3/4] cxl/type3: minimum MHD cci support Gregory Price
2023-08-07 14:56 ` Jonathan Cameron via
2023-08-31 16:59 ` Gregory Price
2023-09-01 9:05 ` Jonathan Cameron via
2023-07-21 16:35 ` [PATCH 4/4] cxl/vendor: SK hynix Niagara Multi-Headed SLD Device Gregory Price
2023-08-07 15:36 ` Jonathan Cameron via
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).