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

[ajay.opensrc <ajay.opensrc@micron.com>]

>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
>>