| Message ID | 20240109070436.21253-3-dave@stgolabs.net |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/cxl: Firmware Update support | expand |
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; > } > }
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
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
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; } }
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(-)