[PATCH -qemu 0/3] cxl: Background commands and device sanitation

[PATCH -qemu 0/3] cxl: Background commands and device sanitation

[Davidlohr Bueso]

Hello,

While device sanitation makes little sense for virtual machines, I am
posting this series as it was used to test the driver equivalent in
a sane manner and might come in handy for others.

My hope is that in the future, the background mailbox handling (first
two patches) might be used when other bg-capable commands are introduced
(we already have cmd_infostat_bg_op_sts(), which while updated, it is
untested).

This applies on top of Jonathan's 'origin/cxl-2023-02-21' branch.

Thanks!

Davidlohr Bueso (3):
  cxl/mbox: Add support for background operations
  cxl/mbox: Wire up interrupts for background completion
  cxl: Add support for device sanitation

 hw/cxl/cxl-device-utils.c   |  13 +-
 hw/cxl/cxl-mailbox-utils.c  | 249 +++++++++++++++++++++++++++++++++++-
 hw/mem/cxl_type3.c          |   9 +-
 include/hw/cxl/cxl_device.h |  28 ++++
 4 files changed, 291 insertions(+), 8 deletions(-)

--
2.39.2

[PATCH 1/3] cxl/mbox: Add support for background operations

[Davidlohr Bueso]

Support background commands in the mailbox, and 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  | 103 ++++++++++++++++++++++++++++++++++--
 include/hw/cxl/cxl_device.h |  10 ++++
 3 files changed, 111 insertions(+), 7 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..82923bb84eb0 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,8 @@ 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 SECURITY_STATE_CHANGE (1 << 5)
+#define BACKGROUND_OPERATION (1 << 6)
 
 static struct cxl_cmd cxl_cmd_set[256][256] = {
     [EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
@@ -856,12 +865,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 1000UL
+
 void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
 {
     uint16_t ret = CXL_MBOX_SUCCESS;
     struct cxl_cmd *cxl_cmd;
-    uint64_t status_reg;
+    uint64_t status_reg = 0;
     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 +890,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 +911,12 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
         ret = CXL_MBOX_UNSUPPORTED;
     }
 
-    /* Set the return code */
-    status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, ERRNO, ret);
+done:
+    /* Set bg and the return code */
+    if (bg_started) {
+        status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, BG_OP, bg_started);
+    }
+    status_reg = FIELD_DP64(status_reg, CXL_DEV_MAILBOX_STS, ERRNO, ret);
 
     /* Set the return length */
     command_reg = FIELD_DP64(command_reg, CXL_DEV_MAILBOX_CMD, COMMAND_SET, 0);
@@ -895,11 +926,70 @@ 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 */
+
+        qemu_log("Background command %04xh finished: %s\n",
+                 cxl_dstate->bg.opcode,
+                 ret == CXL_MBOX_SUCCESS ? "success" : "aborted");
+    } 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 +1010,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/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.39.2

Re: [PATCH 1/3] cxl/mbox: Add support for background operations

[Fan Ni]

On Fri, Feb 24, 2023 at 11:44:41AM -0800, Davidlohr Bueso wrote:

One minor thing. See below under bg_timercb.

> Support background commands in the mailbox, and 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  | 103 ++++++++++++++++++++++++++++++++++--
>  include/hw/cxl/cxl_device.h |  10 ++++
>  3 files changed, 111 insertions(+), 7 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..82923bb84eb0 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,8 @@ 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 SECURITY_STATE_CHANGE (1 << 5)
> +#define BACKGROUND_OPERATION (1 << 6)
>  
>  static struct cxl_cmd cxl_cmd_set[256][256] = {
>      [EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
> @@ -856,12 +865,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 1000UL
> +
>  void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
>  {
>      uint16_t ret = CXL_MBOX_SUCCESS;
>      struct cxl_cmd *cxl_cmd;
> -    uint64_t status_reg;
> +    uint64_t status_reg = 0;
>      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 +890,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 +911,12 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
>          ret = CXL_MBOX_UNSUPPORTED;
>      }
>  
> -    /* Set the return code */
> -    status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, ERRNO, ret);
> +done:
> +    /* Set bg and the return code */
> +    if (bg_started) {
> +        status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, BG_OP, bg_started);
> +    }
> +    status_reg = FIELD_DP64(status_reg, CXL_DEV_MAILBOX_STS, ERRNO, ret);
>  
>      /* Set the return length */
>      command_reg = FIELD_DP64(command_reg, CXL_DEV_MAILBOX_CMD, COMMAND_SET, 0);
> @@ -895,11 +926,70 @@ 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 */
> +
> +        qemu_log("Background command %04xh finished: %s\n",
> +                 cxl_dstate->bg.opcode,
> +                 ret == CXL_MBOX_SUCCESS ? "success" : "aborted");
Seems ret will always be CXL_MBOX_SUCCESS, maybe bg_status_reg?
> +    } 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 +1010,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/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.39.2
> 

