[PATCH -qemu 0/2] hw/cxl: Firmware Update support

[PATCH -qemu 0/2] hw/cxl: Firmware Update support

[Davidlohr Bueso]

Hello,

The following implements the remaining commands for emulating the cxl
firmware update operations, similar to what the mock device does.

# cxl update-firmware mem0 
  ...
  "firmware":{
    "num_slots":2,
    "active_slot":1,
    "staged_slot":1,
    "online_activate_capable":true,
    "slot_1_version":"BWFW VERSION 0",
    "fw_update_in_progress":false
  }
}

# cxl update-firmware mem0 -F ~/fw.bin -w
{
  ...
  "firmware":{
    "num_slots":2,
    "active_slot":1,
    "staged_slot":2,
    "online_activate_capable":true,
    "slot_1_version":"BWFW VERSION 0",
    "slot_2_version":"BWFW VERSION 1",
    "fw_update_in_progress":false
  }
}

One note is that this implementation will optimistically set the new firmware
before the successful completion of a full or end transfer, for example:

  "firmware":{
    "num_slots":2,
    "active_slot":1,
    "staged_slot":2,
    "online_activate_capable":true,
    "slot_1_version":"BWFW VERSION 0",
    "slot_2_version":"BWFW VERSION 1",
    "fw_update_in_progress":true,
    "remaining_size":5091200
  }

Applies against Jonathan's latst branch:
	https://gitlab.com/jic23/qemu/-/tree/cxl-2023-11-02

Thanks!

Davidlohr Bueso (2):
  hw/cxl: Add Transfer FW support
  hw/cxl: Add Activate FW support

 hw/cxl/cxl-mailbox-utils.c  | 179 +++++++++++++++++++++++++++++++++++-
 include/hw/cxl/cxl_device.h |   9 ++
 2 files changed, 183 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH 1/2] hw/cxl: Add Transfer FW support

[Davidlohr Bueso]

