[PATCH] hw/cxl: Add support for abort of background operation

[PATCH] hw/cxl: Add support for abort of background operation

[ajay.opensrc]

From: Ajay Joshi <ajay.opensrc@micron.com>

This patch adds the support for aborting the background
operation if one is progress(r3.1 8.2.9.1.5)

"Request Abort Background Operation" command is requested
by the host to cancel an ongoing background operation. When
the command is sent, the device will abort the ongoing
background operation.  When the operation is aborted,
background command status register will maintain a completion
percentage value of less then 100. The "Request Abort
Background Operation" command support has to be advertised in
the Command Effects Log to be honoured.

Tested-by: Tushar Mulgund <tmmulgund@micron.com>
Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>
---
 hw/cxl/cxl-mailbox-utils.c   | 77 ++++++++++++++++++++++++++++--------
 include/hw/cxl/cxl_device.h  |  1 +
 include/hw/cxl/cxl_mailbox.h |  1 +
 3 files changed, 63 insertions(+), 16 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index c2ed251bb3..43c934f1fc 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -56,6 +56,7 @@ enum {
     INFOSTAT    = 0x00,
         #define IS_IDENTIFY   0x1
         #define BACKGROUND_OPERATION_STATUS    0x2
+        #define REQUEST_ABORT_BACKGROUND_OPERATION  0x5
     EVENTS      = 0x01,
         #define GET_RECORDS   0x0
         #define CLEAR_RECORDS   0x1
@@ -641,6 +642,28 @@ static CXLRetCode cmd_infostat_bg_op_sts(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+/*
+ * CXL r3.1 Section 8.2.9.1.5: Request Background Abort Operation
+ * (Opcode 0005h)
+ */
+static CXLRetCode cmd_infostat_bg_abort_op(const struct cxl_cmd *cmd,
+                                        uint8_t *payload_in,
+                                        size_t len_in,
+                                        uint8_t *payload_out,
+                                        size_t *len_out,
+                                        CXLCCI *cci)
+{
+    if (cci->bg.runtime > 0) {
+        if (cci->bg.abort != true) {
+            cci->bg.abort = true;
+        }
+    } else {
+        return CXL_MBOX_REQUEST_ABORT_NOTSUP;
+    }
+
+    return CXL_MBOX_SUCCESS;
+}
+
 /* CXL r3.1 Section 8.2.9.3.1: Get FW Info (Opcode 0200h) */
 static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
                                                uint8_t *payload_in,
@@ -2502,6 +2525,9 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
 }
 
 static const struct cxl_cmd cxl_cmd_set[256][256] = {
+    [INFOSTAT][REQUEST_ABORT_BACKGROUND_OPERATION] = {
+        "REQUEST_ABORT_BACKGROUND_OPERATION",
+        cmd_infostat_bg_abort_op, 0, 0 },
     [EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
         cmd_events_get_records, 1, 0 },
     [EVENTS][CLEAR_RECORDS] = { "EVENTS_CLEAR_RECORDS",
@@ -2541,7 +2567,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
     [SANITIZE][OVERWRITE] = { "SANITIZE_OVERWRITE", cmd_sanitize_overwrite, 0,
         (CXL_MBOX_IMMEDIATE_DATA_CHANGE |
          CXL_MBOX_SECURITY_STATE_CHANGE |
-         CXL_MBOX_BACKGROUND_OPERATION)},
+         CXL_MBOX_BACKGROUND_OPERATION  |
+         CXL_MBOX_REQUEST_ABORT_BACKGROUND_OPERATION)},
     [PERSISTENT_MEM][GET_SECURITY_STATE] = { "GET_SECURITY_STATE",
         cmd_get_security_state, 0, 0 },
     [MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST",
@@ -2554,7 +2581,9 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
         "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
         cmd_media_get_scan_media_capabilities, 16, 0 },
     [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA",
-        cmd_media_scan_media, 17, CXL_MBOX_BACKGROUND_OPERATION },
+        cmd_media_scan_media, 17,
+        (CXL_MBOX_BACKGROUND_OPERATION |
+        CXL_MBOX_REQUEST_ABORT_BACKGROUND_OPERATION)},
     [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = {
         "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
         cmd_media_get_scan_media_results, 0, 0 },
@@ -2579,6 +2608,9 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
     [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 },
     [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { "BACKGROUND_OPERATION_STATUS",
         cmd_infostat_bg_op_sts, 0, 0 },
+    [INFOSTAT][REQUEST_ABORT_BACKGROUND_OPERATION] = {
+        "REQUEST_ABORT_BACKGROUND_OPERATION",
+        cmd_infostat_bg_abort_op, 0, 0 },
     [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
     [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8,
                          CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
@@ -2673,6 +2705,24 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
     return ret;
 }
 
+static void bg_notify_host(CXLCCI *cci)
+{
+    /* TODO: generalize to switch CCI */
+    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+    CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
+    PCIDevice *pdev = PCI_DEVICE(cci->d);
+
+    cci->bg.starttime = 0;
+    /* registers are updated, allow new bg-capable cmds */
+    cci->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);
+    }
+}
+
 static void bg_timercb(void *opaque)
 {
     CXLCCI *cci = opaque;
@@ -2709,24 +2759,19 @@ static void bg_timercb(void *opaque)
     } else {
         /* estimate only */
         cci->bg.complete_pct = 100 * now / total_time;
+        if (cci->bg.abort == true) {
+            uint16_t ret = CXL_MBOX_SUCCESS;
+
+            cci->bg.ret_code = ret;
+            cci->bg.abort = false;
+            bg_notify_host(cci);
+            return;
+        }
         timer_mod(cci->bg.timer, now + CXL_MBOX_BG_UPDATE_FREQ);
     }
 
     if (cci->bg.complete_pct == 100) {
-        /* TODO: generalize to switch CCI */
-        CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
-        CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
-        PCIDevice *pdev = PCI_DEVICE(cci->d);
-
-        cci->bg.starttime = 0;
-        /* registers are updated, allow new bg-capable cmds */
-        cci->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);
-        }
+        bg_notify_host(cci);
     }
 }
 
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index dd02adda27..daad578027 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -201,6 +201,7 @@ typedef struct CXLCCI {
         uint64_t starttime;
         /* set by each bg cmd, cleared by the bg_timer when complete */
         uint64_t runtime;
+        bool abort; /*abort if the bg operation is in progress*/
         QEMUTimer *timer;
     } bg;
     size_t payload_max;
diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
index beb048052e..a8a6beb712 100644
--- a/include/hw/cxl/cxl_mailbox.h
+++ b/include/hw/cxl/cxl_mailbox.h
@@ -14,5 +14,6 @@
 #define CXL_MBOX_IMMEDIATE_LOG_CHANGE (1 << 4)
 #define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5)
 #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
+#define CXL_MBOX_REQUEST_ABORT_BACKGROUND_OPERATION (1 << 8)
 
 #endif
-- 
2.34.1

Re: [PATCH] hw/cxl: Add support for abort of background operation

[Davidlohr Bueso]

On Mon, 29 Jul 2024, ajay.opensrc@micron.com wrote:\n
>From: Ajay Joshi <ajay.opensrc@micron.com>
>
>This patch adds the support for aborting the background
>operation if one is progress(r3.1 8.2.9.1.5)
>
>"Request Abort Background Operation" command is requested
>by the host to cancel an ongoing background operation. When
>the command is sent, the device will abort the ongoing
>background operation.  When the operation is aborted,
>background command status register will maintain a completion
>percentage value of less then 100. The "Request Abort
>Background Operation" command support has to be advertised in
>the Command Effects Log to be honoured.

So I had a patch for this which I had not sent out, and compared
to this one I think the below is more robust, would you mind
testing this out?

Thanks,
Davidlohr

--8<--------
[PATCH] hw/cxl: Support aborting background commands

As of 3.1 spec, background commands can be canceled with a new
abort command. Implement the support, which is advertised in
the CEL. No ad-hoc context undoing is necessary as all the
command logic of the running bg command is done upon completion.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
  hw/cxl/cxl-device-utils.c    |  2 +-
  hw/cxl/cxl-mailbox-utils.c   | 39 ++++++++++++++++++++++++++++++++++--
  include/hw/cxl/cxl_device.h  |  1 +
  include/hw/cxl/cxl_mailbox.h |  1 +
  4 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index 035d034f6dd8..4f4ccfa92d82 100644