Re: [PATCH 1/3] cxl/mbox: Add support for background operations

[Davidlohr Bueso]

On Wed, 01 Mar 2023, Fan Ni wrote:

>On Fri, Feb 24, 2023 at 11:44:41AM -0800, Davidlohr Bueso wrote:
>
>One minor thing. See below under bg_timercb.

Thanks for taking a look.

...

>> +
>> +        qemu_log("Background command %04xh finished: %s\n",
>> +                 cxl_dstate->bg.opcode,
>> +                 ret == CXL_MBOX_SUCCESS ? "success" : "aborted");
>Seems ret will always be CXL_MBOX_SUCCESS, maybe bg_status_reg?

bg_status_reg is still set to ret, so it doesn't make any difference.
Overall I saw little reason to ever return anything other than success,
most of the code checking against CXL_MBOX_SUCCESS is mostly a formality,
for if this ever changes.

Thanks,
Davidlohr

Re: [PATCH 1/3] cxl/mbox: Add support for background operations

[Jonathan Cameron]

On Fri, 24 Feb 2023 11:44:41 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Support background commands in the mailbox, and 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.

Better to wrap a little nearer 70-75 chars for a commit message.

> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

I suspect we need to ensure the timer is stopped in the tear down
path via timer_free(), but otherwise a few comments inline.

Jonathan

> ---
>  hw/cxl/cxl-device-utils.c   |   5 +-
>  hw/cxl/cxl-mailbox-utils.c  | 103 ++++++++++++++++++++++++++++++++++--
>  include/hw/cxl/cxl_device.h |  10 ++++
>  3 files changed, 111 insertions(+), 7 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..82923bb84eb0 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;

Ah. I'd forgotten I bodged this one in for the switch.  

> +    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,8 @@ 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 SECURITY_STATE_CHANGE (1 << 5)

I'd be either tempted to drop defining SECURITY_STATE_CHANGE or go all the way
and define all the bits in CXL 3.0.  Slight preference for just dropping the
unused one for now.

> +#define BACKGROUND_OPERATION (1 << 6)

>  
>  static struct cxl_cmd cxl_cmd_set[256][256] = {
>      [EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
> @@ -856,12 +865,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 1000UL
> +
>  void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
>  {
>      uint16_t ret = CXL_MBOX_SUCCESS;
>      struct cxl_cmd *cxl_cmd;
> -    uint64_t status_reg;
> +    uint64_t status_reg = 0;
>      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 +890,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 +911,12 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
>          ret = CXL_MBOX_UNSUPPORTED;
>      }
>  
> -    /* Set the return code */
> -    status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, ERRNO, ret);
> +done:
> +    /* Set bg and the return code */
> +    if (bg_started) {
> +        status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, BG_OP, bg_started);

You've set status_reg to 0 above, so just do FIELD_DP64(status_reg, ..
here and ave a reviewer have to check if we might be wiping something out.
Or reorder this to after setting the ERRNO field thus reducing the diff.

> +    }
> +    status_reg = FIELD_DP64(status_reg, CXL_DEV_MAILBOX_STS, ERRNO, ret);
>  
>      /* Set the return length */
>      command_reg = FIELD_DP64(command_reg, CXL_DEV_MAILBOX_CMD, COMMAND_SET, 0);
> @@ -895,11 +926,70 @@ 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 */
> +
> +        qemu_log("Background command %04xh finished: %s\n",
> +                 cxl_dstate->bg.opcode,
> +                 ret == CXL_MBOX_SUCCESS ? "success" : "aborted");

