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

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

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