Re: [QEMU PATCH v3 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release
On Fri, Jun 06, 2025 at 11:48:33AM -0700, Fan Ni wrote: > On Fri, Jun 06, 2025 at 11:43:51AM -0700, Fan Ni wrote: > > On Thu, Jun 05, 2025 at 11:42:23PM +, [email protected] wrote: > > > From: Anisa Su > > > > > > FM DCD Managment command 0x5605 implemented per CXL r3.2 Spec Section > > > 7.6.7.6.6 > > > > > > Signed-off-by: Anisa Su > > > > 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_CONFIG0x2 > > > #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 I'm confused though because in the 3.2 spec it says "The command shall fail with Resources Exhausted if the Extent List would cause the device to exceed its extent or tag tracking ability" (Section 7.6.7.6.6). The only way that releasing an extent can cause the number of extents to increase is if the middle section of an existing extent is released, which is why I used cxl_dc_extent_release_dry_run(), since it already detects that case. Anisa > > 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(c
Re: [QEMU PATCH v3 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release
On Fri, Jun 06, 2025 at 11:43:51AM -0700, Fan Ni wrote: > On Thu, Jun 05, 2025 at 11:42:23PM +, [email protected] wrote: > > From: Anisa Su > > > > FM DCD Managment command 0x5605 implemented per CXL r3.2 Spec Section > > 7.6.7.6.6 > > > > Signed-off-by: Anisa Su > > 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_CONFIG0x2 > > #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
Re: [QEMU PATCH v3 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release
On Thu, Jun 05, 2025 at 11:42:23PM +, [email protected] wrote: > From: Anisa Su > > FM DCD Managment command 0x5605 implemented per CXL r3.2 Spec Section > 7.6.7.6.6 > > Signed-off-by: Anisa Su 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_CONFIG0x2 > #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 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_
Re: [QEMU PATCH v3 9/9] cxl-mailbox-utils: 0x5605 - FMAPI Initiate DC Release
On 06-06-2025 05:12, [email protected] wrote: From: Anisa Su FM DCD Managment command 0x5605 implemented per CXL r3.2 Spec Section 7.6.7.6.6 typo Managment Signed-off-by: Anisa Su --- 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_CONFIG0x2 #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))); +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); +if (rc) { +return rc; list free ? +} +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; unreachable +} + 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) }, }; /* Thanks, Alok