Useful for debug, but we probably don't want it there in final code.

> +    } 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;

Looks like this could all move into the if (now >=.. 
block above as I'm not seeing either of these fields being used after that.

> +    }
> +}
> +
>  void cxl_initialize_mailbox(CXLDeviceState *cxl_dstate, bool switch_cci)
>  {
>      if (!switch_cci) {
> @@ -920,4 +1010,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/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 */

Re: [PATCH 1/3] cxl/mbox: Add support for background operations

[Davidlohr Bueso]

On Mon, 03 Apr 2023, Jonathan Cameron wrote:

>> @@ -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;
>
>Ah. I'd forgotten I bodged this one in for the switch.

General question, do you know the reasoning behind why this command is only listed
for switches (while the overall vocabulary is very generic to all bg-capable commands)?

Thanks,
Davidlohr

Re: [PATCH 1/3] cxl/mbox: Add support for background operations

[Jonathan Cameron]

On Fri, 7 Apr 2023 11:05:12 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Mon, 03 Apr 2023, Jonathan Cameron wrote:
> 
> >> @@ -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;  
> >
> >Ah. I'd forgotten I bodged this one in for the switch.  
> 
> General question, do you know the reasoning behind why this command is only listed
> for switches (while the overall vocabulary is very generic to all bg-capable commands)?
> 
> Thanks,
> Davidlohr

Maybe the thought was that there are other ways for it to be obtained from everywhere
else and duplication (with possibility of disagreement) is bad...

(feel free to ask right person in the consortium).. :)

Re: [PATCH 1/3] cxl/mbox: Add support for background operations

[Davidlohr Bueso]

On Mon, 03 Apr 2023, Jonathan Cameron wrote:

>On Fri, 24 Feb 2023 11:44:41 -0800
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>> +    } 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;
>
>Looks like this could all move into the if (now >=..
>block above as I'm not seeing either of these fields being used after that.

I'd rather keep this branch. For one, the next patch will also add the msi
notifications here; and secondly, we don't want another incoming cmd to see
no bg command running (runtime is used to check CXL_MBOX_BUSY) and status
pct complete not at 100.

Thanks,
Davidlohr

Re: [PATCH 1/3] cxl/mbox: Add support for background operations

[Jonathan Cameron]

On Tue, 11 Apr 2023 12:06:07 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Mon, 03 Apr 2023, Jonathan Cameron wrote:
> 
> >On Fri, 24 Feb 2023 11:44:41 -0800
> >Davidlohr Bueso <dave@stgolabs.net> wrote:  
> >> +    } 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;  
> >
> >Looks like this could all move into the if (now >=..
> >block above as I'm not seeing either of these fields being used after that.  
> 
> I'd rather keep this branch. For one, the next patch will also add the msi
> notifications here; and secondly, we don't want another incoming cmd to see
> no bg command running (runtime is used to check CXL_MBOX_BUSY) and status
> pct complete not at 100.
> 
Fair enough. I still haven't read patch 3 :)

Might make it today!  Only a few weeks after reading patch 2.

> Thanks,
> Davidlohr

[PATCH 2/3] cxl/mbox: Wire up interrupts for background completion

[Davidlohr Bueso]

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 +++++++++++
 include/hw/cxl/cxl_device.h |  1 +
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index 4bb4e85dae19..a4a2c6a80004 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 = 9;
+
+    /* 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 82923bb84eb0..61f0b8d675bc 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"
@@ -984,9 +986,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 = &ct3d->parent_obj;
+
         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/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.39.2

Re: [PATCH 2/3] cxl/mbox: Wire up interrupts for background completion

[Jonathan Cameron]

On Fri, 24 Feb 2023 11:44:42 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Notify when the background operation is done.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Hi Davidlohr,

One trivial inline.  

Also, the interrupt setup for the PCI cap is missing I think.
See handling in ct3_realize()



Jonathan

> ---
>  hw/cxl/cxl-device-utils.c   | 10 +++++++++-
>  hw/cxl/cxl-mailbox-utils.c  | 11 +++++++++++
>  include/hw/cxl/cxl_device.h |  1 +
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index 4bb4e85dae19..a4a2c6a80004 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 = 9;
> +
> +    /* 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 82923bb84eb0..61f0b8d675bc 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"
> @@ -984,9 +986,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 = &ct3d->parent_obj;

Should be casting it rather than directly accessing the parent_obj
PCI_DEVICE(ct3d) should work.

There is an open question about whether we should be doing similar for the
cxl_dstate.  I've not figured out the answer yet, but it may well affect this.


> +
>          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/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];

Re: [PATCH 2/3] cxl/mbox: Wire up interrupts for background completion

[Jonathan Cameron]

On Mon, 3 Apr 2023 17:52:24 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 24 Feb 2023 11:44:42 -0800
> Davidlohr Bueso <dave@stgolabs.net> wrote:
> 
> > Notify when the background operation is done.
> > 
> > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>  
> Hi Davidlohr,
> 
> One trivial inline.  
> 
> Also, the interrupt setup for the PCI cap is missing I think.
> See handling in ct3_realize()

Should have said, for more minor stuff I'm fine just fixing these
up in my staging tree directly.  You can either feedback here or
when I post that series for upstream merge.  

Jonathan
> 
> 
> 
> Jonathan
> 
> > ---
> >  hw/cxl/cxl-device-utils.c   | 10 +++++++++-
> >  hw/cxl/cxl-mailbox-utils.c  | 11 +++++++++++
> >  include/hw/cxl/cxl_device.h |  1 +
> >  3 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> > index 4bb4e85dae19..a4a2c6a80004 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 = 9;
> > +
> > +    /* 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 82923bb84eb0..61f0b8d675bc 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"
> > @@ -984,9 +986,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 = &ct3d->parent_obj;  
> 
> Should be casting it rather than directly accessing the parent_obj
> PCI_DEVICE(ct3d) should work.
> 
> There is an open question about whether we should be doing similar for the
> cxl_dstate.  I've not figured out the answer yet, but it may well affect this.
> 
> 
> > +
> >          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/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];  
> 
> 

Re: [PATCH 2/3] cxl/mbox: Wire up interrupts for background completion

[Davidlohr Bueso]

On Mon, 03 Apr 2023, Jonathan Cameron wrote:

>Also, the interrupt setup for the PCI cap is missing I think.
>See handling in ct3_realize()

Hmm how is this any different that what is already there
for events and cpmu? Once msi is initialized for the device,
I didn't expect any other setup necessary - and I'm also
receiving irqs fine with this patch at the driver side.

Thanks,
Davidlohr

Re: [PATCH 2/3] cxl/mbox: Wire up interrupts for background completion

[Jonathan Cameron]

On Tue, 11 Apr 2023 19:22:44 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Mon, 03 Apr 2023, Jonathan Cameron wrote:
> 
> >Also, the interrupt setup for the PCI cap is missing I think.
> >See handling in ct3_realize()  
> 
> Hmm how is this any different that what is already there
> for events and cpmu? Once msi is initialized for the device,
> I didn't expect any other setup necessary - and I'm also
> receiving irqs fine with this patch at the driver side.

Ah. Seems you are sharing with the DOE for the compliance
mailbox - probably not the intent but as we have no code
that binds to that at the moment I guess you won't notice
the sharing.  It should be harmless but I'd put this on 11
and increase msix_num in ct3_realize.

At somepoint I need to make all the msix code for all usecase
deal with the OS asking for fewer vectors.  Not a problem with
Linux as it always grabs them all, but the emulation isn't currently
spec compliant as those numbers should 'squish' into the available
space if not enough is requested.

When that's not happening I should add an enum for msix vectors
so that we can more easily reorder the patches that add them.

Jonathan


> 
> Thanks,
> Davidlohr

[PATCH 3/3] cxl: Add support for device sanitation

[Davidlohr Bueso]

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.

Estimate times based on:
	 https://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 hw/cxl/cxl-mailbox-utils.c  | 139 +++++++++++++++++++++++++++++++++++-
 hw/mem/cxl_type3.c          |   9 ++-
 include/hw/cxl/cxl_device.h |  17 +++++
 3 files changed, 162 insertions(+), 3 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 61f0b8d675bc..aa0641f786e2 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
+    SANITIZE    = 0x44,
+        #define OVERWRITE     0x0
+        #define SECURE_ERASE  0x1
     MEDIA_AND_POISON = 0x43,
         #define GET_POISON_LIST        0x0
         #define INJECT_POISON          0x1
@@ -626,6 +630,109 @@ static CXLRetCode cmd_ccls_set_lsa(struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+/* Perform the actual device zeroing */
+static void __do_sanitization(CXLDeviceState *cxl_dstate)
+{
+    MemoryRegion *mr;
+    CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
+
+    if (ct3d->hostvmem) {
+        mr = host_memory_backend_get_memory(ct3d->hostvmem);
+        if (mr) {
+            void *hostmem = memory_region_get_ram_ptr(mr);
+            memset(hostmem, 0, memory_region_size(mr));
+        }
+    }
+
+    if (ct3d->hostpmem) {
+        mr = host_memory_backend_get_memory(ct3d->hostpmem);
+        if (mr) {
+            void *hostmem = memory_region_get_ram_ptr(mr);
+            memset(hostmem, 0, memory_region_size(mr));
+        }
+    }
+    if (ct3d->lsa) {
+        mr = host_memory_backend_get_memory(ct3d->lsa);
+        if (mr) {
+            void *lsa = memory_region_get_ram_ptr(mr);
+            memset(lsa, 0, memory_region_size(mr));
+        }
+    }
+}
+
+/*
+ * CXL 3.0 spec section 8.2.9.8.5.1 - Sanitize.
+ *
+ * Once the Sanitize command has started successfully, the device 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. During this state:
+ *
+ * 1. Memory writes to the device will have no effect, and all memory reads
+ * will return random values (no user data returned, even for locations that
+ * the failed Sanitize operation didn’t sanitize yet).
+ *
+ * 2. Mailbox commands shall still be processed in the disabled state, except
+ * that commands that access Sanitized areas shall fail with the Media Disabled
+ * error code.
+ */
+static CXLRetCode cmd_sanitize_overwrite(struct cxl_cmd *cmd,
+                                         CXLDeviceState *cxl_dstate,
+                                         uint16_t *len)
+{
+    uint64_t total_mem; /* in Mb */
+    int secs;
+
+    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 */
+    }
+
+    /* EBUSY other bg cmds as of now */
+    cxl_dstate->bg.runtime = secs * 1000UL;
+    *len = 0;
+
+    qemu_log_mask(LOG_UNIMP,
+                  "Sanitize/overwrite command runtime for %ldMb media: %d seconds\n",
+                  total_mem, secs);
+
+    cxl_dev_disable_media(cxl_dstate);
+
+    if (secs > 2) {
+        /* sanitize when done */
+        return CXL_MBOX_BG_STARTED;
+    } else {
+        __do_sanitization(cxl_dstate);
+        cxl_dev_enable_media(cxl_dstate);
+
+        return CXL_MBOX_SUCCESS;
+    }
+}
+
 /*
  * This is very inefficient, but good enough for now!
  * Also the payload will always fit, so no need to handle the MORE flag and
@@ -843,6 +950,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 },
+    [SANITIZE][OVERWRITE] = { "SANITIZE_OVERWRITE", cmd_sanitize_overwrite,
+        0, IMMEDIATE_DATA_CHANGE | SECURITY_STATE_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",
@@ -898,6 +1007,21 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
                     ret = CXL_MBOX_BUSY;
                     goto done;
             }
+            /* forbid any selected commands while overwriting */
+            if (sanitize_running(cxl_dstate)) {
+                if (h == cmd_events_get_records ||
+                    h == cmd_ccls_get_partition_info ||
+                    h == cmd_ccls_set_lsa ||
+                    h == cmd_ccls_get_lsa ||
+                    h == cmd_logs_get_log ||
+                    h == cmd_media_get_poison_list ||
+                    h == cmd_media_inject_poison ||
+                    h == cmd_media_clear_poison ||
+                    h == cmd_sanitize_overwrite) {
+                        ret = CXL_MBOX_MEDIA_DISABLED;
+                        goto done;
+                }
+            }
             ret = (*h)(cxl_cmd, cxl_dstate, &len);
             if ((cxl_cmd->effect & BACKGROUND_OPERATION) &&
                 ret == CXL_MBOX_BG_STARTED) {
@@ -970,8 +1094,19 @@ 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 */
-
+        if (ret == CXL_MBOX_SUCCESS) {
+            switch (cxl_dstate->bg.opcode) {
+            case 0x4400: /* sanitize */
+                __do_sanitization(cxl_dstate);
+                cxl_dev_enable_media(cxl_dstate);
+                break;
+            case 0x4304: /* TODO: scan media */
+                break;
+            default:
+                __builtin_unreachable();
+                break;
+            }
+        }
         qemu_log("Background command %04xh finished: %s\n",
                  cxl_dstate->bg.opcode,
                  ret == CXL_MBOX_SUCCESS ? "success" : "aborted");
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 334ce92f5e0f..2b483d3d8ea9 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"
@@ -988,6 +989,10 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
         return MEMTX_ERROR;
     }
 
