Re: [QEMU PATCH v3 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release

[Fan Ni <nifan.cxl@gmail.com>]

On Fri, Jun 06, 2025 at 11:43:51AM -0700, Fan Ni wrote:
> On Thu, Jun 05, 2025 at 11:42:23PM +0000, anisa.su887@gmail.com wrote:
> > From: Anisa Su <anisa.su@samsung.com>
> > 
> > FM DCD Managment command 0x5605 implemented per CXL r3.2 Spec Section 7.6.7.6.6
> > 
> > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> 
> See below ..
> 
> > ---
> >  hw/cxl/cxl-mailbox-utils.c | 62 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 7ee5be00bc..6c57e0deac 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -124,6 +124,7 @@ enum {
> >          #define SET_DC_REGION_CONFIG        0x2
> >          #define GET_DC_REGION_EXTENT_LIST   0x3
> >          #define INITIATE_DC_ADD             0x4
> > +        #define INITIATE_DC_RELEASE         0x5
> >  };
> >  
> >  /* CCI Message Format CXL r3.1 Figure 7-19 */
> > @@ -3685,6 +3686,60 @@ static CXLRetCode cmd_fm_initiate_dc_add(const struct cxl_cmd *cmd,
> >      return CXL_MBOX_SUCCESS;
> >  }
> >  
> > +#define CXL_EXTENT_REMOVAL_POLICY_MASK 0x7
> > +/* CXL r3.2 Section 7.6.7.6.6 Initiate Dynamic Capacity Release (Opcode 5605h) */
> > +static CXLRetCode cmd_fm_initiate_dc_release(const struct cxl_cmd *cmd,
> > +                                             uint8_t *payload_in,
> > +                                             size_t len_in,
> > +                                             uint8_t *payload_out,
> > +                                             size_t *len_out,
> > +                                             CXLCCI *cci)
> > +{
> > +    struct {
> > +        uint16_t host_id;
> > +        uint8_t flags;
> > +        uint8_t reg_num;
> > +        uint64_t length;
> > +        uint8_t tag[0x10];
> > +        uint32_t ext_count;
> > +        CXLDCExtentRaw extents[];
> > +    } QEMU_PACKED *in = (void *)payload_in;
> > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > +    CXLUpdateDCExtentListInPl *list;
> > +    CXLDCExtentList updated_list;
> > +    uint32_t updated_list_size;
> > +    int rc;
> > +
> > +    switch (in->flags & CXL_EXTENT_REMOVAL_POLICY_MASK) {
> > +    case CXL_EXTENT_REMOVAL_POLICY_PRESCRIPTIVE:
> > +        list = calloc(1, (sizeof(*list) +
> > +                          in->ext_count * sizeof(*list->updated_entries)));
> 
> Use g_malloc() and free with g_free();
> 
> > +        convert_raw_extents(in->extents, list, in->ext_count);
> > +        rc = cxl_detect_malformed_extent_list(ct3d, list);
> > +        if (rc) {
> > +            return rc;
> > +        }
> > +        rc = cxl_dc_extent_release_dry_run(ct3d,
> > +                                           list,
> > +                                           &updated_list,
> > +                                           &updated_list_size);
> 
> This seems not right.
> this is only fm issue dc release request, not host release dc extents to device.
> So we should follow what we did in the qmp_cxl_process_dynamic_capacity_prescriptive()
> for release case.
> 
> One thing that I can see that making the workflow is different is that, we check
> the extent list with the pending list to make sure fm is not trying to remove
> non-accepted extents, but the host release extent workflow does not need to do
> that as it is filtered out in the first place when fm sends the request if it is
> from FM.
> I have to admit, existing qmp interface can be improved to remove some condition
> checks as they are kind of duplicate.
> For example, if an extent is still pending, it will not be set in the bitmap, so
> we can still tigger the error if it happens by removing the pending list check.
> One justification is that the error message is different for a non-existing
> extent and a pending extent, which is useful for a dmp interface.
> 
> 
> Also, the case to detect exhausted resouces is not different, FM can request to

s/is not different/is different/
I am not sure what I was thinking..

Fan
> release a lot of extents, but what the host actually does can be a subset or
> none.
> 
> Fan
> 
> > +        if (rc) {
> > +            return rc;
> > +        }
> > +        cxl_mbox_create_dc_event_records_for_extents(ct3d,
> > +                                                     DC_EVENT_RELEASE_CAPACITY,
> > +                                                     in->extents,
> > +                                                     in->ext_count);
> > +        return CXL_MBOX_SUCCESS;
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP,
> > +            "CXL extent selection policy not supported.\n");
> > +        return CXL_MBOX_INVALID_INPUT;
> > +    }
> > +
> > +    return CXL_MBOX_SUCCESS;
> > +}
> > +
> >  static const struct cxl_cmd cxl_cmd_set[256][256] = {
> >      [INFOSTAT][BACKGROUND_OPERATION_ABORT] = { "BACKGROUND_OPERATION_ABORT",
> >          cmd_infostat_bg_op_abort, 0, 0 },
> > @@ -3819,6 +3874,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> >          CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> >          CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> >          CXL_MBOX_IMMEDIATE_DATA_CHANGE) },
> > +    [FMAPI_DCD_MGMT][INITIATE_DC_RELEASE] = { "INIT_DC_RELEASE",
> > +        cmd_fm_initiate_dc_release, ~0,
> > +        (CXL_MBOX_CONFIG_CHANGE_COLD_RESET |
> > +         CXL_MBOX_CONFIG_CHANGE_CONV_RESET |
> > +         CXL_MBOX_CONFIG_CHANGE_CXL_RESET |
> > +         CXL_MBOX_IMMEDIATE_CONFIG_CHANGE |
> > +         CXL_MBOX_IMMEDIATE_DATA_CHANGE) },
> >  };
> >  
> >  /*
> > -- 
> > 2.47.2
> >