Per the latest 3.1 spec for supporting firmware update metadata
(no actual buffers). Aborting a xfer is currently unsupported
through a nop.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 hw/cxl/cxl-mailbox-utils.c  | 124 ++++++++++++++++++++++++++++++++++--
 include/hw/cxl/cxl_device.h |   9 +++
 2 files changed, 129 insertions(+), 4 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 11ec8b648baf..0295ea8b29aa 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -60,6 +60,7 @@ enum {
         #define SET_INTERRUPT_POLICY   0x3
     FIRMWARE_UPDATE = 0x02,
         #define GET_INFO      0x0
+        #define TRANSFER      0x1
     TIMESTAMP   = 0x03,
         #define GET           0x0
         #define SET           0x1
@@ -815,6 +816,8 @@ static CXLRetCode cmd_infostat_bg_op_sts(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+#define CXL_FW_SLOTS 2
+
 /* CXL r3.0 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,
@@ -846,15 +849,118 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
     fw_info = (void *)payload_out;
     memset(fw_info, 0, sizeof(*fw_info));
 
-    fw_info->slots_supported = 2;
-    fw_info->slot_info = BIT(0) | BIT(3);
+    fw_info->slots_supported = CXL_FW_SLOTS;
+    fw_info->slot_info = (cci->fw.active_slot & 0x7) |
+            ((cci->fw.staged_slot & 0x7) << 3);
     fw_info->caps = 0;
-    pstrcpy(fw_info->fw_rev1, sizeof(fw_info->fw_rev1), "BWFW VERSION 0");
+
+    if (cci->fw.slot[0]) {
+        pstrcpy(fw_info->fw_rev1, sizeof(fw_info->fw_rev1), "BWFW VERSION 0");
+    }
+    if (cci->fw.slot[1]) {
+        pstrcpy(fw_info->fw_rev2, sizeof(fw_info->fw_rev2), "BWFW VERSION 1");
+    }
+    if (cci->fw.slot[2]) {
+        pstrcpy(fw_info->fw_rev3, sizeof(fw_info->fw_rev3), "BWFW VERSION 2");
+    }
+    if (cci->fw.slot[3]) {
+        pstrcpy(fw_info->fw_rev4, sizeof(fw_info->fw_rev4), "BWFW VERSION 3");
+    }
 
     *len_out = sizeof(*fw_info);
     return CXL_MBOX_SUCCESS;
 }
 
+/* CXL r3.1 section 8.2.9.3.2: Transfer FW (Opcode 0201h) */
+#define CXL_FW_XFER_ALIGNMENT   128
+
+#define CXL_FW_XFER_ACTION_FULL	    0x0
+#define CXL_FW_XFER_ACTION_INIT	    0x1
+#define CXL_FW_XFER_ACTION_CONTINUE 0x2
+#define CXL_FW_XFER_ACTION_END	    0x3
+#define CXL_FW_XFER_ACTION_ABORT    0x4
+
+#define CXL_FW_SIZE  0x02000000 /* 32 mb */
+
+static CXLRetCode cmd_firmware_update_transfer(const struct cxl_cmd *cmd,
+                                               uint8_t *payload_in,
+                                               size_t len,
+                                               uint8_t *payload_out,
+                                               size_t *len_out,
+                                               CXLCCI *cci)
+{
+    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate;
+    struct {
+        uint8_t action;
+        uint8_t slot;
+        uint8_t caps;
+        uint8_t rsvd1[2];
+        uint32_t offset;
+        uint8_t rsvd2[0x78];
+        uint8_t data[];
+    } QEMU_PACKED *fw_transfer;
+    size_t offset, length;
+
+    if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
+        (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    fw_transfer = (void *)payload_in;
+
+    if (cci->fw.transfering &&
+        (fw_transfer->action == CXL_FW_XFER_ACTION_FULL ||
+         fw_transfer->action == CXL_FW_XFER_ACTION_INIT)) {
+        return CXL_MBOX_FW_XFER_IN_PROGRESS;
+    }
+
+    offset = fw_transfer->offset * CXL_FW_XFER_ALIGNMENT;
+    length = len - sizeof(*fw_transfer);
+    if (offset + length > CXL_FW_SIZE) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    switch (fw_transfer->action) {
+    case CXL_FW_XFER_ACTION_FULL: /* ignores offset */
+    case CXL_FW_XFER_ACTION_END:
+        if (fw_transfer->slot == 0 ||
+            fw_transfer->slot == cci->fw.active_slot ||
+            fw_transfer->slot > CXL_FW_SLOTS) {
+            return CXL_MBOX_FW_INVALID_SLOT;
+        }
+        /*
+         * Optimistically mark the slot used now (as opposed
+         * to at the end of the successful transfer). That
+         * way we don't need to keep command context.
+         */
+        cci->fw.slot[fw_transfer->slot - 1] = true;
+        break;
+    case CXL_FW_XFER_ACTION_INIT:
+        if (offset != 0) {
+            return CXL_MBOX_INVALID_INPUT;
+        }
+        /* fallthrough */
+    case CXL_FW_XFER_ACTION_CONTINUE:
+        break;
+    case CXL_FW_XFER_ACTION_ABORT: /* nop */
+        return CXL_MBOX_SUCCESS;
+    default:
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    cci->fw.transfering = true;
+
+    cci->bg.runtime = 2 * 1000UL;
+    *len_out = 0;
+
+    return CXL_MBOX_BG_STARTED;
+}
+
+static void __do_firmware_xfer(CXLCCI *cci)
+{
+    cci->fw.transfering = false;
+}
+
 /* CXL r3.0 Section 8.2.9.4.1: Get Timestamp (Opcode 0300h) */
 static CXLRetCode cmd_timestamp_get(const struct cxl_cmd *cmd,
                                     uint8_t *payload_in,
@@ -2165,6 +2271,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
                                       ~0, CXL_MBOX_IMMEDIATE_CONFIG_CHANGE },
     [FIRMWARE_UPDATE][GET_INFO] = { "FIRMWARE_UPDATE_GET_INFO",
         cmd_firmware_update_get_info, 0, 0 },
+    [FIRMWARE_UPDATE][TRANSFER] = { "FIRMWARE_UPDATE_TRANSFER",
+        cmd_firmware_update_transfer, ~0, CXL_MBOX_BACKGROUND_OPERATION },
     [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
     [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set,
                          8, CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
@@ -2278,7 +2386,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
             h == cmd_media_get_poison_list ||
             h == cmd_media_inject_poison ||
             h == cmd_media_clear_poison ||
-            h == cmd_sanitize_overwrite) {
+            h == cmd_sanitize_overwrite ||
+            h == cmd_firmware_update_transfer) {
             return CXL_MBOX_MEDIA_DISABLED;
         }
     }
@@ -2325,6 +2434,9 @@ static void bg_timercb(void *opaque)
             CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
 
             switch (cci->bg.opcode) {
+            case 0x0201: /* fw transfer */
+                __do_firmware_xfer(cci);
+                break;
             case 0x4400: /* sanitize */
                 __do_sanitization(ct3d);
                 cxl_dev_enable_media(&ct3d->cxl_dstate);
@@ -2388,6 +2500,10 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
     cci->payload_max = payload_max;
     cxl_rebuild_cel(cci);
 
+    cci->fw.active_slot = cci->fw.staged_slot = 1;
+    memset(cci->fw.slot, 0, sizeof(cci->fw.slot));
+    cci->fw.slot[cci->fw.active_slot - 1] = true;
+
     cci->bg.complete_pct = 0;
     cci->bg.starttime = 0;
     cci->bg.runtime = 0;
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index b2cb280e1631..b4373bfe4d2a 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -201,6 +201,15 @@ typedef struct CXLCCI {
         uint64_t runtime;
         QEMUTimer *timer;
     } bg;
+
+    /* firmware update */
+    struct {
+        bool transfering;
+        int active_slot;
+        int staged_slot;
+        bool slot[4];
+    } fw;
+
     size_t payload_max;
     /* Pointer to device hosting the CCI */
     DeviceState *d;
-- 
2.43.0

Re: [PATCH 1/2] hw/cxl: Add Transfer FW support

[fan]

On Mon, Jan 08, 2024 at 11:04:35PM -0800, Davidlohr Bueso wrote:
> Per the latest 3.1 spec for supporting firmware update metadata
> (no actual buffers). Aborting a xfer is currently unsupported
> through a nop.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  hw/cxl/cxl-mailbox-utils.c  | 124 ++++++++++++++++++++++++++++++++++--
>  include/hw/cxl/cxl_device.h |   9 +++
>  2 files changed, 129 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 11ec8b648baf..0295ea8b29aa 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -60,6 +60,7 @@ enum {
>          #define SET_INTERRUPT_POLICY   0x3
>      FIRMWARE_UPDATE = 0x02,
>          #define GET_INFO      0x0
> +        #define TRANSFER      0x1
>      TIMESTAMP   = 0x03,
>          #define GET           0x0
>          #define SET           0x1
> @@ -815,6 +816,8 @@ static CXLRetCode cmd_infostat_bg_op_sts(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +#define CXL_FW_SLOTS 2
> +
>  /* CXL r3.0 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,
> @@ -846,15 +849,118 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
>      fw_info = (void *)payload_out;
>      memset(fw_info, 0, sizeof(*fw_info));
>  
> -    fw_info->slots_supported = 2;
> -    fw_info->slot_info = BIT(0) | BIT(3);
> +    fw_info->slots_supported = CXL_FW_SLOTS;
> +    fw_info->slot_info = (cci->fw.active_slot & 0x7) |
> +            ((cci->fw.staged_slot & 0x7) << 3);
>      fw_info->caps = 0;
> -    pstrcpy(fw_info->fw_rev1, sizeof(fw_info->fw_rev1), "BWFW VERSION 0");
> +
> +    if (cci->fw.slot[0]) {
> +        pstrcpy(fw_info->fw_rev1, sizeof(fw_info->fw_rev1), "BWFW VERSION 0");
> +    }
> +    if (cci->fw.slot[1]) {
> +        pstrcpy(fw_info->fw_rev2, sizeof(fw_info->fw_rev2), "BWFW VERSION 1");
> +    }
> +    if (cci->fw.slot[2]) {
> +        pstrcpy(fw_info->fw_rev3, sizeof(fw_info->fw_rev3), "BWFW VERSION 2");
> +    }
> +    if (cci->fw.slot[3]) {
> +        pstrcpy(fw_info->fw_rev4, sizeof(fw_info->fw_rev4), "BWFW VERSION 3");
> +    }
>  
>      *len_out = sizeof(*fw_info);
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +/* CXL r3.1 section 8.2.9.3.2: Transfer FW (Opcode 0201h) */
> +#define CXL_FW_XFER_ALIGNMENT   128
> +
> +#define CXL_FW_XFER_ACTION_FULL	    0x0
> +#define CXL_FW_XFER_ACTION_INIT	    0x1
> +#define CXL_FW_XFER_ACTION_CONTINUE 0x2
> +#define CXL_FW_XFER_ACTION_END	    0x3
> +#define CXL_FW_XFER_ACTION_ABORT    0x4
> +
> +#define CXL_FW_SIZE  0x02000000 /* 32 mb */
> +
> +static CXLRetCode cmd_firmware_update_transfer(const struct cxl_cmd *cmd,
> +                                               uint8_t *payload_in,
> +                                               size_t len,
> +                                               uint8_t *payload_out,
> +                                               size_t *len_out,
> +                                               CXLCCI *cci)
> +{
> +    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate;
> +    struct {
> +        uint8_t action;
> +        uint8_t slot;
> +        uint8_t caps;
> +        uint8_t rsvd1[2];
> +        uint32_t offset;
> +        uint8_t rsvd2[0x78];
> +        uint8_t data[];
> +    } QEMU_PACKED *fw_transfer;
> +    size_t offset, length;
> +
> +    if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
> +        (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {
> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    fw_transfer = (void *)payload_in;
> +
> +    if (cci->fw.transfering &&
> +        (fw_transfer->action == CXL_FW_XFER_ACTION_FULL ||
> +         fw_transfer->action == CXL_FW_XFER_ACTION_INIT)) {
> +        return CXL_MBOX_FW_XFER_IN_PROGRESS;
> +    }
> +
> +    offset = fw_transfer->offset * CXL_FW_XFER_ALIGNMENT;
> +    length = len - sizeof(*fw_transfer);
> +    if (offset + length > CXL_FW_SIZE) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    switch (fw_transfer->action) {
> +    case CXL_FW_XFER_ACTION_FULL: /* ignores offset */
> +    case CXL_FW_XFER_ACTION_END:
> +        if (fw_transfer->slot == 0 ||
> +            fw_transfer->slot == cci->fw.active_slot ||
> +            fw_transfer->slot > CXL_FW_SLOTS) {
Should here be >= instead of > ?
> +            return CXL_MBOX_FW_INVALID_SLOT;
> +        }
> +        /*
> +         * Optimistically mark the slot used now (as opposed
> +         * to at the end of the successful transfer). That
> +         * way we don't need to keep command context.
> +         */
> +        cci->fw.slot[fw_transfer->slot - 1] = true;
> +        break;
> +    case CXL_FW_XFER_ACTION_INIT:
> +        if (offset != 0) {
> +            return CXL_MBOX_INVALID_INPUT;
> +        }
> +        /* fallthrough */
> +    case CXL_FW_XFER_ACTION_CONTINUE:
> +        break;
> +    case CXL_FW_XFER_ACTION_ABORT: /* nop */
> +        return CXL_MBOX_SUCCESS;
> +    default:
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    cci->fw.transfering = true;
> +
> +    cci->bg.runtime = 2 * 1000UL;

How we get this? I see the spec mentioned the timeout between FW package ports
is 30 seconds.

Fan
> +    *len_out = 0;
> +
> +    return CXL_MBOX_BG_STARTED;
> +}
> +
> +static void __do_firmware_xfer(CXLCCI *cci)
> +{
> +    cci->fw.transfering = false;
> +}
> +
>  /* CXL r3.0 Section 8.2.9.4.1: Get Timestamp (Opcode 0300h) */
>  static CXLRetCode cmd_timestamp_get(const struct cxl_cmd *cmd,
>                                      uint8_t *payload_in,
> @@ -2165,6 +2271,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>                                        ~0, CXL_MBOX_IMMEDIATE_CONFIG_CHANGE },
>      [FIRMWARE_UPDATE][GET_INFO] = { "FIRMWARE_UPDATE_GET_INFO",
>          cmd_firmware_update_get_info, 0, 0 },
> +    [FIRMWARE_UPDATE][TRANSFER] = { "FIRMWARE_UPDATE_TRANSFER",
> +        cmd_firmware_update_transfer, ~0, CXL_MBOX_BACKGROUND_OPERATION },
>      [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
>      [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set,
>                           8, CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
> @@ -2278,7 +2386,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
>              h == cmd_media_get_poison_list ||
>              h == cmd_media_inject_poison ||
>              h == cmd_media_clear_poison ||
> -            h == cmd_sanitize_overwrite) {
> +            h == cmd_sanitize_overwrite ||
> +            h == cmd_firmware_update_transfer) {
>              return CXL_MBOX_MEDIA_DISABLED;
>          }
>      }
> @@ -2325,6 +2434,9 @@ static void bg_timercb(void *opaque)
>              CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
>  
>              switch (cci->bg.opcode) {
> +            case 0x0201: /* fw transfer */
> +                __do_firmware_xfer(cci);
> +                break;
>              case 0x4400: /* sanitize */
>                  __do_sanitization(ct3d);
>                  cxl_dev_enable_media(&ct3d->cxl_dstate);
> @@ -2388,6 +2500,10 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
>      cci->payload_max = payload_max;
>      cxl_rebuild_cel(cci);
>  
> +    cci->fw.active_slot = cci->fw.staged_slot = 1;
> +    memset(cci->fw.slot, 0, sizeof(cci->fw.slot));
> +    cci->fw.slot[cci->fw.active_slot - 1] = true;
> +
>      cci->bg.complete_pct = 0;
>      cci->bg.starttime = 0;
>      cci->bg.runtime = 0;
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index b2cb280e1631..b4373bfe4d2a 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -201,6 +201,15 @@ typedef struct CXLCCI {
>          uint64_t runtime;
>          QEMUTimer *timer;
>      } bg;
> +
> +    /* firmware update */
> +    struct {
> +        bool transfering;
> +        int active_slot;
> +        int staged_slot;
> +        bool slot[4];
> +    } fw;
> +
>      size_t payload_max;
>      /* Pointer to device hosting the CCI */
>      DeviceState *d;
> -- 
> 2.43.0
> 

Re: [PATCH 1/2] hw/cxl: Add Transfer FW support

[Davidlohr Bueso]

On Tue, 09 Jan 2024, fan wrote:

>> +    switch (fw_transfer->action) {
>> +    case CXL_FW_XFER_ACTION_FULL: /* ignores offset */
>> +    case CXL_FW_XFER_ACTION_END:
>> +        if (fw_transfer->slot == 0 ||
>> +            fw_transfer->slot == cci->fw.active_slot ||
>> +            fw_transfer->slot > CXL_FW_SLOTS) {
>Should here be >= instead of > ?

No because the input starts from slot 1.

>> +            return CXL_MBOX_FW_INVALID_SLOT;
>> +        }
>> +        /*
>> +         * Optimistically mark the slot used now (as opposed
>> +         * to at the end of the successful transfer). That
>> +         * way we don't need to keep command context.
>> +         */
>> +        cci->fw.slot[fw_transfer->slot - 1] = true;
>> +        break;
>> +    case CXL_FW_XFER_ACTION_INIT:
>> +        if (offset != 0) {
>> +            return CXL_MBOX_INVALID_INPUT;
>> +        }
>> +        /* fallthrough */
>> +    case CXL_FW_XFER_ACTION_CONTINUE:
>> +        break;
>> +    case CXL_FW_XFER_ACTION_ABORT: /* nop */
>> +        return CXL_MBOX_SUCCESS;
>> +    default:
>> +        return CXL_MBOX_INVALID_INPUT;
>> +    }
>> +
>> +    cci->fw.transfering = true;
>> +
>> +    cci->bg.runtime = 2 * 1000UL;
>
>How we get this? I see the spec mentioned the timeout between FW package ports
>is 30 seconds.

Well 30 secs would be the max, so that 2 seconds is just an arbitrary time
I came up with.

Thanks,
Davidlohr

Re: [PATCH 1/2] hw/cxl: Add Transfer FW support

[Jonathan Cameron]

On Mon,  8 Jan 2024 23:04:35 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Per the latest 3.1 spec for supporting firmware update metadata
> (no actual buffers). Aborting a xfer is currently unsupported
> through a nop.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Hi Davidlohr.

Whilst we may never notice from the OS as we expect it to send valid
sets of transfers only, I'd like the emulation to incorporate more
of the validity checks the spec requires such as:
- Multiple part transfers must all be for same slot - any interleaved
  ones for other slots result in error.
- Retry of previous transfer or one that starts where previous transfer
  ended - otherwise Out of Order detected.

So we have to hold state, and as such will have the information to
set the slot as occupied with just one extra flag stored to say it is
a FULL or END message.

Otherwise lgtm.

Thanks,

Jonathan


> ---
>  hw/cxl/cxl-mailbox-utils.c  | 124 ++++++++++++++++++++++++++++++++++--
>  include/hw/cxl/cxl_device.h |   9 +++
>  2 files changed, 129 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 11ec8b648baf..0295ea8b29aa 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -60,6 +60,7 @@ enum {
>          #define SET_INTERRUPT_POLICY   0x3
>      FIRMWARE_UPDATE = 0x02,
>          #define GET_INFO      0x0
> +        #define TRANSFER      0x1
>      TIMESTAMP   = 0x03,
>          #define GET           0x0
>          #define SET           0x1
> @@ -815,6 +816,8 @@ static CXLRetCode cmd_infostat_bg_op_sts(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +#define CXL_FW_SLOTS 2
> +
>  /* CXL r3.0 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,
> @@ -846,15 +849,118 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
>      fw_info = (void *)payload_out;
>      memset(fw_info, 0, sizeof(*fw_info));
>  
> -    fw_info->slots_supported = 2;
> -    fw_info->slot_info = BIT(0) | BIT(3);
> +    fw_info->slots_supported = CXL_FW_SLOTS;
> +    fw_info->slot_info = (cci->fw.active_slot & 0x7) |
> +            ((cci->fw.staged_slot & 0x7) << 3);
>      fw_info->caps = 0;
> -    pstrcpy(fw_info->fw_rev1, sizeof(fw_info->fw_rev1), "BWFW VERSION 0");
> +
> +    if (cci->fw.slot[0]) {
> +        pstrcpy(fw_info->fw_rev1, sizeof(fw_info->fw_rev1), "BWFW VERSION 0");
> +    }
> +    if (cci->fw.slot[1]) {
> +        pstrcpy(fw_info->fw_rev2, sizeof(fw_info->fw_rev2), "BWFW VERSION 1");
> +    }
> +    if (cci->fw.slot[2]) {

Only support 2 slots... So could skip these 2.

> +        pstrcpy(fw_info->fw_rev3, sizeof(fw_info->fw_rev3), "BWFW VERSION 2");
> +    }
> +    if (cci->fw.slot[3]) {
> +        pstrcpy(fw_info->fw_rev4, sizeof(fw_info->fw_rev4), "BWFW VERSION 3");
> +    }
>  
>      *len_out = sizeof(*fw_info);
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +/* CXL r3.1 section 8.2.9.3.2: Transfer FW (Opcode 0201h) */
> +#define CXL_FW_XFER_ALIGNMENT   128
> +
> +#define CXL_FW_XFER_ACTION_FULL	    0x0
> +#define CXL_FW_XFER_ACTION_INIT	    0x1
> +#define CXL_FW_XFER_ACTION_CONTINUE 0x2
> +#define CXL_FW_XFER_ACTION_END	    0x3
> +#define CXL_FW_XFER_ACTION_ABORT    0x4
> +
> +#define CXL_FW_SIZE  0x02000000 /* 32 mb */
> +
> +static CXLRetCode cmd_firmware_update_transfer(const struct cxl_cmd *cmd,
> +                                               uint8_t *payload_in,
> +                                               size_t len,
> +                                               uint8_t *payload_out,
> +                                               size_t *len_out,
> +                                               CXLCCI *cci)
> +{
> +    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate;
> +    struct {
> +        uint8_t action;
> +        uint8_t slot;
> +        uint8_t caps;
> +        uint8_t rsvd1[2];
> +        uint32_t offset;
> +        uint8_t rsvd2[0x78];
> +        uint8_t data[];
> +    } QEMU_PACKED *fw_transfer;
> +    size_t offset, length;
> +
> +    if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
> +        (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {
> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    fw_transfer = (void *)payload_in;
> +
> +    if (cci->fw.transfering &&
> +        (fw_transfer->action == CXL_FW_XFER_ACTION_FULL ||
> +         fw_transfer->action == CXL_FW_XFER_ACTION_INIT)) {
> +        return CXL_MBOX_FW_XFER_IN_PROGRESS;
Need to return this also if the slot number is different from
an earlier transfer.

> +    }
> +
> +    offset = fw_transfer->offset * CXL_FW_XFER_ALIGNMENT;

If not INIT or FULL, we need to stash the end of previous
transfer and make sure this comes next as otherwise should
return FW Transfer Out of Order.
For extra fun we also need to stash the previous transfer
parameters to allow for 'Back-to-back retransmission of any FW
package part is permitted during a transfer.' in 8.2.9.3.2 or r3.1

So minimum needed across multiple calls is length + end address of
previous transfer (so we can find the start or the end).


> +    length = len - sizeof(*fw_transfer);
> +    if (offset + length > CXL_FW_SIZE) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    switch (fw_transfer->action) {
> +    case CXL_FW_XFER_ACTION_FULL: /* ignores offset */
> +    case CXL_FW_XFER_ACTION_END:
> +        if (fw_transfer->slot == 0 ||
> +            fw_transfer->slot == cci->fw.active_slot ||
> +            fw_transfer->slot > CXL_FW_SLOTS) {
> +            return CXL_MBOX_FW_INVALID_SLOT;
> +        }
> +        /*
> +         * Optimistically mark the slot used now (as opposed
> +         * to at the end of the successful transfer). That
> +         * way we don't need to keep command context.

Hmm. I doubt the OS will even notice this, but it's not ideal.
Given for ordering checks we have to store a bunch of stuff
is it really that bad to store the slot as well?

> +         */
> +        cci->fw.slot[fw_transfer->slot - 1] = true;
> +        break;
> +    case CXL_FW_XFER_ACTION_INIT:
> +        if (offset != 0) {
> +            return CXL_MBOX_INVALID_INPUT;
> +        }
> +        /* fallthrough */
> +    case CXL_FW_XFER_ACTION_CONTINUE:
> +        break;
> +    case CXL_FW_XFER_ACTION_ABORT: /* nop */
> +        return CXL_MBOX_SUCCESS;
> +    default:
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    cci->fw.transfering = true;
> +
> +    cci->bg.runtime = 2 * 1000UL;
> +    *len_out = 0;
> +
> +    return CXL_MBOX_BG_STARTED;
> +}
> +
> +static void __do_firmware_xfer(CXLCCI *cci)
> +{
> +    cci->fw.transfering = false;
> +}
> +
>  /* CXL r3.0 Section 8.2.9.4.1: Get Timestamp (Opcode 0300h) */
>  static CXLRetCode cmd_timestamp_get(const struct cxl_cmd *cmd,
>                                      uint8_t *payload_in,
> @@ -2165,6 +2271,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>                                        ~0, CXL_MBOX_IMMEDIATE_CONFIG_CHANGE },
>      [FIRMWARE_UPDATE][GET_INFO] = { "FIRMWARE_UPDATE_GET_INFO",
>          cmd_firmware_update_get_info, 0, 0 },
> +    [FIRMWARE_UPDATE][TRANSFER] = { "FIRMWARE_UPDATE_TRANSFER",
> +        cmd_firmware_update_transfer, ~0, CXL_MBOX_BACKGROUND_OPERATION },
>      [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
>      [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set,
>                           8, CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
> @@ -2278,7 +2386,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
>              h == cmd_media_get_poison_list ||
>              h == cmd_media_inject_poison ||
>              h == cmd_media_clear_poison ||
> -            h == cmd_sanitize_overwrite) {
> +            h == cmd_sanitize_overwrite ||
> +            h == cmd_firmware_update_transfer) {
>              return CXL_MBOX_MEDIA_DISABLED;
>          }
>      }
> @@ -2325,6 +2434,9 @@ static void bg_timercb(void *opaque)
>              CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
>  
>              switch (cci->bg.opcode) {
> +            case 0x0201: /* fw transfer */
> +                __do_firmware_xfer(cci);
> +                break;
>              case 0x4400: /* sanitize */
>                  __do_sanitization(ct3d);
>                  cxl_dev_enable_media(&ct3d->cxl_dstate);
> @@ -2388,6 +2500,10 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
>      cci->payload_max = payload_max;
>      cxl_rebuild_cel(cci);
>  
> +    cci->fw.active_slot = cci->fw.staged_slot = 1;
> +    memset(cci->fw.slot, 0, sizeof(cci->fw.slot));
> +    cci->fw.slot[cci->fw.active_slot - 1] = true;
> +
>      cci->bg.complete_pct = 0;
>      cci->bg.starttime = 0;
>      cci->bg.runtime = 0;
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index b2cb280e1631..b4373bfe4d2a 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -201,6 +201,15 @@ typedef struct CXLCCI {
>          uint64_t runtime;
>          QEMUTimer *timer;
>      } bg;
> +
> +    /* firmware update */
> +    struct {
> +        bool transfering;
> +        int active_slot;
> +        int staged_slot;
> +        bool slot[4];
> +    } fw;
> +
>      size_t payload_max;
>      /* Pointer to device hosting the CCI */
>      DeviceState *d;

Re: Re: [PATCH 1/2] hw/cxl: Add Transfer FW support

[Davidlohr Bueso]

On Fri, 26 Jan 2024, Jonathan Cameron wrote:

>On Mon,  8 Jan 2024 23:04:35 -0800
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> Per the latest 3.1 spec for supporting firmware update metadata
>> (no actual buffers). Aborting a xfer is currently unsupported
>> through a nop.
>>
>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>Hi Davidlohr.
>
>Whilst we may never notice from the OS as we expect it to send valid
>sets of transfers only, I'd like the emulation to incorporate more
>of the validity checks the spec requires such as:
>- Multiple part transfers must all be for same slot - any interleaved
>  ones for other slots result in error.
>- Retry of previous transfer or one that starts where previous transfer
>  ended - otherwise Out of Order detected.
>
>So we have to hold state, and as such will have the information to
>set the slot as occupied with just one extra flag stored to say it is
>a FULL or END message.
>
>Otherwise lgtm.

I will send a v2 with your feedback. Do you prefer just using your
latest '2024-26-01-draft' branch?

Thanks,
Davidlohr

Re: [PATCH 1/2] hw/cxl: Add Transfer FW support

[Jonathan Cameron]

On Fri, 26 Jan 2024 09:17:13 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Fri, 26 Jan 2024, Jonathan Cameron wrote:
> 
> >On Mon,  8 Jan 2024 23:04:35 -0800
> >Davidlohr Bueso <dave@stgolabs.net> wrote:
> >  
> >> Per the latest 3.1 spec for supporting firmware update metadata
> >> (no actual buffers). Aborting a xfer is currently unsupported
> >> through a nop.
> >>
> >> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>  
> >Hi Davidlohr.
> >
> >Whilst we may never notice from the OS as we expect it to send valid
> >sets of transfers only, I'd like the emulation to incorporate more
> >of the validity checks the spec requires such as:
> >- Multiple part transfers must all be for same slot - any interleaved
> >  ones for other slots result in error.
> >- Retry of previous transfer or one that starts where previous transfer
> >  ended - otherwise Out of Order detected.
> >
> >So we have to hold state, and as such will have the information to
> >set the slot as occupied with just one extra flag stored to say it is
> >a FULL or END message.
> >
> >Otherwise lgtm.  
> 
> I will send a v2 with your feedback. Do you prefer just using your
> latest '2024-26-01-draft' branch?

Yes, that makes my life easiest (though I'm also find dealing with
rebases if you use something reasonable that isn't that)
Nestle it in as far down as possible but after the DCD changes.
Either before or after your scan media set (up to you - they'll
probably get grouped together anyway for sending to Michael).

Thanks!

Jonathan


> 
> Thanks,
> Davidlohr

[PATCH 2/2] hw/cxl: Add Activate FW support

[Davidlohr Bueso]

Per the latest 3.1 spec for supporting firmware activation.
Online mode is supported by the hw but never incurs in a
background operation.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 hw/cxl/cxl-mailbox-utils.c | 57 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 0295ea8b29aa..7878604595dd 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -61,6 +61,7 @@ enum {
     FIRMWARE_UPDATE = 0x02,
         #define GET_INFO      0x0
         #define TRANSFER      0x1
+        #define ACTIVATE      0x2
     TIMESTAMP   = 0x03,
         #define GET           0x0
         #define SET           0x1
@@ -852,7 +853,7 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
     fw_info->slots_supported = CXL_FW_SLOTS;
     fw_info->slot_info = (cci->fw.active_slot & 0x7) |
             ((cci->fw.staged_slot & 0x7) << 3);
-    fw_info->caps = 0;
+    fw_info->caps = BIT(0);
 
     if (cci->fw.slot[0]) {
         pstrcpy(fw_info->fw_rev1, sizeof(fw_info->fw_rev1), "BWFW VERSION 0");
@@ -961,6 +962,55 @@ static void __do_firmware_xfer(CXLCCI *cci)
     cci->fw.transfering = false;
 }
 
+/* CXL r3.1 section 8.2.9.3.3: Activate FW (Opcode 0202h) */
+static CXLRetCode cmd_firmware_update_activate(const struct cxl_cmd *cmd,
+                                               uint8_t *payload_in,
+                                               size_t len,
+                                               uint8_t *payload_out,
+                                               size_t *len_out,
+                                               CXLCCI *cci)
+{
+    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate;
+    struct {
+        uint8_t action;
+        uint8_t slot;
+    } QEMU_PACKED *fw_activate;
+
+    if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
+        (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    fw_activate = (void *)payload_in;
+
+    if (fw_activate->slot == 0 ||
+        fw_activate->slot == cci->fw.active_slot ||
+        fw_activate->slot > CXL_FW_SLOTS) {
+        return CXL_MBOX_FW_INVALID_SLOT;
+    }
+
+    /*
+     * Check that an actual fw package is there, otherwise
+     * just become a nop.
+    */
+    if (!cci->fw.slot[fw_activate->slot - 1]) {
+        return CXL_MBOX_SUCCESS;
+    }
+
+    switch (fw_activate->action) {
+    case 0: /* online */
+        cci->fw.active_slot = fw_activate->slot;
+        break;
+    case 1: /* reset */
+        cci->fw.staged_slot = fw_activate->slot;
+        break;
+    default:
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    return CXL_MBOX_SUCCESS;
+}
+
 /* CXL r3.0 Section 8.2.9.4.1: Get Timestamp (Opcode 0300h) */
 static CXLRetCode cmd_timestamp_get(const struct cxl_cmd *cmd,
                                     uint8_t *payload_in,
@@ -2273,6 +2323,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
         cmd_firmware_update_get_info, 0, 0 },
     [FIRMWARE_UPDATE][TRANSFER] = { "FIRMWARE_UPDATE_TRANSFER",
         cmd_firmware_update_transfer, ~0, CXL_MBOX_BACKGROUND_OPERATION },
+    [FIRMWARE_UPDATE][ACTIVATE] = { "FIRMWARE_UPDATE_ACTIVATE",
+        cmd_firmware_update_activate, 2, CXL_MBOX_BACKGROUND_OPERATION },
     [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
     [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set,
                          8, CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
@@ -2387,7 +2439,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
             h == cmd_media_inject_poison ||
             h == cmd_media_clear_poison ||
             h == cmd_sanitize_overwrite ||
-            h == cmd_firmware_update_transfer) {
+            h == cmd_firmware_update_transfer ||
+            h == cmd_firmware_update_activate) {
             return CXL_MBOX_MEDIA_DISABLED;
         }
     }
-- 
2.43.0

Re: [PATCH 2/2] hw/cxl: Add Activate FW support

[Jonathan Cameron]

On Mon,  8 Jan 2024 23:04:36 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Per the latest 3.1 spec for supporting firmware activation.
> Online mode is supported by the hw but never incurs in a
> background operation.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Hi Davidlohr.

One question inline.

J
> ---
>  hw/cxl/cxl-mailbox-utils.c | 57 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 0295ea8b29aa..7878604595dd 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -61,6 +61,7 @@ enum {
>      FIRMWARE_UPDATE = 0x02,
>          #define GET_INFO      0x0
>          #define TRANSFER      0x1
> +        #define ACTIVATE      0x2
>      TIMESTAMP   = 0x03,
>          #define GET           0x0
>          #define SET           0x1
> @@ -852,7 +853,7 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
>      fw_info->slots_supported = CXL_FW_SLOTS;
>      fw_info->slot_info = (cci->fw.active_slot & 0x7) |
>              ((cci->fw.staged_slot & 0x7) << 3);
> -    fw_info->caps = 0;
> +    fw_info->caps = BIT(0);
>  
>      if (cci->fw.slot[0]) {
>          pstrcpy(fw_info->fw_rev1, sizeof(fw_info->fw_rev1), "BWFW VERSION 0");
> @@ -961,6 +962,55 @@ static void __do_firmware_xfer(CXLCCI *cci)
>      cci->fw.transfering = false;
>  }
>  
> +/* CXL r3.1 section 8.2.9.3.3: Activate FW (Opcode 0202h) */
> +static CXLRetCode cmd_firmware_update_activate(const struct cxl_cmd *cmd,
> +                                               uint8_t *payload_in,
> +                                               size_t len,
> +                                               uint8_t *payload_out,
> +                                               size_t *len_out,
> +                                               CXLCCI *cci)
> +{
> +    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate;
> +    struct {
> +        uint8_t action;
> +        uint8_t slot;
> +    } QEMU_PACKED *fw_activate;
> +
> +    if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
> +        (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {

Why?  Mind you I'm not sure why the get firmware info command is checking
the same. This will probably go upstream post DCD so if we do need
this check it needs to add an option for a DCD only device.

> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    fw_activate = (void *)payload_in;
> +
> +    if (fw_activate->slot == 0 ||
> +        fw_activate->slot == cci->fw.active_slot ||
> +        fw_activate->slot > CXL_FW_SLOTS) {
> +        return CXL_MBOX_FW_INVALID_SLOT;
> +    }
> +
> +    /*
> +     * Check that an actual fw package is there, otherwise
> +     * just become a nop.
> +    */
Odd indent.

Also, Spec reference would be good for this as I can't
immediately see anything beyond the statement about failing
to activate.  I assume that path applies if there is a firmware there
but loading it fails for some reason and we roll back and for
that there is an error code.

So I think this should return an error but not obvious if Invalid Input
or Invalid Slot.

> +    if (!cci->fw.slot[fw_activate->slot - 1]) {
> +        return CXL_MBOX_SUCCESS;
> +    }
> +
> +    switch (fw_activate->action) {
> +    case 0: /* online */
> +        cci->fw.active_slot = fw_activate->slot;
> +        break;
> +    case 1: /* reset */
> +        cci->fw.staged_slot = fw_activate->slot;
> +        break;
> +    default:
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  /* CXL r3.0 Section 8.2.9.4.1: Get Timestamp (Opcode 0300h) */
>  static CXLRetCode cmd_timestamp_get(const struct cxl_cmd *cmd,
>                                      uint8_t *payload_in,
> @@ -2273,6 +2323,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
>          cmd_firmware_update_get_info, 0, 0 },
>      [FIRMWARE_UPDATE][TRANSFER] = { "FIRMWARE_UPDATE_TRANSFER",
>          cmd_firmware_update_transfer, ~0, CXL_MBOX_BACKGROUND_OPERATION },
> +    [FIRMWARE_UPDATE][ACTIVATE] = { "FIRMWARE_UPDATE_ACTIVATE",
> +        cmd_firmware_update_activate, 2, CXL_MBOX_BACKGROUND_OPERATION },
>      [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
>      [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set,
>                           8, CXL_MBOX_IMMEDIATE_POLICY_CHANGE },
> @@ -2387,7 +2439,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
>              h == cmd_media_inject_poison ||
>              h == cmd_media_clear_poison ||
>              h == cmd_sanitize_overwrite ||
> -            h == cmd_firmware_update_transfer) {
> +            h == cmd_firmware_update_transfer ||
> +            h == cmd_firmware_update_activate) {
>              return CXL_MBOX_MEDIA_DISABLED;
>          }
>      }

Re: Re: [PATCH 2/2] hw/cxl: Add Activate FW support

[Davidlohr Bueso]

On Fri, 26 Jan 2024, Jonathan Cameron wrote:

>On Mon,  8 Jan 2024 23:04:36 -0800
>Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> Per the latest 3.1 spec for supporting firmware activation.
>> Online mode is supported by the hw but never incurs in a
>> background operation.
>>
>> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
>Hi Davidlohr.
>
>One question inline.

Thanks for having a look.

>J
>> ---
>>  hw/cxl/cxl-mailbox-utils.c | 57 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index 0295ea8b29aa..7878604595dd 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -61,6 +61,7 @@ enum {
>>      FIRMWARE_UPDATE = 0x02,
>>          #define GET_INFO      0x0
>>          #define TRANSFER      0x1
>> +        #define ACTIVATE      0x2
>>      TIMESTAMP   = 0x03,
>>          #define GET           0x0
>>          #define SET           0x1
>> @@ -852,7 +853,7 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
>>      fw_info->slots_supported = CXL_FW_SLOTS;
>>      fw_info->slot_info = (cci->fw.active_slot & 0x7) |
>>              ((cci->fw.staged_slot & 0x7) << 3);
>> -    fw_info->caps = 0;
>> +    fw_info->caps = BIT(0);
>>
>>      if (cci->fw.slot[0]) {
>>          pstrcpy(fw_info->fw_rev1, sizeof(fw_info->fw_rev1), "BWFW VERSION 0");
>> @@ -961,6 +962,55 @@ static void __do_firmware_xfer(CXLCCI *cci)
>>      cci->fw.transfering = false;
>>  }
>>
>> +/* CXL r3.1 section 8.2.9.3.3: Activate FW (Opcode 0202h) */
>> +static CXLRetCode cmd_firmware_update_activate(const struct cxl_cmd *cmd,
>> +                                               uint8_t *payload_in,
>> +                                               size_t len,
>> +                                               uint8_t *payload_out,
>> +                                               size_t *len_out,
>> +                                               CXLCCI *cci)
>> +{
>> +    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate;
>> +    struct {
>> +        uint8_t action;
>> +        uint8_t slot;
>> +    } QEMU_PACKED *fw_activate;
>> +
>> +    if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
>> +        (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {
>
>Why?  Mind you I'm not sure why the get firmware info command is checking
>the same. This will probably go upstream post DCD so if we do need
>this check it needs to add an option for a DCD only device.

Will remove.

>
>> +        return CXL_MBOX_INTERNAL_ERROR;
>> +    }
>> +
>> +    fw_activate = (void *)payload_in;
>> +
>> +    if (fw_activate->slot == 0 ||
>> +        fw_activate->slot == cci->fw.active_slot ||
>> +        fw_activate->slot > CXL_FW_SLOTS) {
>> +        return CXL_MBOX_FW_INVALID_SLOT;
>> +    }
>> +
>> +    /*
>> +     * Check that an actual fw package is there, otherwise
>> +     * just become a nop.
>> +    */
>Odd indent.
>
>Also, Spec reference would be good for this as I can't
>immediately see anything beyond the statement about failing
>to activate.  I assume that path applies if there is a firmware there
>but loading it fails for some reason and we roll back and for
>that there is an error code.

This case just occurred to me while writing the patch, I could not
find an answer in the spec.

>
>So I think this should return an error but not obvious if Invalid Input
>or Invalid Slot.

Fine with me, I'll go with Invalid Slot.

Thanks,
Davidlohr

Re: [PATCH 2/2] hw/cxl: Add Activate FW support

[Jonathan Cameron]

On Fri, 26 Jan 2024 08:57:07 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Fri, 26 Jan 2024, Jonathan Cameron wrote:
> 
> >On Mon,  8 Jan 2024 23:04:36 -0800
> >Davidlohr Bueso <dave@stgolabs.net> wrote:
> >  
> >> Per the latest 3.1 spec for supporting firmware activation.
> >> Online mode is supported by the hw but never incurs in a
> >> background operation.
> >>
> >> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>  
> >Hi Davidlohr.
> >
> >One question inline.  
> 
> Thanks for having a look.
> 
> >J  
> >> ---
> >>  hw/cxl/cxl-mailbox-utils.c | 57 ++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 55 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> >> index 0295ea8b29aa..7878604595dd 100644
> >> --- a/hw/cxl/cxl-mailbox-utils.c
> >> +++ b/hw/cxl/cxl-mailbox-utils.c
> >> @@ -61,6 +61,7 @@ enum {
> >>      FIRMWARE_UPDATE = 0x02,
> >>          #define GET_INFO      0x0
> >>          #define TRANSFER      0x1
> >> +        #define ACTIVATE      0x2
> >>      TIMESTAMP   = 0x03,
> >>          #define GET           0x0
> >>          #define SET           0x1
> >> @@ -852,7 +853,7 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
> >>      fw_info->slots_supported = CXL_FW_SLOTS;
> >>      fw_info->slot_info = (cci->fw.active_slot & 0x7) |
> >>              ((cci->fw.staged_slot & 0x7) << 3);
> >> -    fw_info->caps = 0;
> >> +    fw_info->caps = BIT(0);
> >>
> >>      if (cci->fw.slot[0]) {
> >>          pstrcpy(fw_info->fw_rev1, sizeof(fw_info->fw_rev1), "BWFW VERSION 0");
> >> @@ -961,6 +962,55 @@ static void __do_firmware_xfer(CXLCCI *cci)
> >>      cci->fw.transfering = false;
> >>  }
> >>
> >> +/* CXL r3.1 section 8.2.9.3.3: Activate FW (Opcode 0202h) */
> >> +static CXLRetCode cmd_firmware_update_activate(const struct cxl_cmd *cmd,
> >> +                                               uint8_t *payload_in,
> >> +                                               size_t len,
> >> +                                               uint8_t *payload_out,
> >> +                                               size_t *len_out,
> >> +                                               CXLCCI *cci)
> >> +{
> >> +    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate;
> >> +    struct {
> >> +        uint8_t action;
> >> +        uint8_t slot;
> >> +    } QEMU_PACKED *fw_activate;
> >> +
> >> +    if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
> >> +        (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {  
> >
> >Why?  Mind you I'm not sure why the get firmware info command is checking
> >the same. This will probably go upstream post DCD so if we do need
> >this check it needs to add an option for a DCD only device.  
> 
> Will remove.
> 
> >  
> >> +        return CXL_MBOX_INTERNAL_ERROR;
> >> +    }
> >> +
> >> +    fw_activate = (void *)payload_in;
> >> +
> >> +    if (fw_activate->slot == 0 ||
> >> +        fw_activate->slot == cci->fw.active_slot ||
> >> +        fw_activate->slot > CXL_FW_SLOTS) {
> >> +        return CXL_MBOX_FW_INVALID_SLOT;
> >> +    }
> >> +
> >> +    /*
> >> +     * Check that an actual fw package is there, otherwise
> >> +     * just become a nop.
> >> +    */  
> >Odd indent.
> >
> >Also, Spec reference would be good for this as I can't
> >immediately see anything beyond the statement about failing
> >to activate.  I assume that path applies if there is a firmware there
> >but loading it fails for some reason and we roll back and for
> >that there is an error code.  
> 
> This case just occurred to me while writing the patch, I could not
> find an answer in the spec.

Let's ping the spec guys for a clarification...
I'll cc you.


Jonathan


> 
> >
> >So I think this should return an error but not obvious if Invalid Input
> >or Invalid Slot.  
> 
> Fine with me, I'll go with Invalid Slot.
> 
> Thanks,
> Davidlohr