* [PATCH 1/5] cxl/mbox: Add support for background operations
2023-04-18 17:23 [PATCH v2 -qemu 0/5] cxl: Background commands and device Sanitation Davidlohr Bueso
@ 2023-04-18 17:23 ` Davidlohr Bueso
2023-05-15 12:32 ` Jonathan Cameron
2023-04-18 17:23 ` [PATCH 2/5] cxl/mbox: Wire up interrupts for background completion Davidlohr Bueso
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Davidlohr Bueso @ 2023-04-18 17:23 UTC (permalink / raw)
To: Jonathan.Cameron
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, dave, linux-cxl
Support background commands in the mailbox, and while at it update
cmd_infostat_bg_op_sts() accordingly. This patch does not implement
mbox interrupts upon completion, so the kernel driver must rely on
polling to know when the operation is done.
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
hw/cxl/cxl-device-utils.c | 5 +-
hw/cxl/cxl-mailbox-utils.c | 95 ++++++++++++++++++++++++++++++++++++-
hw/mem/cxl_type3.c | 2 +
include/hw/cxl/cxl_device.h | 10 ++++
4 files changed, 107 insertions(+), 5 deletions(-)
diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index 50c76c65e755..4bb4e85dae19 100644
--- a/hw/cxl/cxl-device-utils.c
+++ b/hw/cxl/cxl-device-utils.c
@@ -101,8 +101,7 @@ static void mailbox_mem_writeq(uint64_t *reg_state, hwaddr offset,
case A_CXL_DEV_MAILBOX_CMD:
break;
case A_CXL_DEV_BG_CMD_STS:
- /* BG not supported */
- /* fallthrough */
+ break;
case A_CXL_DEV_MAILBOX_STS:
/* Read only register, will get updated by the state machine */
return;
@@ -273,7 +272,7 @@ static void device_reg_init_common(CXLDeviceState *cxl_dstate)
static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
{
- /* 2048 payload size, with no interrupt or background support */
+ /* 2048 payload size, with no interrupt */
ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
PAYLOAD_SIZE, CXL_MAILBOX_PAYLOAD_SHIFT);
cxl_dstate->payload_size = CXL_MAILBOX_MAX_PAYLOAD_SIZE;
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index b02908c4a4ba..1a4480d42908 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -350,7 +350,14 @@ static CXLRetCode cmd_infostat_bg_op_sts(struct cxl_cmd *cmd,
bg_op_status = (void *)cmd->payload;
memset(bg_op_status, 0, sizeof(*bg_op_status));
- /* No support yet for background operations so status all 0 */
+ bg_op_status->status = ARRAY_FIELD_EX64(cxl_dstate->mbox_reg_state64,
+ CXL_DEV_BG_CMD_STS, PERCENTAGE_COMP) << 1;
+ if (cxl_dstate->bg.runtime > 0) {
+ bg_op_status->status |= 1U << 0;
+ }
+ bg_op_status->opcode = cxl_dstate->bg.opcode;
+ bg_op_status->returncode = ARRAY_FIELD_EX64(cxl_dstate->mbox_reg_state64,
+ CXL_DEV_BG_CMD_STS, RET_CODE);
*len = sizeof(*bg_op_status);
return CXL_MBOX_SUCCESS;
}
@@ -808,6 +815,7 @@ static CXLRetCode cmd_media_clear_poison(struct cxl_cmd *cmd,
#define IMMEDIATE_DATA_CHANGE (1 << 2)
#define IMMEDIATE_POLICY_CHANGE (1 << 3)
#define IMMEDIATE_LOG_CHANGE (1 << 4)
+#define BACKGROUND_OPERATION (1 << 5)
static struct cxl_cmd cxl_cmd_set[256][256] = {
[EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
@@ -856,12 +864,20 @@ static struct cxl_cmd cxl_cmd_set_sw[256][256] = {
cmd_identify_switch_device, 0, 0x49 },
};
+/*
+ * While the command is executing in the background, the device should
+ * update the percentage complete in the Background Command Status Register
+ * at least once per second.
+ */
+#define CXL_MBOX_BG_UPDATE_FREQ 1000ULL
+
void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
{
uint16_t ret = CXL_MBOX_SUCCESS;
struct cxl_cmd *cxl_cmd;
uint64_t status_reg;
opcode_handler h;
+ uint8_t bg_started = 0;
uint64_t command_reg = cxl_dstate->mbox_reg_state64[R_CXL_DEV_MAILBOX_CMD];
uint8_t set = FIELD_EX64(command_reg, CXL_DEV_MAILBOX_CMD, COMMAND_SET);
@@ -873,7 +889,17 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
if (len == cxl_cmd->in || cxl_cmd->in == ~0) {
cxl_cmd->payload = cxl_dstate->mbox_reg_state +
A_CXL_DEV_CMD_PAYLOAD;
+ /* Only one bg command at a time */
+ if ((cxl_cmd->effect & BACKGROUND_OPERATION) &&
+ cxl_dstate->bg.runtime > 0) {
+ ret = CXL_MBOX_BUSY;
+ goto done;
+ }
ret = (*h)(cxl_cmd, cxl_dstate, &len);
+ if ((cxl_cmd->effect & BACKGROUND_OPERATION) &&
+ ret == CXL_MBOX_BG_STARTED) {
+ bg_started = 1;
+ }
assert(len <= cxl_dstate->payload_size);
} else {
ret = CXL_MBOX_INVALID_PAYLOAD_LENGTH;
@@ -884,8 +910,13 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
ret = CXL_MBOX_UNSUPPORTED;
}
- /* Set the return code */
+done:
+ /* Set the return code and background status */
status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, ERRNO, ret);
+ if (bg_started) {
+ status_reg = FIELD_DP64(status_reg, CXL_DEV_MAILBOX_STS,
+ BG_OP, bg_started);
+ }
/* Set the return length */
command_reg = FIELD_DP64(command_reg, CXL_DEV_MAILBOX_CMD, COMMAND_SET, 0);
@@ -895,11 +926,66 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
cxl_dstate->mbox_reg_state64[R_CXL_DEV_MAILBOX_CMD] = command_reg;
cxl_dstate->mbox_reg_state64[R_CXL_DEV_MAILBOX_STS] = status_reg;
+ if (bg_started) {
+ uint64_t bg_status_reg, now;
+
+ cxl_dstate->bg.opcode = (set << 8) | cmd;
+
+ bg_status_reg = FIELD_DP64(0, CXL_DEV_BG_CMD_STS, OP, cxl_dstate->bg.opcode);
+ bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS,
+ PERCENTAGE_COMP, 0);
+ cxl_dstate->mbox_reg_state64[R_CXL_DEV_BG_CMD_STS] = bg_status_reg;
+
+ now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
+ cxl_dstate->bg.starttime = now;
+ timer_mod(cxl_dstate->bg.timer, now + CXL_MBOX_BG_UPDATE_FREQ);
+ }
+
/* Tell the host we're done */
ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CTRL,
DOORBELL, 0);
}
+static void bg_timercb(void *opaque)
+{
+ CXLDeviceState *cxl_dstate = opaque;
+ uint64_t bg_status_reg = 0;
+ uint64_t now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
+ uint64_t total_time = cxl_dstate->bg.starttime + cxl_dstate->bg.runtime;
+
+ assert(cxl_dstate->bg.runtime > 0);
+ bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS,
+ OP, cxl_dstate->bg.opcode);
+
+ if (now >= total_time) { /* we are done */
+ uint64_t status_reg;
+ uint16_t ret = CXL_MBOX_SUCCESS;
+
+ cxl_dstate->bg.complete_pct = 100;
+ /* clear bg */
+ status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, BG_OP, 0);
+ cxl_dstate->mbox_reg_state64[R_CXL_DEV_MAILBOX_STS] = status_reg;
+
+ bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS, RET_CODE, ret);
+
+ /* TODO add ad-hoc cmd succesful completion handling */
+ } else {
+ /* estimate only */
+ cxl_dstate->bg.complete_pct = 100 * now / total_time;
+ timer_mod(cxl_dstate->bg.timer, now + CXL_MBOX_BG_UPDATE_FREQ);
+ }
+
+ bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS, PERCENTAGE_COMP,
+ cxl_dstate->bg.complete_pct);
+ cxl_dstate->mbox_reg_state64[R_CXL_DEV_BG_CMD_STS] = bg_status_reg;
+
+ if (cxl_dstate->bg.complete_pct == 100) {
+ cxl_dstate->bg.starttime = 0;
+ /* registers are updated, allow new bg-capable cmds */
+ cxl_dstate->bg.runtime = 0;
+ }
+}
+
void cxl_initialize_mailbox(CXLDeviceState *cxl_dstate, bool switch_cci)
{
if (!switch_cci) {
@@ -920,4 +1006,9 @@ void cxl_initialize_mailbox(CXLDeviceState *cxl_dstate, bool switch_cci)
}
}
}
+ cxl_dstate->bg.complete_pct = 0;
+ cxl_dstate->bg.starttime = 0;
+ cxl_dstate->bg.runtime = 0;
+ cxl_dstate->bg.timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
+ bg_timercb, cxl_dstate);
}
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 334ce92f5e0f..63c28a1ed5d2 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -870,12 +870,14 @@ static void ct3_exit(PCIDevice *pci_dev)
{
CXLType3Dev *ct3d = CXL_TYPE3(pci_dev);
CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
+ CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
ComponentRegisters *regs = &cxl_cstate->crb;
pcie_aer_exit(pci_dev);
cxl_doe_cdat_release(cxl_cstate);
spdm_sock_fini(ct3d->doe_spdm.socket);
g_free(regs->special_ops);
+ timer_free(cxl_dstate->bg.timer);
if (ct3d->hostpmem) {
address_space_destroy(&ct3d->hostpmem_as);
}
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 1fa8522d33fa..dbb8a955723b 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -216,6 +216,16 @@ typedef struct cxl_device_state {
struct cxl_cmd (*cxl_cmd_set)[256];
CPMUState cpmu[CXL_NUM_CPMU_INSTANCES];
CXLEventLog event_logs[CXL_EVENT_TYPE_MAX];
+
+ /* background command handling (times in ms) */
+ struct {
+ uint16_t opcode;
+ uint16_t complete_pct;
+ uint64_t starttime;
+ /* set by each bg cmd, cleared by the bg_timer when complete */
+ uint64_t runtime;
+ QEMUTimer *timer;
+ } bg;
} CXLDeviceState;
/* Initialize the register block for a device */
--
2.40.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] cxl/mbox: Add support for background operations
2023-04-18 17:23 ` [PATCH 1/5] cxl/mbox: Add support for background operations Davidlohr Bueso
@ 2023-05-15 12:32 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2023-05-15 12:32 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, linux-cxl
On Tue, 18 Apr 2023 10:23:33 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:
> Support background commands in the mailbox, and while at it update
> cmd_infostat_bg_op_sts() accordingly. This patch does not implement
> mbox interrupts upon completion, so the kernel driver must rely on
> polling to know when the operation is done.
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
I've (mostly) ignored the endian fun in reviewing this as it's not making it any
worse and I should really just do a blanket fix of remaining code.
> ---
> hw/cxl/cxl-device-utils.c | 5 +-
> hw/cxl/cxl-mailbox-utils.c | 95 ++++++++++++++++++++++++++++++++++++-
> hw/mem/cxl_type3.c | 2 +
> include/hw/cxl/cxl_device.h | 10 ++++
> 4 files changed, 107 insertions(+), 5 deletions(-)
>
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index 50c76c65e755..4bb4e85dae19 100644
> --- a/hw/cxl/cxl-device-utils.c
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -101,8 +101,7 @@ static void mailbox_mem_writeq(uint64_t *reg_state, hwaddr offset,
> case A_CXL_DEV_MAILBOX_CMD:
> break;
> case A_CXL_DEV_BG_CMD_STS:
> - /* BG not supported */
> - /* fallthrough */
> + break;
It's a read only register. So why allow it to be written? More than likely
I'm forgetting how this works.
> case A_CXL_DEV_MAILBOX_STS:
> /* Read only register, will get updated by the state machine */
> return;
> @@ -273,7 +272,7 @@ static void device_reg_init_common(CXLDeviceState *cxl_dstate)
>
> static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
> {
> - /* 2048 payload size, with no interrupt or background support */
> + /* 2048 payload size, with no interrupt */
> ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> PAYLOAD_SIZE, CXL_MAILBOX_PAYLOAD_SHIFT);
> cxl_dstate->payload_size = CXL_MAILBOX_MAX_PAYLOAD_SIZE;
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index b02908c4a4ba..1a4480d42908 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
...
> +/*
> + * While the command is executing in the background, the device should
> + * update the percentage complete in the Background Command Status Register
> + * at least once per second.
> + */
> +#define CXL_MBOX_BG_UPDATE_FREQ 1000ULL
> +
> void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> {
> uint16_t ret = CXL_MBOX_SUCCESS;
> struct cxl_cmd *cxl_cmd;
> uint64_t status_reg;
> opcode_handler h;
> + uint8_t bg_started = 0;
> uint64_t command_reg = cxl_dstate->mbox_reg_state64[R_CXL_DEV_MAILBOX_CMD];
>
> uint8_t set = FIELD_EX64(command_reg, CXL_DEV_MAILBOX_CMD, COMMAND_SET);
> @@ -873,7 +889,17 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> if (len == cxl_cmd->in || cxl_cmd->in == ~0) {
> cxl_cmd->payload = cxl_dstate->mbox_reg_state +
> A_CXL_DEV_CMD_PAYLOAD;
> + /* Only one bg command at a time */
> + if ((cxl_cmd->effect & BACKGROUND_OPERATION) &&
> + cxl_dstate->bg.runtime > 0) {
> + ret = CXL_MBOX_BUSY;
> + goto done;
> + }
> ret = (*h)(cxl_cmd, cxl_dstate, &len);
> + if ((cxl_cmd->effect & BACKGROUND_OPERATION) &&
> + ret == CXL_MBOX_BG_STARTED) {
> + bg_started = 1;
> + }
> assert(len <= cxl_dstate->payload_size);
> } else {
> ret = CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> @@ -884,8 +910,13 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> ret = CXL_MBOX_UNSUPPORTED;
> }
>
> - /* Set the return code */
> +done:
> + /* Set the return code and background status */
> status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, ERRNO, ret);
> + if (bg_started) {
> + status_reg = FIELD_DP64(status_reg, CXL_DEV_MAILBOX_STS,
> + BG_OP, bg_started);
Isn't this a noop if bg_started == 0 in which case, no real point in
making it conditional on if (bg_started)?
> + }
>
> /* Set the return length */
> command_reg = FIELD_DP64(command_reg, CXL_DEV_MAILBOX_CMD, COMMAND_SET, 0);
> @@ -895,11 +926,66 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> cxl_dstate->mbox_reg_state64[R_CXL_DEV_MAILBOX_CMD] = command_reg;
> cxl_dstate->mbox_reg_state64[R_CXL_DEV_MAILBOX_STS] = status_reg;
>
> + if (bg_started) {
> + uint64_t bg_status_reg, now;
> +
> + cxl_dstate->bg.opcode = (set << 8) | cmd;
> +
> + bg_status_reg = FIELD_DP64(0, CXL_DEV_BG_CMD_STS, OP, cxl_dstate->bg.opcode);
Line wraps a bit to variable. I'd split the one above.
> + bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS,
> + PERCENTAGE_COMP, 0);
> + cxl_dstate->mbox_reg_state64[R_CXL_DEV_BG_CMD_STS] = bg_status_reg;
> +
> + now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> + cxl_dstate->bg.starttime = now;
> + timer_mod(cxl_dstate->bg.timer, now + CXL_MBOX_BG_UPDATE_FREQ);
> + }
> +
> /* Tell the host we're done */
> ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CTRL,
> DOORBELL, 0);
> }
>
> +static void bg_timercb(void *opaque)
> +{
> + CXLDeviceState *cxl_dstate = opaque;
> + uint64_t bg_status_reg = 0;
> + uint64_t now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> + uint64_t total_time = cxl_dstate->bg.starttime + cxl_dstate->bg.runtime;
> +
> + assert(cxl_dstate->bg.runtime > 0);
> + bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS,
> + OP, cxl_dstate->bg.opcode);
> +
> + if (now >= total_time) { /* we are done */
> + uint64_t status_reg;
> + uint16_t ret = CXL_MBOX_SUCCESS;
> +
> + cxl_dstate->bg.complete_pct = 100;
So far at least, this is only used in this function. So local variable?
> + /* clear bg */
> + status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, BG_OP, 0);
> + cxl_dstate->mbox_reg_state64[R_CXL_DEV_MAILBOX_STS] = status_reg;
> +
> + bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS, RET_CODE, ret);
> +
> + /* TODO add ad-hoc cmd succesful completion handling */
What does that mean? Short cutting the timing for some commands?
> + } else {
> + /* estimate only */
> + cxl_dstate->bg.complete_pct = 100 * now / total_time;
> + timer_mod(cxl_dstate->bg.timer, now + CXL_MBOX_BG_UPDATE_FREQ);
> + }
> +
> + bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS, PERCENTAGE_COMP,
> + cxl_dstate->bg.complete_pct);
> + cxl_dstate->mbox_reg_state64[R_CXL_DEV_BG_CMD_STS] = bg_status_reg;
Shouldn't be doing this directly as not handling big endian hosts etc.
> +
> + if (cxl_dstate->bg.complete_pct == 100) {
> + cxl_dstate->bg.starttime = 0;
> + /* registers are updated, allow new bg-capable cmds */
> + cxl_dstate->bg.runtime = 0;
> + }
> +}
> +
> void cxl_initialize_mailbox(CXLDeviceState *cxl_dstate, bool switch_cci)
> {
> if (!switch_cci) {
> @@ -920,4 +1006,9 @@ void cxl_initialize_mailbox(CXLDeviceState *cxl_dstate, bool switch_cci)
> }
> }
> }
> + cxl_dstate->bg.complete_pct = 0;
> + cxl_dstate->bg.starttime = 0;
> + cxl_dstate->bg.runtime = 0;
> + cxl_dstate->bg.timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> + bg_timercb, cxl_dstate);
> }
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 334ce92f5e0f..63c28a1ed5d2 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -870,12 +870,14 @@ static void ct3_exit(PCIDevice *pci_dev)
> {
> CXLType3Dev *ct3d = CXL_TYPE3(pci_dev);
> CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
> + CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
> ComponentRegisters *regs = &cxl_cstate->crb;
>
> pcie_aer_exit(pci_dev);
> cxl_doe_cdat_release(cxl_cstate);
> spdm_sock_fini(ct3d->doe_spdm.socket);
> g_free(regs->special_ops);
> + timer_free(cxl_dstate->bg.timer);
> if (ct3d->hostpmem) {
> address_space_destroy(&ct3d->hostpmem_as);
> }
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 1fa8522d33fa..dbb8a955723b 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -216,6 +216,16 @@ typedef struct cxl_device_state {
> struct cxl_cmd (*cxl_cmd_set)[256];
> CPMUState cpmu[CXL_NUM_CPMU_INSTANCES];
> CXLEventLog event_logs[CXL_EVENT_TYPE_MAX];
> +
> + /* background command handling (times in ms) */
Trivial but I'm a fan of naming variables so you can tell units without
relying on comments.
> + struct {
> + uint16_t opcode;
> + uint16_t complete_pct;
> + uint64_t starttime;
starttime_ms
> + /* set by each bg cmd, cleared by the bg_timer when complete */
> + uint64_t runtime;
runtime_ms
> + QEMUTimer *timer;
> + } bg;
> } CXLDeviceState;
>
> /* Initialize the register block for a device */
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/5] cxl/mbox: Wire up interrupts for background completion
2023-04-18 17:23 [PATCH v2 -qemu 0/5] cxl: Background commands and device Sanitation Davidlohr Bueso
2023-04-18 17:23 ` [PATCH 1/5] cxl/mbox: Add support for background operations Davidlohr Bueso
@ 2023-04-18 17:23 ` Davidlohr Bueso
2023-05-15 12:35 ` Jonathan Cameron
2023-04-18 17:23 ` [PATCH 3/5] cxl/device: Handle Media Disabled state Davidlohr Bueso
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Davidlohr Bueso @ 2023-04-18 17:23 UTC (permalink / raw)
To: Jonathan.Cameron
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, dave, linux-cxl
Notify when the background operation is done.
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
hw/cxl/cxl-device-utils.c | 10 +++++++++-
hw/cxl/cxl-mailbox-utils.c | 11 +++++++++++
hw/mem/cxl_type3.c | 2 +-
include/hw/cxl/cxl_device.h | 1 +
4 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index 4bb4e85dae19..b10ab390234c 100644
--- a/hw/cxl/cxl-device-utils.c
+++ b/hw/cxl/cxl-device-utils.c
@@ -272,10 +272,18 @@ static void device_reg_init_common(CXLDeviceState *cxl_dstate)
static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
{
- /* 2048 payload size, with no interrupt */
+ const uint8_t msi_n = 11;
+
+ /* 2048 payload size */
ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
PAYLOAD_SIZE, CXL_MAILBOX_PAYLOAD_SHIFT);
cxl_dstate->payload_size = CXL_MAILBOX_MAX_PAYLOAD_SIZE;
+ /* irq support */
+ ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
+ BG_INT_CAP, 1);
+ ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
+ MSI_N, msi_n);
+ cxl_dstate->mbox_msi_n = msi_n;
}
static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) { }
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 1a4480d42908..224bfdb4bfca 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -8,6 +8,8 @@
*/
#include "qemu/osdep.h"
+#include "hw/pci/msi.h"
+#include "hw/pci/msix.h"
#include "hw/cxl/cxl.h"
#include "hw/cxl/cxl_events.h"
#include "hw/pci/pci.h"
@@ -980,9 +982,18 @@ static void bg_timercb(void *opaque)
cxl_dstate->mbox_reg_state64[R_CXL_DEV_BG_CMD_STS] = bg_status_reg;
if (cxl_dstate->bg.complete_pct == 100) {
+ CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
+ PCIDevice *pdev = PCI_DEVICE(ct3d);
+
cxl_dstate->bg.starttime = 0;
/* registers are updated, allow new bg-capable cmds */
cxl_dstate->bg.runtime = 0;
+
+ if (msix_enabled(pdev)) {
+ msix_notify(pdev, cxl_dstate->mbox_msi_n);
+ } else if (msi_enabled(pdev)) {
+ msi_notify(pdev, cxl_dstate->mbox_msi_n);
+ }
}
}
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 63c28a1ed5d2..801f72c5fcae 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -777,7 +777,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
ComponentRegisters *regs = &cxl_cstate->crb;
MemoryRegion *mr = ®s->component_registers;
uint8_t *pci_conf = pci_dev->config;
- unsigned short msix_num = 10;
+ unsigned short msix_num = 12;
int i, rc;
QTAILQ_INIT(&ct3d->error_list);
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index dbb8a955723b..f986651b6ead 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -189,6 +189,7 @@ typedef struct cxl_device_state {
struct {
MemoryRegion mailbox;
uint16_t payload_size;
+ uint8_t mbox_msi_n;
union {
uint8_t mbox_reg_state[CXL_MAILBOX_REGISTERS_LENGTH];
uint16_t mbox_reg_state16[CXL_MAILBOX_REGISTERS_LENGTH / 2];
--
2.40.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/5] cxl/mbox: Wire up interrupts for background completion
2023-04-18 17:23 ` [PATCH 2/5] cxl/mbox: Wire up interrupts for background completion Davidlohr Bueso
@ 2023-05-15 12:35 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2023-05-15 12:35 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, linux-cxl
On Tue, 18 Apr 2023 10:23:34 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:
> Notify when the background operation is done.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
A comment inline on an overall limitation of the qemu CXL code so far.
Otherwise LGTM.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> hw/cxl/cxl-device-utils.c | 10 +++++++++-
> hw/cxl/cxl-mailbox-utils.c | 11 +++++++++++
> hw/mem/cxl_type3.c | 2 +-
> include/hw/cxl/cxl_device.h | 1 +
> 4 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index 4bb4e85dae19..b10ab390234c 100644
> --- a/hw/cxl/cxl-device-utils.c
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -272,10 +272,18 @@ static void device_reg_init_common(CXLDeviceState *cxl_dstate)
>
> static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
> {
> - /* 2048 payload size, with no interrupt */
> + const uint8_t msi_n = 11;
Not expecting you to do it in this series, but note that the interrupt handling
should allow for us not getting enough vectors and having to change the mapping
on the fly. I've been meaning to tidy that up for a while. IT will mean this
needs to be non const and not written just at init time.
> +
> + /* 2048 payload size */
> ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> PAYLOAD_SIZE, CXL_MAILBOX_PAYLOAD_SHIFT);
> cxl_dstate->payload_size = CXL_MAILBOX_MAX_PAYLOAD_SIZE;
> + /* irq support */
> + ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> + BG_INT_CAP, 1);
> + ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> + MSI_N, msi_n);
> + cxl_dstate->mbox_msi_n = msi_n;
> }
>
> static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) { }
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 1a4480d42908..224bfdb4bfca 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -8,6 +8,8 @@
> */
>
> #include "qemu/osdep.h"
> +#include "hw/pci/msi.h"
> +#include "hw/pci/msix.h"
> #include "hw/cxl/cxl.h"
> #include "hw/cxl/cxl_events.h"
> #include "hw/pci/pci.h"
> @@ -980,9 +982,18 @@ static void bg_timercb(void *opaque)
> cxl_dstate->mbox_reg_state64[R_CXL_DEV_BG_CMD_STS] = bg_status_reg;
>
> if (cxl_dstate->bg.complete_pct == 100) {
> + CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> + PCIDevice *pdev = PCI_DEVICE(ct3d);
> +
> cxl_dstate->bg.starttime = 0;
> /* registers are updated, allow new bg-capable cmds */
> cxl_dstate->bg.runtime = 0;
> +
> + if (msix_enabled(pdev)) {
> + msix_notify(pdev, cxl_dstate->mbox_msi_n);
> + } else if (msi_enabled(pdev)) {
> + msi_notify(pdev, cxl_dstate->mbox_msi_n);
> + }
> }
> }
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 63c28a1ed5d2..801f72c5fcae 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -777,7 +777,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> ComponentRegisters *regs = &cxl_cstate->crb;
> MemoryRegion *mr = ®s->component_registers;
> uint8_t *pci_conf = pci_dev->config;
> - unsigned short msix_num = 10;
> + unsigned short msix_num = 12;
> int i, rc;
>
> QTAILQ_INIT(&ct3d->error_list);
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index dbb8a955723b..f986651b6ead 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -189,6 +189,7 @@ typedef struct cxl_device_state {
> struct {
> MemoryRegion mailbox;
> uint16_t payload_size;
> + uint8_t mbox_msi_n;
> union {
> uint8_t mbox_reg_state[CXL_MAILBOX_REGISTERS_LENGTH];
> uint16_t mbox_reg_state16[CXL_MAILBOX_REGISTERS_LENGTH / 2];
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/5] cxl/device: Handle Media Disabled state
2023-04-18 17:23 [PATCH v2 -qemu 0/5] cxl: Background commands and device Sanitation Davidlohr Bueso
2023-04-18 17:23 ` [PATCH 1/5] cxl/mbox: Add support for background operations Davidlohr Bueso
2023-04-18 17:23 ` [PATCH 2/5] cxl/mbox: Wire up interrupts for background completion Davidlohr Bueso
@ 2023-04-18 17:23 ` Davidlohr Bueso
2023-05-15 12:39 ` Jonathan Cameron
2023-04-18 17:23 ` [PATCH 4/5] hw/cxl: Add support for device sanitation Davidlohr Bueso
2023-04-18 17:23 ` [PATCH 5/5] cxl/type3: Remove superfluous statements Davidlohr Bueso
4 siblings, 1 reply; 13+ messages in thread
From: Davidlohr Bueso @ 2023-04-18 17:23 UTC (permalink / raw)
To: Jonathan.Cameron
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, dave, linux-cxl
Ensure that both memory device read/writes and mailbox commands
can deal with the device media being disabled - per CXL 3.0 specs
8.2.9.8.5.1 - Sanitize. Disabled semantics here are strictly 11b,
however noting that if the media is not in the ready state (01b),
user data is not accessible.
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
hw/cxl/cxl-mailbox-utils.c | 15 +++++++++++++++
hw/mem/cxl_type3.c | 10 ++++++++++
include/hw/cxl/cxl_device.h | 33 +++++++++++++++++++++++++++++++++
3 files changed, 58 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 224bfdb4bfca..e0e20bc3a2bb 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -891,6 +891,21 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
if (len == cxl_cmd->in || cxl_cmd->in == ~0) {
cxl_cmd->payload = cxl_dstate->mbox_reg_state +
A_CXL_DEV_CMD_PAYLOAD;
+
+ if (cxl_dev_media_disabled(cxl_dstate)) {
+ if (h == cmd_events_get_records ||
+ h == cmd_logs_get_log ||
+ h == cmd_ccls_get_partition_info ||
+ h == cmd_ccls_get_lsa ||
+ h == cmd_ccls_set_lsa ||
+ h == cmd_media_get_poison_list ||
+ h == cmd_media_inject_poison ||
+ h == cmd_media_clear_poison) {
+ ret = CXL_MBOX_MEDIA_DISABLED;
+ goto done;
+ }
+ }
+
/* Only one bg command at a time */
if ((cxl_cmd->effect & BACKGROUND_OPERATION) &&
cxl_dstate->bg.runtime > 0) {
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 801f72c5fcae..707bdc263f03 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -12,6 +12,7 @@
#include "qemu/pmem.h"
#include "qemu/range.h"
#include "qemu/rcu.h"
+#include "qemu/guest-random.h"
#include "sysemu/hostmem.h"
#include "sysemu/numa.h"
#include "hw/cxl/cxl.h"
@@ -989,6 +990,11 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
if (res) {
return MEMTX_ERROR;
}
+ /* all memory reads will return random values */
+ if (cxl_dev_media_disabled(&CXL_TYPE3(d)->cxl_dstate)) {
+ qemu_guest_getrandom_nofail(data, size);
+ return MEMTX_OK;
+ }
return address_space_read(as, dpa_offset, attrs, data, size);
}
@@ -1005,6 +1011,10 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
if (res) {
return MEMTX_ERROR;
}
+ /* memory writes to the device will have no effect */
+ if (cxl_dev_media_disabled(&CXL_TYPE3(d)->cxl_dstate)) {
+ return MEMTX_OK;
+ }
return address_space_write(as, dpa_offset, attrs, &data, size);
}
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index f986651b6ead..3ef7a7cded95 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -346,6 +346,39 @@ REG64(CXL_MEM_DEV_STS, 0)
FIELD(CXL_MEM_DEV_STS, MBOX_READY, 4, 1)
FIELD(CXL_MEM_DEV_STS, RESET_NEEDED, 5, 3)
+static inline void cxl_dev_media_toggle(CXLDeviceState *cxl_dstate, int val)
+{
+ uint64_t reg;
+
+ reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
+ if (FIELD_EX64(reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == val) {
+ return;
+ }
+
+ reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
+ reg = FIELD_DP64(reg, CXL_MEM_DEV_STS, MBOX_READY, 0x1);
+
+ stq_le_p(&cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS, reg);
+}
+
+static inline void cxl_dev_media_enable(CXLDeviceState *cxl_dstate)
+{
+ cxl_dev_media_toggle(cxl_dstate, 0x1);
+}
+
+static inline void cxl_dev_media_disable(CXLDeviceState *cxl_dstate)
+{
+ cxl_dev_media_toggle(cxl_dstate, 0x3);
+}
+
+static inline bool cxl_dev_media_disabled(CXLDeviceState *cxl_dstate)
+{
+ uint64_t reg;
+
+ reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
+ return unlikely(FIELD_EX64(reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == 0x3);
+}
+
typedef struct CXLError {
QTAILQ_ENTRY(CXLError) node;
int type; /* Error code as per FE definition */
--
2.40.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] cxl/device: Handle Media Disabled state
2023-04-18 17:23 ` [PATCH 3/5] cxl/device: Handle Media Disabled state Davidlohr Bueso
@ 2023-05-15 12:39 ` Jonathan Cameron
2023-05-15 16:41 ` Davidlohr Bueso
0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2023-05-15 12:39 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, linux-cxl
On Tue, 18 Apr 2023 10:23:35 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:
> Ensure that both memory device read/writes and mailbox commands
> can deal with the device media being disabled - per CXL 3.0 specs
> 8.2.9.8.5.1 - Sanitize. Disabled semantics here are strictly 11b,
> however noting that if the media is not in the ready state (01b),
> user data is not accessible.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
> hw/cxl/cxl-mailbox-utils.c | 15 +++++++++++++++
> hw/mem/cxl_type3.c | 10 ++++++++++
> include/hw/cxl/cxl_device.h | 33 +++++++++++++++++++++++++++++++++
> 3 files changed, 58 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 224bfdb4bfca..e0e20bc3a2bb 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -891,6 +891,21 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> if (len == cxl_cmd->in || cxl_cmd->in == ~0) {
> cxl_cmd->payload = cxl_dstate->mbox_reg_state +
> A_CXL_DEV_CMD_PAYLOAD;
> +
> + if (cxl_dev_media_disabled(cxl_dstate)) {
> + if (h == cmd_events_get_records ||
> + h == cmd_logs_get_log ||
> + h == cmd_ccls_get_partition_info ||
> + h == cmd_ccls_get_lsa ||
> + h == cmd_ccls_set_lsa ||
> + h == cmd_media_get_poison_list ||
> + h == cmd_media_inject_poison ||
> + h == cmd_media_clear_poison) {
> + ret = CXL_MBOX_MEDIA_DISABLED;
> + goto done;
> + }
> + }
> +
> /* Only one bg command at a time */
> if ((cxl_cmd->effect & BACKGROUND_OPERATION) &&
> cxl_dstate->bg.runtime > 0) {
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 801f72c5fcae..707bdc263f03 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -12,6 +12,7 @@
> #include "qemu/pmem.h"
> #include "qemu/range.h"
> #include "qemu/rcu.h"
> +#include "qemu/guest-random.h"
> #include "sysemu/hostmem.h"
> #include "sysemu/numa.h"
> #include "hw/cxl/cxl.h"
> @@ -989,6 +990,11 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> if (res) {
> return MEMTX_ERROR;
> }
> + /* all memory reads will return random values */
Reference for this would be good. Random is expensive if we can get
away with a constant value.
> + if (cxl_dev_media_disabled(&CXL_TYPE3(d)->cxl_dstate)) {
> + qemu_guest_getrandom_nofail(data, size);
> + return MEMTX_OK;
> + }
>
> return address_space_read(as, dpa_offset, attrs, data, size);
> }
> @@ -1005,6 +1011,10 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
> if (res) {
> return MEMTX_ERROR;
> }
> + /* memory writes to the device will have no effect */
> + if (cxl_dev_media_disabled(&CXL_TYPE3(d)->cxl_dstate)) {
> + return MEMTX_OK;
> + }
>
> return address_space_write(as, dpa_offset, attrs, &data, size);
> }
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index f986651b6ead..3ef7a7cded95 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -346,6 +346,39 @@ REG64(CXL_MEM_DEV_STS, 0)
> FIELD(CXL_MEM_DEV_STS, MBOX_READY, 4, 1)
> FIELD(CXL_MEM_DEV_STS, RESET_NEEDED, 5, 3)
>
> +static inline void cxl_dev_media_toggle(CXLDeviceState *cxl_dstate, int val)
> +{
> + uint64_t reg;
> +
> + reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
> + if (FIELD_EX64(reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == val) {
> + return;
> + }
> +
> + reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
> + reg = FIELD_DP64(reg, CXL_MEM_DEV_STS, MBOX_READY, 0x1);
> +
> + stq_le_p(&cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS, reg);
Why & ?
> +}
> +
> +static inline void cxl_dev_media_enable(CXLDeviceState *cxl_dstate)
> +{
> + cxl_dev_media_toggle(cxl_dstate, 0x1);
> +}
> +
> +static inline void cxl_dev_media_disable(CXLDeviceState *cxl_dstate)
> +{
> + cxl_dev_media_toggle(cxl_dstate, 0x3);
> +}
> +
> +static inline bool cxl_dev_media_disabled(CXLDeviceState *cxl_dstate)
> +{
> + uint64_t reg;
> +
> + reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
> + return unlikely(FIELD_EX64(reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == 0x3);
> +}
> +
> typedef struct CXLError {
> QTAILQ_ENTRY(CXLError) node;
> int type; /* Error code as per FE definition */
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] cxl/device: Handle Media Disabled state
2023-05-15 12:39 ` Jonathan Cameron
@ 2023-05-15 16:41 ` Davidlohr Bueso
2023-05-16 9:36 ` Jonathan Cameron
0 siblings, 1 reply; 13+ messages in thread
From: Davidlohr Bueso @ 2023-05-15 16:41 UTC (permalink / raw)
To: Jonathan Cameron
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, linux-cxl
On Mon, 15 May 2023, Jonathan Cameron wrote:
>On Tue, 18 Apr 2023 10:23:35 -0700
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> Ensure that both memory device read/writes and mailbox commands
>> can deal with the device media being disabled - per CXL 3.0 specs
>> 8.2.9.8.5.1 - Sanitize. Disabled semantics here are strictly 11b,
>> however noting that if the media is not in the ready state (01b),
>> user data is not accessible.
>>
>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>> ---
>> hw/cxl/cxl-mailbox-utils.c | 15 +++++++++++++++
>> hw/mem/cxl_type3.c | 10 ++++++++++
>> include/hw/cxl/cxl_device.h | 33 +++++++++++++++++++++++++++++++++
>> 3 files changed, 58 insertions(+)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index 224bfdb4bfca..e0e20bc3a2bb 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -891,6 +891,21 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
>> if (len == cxl_cmd->in || cxl_cmd->in == ~0) {
>> cxl_cmd->payload = cxl_dstate->mbox_reg_state +
>> A_CXL_DEV_CMD_PAYLOAD;
>> +
>> + if (cxl_dev_media_disabled(cxl_dstate)) {
>> + if (h == cmd_events_get_records ||
>> + h == cmd_logs_get_log ||
>> + h == cmd_ccls_get_partition_info ||
>> + h == cmd_ccls_get_lsa ||
>> + h == cmd_ccls_set_lsa ||
>> + h == cmd_media_get_poison_list ||
>> + h == cmd_media_inject_poison ||
>> + h == cmd_media_clear_poison) {
>> + ret = CXL_MBOX_MEDIA_DISABLED;
>> + goto done;
>> + }
>> + }
>> +
>> /* Only one bg command at a time */
>> if ((cxl_cmd->effect & BACKGROUND_OPERATION) &&
>> cxl_dstate->bg.runtime > 0) {
>> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
>> index 801f72c5fcae..707bdc263f03 100644
>> --- a/hw/mem/cxl_type3.c
>> +++ b/hw/mem/cxl_type3.c
>> @@ -12,6 +12,7 @@
>> #include "qemu/pmem.h"
>> #include "qemu/range.h"
>> #include "qemu/rcu.h"
>> +#include "qemu/guest-random.h"
>> #include "sysemu/hostmem.h"
>> #include "sysemu/numa.h"
>> #include "hw/cxl/cxl.h"
>> @@ -989,6 +990,11 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
>> if (res) {
>> return MEMTX_ERROR;
>> }
>> + /* all memory reads will return random values */
>
>Reference for this would be good. Random is expensive if we can get
>away with a constant value.
Oh I threw out any performance expectations for qemu out the window, otherwise
I'd cry. Unless it's problem (this is media disabled, so bleh), I would
prefer keeping it as is.
>> + if (cxl_dev_media_disabled(&CXL_TYPE3(d)->cxl_dstate)) {
>> + qemu_guest_getrandom_nofail(data, size);
>> + return MEMTX_OK;
>> + }
>>
>> return address_space_read(as, dpa_offset, attrs, data, size);
>> }
>> @@ -1005,6 +1011,10 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
>> if (res) {
>> return MEMTX_ERROR;
>> }
>> + /* memory writes to the device will have no effect */
>> + if (cxl_dev_media_disabled(&CXL_TYPE3(d)->cxl_dstate)) {
>> + return MEMTX_OK;
>> + }
>>
>> return address_space_write(as, dpa_offset, attrs, &data, size);
>> }
>> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
>> index f986651b6ead..3ef7a7cded95 100644
>> --- a/include/hw/cxl/cxl_device.h
>> +++ b/include/hw/cxl/cxl_device.h
>> @@ -346,6 +346,39 @@ REG64(CXL_MEM_DEV_STS, 0)
>> FIELD(CXL_MEM_DEV_STS, MBOX_READY, 4, 1)
>> FIELD(CXL_MEM_DEV_STS, RESET_NEEDED, 5, 3)
>>
>> +static inline void cxl_dev_media_toggle(CXLDeviceState *cxl_dstate, int val)
>> +{
>> + uint64_t reg;
>> +
>> + reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
>> + if (FIELD_EX64(reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == val) {
>> + return;
>> + }
>> +
>> + reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
>> + reg = FIELD_DP64(reg, CXL_MEM_DEV_STS, MBOX_READY, 0x1);
>> +
>> + stq_le_p(&cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS, reg);
>
>Why & ?
braino
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] cxl/device: Handle Media Disabled state
2023-05-15 16:41 ` Davidlohr Bueso
@ 2023-05-16 9:36 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2023-05-16 9:36 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, linux-cxl
On Mon, 15 May 2023 09:41:29 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:
> On Mon, 15 May 2023, Jonathan Cameron wrote:
>
> >On Tue, 18 Apr 2023 10:23:35 -0700
> >Davidlohr Bueso <dave@stgolabs.net> wrote:
> >
> >> Ensure that both memory device read/writes and mailbox commands
> >> can deal with the device media being disabled - per CXL 3.0 specs
> >> 8.2.9.8.5.1 - Sanitize. Disabled semantics here are strictly 11b,
> >> however noting that if the media is not in the ready state (01b),
> >> user data is not accessible.
> >>
> >> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> >> ---
> >> hw/cxl/cxl-mailbox-utils.c | 15 +++++++++++++++
> >> hw/mem/cxl_type3.c | 10 ++++++++++
> >> include/hw/cxl/cxl_device.h | 33 +++++++++++++++++++++++++++++++++
> >> 3 files changed, 58 insertions(+)
> >>
> >> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> >> index 224bfdb4bfca..e0e20bc3a2bb 100644
> >> --- a/hw/cxl/cxl-mailbox-utils.c
> >> +++ b/hw/cxl/cxl-mailbox-utils.c
> >> @@ -891,6 +891,21 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> >> if (len == cxl_cmd->in || cxl_cmd->in == ~0) {
> >> cxl_cmd->payload = cxl_dstate->mbox_reg_state +
> >> A_CXL_DEV_CMD_PAYLOAD;
> >> +
> >> + if (cxl_dev_media_disabled(cxl_dstate)) {
> >> + if (h == cmd_events_get_records ||
> >> + h == cmd_logs_get_log ||
> >> + h == cmd_ccls_get_partition_info ||
> >> + h == cmd_ccls_get_lsa ||
> >> + h == cmd_ccls_set_lsa ||
> >> + h == cmd_media_get_poison_list ||
> >> + h == cmd_media_inject_poison ||
> >> + h == cmd_media_clear_poison) {
> >> + ret = CXL_MBOX_MEDIA_DISABLED;
> >> + goto done;
> >> + }
> >> + }
> >> +
> >> /* Only one bg command at a time */
> >> if ((cxl_cmd->effect & BACKGROUND_OPERATION) &&
> >> cxl_dstate->bg.runtime > 0) {
> >> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> >> index 801f72c5fcae..707bdc263f03 100644
> >> --- a/hw/mem/cxl_type3.c
> >> +++ b/hw/mem/cxl_type3.c
> >> @@ -12,6 +12,7 @@
> >> #include "qemu/pmem.h"
> >> #include "qemu/range.h"
> >> #include "qemu/rcu.h"
> >> +#include "qemu/guest-random.h"
> >> #include "sysemu/hostmem.h"
> >> #include "sysemu/numa.h"
> >> #include "hw/cxl/cxl.h"
> >> @@ -989,6 +990,11 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> >> if (res) {
> >> return MEMTX_ERROR;
> >> }
> >> + /* all memory reads will return random values */
> >
> >Reference for this would be good. Random is expensive if we can get
> >away with a constant value.
>
> Oh I threw out any performance expectations for qemu out the window, otherwise
> I'd cry. Unless it's problem (this is media disabled, so bleh), I would
> prefer keeping it as is.
Fair enough though marked data (the old DEADBEEF or similar) might be more
useful for verifying it worked as expected.
>
> >> + if (cxl_dev_media_disabled(&CXL_TYPE3(d)->cxl_dstate)) {
> >> + qemu_guest_getrandom_nofail(data, size);
> >> + return MEMTX_OK;
> >> + }
> >>
> >> return address_space_read(as, dpa_offset, attrs, data, size);
> >> }
> >> @@ -1005,6 +1011,10 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
> >> if (res) {
> >> return MEMTX_ERROR;
> >> }
> >> + /* memory writes to the device will have no effect */
> >> + if (cxl_dev_media_disabled(&CXL_TYPE3(d)->cxl_dstate)) {
> >> + return MEMTX_OK;
> >> + }
> >>
> >> return address_space_write(as, dpa_offset, attrs, &data, size);
> >> }
> >> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> >> index f986651b6ead..3ef7a7cded95 100644
> >> --- a/include/hw/cxl/cxl_device.h
> >> +++ b/include/hw/cxl/cxl_device.h
> >> @@ -346,6 +346,39 @@ REG64(CXL_MEM_DEV_STS, 0)
> >> FIELD(CXL_MEM_DEV_STS, MBOX_READY, 4, 1)
> >> FIELD(CXL_MEM_DEV_STS, RESET_NEEDED, 5, 3)
> >>
> >> +static inline void cxl_dev_media_toggle(CXLDeviceState *cxl_dstate, int val)
> >> +{
> >> + uint64_t reg;
> >> +
> >> + reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
> >> + if (FIELD_EX64(reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == val) {
> >> + return;
> >> + }
> >> +
> >> + reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
> >> + reg = FIELD_DP64(reg, CXL_MEM_DEV_STS, MBOX_READY, 0x1);
> >> +
> >> + stq_le_p(&cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS, reg);
> >
> >Why & ?
>
> braino
>
> Thanks,
> Davidlohr
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/5] hw/cxl: Add support for device sanitation
2023-04-18 17:23 [PATCH v2 -qemu 0/5] cxl: Background commands and device Sanitation Davidlohr Bueso
` (2 preceding siblings ...)
2023-04-18 17:23 ` [PATCH 3/5] cxl/device: Handle Media Disabled state Davidlohr Bueso
@ 2023-04-18 17:23 ` Davidlohr Bueso
2023-05-15 12:51 ` Jonathan Cameron
2023-04-18 17:23 ` [PATCH 5/5] cxl/type3: Remove superfluous statements Davidlohr Bueso
4 siblings, 1 reply; 13+ messages in thread
From: Davidlohr Bueso @ 2023-04-18 17:23 UTC (permalink / raw)
To: Jonathan.Cameron
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, dave, linux-cxl
Make use of the background operations through the sanitize
command, per CXL 3.0 specs. Traditionally run times can be
rather long, depending on the size of the media.
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
hw/cxl/cxl-mailbox-utils.c | 125 ++++++++++++++++++++++++++++++++++++-
1 file changed, 124 insertions(+), 1 deletion(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index e0e20bc3a2bb..2808d0161a38 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -18,6 +18,7 @@
#include "qemu/log.h"
#include "qemu/units.h"
#include "qemu/uuid.h"
+#include "sysemu/hostmem.h"
#define CXL_CAPACITY_MULTIPLIER (256 * MiB)
@@ -71,6 +72,9 @@ enum {
#define GET_PARTITION_INFO 0x0
#define GET_LSA 0x2
#define SET_LSA 0x3
+ SANITATION = 0x44,
+ #define SANITIZE 0x0
+ #define SECURE_ERASE 0x1
MEDIA_AND_POISON = 0x43,
#define GET_POISON_LIST 0x0
#define INJECT_POISON 0x1
@@ -819,6 +823,112 @@ static CXLRetCode cmd_media_clear_poison(struct cxl_cmd *cmd,
#define IMMEDIATE_LOG_CHANGE (1 << 4)
#define BACKGROUND_OPERATION (1 << 5)
+/* re-initialize the address space */
+static void __do_sanitization(CXLDeviceState *cxl_dstate)
+{
+ CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
+ DeviceState *ds = DEVICE(ct3d);
+ char *name;
+ MemoryRegion *mr;
+
+ if (ct3d->hostvmem) {
+ mr = host_memory_backend_get_memory(ct3d->hostvmem);
+ if (!mr) {
+ return;
+ }
+ if (ds->id) {
+ name = g_strdup_printf("cxl-type3-dpa-vmem-space:%s", ds->id);
+ } else {
+ name = g_strdup("cxl-type3-dpa-vmem-space");
+ }
+
+ address_space_destroy(&ct3d->hostvmem_as);
+ address_space_init(&ct3d->hostvmem_as, mr, name);
+ g_free(name);
+ }
+ if (ct3d->hostpmem) {
+ mr = host_memory_backend_get_memory(ct3d->hostpmem);
+ if (!mr) {
+ return;
+ }
+ if (ds->id) {
+ name = g_strdup_printf("cxl-type3-dpa-pmem-space:%s", ds->id);
+ } else {
+ name = g_strdup("cxl-type3-dpa-pmem-space");
+ }
+
+ address_space_destroy(&ct3d->hostpmem_as);
+ address_space_init(&ct3d->hostpmem_as, mr, name);
+ g_free(name);
+ }
+}
+
+/*
+ * CXL 3.0 spec section 8.2.9.8.5.1 - Sanitize.
+ *
+ * Once the Sanitize command has started successfully, the device shall
+ * shall be placed in the media disabled state. If the command fails or
+ * is interrupted by a reset or power failure, it shall remain in the
+ * media disabled state until a successful Sanitize command has been
+ * completed.
+ */
+static CXLRetCode cmd_sanitation_sanitize(struct cxl_cmd *cmd,
+ CXLDeviceState *cxl_dstate,
+ uint16_t *len)
+{
+ unsigned int secs;
+ uint64_t total_mem; /* in Mb */
+ struct cxl_cmd *c;
+
+ *len = 0;
+
+ /* Reported in CEL */
+ c = &cxl_dstate->cxl_cmd_set[SANITATION][SANITIZE];
+ if (!(c->effect & BACKGROUND_OPERATION)) {
+ __do_sanitization(cxl_dstate);
+ return CXL_MBOX_SUCCESS;
+ }
+
+ /*
+ * Estimated times based on:
+ * https://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf
+ */
+ total_mem = (cxl_dstate->vmem_size + cxl_dstate->pmem_size) >> 20;
+ if (total_mem <= 512) {
+ secs = 4;
+ } else if (total_mem <= 1024) {
+ secs = 8;
+ } else if (total_mem <= 2 * 1024) {
+ secs = 15;
+ } else if (total_mem <= 4 * 1024) {
+ secs = 30;
+ } else if (total_mem <= 8 * 1024) {
+ secs = 60;
+ } else if (total_mem <= 16 * 1024) {
+ secs = 2 * 60;
+ } else if (total_mem <= 32 * 1024) {
+ secs = 4 * 60;
+ } else if (total_mem <= 64 * 1024) {
+ secs = 8 * 60;
+ } else if (total_mem <= 128 * 1024) {
+ secs = 15 * 60;
+ } else if (total_mem <= 256 * 1024) {
+ secs = 30 * 60;
+ } else if (total_mem <= 512 * 1024) {
+ secs = 60 * 60;
+ } else if (total_mem <= 1024 * 1024) {
+ secs = 120 * 60;
+ } else {
+ secs = 240 * 60; /* max 4 hrs */
+ }
+
+ /* sanitize when done */
+ cxl_dev_media_disable(cxl_dstate);
+ cxl_dstate->bg.runtime = secs * 1000UL;
+
+ return CXL_MBOX_BG_STARTED;
+}
+
static struct cxl_cmd cxl_cmd_set[256][256] = {
[EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
cmd_events_get_records, 1, 0 },
@@ -842,6 +952,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
[CCLS][GET_LSA] = { "CCLS_GET_LSA", cmd_ccls_get_lsa, 8, 0 },
[CCLS][SET_LSA] = { "CCLS_SET_LSA", cmd_ccls_set_lsa,
~0, IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE },
+ [SANITATION][SANITIZE] = { "SANITATION_SANITIZE", cmd_sanitation_sanitize,
+ 0, IMMEDIATE_DATA_CHANGE | BACKGROUND_OPERATION },
[MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST",
cmd_media_get_poison_list, 16, 0 },
[MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON",
@@ -985,7 +1097,18 @@ static void bg_timercb(void *opaque)
bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS, RET_CODE, ret);
- /* TODO add ad-hoc cmd succesful completion handling */
+ /* ad-hoc cmd succesful completion handling */
+ if (ret == CXL_MBOX_SUCCESS) {
+ switch (cxl_dstate->bg.opcode) {
+ case 0x4400: /* sanitize */
+ __do_sanitization(cxl_dstate);
+ cxl_dev_media_enable(cxl_dstate);
+ break;
+ default:
+ __builtin_unreachable();
+ break;
+ }
+ }
} else {
/* estimate only */
cxl_dstate->bg.complete_pct = 100 * now / total_time;
--
2.40.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/5] hw/cxl: Add support for device sanitation
2023-04-18 17:23 ` [PATCH 4/5] hw/cxl: Add support for device sanitation Davidlohr Bueso
@ 2023-05-15 12:51 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2023-05-15 12:51 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, linux-cxl
On Tue, 18 Apr 2023 10:23:36 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:
> Make use of the background operations through the sanitize
> command, per CXL 3.0 specs.
Specific reference would be good.
> Traditionally run times can be
> rather long, depending on the size of the media.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
> hw/cxl/cxl-mailbox-utils.c | 125 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index e0e20bc3a2bb..2808d0161a38 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -18,6 +18,7 @@
> #include "qemu/log.h"
> #include "qemu/units.h"
> #include "qemu/uuid.h"
> +#include "sysemu/hostmem.h"
>
> #define CXL_CAPACITY_MULTIPLIER (256 * MiB)
>
> @@ -71,6 +72,9 @@ enum {
> #define GET_PARTITION_INFO 0x0
> #define GET_LSA 0x2
> #define SET_LSA 0x3
> + SANITATION = 0x44,
> + #define SANITIZE 0x0
> + #define SECURE_ERASE 0x1
> MEDIA_AND_POISON = 0x43,
> #define GET_POISON_LIST 0x0
> #define INJECT_POISON 0x1
> @@ -819,6 +823,112 @@ static CXLRetCode cmd_media_clear_poison(struct cxl_cmd *cmd,
> #define IMMEDIATE_LOG_CHANGE (1 << 4)
> #define BACKGROUND_OPERATION (1 << 5)
>
> +/* re-initialize the address space */
Why does that result in sanitization?
Given we do this for pmem on all exit paths and the pmem is still full
of data when we bring it back up on a fresh boot, I don't think this
works. Probably have to use address space accessing functions and wipe
it out a small chunk at a time.
Confusion here may be that you can't do host_memory_backend_get_memory
mainly because it might not be memory. I tend to back with files.
address_space_write() is probably the way to go.
Jonathan
> +static void __do_sanitization(CXLDeviceState *cxl_dstate)
> +{
> + CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> + DeviceState *ds = DEVICE(ct3d);
> + char *name;
> + MemoryRegion *mr;
> +
> + if (ct3d->hostvmem) {
> + mr = host_memory_backend_get_memory(ct3d->hostvmem);
> + if (!mr) {
> + return;
> + }
> + if (ds->id) {
> + name = g_strdup_printf("cxl-type3-dpa-vmem-space:%s", ds->id);
> + } else {
> + name = g_strdup("cxl-type3-dpa-vmem-space");
> + }
> +
> + address_space_destroy(&ct3d->hostvmem_as);
> + address_space_init(&ct3d->hostvmem_as, mr, name);
> + g_free(name);
> + }
> + if (ct3d->hostpmem) {
> + mr = host_memory_backend_get_memory(ct3d->hostpmem);
> + if (!mr) {
> + return;
> + }
> + if (ds->id) {
> + name = g_strdup_printf("cxl-type3-dpa-pmem-space:%s", ds->id);
> + } else {
> + name = g_strdup("cxl-type3-dpa-pmem-space");
> + }
> +
> + address_space_destroy(&ct3d->hostpmem_as);
> + address_space_init(&ct3d->hostpmem_as, mr, name);
> + g_free(name);
> + }
> +}
> +
> +/*
> + * CXL 3.0 spec section 8.2.9.8.5.1 - Sanitize.
> + *
> + * Once the Sanitize command has started successfully, the device shall
> + * shall be placed in the media disabled state. If the command fails or
> + * is interrupted by a reset or power failure, it shall remain in the
> + * media disabled state until a successful Sanitize command has been
> + * completed.
> + */
> +static CXLRetCode cmd_sanitation_sanitize(struct cxl_cmd *cmd,
> + CXLDeviceState *cxl_dstate,
> + uint16_t *len)
> +{
> + unsigned int secs;
> + uint64_t total_mem; /* in Mb */
> + struct cxl_cmd *c;
> +
> + *len = 0;
> +
> + /* Reported in CEL */
> + c = &cxl_dstate->cxl_cmd_set[SANITATION][SANITIZE];
> + if (!(c->effect & BACKGROUND_OPERATION)) {
We don't yet provide a means to test this? Perhaps a command line parameter
would be good if we want to try both paths.
> + __do_sanitization(cxl_dstate);
> + return CXL_MBOX_SUCCESS;
> + }
> +
> + /*
> + * Estimated times based on:
> + * https://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf
> + */
> + total_mem = (cxl_dstate->vmem_size + cxl_dstate->pmem_size) >> 20;
> + if (total_mem <= 512) {
> + secs = 4;
> + } else if (total_mem <= 1024) {
> + secs = 8;
> + } else if (total_mem <= 2 * 1024) {
> + secs = 15;
> + } else if (total_mem <= 4 * 1024) {
> + secs = 30;
> + } else if (total_mem <= 8 * 1024) {
> + secs = 60;
> + } else if (total_mem <= 16 * 1024) {
> + secs = 2 * 60;
> + } else if (total_mem <= 32 * 1024) {
> + secs = 4 * 60;
> + } else if (total_mem <= 64 * 1024) {
> + secs = 8 * 60;
> + } else if (total_mem <= 128 * 1024) {
> + secs = 15 * 60;
> + } else if (total_mem <= 256 * 1024) {
> + secs = 30 * 60;
> + } else if (total_mem <= 512 * 1024) {
> + secs = 60 * 60;
> + } else if (total_mem <= 1024 * 1024) {
> + secs = 120 * 60;
> + } else {
> + secs = 240 * 60; /* max 4 hrs */
> + }
> +
> + /* sanitize when done */
> + cxl_dev_media_disable(cxl_dstate);
> + cxl_dstate->bg.runtime = secs * 1000UL;
> +
> + return CXL_MBOX_BG_STARTED;
> +}
> +
> static struct cxl_cmd cxl_cmd_set[256][256] = {
> [EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
> cmd_events_get_records, 1, 0 },
> @@ -842,6 +952,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
> [CCLS][GET_LSA] = { "CCLS_GET_LSA", cmd_ccls_get_lsa, 8, 0 },
> [CCLS][SET_LSA] = { "CCLS_SET_LSA", cmd_ccls_set_lsa,
> ~0, IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE },
> + [SANITATION][SANITIZE] = { "SANITATION_SANITIZE", cmd_sanitation_sanitize,
> + 0, IMMEDIATE_DATA_CHANGE | BACKGROUND_OPERATION },
> [MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST",
> cmd_media_get_poison_list, 16, 0 },
> [MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON",
> @@ -985,7 +1097,18 @@ static void bg_timercb(void *opaque)
>
> bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS, RET_CODE, ret);
>
> - /* TODO add ad-hoc cmd succesful completion handling */
> + /* ad-hoc cmd succesful completion handling */
Why are these 'ad-hoc'? I'd drop that bit of comment.
> + if (ret == CXL_MBOX_SUCCESS) {
> + switch (cxl_dstate->bg.opcode) {
> + case 0x4400: /* sanitize */
> + __do_sanitization(cxl_dstate);
> + cxl_dev_media_enable(cxl_dstate);
> + break;
> + default:
> + __builtin_unreachable();
> + break;
> + }
> + }
> } else {
> /* estimate only */
> cxl_dstate->bg.complete_pct = 100 * now / total_time;
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 5/5] cxl/type3: Remove superfluous statements
2023-04-18 17:23 [PATCH v2 -qemu 0/5] cxl: Background commands and device Sanitation Davidlohr Bueso
` (3 preceding siblings ...)
2023-04-18 17:23 ` [PATCH 4/5] hw/cxl: Add support for device sanitation Davidlohr Bueso
@ 2023-04-18 17:23 ` Davidlohr Bueso
2023-05-15 12:53 ` Jonathan Cameron
4 siblings, 1 reply; 13+ messages in thread
From: Davidlohr Bueso @ 2023-04-18 17:23 UTC (permalink / raw)
To: Jonathan.Cameron
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, dave, linux-cxl
Simple cleanup while going through the code, remove the
bogus return statements from end of void returning
functions.
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
hw/mem/cxl_type3.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 707bdc263f03..415a8c8a3973 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -864,7 +864,6 @@ err_address_space_free:
if (ct3d->hostvmem) {
address_space_destroy(&ct3d->hostvmem_as);
}
- return;
}
static void ct3_exit(PCIDevice *pci_dev)
@@ -1290,8 +1289,6 @@ void qmp_cxl_inject_uncorrectable_errors(const char *path,
stl_le_p(reg_state + R_CXL_RAS_UNC_ERR_STATUS, unc_err);
pcie_aer_inject_error(PCI_DEVICE(obj), &err);
-
- return;
}
void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
@@ -1564,7 +1561,6 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
if (cxl_event_insert(cxlds, enc_log, (CXLEventRecordRaw *)&dram)) {
cxl_event_irq_assert(ct3d);
}
- return;
}
void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
--
2.40.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] cxl/type3: Remove superfluous statements
2023-04-18 17:23 ` [PATCH 5/5] cxl/type3: Remove superfluous statements Davidlohr Bueso
@ 2023-05-15 12:53 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2023-05-15 12:53 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: dan.j.williams, fan.ni, a.manzanares, dave.jiang, linux-cxl
On Tue, 18 Apr 2023 10:23:37 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:
> Simple cleanup while going through the code, remove the
> bogus return statements from end of void returning
> functions.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
I'll pick up this version if its the appropriate one when
I next update my qemu tree. Might make minimal tweaks as per
feedback if I remember to do so. I'll let you know if I do
anything more significant.
Thanks,
Jonathan
> ---
> hw/mem/cxl_type3.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 707bdc263f03..415a8c8a3973 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -864,7 +864,6 @@ err_address_space_free:
> if (ct3d->hostvmem) {
> address_space_destroy(&ct3d->hostvmem_as);
> }
> - return;
> }
>
> static void ct3_exit(PCIDevice *pci_dev)
> @@ -1290,8 +1289,6 @@ void qmp_cxl_inject_uncorrectable_errors(const char *path,
>
> stl_le_p(reg_state + R_CXL_RAS_UNC_ERR_STATUS, unc_err);
> pcie_aer_inject_error(PCI_DEVICE(obj), &err);
> -
> - return;
> }
>
> void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
> @@ -1564,7 +1561,6 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
> if (cxl_event_insert(cxlds, enc_log, (CXLEventRecordRaw *)&dram)) {
> cxl_event_irq_assert(ct3d);
> }
> - return;
> }
>
> void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
^ permalink raw reply [flat|nested] 13+ messages in thread