--- a/hw/cxl/cxl-device-utils.c
+++ b/hw/cxl/cxl-device-utils.c
@@ -95,7 +95,7 @@ static uint64_t mailbox_reg_read(void *opaque, hwaddr offset, unsigned size)
	 }
	 if (offset == A_CXL_DEV_MAILBOX_STS) {
	     uint64_t status_reg = cxl_dstate->mbox_reg_state64[offset / size];
-            if (cci->bg.complete_pct) {
+            if (cci->bg.complete_pct || cci->bg.aborted) {
		 status_reg = FIELD_DP64(status_reg, CXL_DEV_MAILBOX_STS, BG_OP,
					 0);
		 cxl_dstate->mbox_reg_state64[offset / size] = status_reg;
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 80a80f1ec29b..ae4b3cabbb86 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -53,6 +53,7 @@ enum {
      INFOSTAT    = 0x00,
	 #define IS_IDENTIFY   0x1
	 #define BACKGROUND_OPERATION_STATUS    0x2
+        #define BACKGROUND_OPERATION_ABORT     0x5
      EVENTS      = 0x01,
	 #define GET_RECORDS   0x0
	 #define CLEAR_RECORDS   0x1
@@ -815,6 +816,33 @@ static CXLRetCode cmd_infostat_bg_op_sts(const struct cxl_cmd *cmd,
      return CXL_MBOX_SUCCESS;
  }

+/* CXL r3.1 Section 8.2.9.1.5: Request Abort Background Operation (Opcode 0005h) */
+static CXLRetCode cmd_infostat_bg_op_abort(const struct cxl_cmd *cmd,
+                                           uint8_t *payload_in,
+                                           size_t len_in,
+                                           uint8_t *payload_out,
+                                           size_t *len_out,
+                                           CXLCCI *cci)
+{
+    int bg_set = cci->bg.opcode >> 8;
+    int bg_cmd = cci->bg.opcode & 0xff;
+    const struct cxl_cmd *bg_c = &cci->cxl_cmd_set[bg_set][bg_cmd];
+
+    if (!(bg_c->effect & CXL_MBOX_BACKGROUND_OPERATION_ABORT)) {
+        return CXL_MBOX_REQUEST_ABORT_NOTSUP;
+    }
+
+    if (cci->bg.runtime) {
+        assert(cci->bg.complete_pct < 100);
+        timer_del(cci->bg.timer);
+        cci->bg.ret_code = CXL_MBOX_ABORTED;
+        cci->bg.starttime = 0;
+        cci->bg.runtime = 0;
+        cci->bg.aborted = true;
+    }
+    return CXL_MBOX_SUCCESS;
+}
+
  /* CXL r3.1 Section 8.2.9.3.1: Get FW Info (Opcode 0200h) */
  static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
						uint8_t *payload_in,
@@ -2149,6 +2177,8 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
  }

  static const struct cxl_cmd cxl_cmd_set[256][256] = {
+    [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
+        cmd_infostat_bg_op_abort, 0, 0 },
      [EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
	 cmd_events_get_records, 1, 0 },
      [EVENTS][CLEAR_RECORDS] = { "EVENTS_CLEAR_RECORDS",
@@ -2176,7 +2206,7 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
      [SANITIZE][OVERWRITE] = { "SANITIZE_OVERWRITE", cmd_sanitize_overwrite, 0,
	 (CXL_MBOX_IMMEDIATE_DATA_CHANGE |
	  CXL_MBOX_SECURITY_STATE_CHANGE |
-         CXL_MBOX_BACKGROUND_OPERATION)},
+         CXL_MBOX_BACKGROUND_OPERATION  | CXL_MBOX_BACKGROUND_OPERATION_ABORT)},
      [PERSISTENT_MEM][GET_SECURITY_STATE] = { "GET_SECURITY_STATE",
	 cmd_get_security_state, 0, 0 },
      [MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST",
@@ -2189,7 +2219,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
	 "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
	 cmd_media_get_scan_media_capabilities, 16, 0 },
      [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA",
-        cmd_media_scan_media, 17, CXL_MBOX_BACKGROUND_OPERATION },
+        cmd_media_scan_media, 17,
+        CXL_MBOX_BACKGROUND_OPERATION | CXL_MBOX_BACKGROUND_OPERATION_ABORT},
      [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = {
	 "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
	 cmd_media_get_scan_media_results, 0, 0 },
@@ -2214,6 +2245,8 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
      [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 },
      [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { "BACKGROUND_OPERATION_STATUS",
	 cmd_infostat_bg_op_sts, 0, 0 },
+    [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
+        cmd_infostat_bg_op_abort, 0, 0 },
      [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
      [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8,
			  CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
@@ -2295,6 +2328,7 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
	 cci->bg.opcode = (set << 8) | cmd;

	 cci->bg.complete_pct = 0;
+        cci->bg.aborted = false;
	 cci->bg.ret_code = 0;

	 now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
@@ -2386,6 +2420,7 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
      cxl_rebuild_cel(cci);

      cci->bg.complete_pct = 0;
+    cci->bg.aborted = false;
      cci->bg.starttime = 0;
      cci->bg.runtime = 0;
      cci->bg.timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index d38391b26f0e..d83b359e9994 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -197,6 +197,7 @@ typedef struct CXLCCI {
      struct {
	 uint16_t opcode;
	 uint16_t complete_pct;
+        bool aborted;
	 uint16_t ret_code; /* Current value of retcode */
	 uint64_t starttime;
	 /* set by each bg cmd, cleared by the bg_timer when complete */
diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
index beb048052e1b..9008402d1c46 100644
--- a/include/hw/cxl/cxl_mailbox.h
+++ b/include/hw/cxl/cxl_mailbox.h
@@ -14,5 +14,6 @@
  #define CXL_MBOX_IMMEDIATE_LOG_CHANGE (1 << 4)
  #define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5)
  #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
+#define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)

  #endif
--
2.44.0

Re: [EXT] Re: [PATCH] hw/cxl: Add support for abort of background operation

[ajay.opensrc]

>From: Davidlohr Bueso <dave@stgolabs.net>
>>From: Ajay Joshi <ajay.opensrc@micron.com>
>>
>>This patch adds the support for aborting the background
>>operation if one is progress(r3.1 8.2.9.1.5)
>>
>>"Request Abort Background Operation" command is requested
>>by the host to cancel an ongoing background operation. When
>>the command is sent, the device will abort the ongoing
>>background operation.  When the operation is aborted,
>>background command status register will maintain a completion
>>percentage value of less then 100. The "Request Abort
>>Background Operation" command support has to be advertised in
>>the Command Effects Log to be honoured.
>
>So I had a patch for this which I had not sent out, and compared
>to this one I think the below is more robust, would you mind
>testing this out?

Didn't realize you were working on one already.
This looks good to me as well. I tested this and it seems to be
working as expected. Feel free to use:

Tested-by: Ajay Joshi <ajay.opensrc@micron.com>
Reviewed-by: Ajay Joshi <ajay.opensrc@micron.com>

>Thanks,
>Davidlohr
>
>--8<--------
>[PATCH] hw/cxl: Support aborting background commands
>
>As of 3.1 spec, background commands can be canceled with a new
>abort command. Implement the support, which is advertised in
>the CEL. No ad-hoc context undoing is necessary as all the
>command logic of the running bg command is done upon completion.
>
>Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>---
>  hw/cxl/cxl-device-utils.c    |  2 +-
>  hw/cxl/cxl-mailbox-utils.c   | 39 ++++++++++++++++++++++++++++++++++--
>  include/hw/cxl/cxl_device.h  |  1 +
>  include/hw/cxl/cxl_mailbox.h |  1 +
>  4 files changed, 40 insertions(+), 3 deletions(-)
>
>diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
>index 035d034f6dd8..4f4ccfa92d82 100644
>--- a/hw/cxl/cxl-device-utils.c
>+++ b/hw/cxl/cxl-device-utils.c
>@@ -95,7 +95,7 @@ static uint64_t mailbox_reg_read(void *opaque, hwaddr offset, >unsigned size)
>         }
>         if (offset == A_CXL_DEV_MAILBOX_STS) {
>             uint64_t status_reg = cxl_dstate->mbox_reg_state64[offset / size];
>-            if (cci->bg.complete_pct) {
>+            if (cci->bg.complete_pct || cci->bg.aborted) {
>                 status_reg = FIELD_DP64(status_reg, CXL_DEV_MAILBOX_STS, BG_OP,
>                                         0);
>                 cxl_dstate->mbox_reg_state64[offset / size] = status_reg;
>diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>index 80a80f1ec29b..ae4b3cabbb86 100644
>--- a/hw/cxl/cxl-mailbox-utils.c
>+++ b/hw/cxl/cxl-mailbox-utils.c
>@@ -53,6 +53,7 @@ enum {
>      INFOSTAT    = 0x00,
>         #define IS_IDENTIFY   0x1
>         #define BACKGROUND_OPERATION_STATUS    0x2
>+        #define BACKGROUND_OPERATION_ABORT     0x5
>      EVENTS      = 0x01,
>         #define GET_RECORDS   0x0
>         #define CLEAR_RECORDS   0x1
>@@ -815,6 +816,33 @@ static CXLRetCode cmd_infostat_bg_op_sts(const struct >cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>
>+/* CXL r3.1 Section 8.2.9.1.5: Request Abort Background Operation (Opcode >0005h) */
>+static CXLRetCode cmd_infostat_bg_op_abort(const struct cxl_cmd *cmd,
>+                                           uint8_t *payload_in,
>+                                           size_t len_in,
>+                                           uint8_t *payload_out,
>+                                           size_t *len_out,
>+                                           CXLCCI *cci)
>+{
>+    int bg_set = cci->bg.opcode >> 8;
>+    int bg_cmd = cci->bg.opcode & 0xff;
>+    const struct cxl_cmd *bg_c = &cci->cxl_cmd_set[bg_set][bg_cmd];
>+
>+    if (!(bg_c->effect & CXL_MBOX_BACKGROUND_OPERATION_ABORT)) {
>+        return CXL_MBOX_REQUEST_ABORT_NOTSUP;
>+    }
>+
>+    if (cci->bg.runtime) {
>+        assert(cci->bg.complete_pct < 100);
>+        timer_del(cci->bg.timer);
>+        cci->bg.ret_code = CXL_MBOX_ABORTED;
>+        cci->bg.starttime = 0;
>+        cci->bg.runtime = 0;
>+        cci->bg.aborted = true;
>+    }
>+    return CXL_MBOX_SUCCESS;
>+}
>+
>  /* CXL r3.1 Section 8.2.9.3.1: Get FW Info (Opcode 0200h) */
>  static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
>                                                uint8_t *payload_in,
>@@ -2149,6 +2177,8 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct >cxl_cmd *cmd,
>  }
>
>  static const struct cxl_cmd cxl_cmd_set[256][256] = {
>+    [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
>+        cmd_infostat_bg_op_abort, 0, 0 },
>      [EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
>         cmd_events_get_records, 1, 0 },
>      [EVENTS][CLEAR_RECORDS] = { "EVENTS_CLEAR_RECORDS",
>@@ -2176,7 +2206,7 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>      [SANITIZE][OVERWRITE] = { "SANITIZE_OVERWRITE", cmd_sanitize_overwrite, 0,
>         (CXL_MBOX_IMMEDIATE_DATA_CHANGE |
>          CXL_MBOX_SECURITY_STATE_CHANGE |
>-         CXL_MBOX_BACKGROUND_OPERATION)},
>+         CXL_MBOX_BACKGROUND_OPERATION  | >CXL_MBOX_BACKGROUND_OPERATION_ABORT)},
>      [PERSISTENT_MEM][GET_SECURITY_STATE] = { "GET_SECURITY_STATE",
>         cmd_get_security_state, 0, 0 },
>      [MEDIA_AND_POISON][GET_POISON_LIST] = { >"MEDIA_AND_POISON_GET_POISON_LIST",
>@@ -2189,7 +2219,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>         "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
>         cmd_media_get_scan_media_capabilities, 16, 0 },
>      [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA",
>-        cmd_media_scan_media, 17, CXL_MBOX_BACKGROUND_OPERATION },
>+        cmd_media_scan_media, 17,
>+        CXL_MBOX_BACKGROUND_OPERATION | CXL_MBOX_BACKGROUND_OPERATION_ABORT},
>      [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = {
>         "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
>         cmd_media_get_scan_media_results, 0, 0 },
>@@ -2214,6 +2245,8 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
>      [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 },
>      [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { "BACKGROUND_OPERATION_STATUS",
>         cmd_infostat_bg_op_sts, 0, 0 },
>+    [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
>+        cmd_infostat_bg_op_abort, 0, 0 },
>      [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
>      [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8,
>                          CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
>@@ -2295,6 +2328,7 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, >uint8_t cmd,
>         cci->bg.opcode = (set << 8) | cmd;
>
>         cci->bg.complete_pct = 0;
>+        cci->bg.aborted = false;
>         cci->bg.ret_code = 0;
>
>         now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
>@@ -2386,6 +2420,7 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
>      cxl_rebuild_cel(cci);
>
>      cci->bg.complete_pct = 0;
>+    cci->bg.aborted = false;
>      cci->bg.starttime = 0;
>      cci->bg.runtime = 0;
>      cci->bg.timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
>diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
>index d38391b26f0e..d83b359e9994 100644
>--- a/include/hw/cxl/cxl_device.h
>+++ b/include/hw/cxl/cxl_device.h
>@@ -197,6 +197,7 @@ typedef struct CXLCCI {
>      struct {
>         uint16_t opcode;
>         uint16_t complete_pct;
>+        bool aborted;
>         uint16_t ret_code; /* Current value of retcode */
>         uint64_t starttime;
>         /* set by each bg cmd, cleared by the bg_timer when complete */
>diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
>index beb048052e1b..9008402d1c46 100644
>--- a/include/hw/cxl/cxl_mailbox.h
>+++ b/include/hw/cxl/cxl_mailbox.h
>@@ -14,5 +14,6 @@
>  #define CXL_MBOX_IMMEDIATE_LOG_CHANGE (1 << 4)
>  #define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5)
>  #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
>+#define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
>
>  #endif
>--
>2.44.0
    

Re: [EXT] Re: [PATCH] hw/cxl: Add support for abort of background operation

[Jonathan Cameron]

On Fri, 2 Aug 2024 13:04:57 +0000
ajay.opensrc <ajay.opensrc@micron.com> wrote:

> >From: Davidlohr Bueso <dave@stgolabs.net>  
> >>From: Ajay Joshi <ajay.opensrc@micron.com>
> >>
> >>This patch adds the support for aborting the background
> >>operation if one is progress(r3.1 8.2.9.1.5)
> >>
> >>"Request Abort Background Operation" command is requested
> >>by the host to cancel an ongoing background operation. When
> >>the command is sent, the device will abort the ongoing
> >>background operation.  When the operation is aborted,
> >>background command status register will maintain a completion
> >>percentage value of less then 100. The "Request Abort
> >>Background Operation" command support has to be advertised in
> >>the Command Effects Log to be honoured.  
> >
> >So I had a patch for this which I had not sent out, and compared
> >to this one I think the below is more robust, would you mind
> >testing this out?  
> 
> Didn't realize you were working on one already.
> This looks good to me as well. I tested this and it seems to be
> working as expected. Feel free to use:
> 
> Tested-by: Ajay Joshi <ajay.opensrc@micron.com>
> Reviewed-by: Ajay Joshi <ajay.opensrc@micron.com>
From a quick look Davidlohr's patch looks good to me as well.
I 'could' pick it out of the middle of this thread, but
probably cleaner if Davlidlohr sends a formal patch
(picking up Ajay's tags) - maybe with a link tag to this
thread so we can see where those came from.

One comment inline - feels like a spec hole to me but I'd like some
more eyes on it before I query it more formally.

Thanks,

Jonathan

> 
> >Thanks,
> >Davidlohr
> >
> >--8<--------
> >[PATCH] hw/cxl: Support aborting background commands
> >
> >As of 3.1 spec, background commands can be canceled with a new
> >abort command. Implement the support, which is advertised in
> >the CEL. No ad-hoc context undoing is necessary as all the
> >command logic of the running bg command is done upon completion.
> >
> >Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> >---
> >  hw/cxl/cxl-device-utils.c    |  2 +-
> >  hw/cxl/cxl-mailbox-utils.c   | 39 ++++++++++++++++++++++++++++++++++--
> >  include/hw/cxl/cxl_device.h  |  1 +
> >  include/hw/cxl/cxl_mailbox.h |  1 +
> >  4 files changed, 40 insertions(+), 3 deletions(-)
> >
> >diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> >index 035d034f6dd8..4f4ccfa92d82 100644
> >--- a/hw/cxl/cxl-device-utils.c
> >+++ b/hw/cxl/cxl-device-utils.c
> >@@ -95,7 +95,7 @@ static uint64_t mailbox_reg_read(void *opaque, hwaddr offset, >unsigned size)
> >         }
> >         if (offset == A_CXL_DEV_MAILBOX_STS) {
> >             uint64_t status_reg = cxl_dstate->mbox_reg_state64[offset / size];
> >-            if (cci->bg.complete_pct) {
> >+            if (cci->bg.complete_pct || cci->bg.aborted) {
> >                 status_reg = FIELD_DP64(status_reg, CXL_DEV_MAILBOX_STS, BG_OP,
> >                                         0);
> >                 cxl_dstate->mbox_reg_state64[offset / size] = status_reg;
> >diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> >index 80a80f1ec29b..ae4b3cabbb86 100644
> >--- a/hw/cxl/cxl-mailbox-utils.c
> >+++ b/hw/cxl/cxl-mailbox-utils.c
> >@@ -53,6 +53,7 @@ enum {
> >      INFOSTAT    = 0x00,
> >         #define IS_IDENTIFY   0x1
> >         #define BACKGROUND_OPERATION_STATUS    0x2
> >+        #define BACKGROUND_OPERATION_ABORT     0x5
> >      EVENTS      = 0x01,
> >         #define GET_RECORDS   0x0
> >         #define CLEAR_RECORDS   0x1
> >@@ -815,6 +816,33 @@ static CXLRetCode cmd_infostat_bg_op_sts(const struct >cxl_cmd *cmd,
> >      return CXL_MBOX_SUCCESS;
> >  }
> >
> >+/* CXL r3.1 Section 8.2.9.1.5: Request Abort Background Operation (Opcode >0005h) */
> >+static CXLRetCode cmd_infostat_bg_op_abort(const struct cxl_cmd *cmd,
> >+                                           uint8_t *payload_in,
> >+                                           size_t len_in,
> >+                                           uint8_t *payload_out,
> >+                                           size_t *len_out,
> >+                                           CXLCCI *cci)
> >+{
> >+    int bg_set = cci->bg.opcode >> 8;
> >+    int bg_cmd = cci->bg.opcode & 0xff;
> >+    const struct cxl_cmd *bg_c = &cci->cxl_cmd_set[bg_set][bg_cmd];
> >+
> >+    if (!(bg_c->effect & CXL_MBOX_BACKGROUND_OPERATION_ABORT)) {
> >+        return CXL_MBOX_REQUEST_ABORT_NOTSUP;
> >+    }
> >+
> >+    if (cci->bg.runtime) {
> >+        assert(cci->bg.complete_pct < 100);

This seems fragile.  I'm not sure what guarantees no races.
I'd be tempted to follow the allowed path of just letting
the thing complete if that's the case.  Interesting 8.2.9.1.5
Request Abort Background operation states
"In resposne, the device may choose to interrupt he ongoing
background operation or may choose to continue it's
execution until completion. If the background operation is
interrupted, a Command Return of Success shall be returned..
If the ongoing background operation does not support abort
capability.. return code shall be set to Request Abort Not supported"

So to my reading if you elect not to stop because it's very
nearly done, or indeed it raced and the command is done,
the specification doesn't tell us what to return.
That operation supports abort, we just didn't.

What do you think should happen here?

> >+        timer_del(cci->bg.timer);
> >+        cci->bg.ret_code = CXL_MBOX_ABORTED;
> >+        cci->bg.starttime = 0;
> >+        cci->bg.runtime = 0;
> >+        cci->bg.aborted = true;
> >+    }
> >+    return CXL_MBOX_SUCCESS;
> >+}
> >+
> >  /* CXL r3.1 Section 8.2.9.3.1: Get FW Info (Opcode 0200h) */
> >  static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
> >                                                uint8_t *payload_in,
> >@@ -2149,6 +2177,8 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct >cxl_cmd *cmd,
> >  }
> >
> >  static const struct cxl_cmd cxl_cmd_set[256][256] = {
> >+    [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> >+        cmd_infostat_bg_op_abort, 0, 0 },
> >      [EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
> >         cmd_events_get_records, 1, 0 },
> >      [EVENTS][CLEAR_RECORDS] = { "EVENTS_CLEAR_RECORDS",
> >@@ -2176,7 +2206,7 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
> >      [SANITIZE][OVERWRITE] = { "SANITIZE_OVERWRITE", cmd_sanitize_overwrite, 0,
> >         (CXL_MBOX_IMMEDIATE_DATA_CHANGE |
> >          CXL_MBOX_SECURITY_STATE_CHANGE |
> >-         CXL_MBOX_BACKGROUND_OPERATION)},
> >+         CXL_MBOX_BACKGROUND_OPERATION  | >CXL_MBOX_BACKGROUND_OPERATION_ABORT)},
> >      [PERSISTENT_MEM][GET_SECURITY_STATE] = { "GET_SECURITY_STATE",
> >         cmd_get_security_state, 0, 0 },
> >      [MEDIA_AND_POISON][GET_POISON_LIST] = { >"MEDIA_AND_POISON_GET_POISON_LIST",
> >@@ -2189,7 +2219,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
> >         "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
> >         cmd_media_get_scan_media_capabilities, 16, 0 },
> >      [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA",
> >-        cmd_media_scan_media, 17, CXL_MBOX_BACKGROUND_OPERATION },
> >+        cmd_media_scan_media, 17,
> >+        CXL_MBOX_BACKGROUND_OPERATION | CXL_MBOX_BACKGROUND_OPERATION_ABORT},
> >      [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = {
> >         "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
> >         cmd_media_get_scan_media_results, 0, 0 },
> >@@ -2214,6 +2245,8 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> >      [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 },
> >      [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { "BACKGROUND_OPERATION_STATUS",
> >         cmd_infostat_bg_op_sts, 0, 0 },
> >+    [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> >+        cmd_infostat_bg_op_abort, 0, 0 },
> >      [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
> >      [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8,
> >                          CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
> >@@ -2295,6 +2328,7 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, >uint8_t cmd,
> >         cci->bg.opcode = (set << 8) | cmd;
> >
> >         cci->bg.complete_pct = 0;
> >+        cci->bg.aborted = false;
> >         cci->bg.ret_code = 0;
> >
> >         now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> >@@ -2386,6 +2420,7 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
> >      cxl_rebuild_cel(cci);
> >
> >      cci->bg.complete_pct = 0;
> >+    cci->bg.aborted = false;
> >      cci->bg.starttime = 0;
> >      cci->bg.runtime = 0;
> >      cci->bg.timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> >diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> >index d38391b26f0e..d83b359e9994 100644
> >--- a/include/hw/cxl/cxl_device.h
> >+++ b/include/hw/cxl/cxl_device.h
> >@@ -197,6 +197,7 @@ typedef struct CXLCCI {
> >      struct {
> >         uint16_t opcode;
> >         uint16_t complete_pct;
> >+        bool aborted;
> >         uint16_t ret_code; /* Current value of retcode */
> >         uint64_t starttime;
> >         /* set by each bg cmd, cleared by the bg_timer when complete */
> >diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
> >index beb048052e1b..9008402d1c46 100644
> >--- a/include/hw/cxl/cxl_mailbox.h
> >+++ b/include/hw/cxl/cxl_mailbox.h
> >@@ -14,5 +14,6 @@
> >  #define CXL_MBOX_IMMEDIATE_LOG_CHANGE (1 << 4)
> >  #define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5)
> >  #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
> >+#define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
> >
> >  #endif
> >--
> >2.44.0  
>     

Re: [EXT] Re: [PATCH] hw/cxl: Add support for abort of background operation

[ajay.opensrc]

>From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>>ajay.opensrc <ajay.opensrc@micron.com> wrote:
>
>> >From: Davidlohr Bueso <dave@stgolabs.net>
>> >>From: Ajay Joshi <ajay.opensrc@micron.com>
>> >>
>> >>This patch adds the support for aborting the background
>> >>operation if one is progress(r3.1 8.2.9.1.5)
>> >>
>> >>"Request Abort Background Operation" command is requested
>> >>by the host to cancel an ongoing background operation. When
>> >>the command is sent, the device will abort the ongoing
>> >>background operation.  When the operation is aborted,
>> >>background command status register will maintain a completion
>> >>percentage value of less then 100. The "Request Abort
>> >>Background Operation" command support has to be advertised in
>> >>the Command Effects Log to be honoured.
>> >
>> >So I had a patch for this which I had not sent out, and compared
>> >to this one I think the below is more robust, would you mind
>> >testing this out?
>>
>> Didn't realize you were working on one already.
>> This looks good to me as well. I tested this and it seems to be
>> working as expected. Feel free to use:
>>
>> Tested-by: Ajay Joshi <ajay.opensrc@micron.com>
>> Reviewed-by: Ajay Joshi <ajay.opensrc@micron.com>
>From a quick look Davidlohr's patch looks good to me as well.
>I 'could' pick it out of the middle of this thread, but
>probably cleaner if Davlidlohr sends a formal patch
>(picking up Ajay's tags) - maybe with a link tag to this
>thread so we can see where those came from.
>
>One comment inline - feels like a spec hole to me but I'd like some
>more eyes on it before I query it more formally.
>
>Thanks,
>
>Jonathan
>
>>
>> >Thanks,
>> >Davidlohr
>> >
>> >--8<--------
>> >[PATCH] hw/cxl: Support aborting background commands
>> >
>> >As of 3.1 spec, background commands can be canceled with a new
>> >abort command. Implement the support, which is advertised in
>> >the CEL. No ad-hoc context undoing is necessary as all the
>> >command logic of the running bg command is done upon completion.
>> >
>> >Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>> >---
>> >  hw/cxl/cxl-device-utils.c    |  2 +-
>> >  hw/cxl/cxl-mailbox-utils.c   | 39 ++++++++++++++++++++++++++++++++++--
>> >  include/hw/cxl/cxl_device.h  |  1 +
>> >  include/hw/cxl/cxl_mailbox.h |  1 +
>> >  4 files changed, 40 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
>> >index 035d034f6dd8..4f4ccfa92d82 100644
>> >--- a/hw/cxl/cxl-device-utils.c
>> >+++ b/hw/cxl/cxl-device-utils.c
>> >@@ -95,7 +95,7 @@ static uint64_t mailbox_reg_read(void *opaque, hwaddr >offset, >unsigned size)
>> >         }
>> >         if (offset == A_CXL_DEV_MAILBOX_STS) {
>> >             uint64_t status_reg = cxl_dstate->mbox_reg_state64[offset / >size];
>> >-            if (cci->bg.complete_pct) {
>> >+            if (cci->bg.complete_pct || cci->bg.aborted) {
>> >                 status_reg = FIELD_DP64(status_reg, CXL_DEV_MAILBOX_STS, >BG_OP,
>> >                                         0);
>> >                 cxl_dstate->mbox_reg_state64[offset / size] = status_reg;
>> >diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> >index 80a80f1ec29b..ae4b3cabbb86 100644
>> >--- a/hw/cxl/cxl-mailbox-utils.c
>> >+++ b/hw/cxl/cxl-mailbox-utils.c
>> >@@ -53,6 +53,7 @@ enum {
>> >      INFOSTAT    = 0x00,
>> >         #define IS_IDENTIFY   0x1
>> >         #define BACKGROUND_OPERATION_STATUS    0x2
>> >+        #define BACKGROUND_OPERATION_ABORT     0x5
>> >      EVENTS      = 0x01,
>> >         #define GET_RECORDS   0x0
>> >         #define CLEAR_RECORDS   0x1
>> >@@ -815,6 +816,33 @@ static CXLRetCode cmd_infostat_bg_op_sts(const struct >>cxl_cmd *cmd,
>> >      return CXL_MBOX_SUCCESS;
>> >  }
>> >
>> >+/* CXL r3.1 Section 8.2.9.1.5: Request Abort Background Operation (Opcode >>0005h) */
>> >+static CXLRetCode cmd_infostat_bg_op_abort(const struct cxl_cmd *cmd,
>> >+                                           uint8_t *payload_in,
>> >+                                           size_t len_in,
>> >+                                           uint8_t *payload_out,
>> >+                                           size_t *len_out,
>> >+                                           CXLCCI *cci)
>> >+{
>> >+    int bg_set = cci->bg.opcode >> 8;
>> >+    int bg_cmd = cci->bg.opcode & 0xff;
>> >+    const struct cxl_cmd *bg_c = &cci->cxl_cmd_set[bg_set][bg_cmd];
>> >+
>> >+    if (!(bg_c->effect & CXL_MBOX_BACKGROUND_OPERATION_ABORT)) {
>> >+        return CXL_MBOX_REQUEST_ABORT_NOTSUP;
>> >+    }
>> >+
>> >+    if (cci->bg.runtime) {
>> >+        assert(cci->bg.complete_pct < 100);
>
>This seems fragile.  I'm not sure what guarantees no races.
>I'd be tempted to follow the allowed path of just letting
>the thing complete if that's the case.  Interesting 8.2.9.1.5
>Request Abort Background operation states
>"In resposne, the device may choose to interrupt he ongoing
>background operation or may choose to continue it's
>execution until completion. If the background operation is
>interrupted, a Command Return of Success shall be returned..
>If the ongoing background operation does not support abort
>capability.. return code shall be set to Request Abort Not supported"
>
>So to my reading if you elect not to stop because it's very
>nearly done, or indeed it raced and the command is done,
>the specification doesn't tell us what to return.
>That operation supports abort, we just didn't.
>
>What do you think should happen here?

IMHO, device should ignore the abort command and 
most likely return "Unsupported" code (since, the spec is not 
clear on what should be the return code in case the command 
is ignored). Does it make sense?

Agree that it's a spec hole and may need to be clarified.

>
>> >+        timer_del(cci->bg.timer);
>> >+        cci->bg.ret_code = CXL_MBOX_ABORTED;
>> >+        cci->bg.starttime = 0;
>> >+        cci->bg.runtime = 0;
>> >+        cci->bg.aborted = true;
>> >+    }
>> >+    return CXL_MBOX_SUCCESS;
>> >+}
>> >+
>> >  /* CXL r3.1 Section 8.2.9.3.1: Get FW Info (Opcode 0200h) */
>> >  static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
>> >                                                uint8_t *payload_in,
>> >@@ -2149,6 +2177,8 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct >>cxl_cmd *cmd,
>> >  }
>> >
>> >  static const struct cxl_cmd cxl_cmd_set[256][256] = {
>> >+    [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
>> >+        cmd_infostat_bg_op_abort, 0, 0 },
>> >      [EVENTS][GET_RECORDS] = { "EVENTS_GET_RECORDS",
>> >         cmd_events_get_records, 1, 0 },
>> >      [EVENTS][CLEAR_RECORDS] = { "EVENTS_CLEAR_RECORDS",
>> >@@ -2176,7 +2206,7 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>> >      [SANITIZE][OVERWRITE] = { "SANITIZE_OVERWRITE", >cmd_sanitize_overwrite, 0,
>> >         (CXL_MBOX_IMMEDIATE_DATA_CHANGE |
>> >          CXL_MBOX_SECURITY_STATE_CHANGE |
>> >-         CXL_MBOX_BACKGROUND_OPERATION)},
>> >+         CXL_MBOX_BACKGROUND_OPERATION  | >>CXL_MBOX_BACKGROUND_OPERATION_ABORT)},
>> >      [PERSISTENT_MEM][GET_SECURITY_STATE] = { "GET_SECURITY_STATE",
>> >         cmd_get_security_state, 0, 0 },
>> >      [MEDIA_AND_POISON][GET_POISON_LIST] = { >>"MEDIA_AND_POISON_GET_POISON_LIST",
>> >@@ -2189,7 +2219,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>> >         "MEDIA_AND_POISON_GET_SCAN_MEDIA_CAPABILITIES",
>> >         cmd_media_get_scan_media_capabilities, 16, 0 },
>> >      [MEDIA_AND_POISON][SCAN_MEDIA] = { "MEDIA_AND_POISON_SCAN_MEDIA",
>> >-        cmd_media_scan_media, 17, CXL_MBOX_BACKGROUND_OPERATION },
>> >+        cmd_media_scan_media, 17,
>> >+        CXL_MBOX_BACKGROUND_OPERATION | >CXL_MBOX_BACKGROUND_OPERATION_ABORT},
>> >      [MEDIA_AND_POISON][GET_SCAN_MEDIA_RESULTS] = {
>> >         "MEDIA_AND_POISON_GET_SCAN_MEDIA_RESULTS",
>> >         cmd_media_get_scan_media_results, 0, 0 },
>> >@@ -2214,6 +2245,8 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256] = >{
>> >      [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 0 },
>> >      [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { >"BACKGROUND_OPERATION_STATUS",
>> >         cmd_infostat_bg_op_sts, 0, 0 },
>> >+    [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
>> >+        cmd_infostat_bg_op_abort, 0, 0 },
>> >      [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
>> >      [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8,
>> >                          CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
>> >@@ -2295,6 +2328,7 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, >>uint8_t cmd,
>> >         cci->bg.opcode = (set << 8) | cmd;
>> >
>> >         cci->bg.complete_pct = 0;
>> >+        cci->bg.aborted = false;
>> >         cci->bg.ret_code = 0;
>> >
>> >         now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
>> >@@ -2386,6 +2420,7 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
>> >      cxl_rebuild_cel(cci);
>> >
>> >      cci->bg.complete_pct = 0;
>> >+    cci->bg.aborted = false;
>> >      cci->bg.starttime = 0;
>> >      cci->bg.runtime = 0;
>> >      cci->bg.timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
>> >diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
>> >index d38391b26f0e..d83b359e9994 100644
>> >--- a/include/hw/cxl/cxl_device.h
>> >+++ b/include/hw/cxl/cxl_device.h
>> >@@ -197,6 +197,7 @@ typedef struct CXLCCI {
>> >      struct {
>> >         uint16_t opcode;
>> >         uint16_t complete_pct;
>> >+        bool aborted;
>> >         uint16_t ret_code; /* Current value of retcode */
>> >         uint64_t starttime;
>> >         /* set by each bg cmd, cleared by the bg_timer when complete */
>> >diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
>> >index beb048052e1b..9008402d1c46 100644
>> >--- a/include/hw/cxl/cxl_mailbox.h
>> >+++ b/include/hw/cxl/cxl_mailbox.h
>> >@@ -14,5 +14,6 @@
>> >  #define CXL_MBOX_IMMEDIATE_LOG_CHANGE (1 << 4)
>> >  #define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5)
>> >  #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
>> >+#define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
>> >
>> >  #endif
>> >--
>> >2.44.0
>>

Re: [EXT] Re: [PATCH] hw/cxl: Add support for abort of background operation

[Davidlohr Bueso]

On Wed, 07 Aug 2024, ajay.opensrc wrote:\n
>>From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>>So to my reading if you elect not to stop because it's very
>>nearly done, or indeed it raced and the command is done,
>>the specification doesn't tell us what to return.
>>That operation supports abort, we just didn't.
>>
>>What do you think should happen here?

I had interpreted this case to return success for the req abort
command, but the caller had to check the Mailbox Status register
to see if result had actually canceled the on going bg op.
And I don't think there's a race between checking the status
register and the on-going bg command completing. Aborting
means percentage complete < 100 && bg field cleared.

Jonathan, would you want the patch to have an arbitrary "don't
abort the bg op over 85% done"? I see this useful as it allows
the cancel a bit more versatility, albeit potentially getting
in the way of testing. What do you think?

>
>IMHO, device should ignore the abort command and
>most likely return "Unsupported" code (since, the spec is not
>clear on what should be the return code in case the command
>is ignored). Does it make sense?

I don't think so. This is not an error imo. This case is about
supporting the request abort command, but the actual command
not actually triggering the desired cancel.

>
>Agree that it's a spec hole and may need to be clarified.

Yes, it would be nice to have this case be explicitly stated.
And it is not even a corner case, the way the spec describes
it, this case can perfectly happen.

Thanks,
Davidlohr

Re: [EXT] Re: [PATCH] hw/cxl: Add support for abort of background operation

[Jonathan Cameron]

On Mon, 12 Aug 2024 13:04:36 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Wed, 07 Aug 2024, ajay.opensrc wrote:\n
> >>From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> >>So to my reading if you elect not to stop because it's very
> >>nearly done, or indeed it raced and the command is done,
> >>the specification doesn't tell us what to return.
> >>That operation supports abort, we just didn't.
> >>
> >>What do you think should happen here?  
> 
> I had interpreted this case to return success for the req abort
> command, but the caller had to check the Mailbox Status register
> to see if result had actually canceled the on going bg op.
> And I don't think there's a race between checking the status
> register and the on-going bg command completing. Aborting
> means percentage complete < 100 && bg field cleared.
I'm fine with it returning success, but I'm didn't read the spec
as actually saying it would. Maybe I missed something as these
cases where a call is a noop are classic corner cases where it
might return
a) you are crazy, why did you call that?
b) sure I'll do nothing.

> 
> Jonathan, would you want the patch to have an arbitrary "don't
> abort the bg op over 85% done"? I see this useful as it allows
> the cancel a bit more versatility, albeit potentially getting
> in the way of testing. What do you think?
I'd not bother - I think we will cancel very rarely in reality.
This is mostly a route around possible really long delays, not
a tool for normal operation.
> 
> >
> >IMHO, device should ignore the abort command and
> >most likely return "Unsupported" code (since, the spec is not
> >clear on what should be the return code in case the command
> >is ignored). Does it make sense?  
> 
> I don't think so. This is not an error imo. This case is about
> supporting the request abort command, but the actual command
> not actually triggering the desired cancel.
> 
> >
> >Agree that it's a spec hole and may need to be clarified.  
> 
> Yes, it would be nice to have this case be explicitly stated.
> And it is not even a corner case, the way the spec describes
> it, this case can perfectly happen.
Absolutely. I'll poke the relevant person.,

Jonathan

> 
> Thanks,
> Davidlohr

Re: [EXT] Re: [PATCH] hw/cxl: Add support for abort of background operation

[Jonathan Cameron]

On Thu, 15 Aug 2024 18:04:03 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon, 12 Aug 2024 13:04:36 -0700
> Davidlohr Bueso <dave@stgolabs.net> wrote:
> 
> > On Wed, 07 Aug 2024, ajay.opensrc wrote:\n  
> > >>From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > >>So to my reading if you elect not to stop because it's very
> > >>nearly done, or indeed it raced and the command is done,
> > >>the specification doesn't tell us what to return.
> > >>That operation supports abort, we just didn't.
> > >>
> > >>What do you think should happen here?    
> > 
> > I had interpreted this case to return success for the req abort
> > command, but the caller had to check the Mailbox Status register
> > to see if result had actually canceled the on going bg op.
> > And I don't think there's a race between checking the status
> > register and the on-going bg command completing. Aborting
> > means percentage complete < 100 && bg field cleared.  
> I'm fine with it returning success, but I'm didn't read the spec
> as actually saying it would. Maybe I missed something as these
> cases where a call is a noop are classic corner cases where it
> might return
> a) you are crazy, why did you call that?
> b) sure I'll do nothing.

Clarification received was indeed that the device should return
success to a BG command abort if there isn't one to abort.

So that's easy :)

Jonathan
> 
> > 
> > Jonathan, would you want the patch to have an arbitrary "don't
> > abort the bg op over 85% done"? I see this useful as it allows
> > the cancel a bit more versatility, albeit potentially getting
> > in the way of testing. What do you think?  
> I'd not bother - I think we will cancel very rarely in reality.
> This is mostly a route around possible really long delays, not
> a tool for normal operation.
> >   
> > >
> > >IMHO, device should ignore the abort command and
> > >most likely return "Unsupported" code (since, the spec is not
> > >clear on what should be the return code in case the command
> > >is ignored). Does it make sense?    
> > 
> > I don't think so. This is not an error imo. This case is about
> > supporting the request abort command, but the actual command
> > not actually triggering the desired cancel.
> >   
> > >
> > >Agree that it's a spec hole and may need to be clarified.    
> > 
> > Yes, it would be nice to have this case be explicitly stated.
> > And it is not even a corner case, the way the spec describes
> > it, this case can perfectly happen.  
> Absolutely. I'll poke the relevant person.,
> 
> Jonathan
> 
> > 
> > Thanks,
> > Davidlohr  
> 
>