+    if (sanitize_running(&CXL_TYPE3(d)->cxl_dstate)) {
+        qemu_guest_getrandom_nofail(data, size);
+        return MEMTX_OK;
+    }
     return address_space_read(as, dpa_offset, attrs, data, size);
 }
 
@@ -1003,7 +1008,9 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
     if (res) {
         return MEMTX_ERROR;
     }
-
+    if (sanitize_running(&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..e28536969397 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -346,6 +346,23 @@ 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 __toggle_media(CXLDeviceState *cxl_dstate, int val)
+{
+    uint64_t dev_status_reg;
+
+    dev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
+    cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg;
+}
+#define cxl_dev_disable_media(cxlds)                    \
+        do { __toggle_media((cxlds), 0x3); } while (0)
+#define cxl_dev_enable_media(cxlds)                     \
+        do { __toggle_media((cxlds), 0x1); } while (0)
+
+static inline bool sanitize_running(CXLDeviceState *cxl_dstate)
+{
+    return !!cxl_dstate->bg.runtime && cxl_dstate->bg.opcode == 0x4400;
+}
+
 typedef struct CXLError {
     QTAILQ_ENTRY(CXLError) node;
     int type; /* Error code as per FE definition */
-- 
2.39.2

Re: [PATCH 3/3] cxl: Add support for device sanitation

[Jonathan Cameron]

On Fri, 24 Feb 2023 11:44:43 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> 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.
> 
> Estimate times based on:
> 	 https://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Various comments inline.  I haven't tested this yet so one
or two are educated guesses.

Jonathan

> ---
>  hw/cxl/cxl-mailbox-utils.c  | 139 +++++++++++++++++++++++++++++++++++-
>  hw/mem/cxl_type3.c          |   9 ++-
>  include/hw/cxl/cxl_device.h |  17 +++++
>  3 files changed, 162 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 61f0b8d675bc..aa0641f786e2 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
> +    SANITIZE    = 0x44,
> +        #define OVERWRITE     0x0

I'd add the define along with the feature.
(not that I'm sure we've been doing that for all the existing ones!)

> +        #define SECURE_ERASE  0x1
>      MEDIA_AND_POISON = 0x43,
>          #define GET_POISON_LIST        0x0
>          #define INJECT_POISON          0x1
> @@ -626,6 +630,109 @@ static CXLRetCode cmd_ccls_set_lsa(struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +/* Perform the actual device zeroing */
> +static void __do_sanitization(CXLDeviceState *cxl_dstate)
> +{
> +    MemoryRegion *mr;
> +    CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> +
> +    if (ct3d->hostvmem) {
> +        mr = host_memory_backend_get_memory(ct3d->hostvmem);
> +        if (mr) {
> +            void *hostmem = memory_region_get_ram_ptr(mr);

I think this only works for ram backends.  Use the
address space writers we use for the decoded accesses.

> +            memset(hostmem, 0, memory_region_size(mr));
> +        }
> +    }
> +
> +    if (ct3d->hostpmem) {
> +        mr = host_memory_backend_get_memory(ct3d->hostpmem);
> +        if (mr) {
> +            void *hostmem = memory_region_get_ram_ptr(mr);
> +            memset(hostmem, 0, memory_region_size(mr));
> +        }
> +    }
> +    if (ct3d->lsa) {
> +        mr = host_memory_backend_get_memory(ct3d->lsa);
> +        if (mr) {
> +            void *lsa = memory_region_get_ram_ptr(mr);
> +            memset(lsa, 0, memory_region_size(mr));

Be odd if people are using ram for pmem of lsa as it's obviously
not going to persist!

> +        }
> +    }
> +}
> +
> +/*
> + * CXL 3.0 spec section 8.2.9.8.5.1 - Sanitize.
> + *
> + * Once the Sanitize command has started successfully, the device 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. During this state:
> + *
> + * 1. Memory writes to the device will have no effect, and all memory reads
> + * will return random values (no user data returned, even for locations that
> + * the failed Sanitize operation didn’t sanitize yet).

Reference..  Can random be a fixed string?  Easier to spot if testing then
a random number.

> + *
> + * 2. Mailbox commands shall still be processed in the disabled state, except
> + * that commands that access Sanitized areas shall fail with the Media Disabled
> + * error code.
Reference.
> + */
> +static CXLRetCode cmd_sanitize_overwrite(struct cxl_cmd *cmd,
> +                                         CXLDeviceState *cxl_dstate,
> +                                         uint16_t *len)
> +{
> +    uint64_t total_mem; /* in Mb */
> +    int secs;
> +
> +    total_mem = (cxl_dstate->vmem_size + cxl_dstate->pmem_size) >> 20;

The command effects log needs to 'say' if this is a background operation
or not.  Annoying though it is (and I doubt software will care) you can't
mix and match depending on how long the operation happens to take.

Annoying because this is rather cute :)

If we want to test both paths, need to add a parameter for device
creation to say it if is should be background or not.

> +    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 */
> +    }
> +
> +    /* EBUSY other bg cmds as of now */
> +    cxl_dstate->bg.runtime = secs * 1000UL;
> +    *len = 0;
> +
> +    qemu_log_mask(LOG_UNIMP,
> +                  "Sanitize/overwrite command runtime for %ldMb media: %d seconds\n",
> +                  total_mem, secs);
> +
> +    cxl_dev_disable_media(cxl_dstate);
> +
> +    if (secs > 2) {
> +        /* sanitize when done */
> +        return CXL_MBOX_BG_STARTED;
> +    } else {
> +        __do_sanitization(cxl_dstate);
> +        cxl_dev_enable_media(cxl_dstate);
> +
> +        return CXL_MBOX_SUCCESS;
> +    }
> +}
> +


> @@ -970,8 +1094,19 @@ 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 */
> -
> +        if (ret == CXL_MBOX_SUCCESS) {
> +            switch (cxl_dstate->bg.opcode) {
> +            case 0x4400: /* sanitize */

Build this from the defines and you won't need the comment.

> +                __do_sanitization(cxl_dstate);
> +                cxl_dev_enable_media(cxl_dstate);
> +                break;
> +            case 0x4304: /* TODO: scan media */

Don't add it then.  

> +                break;
> +            default:
> +                __builtin_unreachable();
> +                break;
> +            }
> +        }
>          qemu_log("Background command %04xh finished: %s\n",
>                   cxl_dstate->bg.opcode,
>                   ret == CXL_MBOX_SUCCESS ? "success" : "aborted");
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 334ce92f5e0f..2b483d3d8ea9 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"
> @@ -988,6 +989,10 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
>          return MEMTX_ERROR;
>      }
>  
> +    if (sanitize_running(&CXL_TYPE3(d)->cxl_dstate)) {
> +        qemu_guest_getrandom_nofail(data, size);
> +        return MEMTX_OK;
> +    }

Key here is that you are in media disable status.  I'd prefer checking
that to whether sanitize is running. There may be other routes to disabling
it in future.  Also makes me look at right bit of spec for what should be returned.

Speaking of which, where does the spec say what should be returned on
reads to disabled media?  Reference here (or a comment to say it's
not explicitly stated if it isn't) would be good.


>      return address_space_read(as, dpa_offset, attrs, data, size);
>  }
>  
> @@ -1003,7 +1008,9 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
>      if (res) {
>          return MEMTX_ERROR;
>      }
> -
> +    if (sanitize_running(&CXL_TYPE3(d)->cxl_dstate)) {
> +        return MEMTX_OK;
> +    }

Leave the blank line here. Maybe one above the new code as well.

>      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..e28536969397 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -346,6 +346,23 @@ 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 __toggle_media(CXLDeviceState *cxl_dstate, int val)
> +{
> +    uint64_t dev_status_reg;
reg or val or similar would do.
> +
> +    dev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
> +    cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg;

I haven't confirmed but I think this needs the endian fixes that we
are slowly pushing through as we touch particular code.  So you'd need to
use a little endian write.

Doesn't this wipe out the mailbox interface ready bit?  Don't want to do that!


> +}
> +#define cxl_dev_disable_media(cxlds)                    \
> +        do { __toggle_media((cxlds), 0x3); } while (0)

I'm surprised we need the do while around this... I guess checkpatch
is moaning about it?

> +#define cxl_dev_enable_media(cxlds)                     \
> +        do { __toggle_media((cxlds), 0x1); } while (0)
> +
> +static inline bool sanitize_running(CXLDeviceState *cxl_dstate)
> +{
> +    return !!cxl_dstate->bg.runtime && cxl_dstate->bg.opcode == 0x4400;

No need for the !! as you are treating it as boolean anyway via the &&

> +}
> +
>  typedef struct CXLError {
>      QTAILQ_ENTRY(CXLError) node;
>      int type; /* Error code as per FE definition */

Re: [PATCH 3/3] cxl: Add support for device sanitation

[Davidlohr Bueso]

On Fri, 14 Apr 2023, Jonathan Cameron wrote:

>On Fri, 24 Feb 2023 11:44:43 -0800
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> 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.
>>
>> Estimate times based on:
>>	 https://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf
>>
>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>
>Various comments inline.  I haven't tested this yet so one
>or two are educated guesses.

Thanks for having a look at the series and all the feedback.

...

>> @@ -988,6 +989,10 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
>>          return MEMTX_ERROR;
>>      }
>>
>> +    if (sanitize_running(&CXL_TYPE3(d)->cxl_dstate)) {
>> +        qemu_guest_getrandom_nofail(data, size);
>> +        return MEMTX_OK;
>> +    }
>
>Key here is that you are in media disable status.  I'd prefer checking
>that to whether sanitize is running. There may be other routes to disabling
>it in future.  Also makes me look at right bit of spec for what should be returned.

Yeah I agree, checking the media status seems more appropriate, but because
this media disabled behavior is described specifically under the Sanitation
section (more below), and the fact that no other cmd disables the media,
I went with sanitize check. Of course, I would be surprised if the read/write
semantics would ever vary if anything else also required it, so we could
generalize instead.


>Speaking of which, where does the spec say what should be returned on
>reads to disabled media?  Reference here (or a comment to say it's
>not explicitly stated if it isn't) would be good.

I found it in CXL 3.0 spec page 556:

"In this state, the Media Status field in the Memory Device Status
register will indicate 11b (Disabled), all memory writes to the device
will have no effect, and all memory reads will return random values"

fyi I ended up adding a new patch for media disable management.

Thanks,
Davidlohr