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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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