From: Anisa Su <anisa.su@samsung.com>

FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3

Reviewed-by: Fan Ni <fan.ni@samsung.com>
Signed-off-by: Anisa Su <anisa.su@samsung.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/hw/cxl/cxl_device.h  |  3 ++
 include/hw/cxl/cxl_mailbox.h |  6 +++
 hw/cxl/cxl-mailbox-utils.c   | 86 ++++++++++++++++++++++++++++++++++++
 hw/mem/cxl_type3.c           |  6 +--
 4 files changed, 98 insertions(+), 3 deletions(-)

diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 96ef9be444..76af75d2d0 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -721,4 +721,7 @@ void ct3_clear_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
                                    uint64_t len);
 bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
                                   uint64_t len);
+void cxl_assign_event_header(CXLEventRecordHdr *hdr,
+                             const QemuUUID *uuid, uint32_t flags,
+                             uint8_t length, uint64_t timestamp);
 #endif
diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
index 9008402d1c..a05d7cb5b7 100644
--- a/include/hw/cxl/cxl_mailbox.h
+++ b/include/hw/cxl/cxl_mailbox.h
@@ -8,6 +8,7 @@
 #ifndef CXL_MAILBOX_H
 #define CXL_MAILBOX_H
 
+#define CXL_MBOX_CONFIG_CHANGE_COLD_RESET (1)
 #define CXL_MBOX_IMMEDIATE_CONFIG_CHANGE (1 << 1)
 #define CXL_MBOX_IMMEDIATE_DATA_CHANGE (1 << 2)
 #define CXL_MBOX_IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -15,5 +16,10 @@
 #define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5)
 #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
 #define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
+#define CXL_MBOX_SECONDARY_MBOX_SUPPORTED (1 << 8)
+#define CXL_MBOX_REQUEST_ABORT_BACKGROUND_OP_SUPPORTED (1 << 9)
+#define CXL_MBOX_CEL_10_TO_11_VALID (1 << 10)
+#define CXL_MBOX_CONFIG_CHANGE_CONV_RESET (1 << 11)
+#define CXL_MBOX_CONFIG_CHANGE_CXL_RESET (1 << 12)
 
 #endif
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index bf1710b251..1fc453f70d 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -121,6 +121,7 @@ enum {
     FMAPI_DCD_MGMT = 0x56,
         #define GET_DCD_INFO    0x0
         #define GET_HOST_DC_REGION_CONFIG   0x1
+        #define SET_DC_REGION_CONFIG        0x2
 };
 
 /* CCI Message Format CXL r3.1 Figure 7-19 */
@@ -3390,6 +3391,84 @@ static CXLRetCode cmd_fm_get_host_dc_region_config(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+/* CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602) */
+static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
+                                              uint8_t *payload_in,
+                                              size_t len_in,
+                                              uint8_t *payload_out,
+                                              size_t *len_out,
+                                              CXLCCI *cci)
+{
+    struct {
+        uint8_t reg_id;
+        uint8_t rsvd[3];
+        uint64_t block_sz;
+        uint8_t flags;
+        uint8_t rsvd2[3];
+    } QEMU_PACKED *in = (void *)payload_in;
+    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+    CXLEventDynamicCapacity dcEvent = {};
+    CXLDCRegion *region = &ct3d->dc.regions[in->reg_id];
+
+    /*
+     * CXL r3.2 7.6.7.6.3: Set DC Region Configuration
+     * This command shall fail with Unsupported when the Sanitize on Release
+     * field does not match the region’s configuration... and the device
+     * does not support reconfiguration of the Sanitize on Release setting.
+     *
+     * Currently not reconfigurable, so always fail if sanitize bit (bit 0)
+     * doesn't match.
+     */
+    if ((in->flags & 0x1) != (region->flags & 0x1)) {
+        return CXL_MBOX_UNSUPPORTED;
+    }
+
+    if (in->reg_id >= DCD_MAX_NUM_REGION) {
+        return CXL_MBOX_UNSUPPORTED;
+    }
+
+    /* Check that no extents are in the region being reconfigured */
+    if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
+        return CXL_MBOX_UNSUPPORTED;
+    }
+
+    /* Check that new block size is supported */
+    if (!test_bit(BIT((int) log2(in->block_sz)),
+                  &region->supported_blk_size_bitmask)) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    /* Return success if new block size == current block size */
+    if (in->block_sz == region->block_size) {
+        return CXL_MBOX_SUCCESS;
+    }
+
+    /* Free bitmap and create new one for new block size. */
+    qemu_mutex_lock(&region->bitmap_lock);
+    g_free(region->blk_bitmap);
+    region->blk_bitmap = bitmap_new(region->len / in->block_sz);
+    qemu_mutex_unlock(&region->bitmap_lock);
+    region->block_size = in->block_sz;
+
+    /* Create event record and insert into event log */
+    cxl_assign_event_header(&dcEvent.hdr,
+                            &dynamic_capacity_uuid,
+                            (1 << CXL_EVENT_TYPE_INFO),
+                            sizeof(dcEvent),
+                            cxl_device_get_timestamp(&ct3d->cxl_dstate));
+    dcEvent.type = DC_EVENT_REGION_CONFIG_UPDATED;
+    dcEvent.validity_flags = 1;
+    dcEvent.host_id = 0;
+    dcEvent.updated_region_id = in->reg_id;
+
+    if (cxl_event_insert(&ct3d->cxl_dstate,
+                         CXL_EVENT_TYPE_DYNAMIC_CAP,
+                         (CXLEventRecordRaw *)&dcEvent)) {
+        cxl_event_irq_assert(ct3d);
+    }
+    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 },
@@ -3508,6 +3587,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
         cmd_fm_get_dcd_info, 0, 0 },
     [FMAPI_DCD_MGMT][GET_HOST_DC_REGION_CONFIG] = { "GET_HOST_DC_REGION_CONFIG",
         cmd_fm_get_host_dc_region_config, 4, 0 },
+    [FMAPI_DCD_MGMT][SET_DC_REGION_CONFIG] = { "SET_DC_REGION_CONFIG",
+        cmd_fm_set_dc_region_config, 16,
+        (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) },
 };
 
 /*
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index b872a26173..ee554a77be 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1590,9 +1590,9 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
     pcie_aer_inject_error(PCI_DEVICE(obj), &err);
 }
 
-static void cxl_assign_event_header(CXLEventRecordHdr *hdr,
-                                    const QemuUUID *uuid, uint32_t flags,
-                                    uint8_t length, uint64_t timestamp)
+void cxl_assign_event_header(CXLEventRecordHdr *hdr,
+                             const QemuUUID *uuid, uint32_t flags,
+                             uint8_t length, uint64_t timestamp)
 {
     st24_le_p(&hdr->flags, flags);
     hdr->length = length;
-- 
2.48.1

On Wed, Jul 02, 2025 at 05:02:13PM +0100, Jonathan Cameron wrote:
> From: Anisa Su <anisa.su@samsung.com>
> 
> FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
> 
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  include/hw/cxl/cxl_device.h  |  3 ++
>  include/hw/cxl/cxl_mailbox.h |  6 +++
>  hw/cxl/cxl-mailbox-utils.c   | 86 ++++++++++++++++++++++++++++++++++++
>  hw/mem/cxl_type3.c           |  6 +--
>  4 files changed, 98 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 96ef9be444..76af75d2d0 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -721,4 +721,7 @@ void ct3_clear_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
>                                     uint64_t len);
>  bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
>                                    uint64_t len);
> +void cxl_assign_event_header(CXLEventRecordHdr *hdr,
> +                             const QemuUUID *uuid, uint32_t flags,
> +                             uint8_t length, uint64_t timestamp);
>  #endif
> diff --git a/include/hw/cxl/cxl_mailbox.h b/include/hw/cxl/cxl_mailbox.h
> index 9008402d1c..a05d7cb5b7 100644
> --- a/include/hw/cxl/cxl_mailbox.h
> +++ b/include/hw/cxl/cxl_mailbox.h
> @@ -8,6 +8,7 @@
>  #ifndef CXL_MAILBOX_H
>  #define CXL_MAILBOX_H
>  
> +#define CXL_MBOX_CONFIG_CHANGE_COLD_RESET (1)
>  #define CXL_MBOX_IMMEDIATE_CONFIG_CHANGE (1 << 1)
>  #define CXL_MBOX_IMMEDIATE_DATA_CHANGE (1 << 2)
>  #define CXL_MBOX_IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -15,5 +16,10 @@
>  #define CXL_MBOX_SECURITY_STATE_CHANGE (1 << 5)
>  #define CXL_MBOX_BACKGROUND_OPERATION (1 << 6)
>  #define CXL_MBOX_BACKGROUND_OPERATION_ABORT (1 << 7)
> +#define CXL_MBOX_SECONDARY_MBOX_SUPPORTED (1 << 8)
> +#define CXL_MBOX_REQUEST_ABORT_BACKGROUND_OP_SUPPORTED (1 << 9)
> +#define CXL_MBOX_CEL_10_TO_11_VALID (1 << 10)
> +#define CXL_MBOX_CONFIG_CHANGE_CONV_RESET (1 << 11)
> +#define CXL_MBOX_CONFIG_CHANGE_CXL_RESET (1 << 12)
>  
>  #endif
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index bf1710b251..1fc453f70d 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -121,6 +121,7 @@ enum {
>      FMAPI_DCD_MGMT = 0x56,
>          #define GET_DCD_INFO    0x0
>          #define GET_HOST_DC_REGION_CONFIG   0x1
> +        #define SET_DC_REGION_CONFIG        0x2
>  };
>  
>  /* CCI Message Format CXL r3.1 Figure 7-19 */
> @@ -3390,6 +3391,84 @@ static CXLRetCode cmd_fm_get_host_dc_region_config(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +/* CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602) */
> +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> +                                              uint8_t *payload_in,
> +                                              size_t len_in,
> +                                              uint8_t *payload_out,
> +                                              size_t *len_out,
> +                                              CXLCCI *cci)
> +{
> +    struct {
> +        uint8_t reg_id;
> +        uint8_t rsvd[3];
> +        uint64_t block_sz;
> +        uint8_t flags;
> +        uint8_t rsvd2[3];
> +    } QEMU_PACKED *in = (void *)payload_in;
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +    CXLEventDynamicCapacity dcEvent = {};
> +    CXLDCRegion *region = &ct3d->dc.regions[in->reg_id];
> +
> +    /*
> +     * CXL r3.2 7.6.7.6.3: Set DC Region Configuration
> +     * This command shall fail with Unsupported when the Sanitize on Release
> +     * field does not match the region’s configuration... and the device
> +     * does not support reconfiguration of the Sanitize on Release setting.
> +     *
> +     * Currently not reconfigurable, so always fail if sanitize bit (bit 0)
> +     * doesn't match.
> +     */
> +    if ((in->flags & 0x1) != (region->flags & 0x1)) {
> +        return CXL_MBOX_UNSUPPORTED;
> +    }
> +
> +    if (in->reg_id >= DCD_MAX_NUM_REGION) {
> +        return CXL_MBOX_UNSUPPORTED;
> +    }
> +
> +    /* Check that no extents are in the region being reconfigured */
> +    if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
> +        return CXL_MBOX_UNSUPPORTED;
> +    }
> +
> +    /* Check that new block size is supported */
> +    if (!test_bit(BIT((int) log2(in->block_sz)),
> +                  &region->supported_blk_size_bitmask)) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }

This does not work: test_bit works on unsigned long, while
supported_blk_size_bitmask is uint64_t.

Why so funky? what is wrong with:

if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask))

And BTW why cast to int here?


> +
> +    /* Return success if new block size == current block size */
> +    if (in->block_sz == region->block_size) {
> +        return CXL_MBOX_SUCCESS;
> +    }
> +
> +    /* Free bitmap and create new one for new block size. */
> +    qemu_mutex_lock(&region->bitmap_lock);
> +    g_free(region->blk_bitmap);
> +    region->blk_bitmap = bitmap_new(region->len / in->block_sz);
> +    qemu_mutex_unlock(&region->bitmap_lock);
> +    region->block_size = in->block_sz;
> +
> +    /* Create event record and insert into event log */
> +    cxl_assign_event_header(&dcEvent.hdr,
> +                            &dynamic_capacity_uuid,
> +                            (1 << CXL_EVENT_TYPE_INFO),
> +                            sizeof(dcEvent),
> +                            cxl_device_get_timestamp(&ct3d->cxl_dstate));
> +    dcEvent.type = DC_EVENT_REGION_CONFIG_UPDATED;
> +    dcEvent.validity_flags = 1;
> +    dcEvent.host_id = 0;
> +    dcEvent.updated_region_id = in->reg_id;
> +
> +    if (cxl_event_insert(&ct3d->cxl_dstate,
> +                         CXL_EVENT_TYPE_DYNAMIC_CAP,
> +                         (CXLEventRecordRaw *)&dcEvent)) {
> +        cxl_event_irq_assert(ct3d);
> +    }
> +    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 },
> @@ -3508,6 +3587,13 @@ static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
>          cmd_fm_get_dcd_info, 0, 0 },
>      [FMAPI_DCD_MGMT][GET_HOST_DC_REGION_CONFIG] = { "GET_HOST_DC_REGION_CONFIG",
>          cmd_fm_get_host_dc_region_config, 4, 0 },
> +    [FMAPI_DCD_MGMT][SET_DC_REGION_CONFIG] = { "SET_DC_REGION_CONFIG",
> +        cmd_fm_set_dc_region_config, 16,
> +        (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) },
>  };
>  
>  /*
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b872a26173..ee554a77be 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1590,9 +1590,9 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
>      pcie_aer_inject_error(PCI_DEVICE(obj), &err);
>  }
>  
> -static void cxl_assign_event_header(CXLEventRecordHdr *hdr,
> -                                    const QemuUUID *uuid, uint32_t flags,
> -                                    uint8_t length, uint64_t timestamp)
> +void cxl_assign_event_header(CXLEventRecordHdr *hdr,
> +                             const QemuUUID *uuid, uint32_t flags,
> +                             uint8_t length, uint64_t timestamp)
>  {
>      st24_le_p(&hdr->flags, flags);
>      hdr->length = length;
> -- 
> 2.48.1

On Mon, 14 Jul 2025 05:32:19 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 02, 2025 at 05:02:13PM +0100, Jonathan Cameron wrote:
> > From: Anisa Su <anisa.su@samsung.com>
> > 
> > FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
> > 
> > Reviewed-by: Fan Ni <fan.ni@samsung.com>
> > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index bf1710b251..1fc453f70d 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c

> > +/* CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602) */
> > +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> > +                                              uint8_t *payload_in,
> > +                                              size_t len_in,
> > +                                              uint8_t *payload_out,
> > +                                              size_t *len_out,
> > +                                              CXLCCI *cci)
> > +{
> > +    struct {
> > +        uint8_t reg_id;
> > +        uint8_t rsvd[3];
> > +        uint64_t block_sz;
> > +        uint8_t flags;
> > +        uint8_t rsvd2[3];
> > +    } QEMU_PACKED *in = (void *)payload_in;
> > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > +    CXLEventDynamicCapacity dcEvent = {};
> > +    CXLDCRegion *region = &ct3d->dc.regions[in->reg_id];
> > +
> > +    /*
> > +     * CXL r3.2 7.6.7.6.3: Set DC Region Configuration
> > +     * This command shall fail with Unsupported when the Sanitize on Release
> > +     * field does not match the region’s configuration... and the device
> > +     * does not support reconfiguration of the Sanitize on Release setting.
> > +     *
> > +     * Currently not reconfigurable, so always fail if sanitize bit (bit 0)
> > +     * doesn't match.
> > +     */
> > +    if ((in->flags & 0x1) != (region->flags & 0x1)) {
> > +        return CXL_MBOX_UNSUPPORTED;
> > +    }
> > +
> > +    if (in->reg_id >= DCD_MAX_NUM_REGION) {
> > +        return CXL_MBOX_UNSUPPORTED;
> > +    }
> > +
> > +    /* Check that no extents are in the region being reconfigured */
> > +    if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
> > +        return CXL_MBOX_UNSUPPORTED;
> > +    }
> > +
> > +    /* Check that new block size is supported */
> > +    if (!test_bit(BIT((int) log2(in->block_sz)),
> > +                  &region->supported_blk_size_bitmask)) {
> > +        return CXL_MBOX_INVALID_INPUT;
> > +    }  
> 
> This does not work: test_bit works on unsigned long, while
> supported_blk_size_bitmask is uint64_t.
> 
> Why so funky? what is wrong with:
> 
> if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask))
> 
> And BTW why cast to int here?

Change looks fine to me, so I'll prepare an updated set with this
and the missing semi colon.  Anisa if you can have a look at this
that would be great. 

Sorry I seem to have missed Anisa off the cc for this!

Jonathan

On Mon, 14 Jul 2025 15:02:18 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Mon, 14 Jul 2025 05:32:19 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 02, 2025 at 05:02:13PM +0100, Jonathan Cameron wrote:  
> > > From: Anisa Su <anisa.su@samsung.com>
> > > 
> > > FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
> > > 
> > > Reviewed-by: Fan Ni <fan.ni@samsung.com>
> > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> 
> > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > index bf1710b251..1fc453f70d 100644
> > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > +++ b/hw/cxl/cxl-mailbox-utils.c  
> 
> > > +/* CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602) */
> > > +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> > > +                                              uint8_t *payload_in,
> > > +                                              size_t len_in,
> > > +                                              uint8_t *payload_out,
> > > +                                              size_t *len_out,
> > > +                                              CXLCCI *cci)
> > > +{
> > > +    struct {
> > > +        uint8_t reg_id;
> > > +        uint8_t rsvd[3];
> > > +        uint64_t block_sz;
> > > +        uint8_t flags;
> > > +        uint8_t rsvd2[3];
> > > +    } QEMU_PACKED *in = (void *)payload_in;
> > > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > +    CXLEventDynamicCapacity dcEvent = {};
> > > +    CXLDCRegion *region = &ct3d->dc.regions[in->reg_id];
> > > +
> > > +    /*
> > > +     * CXL r3.2 7.6.7.6.3: Set DC Region Configuration
> > > +     * This command shall fail with Unsupported when the Sanitize on Release
> > > +     * field does not match the region’s configuration... and the device
> > > +     * does not support reconfiguration of the Sanitize on Release setting.
> > > +     *
> > > +     * Currently not reconfigurable, so always fail if sanitize bit (bit 0)
> > > +     * doesn't match.
> > > +     */
> > > +    if ((in->flags & 0x1) != (region->flags & 0x1)) {
> > > +        return CXL_MBOX_UNSUPPORTED;
> > > +    }
> > > +
> > > +    if (in->reg_id >= DCD_MAX_NUM_REGION) {
> > > +        return CXL_MBOX_UNSUPPORTED;
> > > +    }
> > > +
> > > +    /* Check that no extents are in the region being reconfigured */
> > > +    if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
> > > +        return CXL_MBOX_UNSUPPORTED;
> > > +    }
> > > +
> > > +    /* Check that new block size is supported */
> > > +    if (!test_bit(BIT((int) log2(in->block_sz)),
> > > +                  &region->supported_blk_size_bitmask)) {
> > > +        return CXL_MBOX_INVALID_INPUT;
> > > +    }    
> > 
> > This does not work: test_bit works on unsigned long, while
> > supported_blk_size_bitmask is uint64_t.
> > 
> > Why so funky? what is wrong with:
> > 
> > if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask))
> > 
> > And BTW why cast to int here?  
This became obvious when your suggestion didn't build :(

./../hw/cxl/cxl-mailbox-utils.c: In function ‘cmd_fm_set_dc_region_config’:
/home/jic23/src/qemu/include/qemu/bitops.h:25:39: error: invalid operands to binary << (have ‘long long unsigned int’ and ‘double’)
   25 | #define BIT_ULL(nr)             (1ULL << (nr))
      |                                       ^~ ~~~~
../../hw/cxl/cxl-mailbox-utils.c:3436:11: note: in expansion of macro ‘BIT_ULL’
 3436 |     if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask)) {
      |           ^~~~~~~

Now I look again, this is effectively 2**(log_2(x)) or x. So
if (in->block_sz & region->supporte_blk_size_bitmask)
Should work as long as we know block_size is a power of 2 (which the specification
says it must be).

Anisa?

> 
> Change looks fine to me, so I'll prepare an updated set with this
> and the missing semi colon.  Anisa if you can have a look at this
> that would be great. 
> 
> Sorry I seem to have missed Anisa off the cc for this!
> 
> Jonathan
> 

On Mon, 14 Jul 2025 15:15:12 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Mon, 14 Jul 2025 15:02:18 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > On Mon, 14 Jul 2025 05:32:19 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Wed, Jul 02, 2025 at 05:02:13PM +0100, Jonathan Cameron wrote:    
> > > > From: Anisa Su <anisa.su@samsung.com>
> > > > 
> > > > FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
> > > > 
> > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>
> > > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>    
> > 
> >   
> > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > > index bf1710b251..1fc453f70d 100644
> > > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > > +++ b/hw/cxl/cxl-mailbox-utils.c    
> >   
> > > > +/* CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602) */
> > > > +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> > > > +                                              uint8_t *payload_in,
> > > > +                                              size_t len_in,
> > > > +                                              uint8_t *payload_out,
> > > > +                                              size_t *len_out,
> > > > +                                              CXLCCI *cci)
> > > > +{
> > > > +    struct {
> > > > +        uint8_t reg_id;
> > > > +        uint8_t rsvd[3];
> > > > +        uint64_t block_sz;
> > > > +        uint8_t flags;
> > > > +        uint8_t rsvd2[3];
> > > > +    } QEMU_PACKED *in = (void *)payload_in;
> > > > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > > +    CXLEventDynamicCapacity dcEvent = {};
> > > > +    CXLDCRegion *region = &ct3d->dc.regions[in->reg_id];
> > > > +
> > > > +    /*
> > > > +     * CXL r3.2 7.6.7.6.3: Set DC Region Configuration
> > > > +     * This command shall fail with Unsupported when the Sanitize on Release
> > > > +     * field does not match the region’s configuration... and the device
> > > > +     * does not support reconfiguration of the Sanitize on Release setting.
> > > > +     *
> > > > +     * Currently not reconfigurable, so always fail if sanitize bit (bit 0)
> > > > +     * doesn't match.
> > > > +     */
> > > > +    if ((in->flags & 0x1) != (region->flags & 0x1)) {
> > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > +    }
> > > > +
> > > > +    if (in->reg_id >= DCD_MAX_NUM_REGION) {
> > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > +    }
> > > > +
> > > > +    /* Check that no extents are in the region being reconfigured */
> > > > +    if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
> > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > +    }
> > > > +
> > > > +    /* Check that new block size is supported */
> > > > +    if (!test_bit(BIT((int) log2(in->block_sz)),
> > > > +                  &region->supported_blk_size_bitmask)) {
> > > > +        return CXL_MBOX_INVALID_INPUT;
> > > > +    }      
> > > 
> > > This does not work: test_bit works on unsigned long, while
> > > supported_blk_size_bitmask is uint64_t.
> > > 
> > > Why so funky? what is wrong with:
> > > 
> > > if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask))
> > > 
> > > And BTW why cast to int here?    
> This became obvious when your suggestion didn't build :(
> 
> ./../hw/cxl/cxl-mailbox-utils.c: In function ‘cmd_fm_set_dc_region_config’:
> /home/jic23/src/qemu/include/qemu/bitops.h:25:39: error: invalid operands to binary << (have ‘long long unsigned int’ and ‘double’)
>    25 | #define BIT_ULL(nr)             (1ULL << (nr))
>       |                                       ^~ ~~~~
> ../../hw/cxl/cxl-mailbox-utils.c:3436:11: note: in expansion of macro ‘BIT_ULL’
>  3436 |     if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask)) {
>       |           ^~~~~~~
> 
> Now I look again, this is effectively 2**(log_2(x)) or x. So
> if (in->block_sz & region->supporte_blk_size_bitmask)

it (!(in->block_sz & region->supports_blk_size_bitmask))

I mean.


> Should work as long as we know block_size is a power of 2 (which the specification
> says it must be).
> 
> Anisa?
> 
> > 
> > Change looks fine to me, so I'll prepare an updated set with this
> > and the missing semi colon.  Anisa if you can have a look at this
> > that would be great. 
> > 
> > Sorry I seem to have missed Anisa off the cc for this!
> > 
> > Jonathan
> >   
> 

On Mon, Jul 14, 2025 at 03:16:38PM +0100, Jonathan Cameron wrote:
> On Mon, 14 Jul 2025 15:15:12 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > On Mon, 14 Jul 2025 15:02:18 +0100
> > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > 
> > > On Mon, 14 Jul 2025 05:32:19 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Wed, Jul 02, 2025 at 05:02:13PM +0100, Jonathan Cameron wrote:    
> > > > > From: Anisa Su <anisa.su@samsung.com>
> > > > > 
> > > > > FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
> > > > > 
> > > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>
> > > > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>    
> > > 
> > >   
> > > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > > > index bf1710b251..1fc453f70d 100644
> > > > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > > > +++ b/hw/cxl/cxl-mailbox-utils.c    
> > >   
> > > > > +/* CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602) */
> > > > > +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> > > > > +                                              uint8_t *payload_in,
> > > > > +                                              size_t len_in,
> > > > > +                                              uint8_t *payload_out,
> > > > > +                                              size_t *len_out,
> > > > > +                                              CXLCCI *cci)
> > > > > +{
> > > > > +    struct {
> > > > > +        uint8_t reg_id;
> > > > > +        uint8_t rsvd[3];
> > > > > +        uint64_t block_sz;
> > > > > +        uint8_t flags;
> > > > > +        uint8_t rsvd2[3];
> > > > > +    } QEMU_PACKED *in = (void *)payload_in;
> > > > > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > > > +    CXLEventDynamicCapacity dcEvent = {};
> > > > > +    CXLDCRegion *region = &ct3d->dc.regions[in->reg_id];
> > > > > +
> > > > > +    /*
> > > > > +     * CXL r3.2 7.6.7.6.3: Set DC Region Configuration
> > > > > +     * This command shall fail with Unsupported when the Sanitize on Release
> > > > > +     * field does not match the region’s configuration... and the device
> > > > > +     * does not support reconfiguration of the Sanitize on Release setting.
> > > > > +     *
> > > > > +     * Currently not reconfigurable, so always fail if sanitize bit (bit 0)
> > > > > +     * doesn't match.
> > > > > +     */
> > > > > +    if ((in->flags & 0x1) != (region->flags & 0x1)) {
> > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > +    }
> > > > > +
> > > > > +    if (in->reg_id >= DCD_MAX_NUM_REGION) {
> > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > +    }
> > > > > +
> > > > > +    /* Check that no extents are in the region being reconfigured */
> > > > > +    if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
> > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > +    }
> > > > > +
> > > > > +    /* Check that new block size is supported */
> > > > > +    if (!test_bit(BIT((int) log2(in->block_sz)),
> > > > > +                  &region->supported_blk_size_bitmask)) {
> > > > > +        return CXL_MBOX_INVALID_INPUT;
> > > > > +    }      
> > > > 
> > > > This does not work: test_bit works on unsigned long, while
> > > > supported_blk_size_bitmask is uint64_t.
> > > > 
> > > > Why so funky? what is wrong with:
> > > > 
> > > > if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask))
> > > > 
> > > > And BTW why cast to int here?    
> > This became obvious when your suggestion didn't build :(
> > 
> > ./../hw/cxl/cxl-mailbox-utils.c: In function ‘cmd_fm_set_dc_region_config’:
> > /home/jic23/src/qemu/include/qemu/bitops.h:25:39: error: invalid operands to binary << (have ‘long long unsigned int’ and ‘double’)
> >    25 | #define BIT_ULL(nr)             (1ULL << (nr))
> >       |                                       ^~ ~~~~
> > ../../hw/cxl/cxl-mailbox-utils.c:3436:11: note: in expansion of macro ‘BIT_ULL’
> >  3436 |     if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask)) {
> >       |           ^~~~~~~
> > 
> > Now I look again, this is effectively 2**(log_2(x)) or x. So
> > if (in->block_sz & region->supporte_blk_size_bitmask)
> 
> it (!(in->block_sz & region->supports_blk_size_bitmask))
> 
> I mean.

Make sense to me. 

The only thing is how to detect the violation if the passed in block_sz
is not power of 2.
Or who will do the check if not in qemu?

Fan

> 
> 
> > Should work as long as we know block_size is a power of 2 (which the specification
> > says it must be).
> > 
> > Anisa?
> > 
> > > 
> > > Change looks fine to me, so I'll prepare an updated set with this
> > > and the missing semi colon.  Anisa if you can have a look at this
> > > that would be great. 
> > > 
> > > Sorry I seem to have missed Anisa off the cc for this!
> > > 
> > > Jonathan
> > >   
> > 
> 

-- 
Fan Ni

On Mon, 14 Jul 2025 09:45:31 -0700
Fan Ni <nifan.cxl@gmail.com> wrote:

> On Mon, Jul 14, 2025 at 03:16:38PM +0100, Jonathan Cameron wrote:
> > On Mon, 14 Jul 2025 15:15:12 +0100
> > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> >   
> > > On Mon, 14 Jul 2025 15:02:18 +0100
> > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > >   
> > > > On Mon, 14 Jul 2025 05:32:19 -0400
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > On Wed, Jul 02, 2025 at 05:02:13PM +0100, Jonathan Cameron wrote:      
> > > > > > From: Anisa Su <anisa.su@samsung.com>
> > > > > > 
> > > > > > FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
> > > > > > 
> > > > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>
> > > > > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>      
> > > > 
> > > >     
> > > > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > > > > index bf1710b251..1fc453f70d 100644
> > > > > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > > > > +++ b/hw/cxl/cxl-mailbox-utils.c      
> > > >     
> > > > > > +/* CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602) */
> > > > > > +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> > > > > > +                                              uint8_t *payload_in,
> > > > > > +                                              size_t len_in,
> > > > > > +                                              uint8_t *payload_out,
> > > > > > +                                              size_t *len_out,
> > > > > > +                                              CXLCCI *cci)
> > > > > > +{
> > > > > > +    struct {
> > > > > > +        uint8_t reg_id;
> > > > > > +        uint8_t rsvd[3];
> > > > > > +        uint64_t block_sz;
> > > > > > +        uint8_t flags;
> > > > > > +        uint8_t rsvd2[3];
> > > > > > +    } QEMU_PACKED *in = (void *)payload_in;
> > > > > > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > > > > +    CXLEventDynamicCapacity dcEvent = {};
> > > > > > +    CXLDCRegion *region = &ct3d->dc.regions[in->reg_id];
> > > > > > +
> > > > > > +    /*
> > > > > > +     * CXL r3.2 7.6.7.6.3: Set DC Region Configuration
> > > > > > +     * This command shall fail with Unsupported when the Sanitize on Release
> > > > > > +     * field does not match the region’s configuration... and the device
> > > > > > +     * does not support reconfiguration of the Sanitize on Release setting.
> > > > > > +     *
> > > > > > +     * Currently not reconfigurable, so always fail if sanitize bit (bit 0)
> > > > > > +     * doesn't match.
> > > > > > +     */
> > > > > > +    if ((in->flags & 0x1) != (region->flags & 0x1)) {
> > > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (in->reg_id >= DCD_MAX_NUM_REGION) {
> > > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* Check that no extents are in the region being reconfigured */
> > > > > > +    if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
> > > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* Check that new block size is supported */
> > > > > > +    if (!test_bit(BIT((int) log2(in->block_sz)),
> > > > > > +                  &region->supported_blk_size_bitmask)) {
> > > > > > +        return CXL_MBOX_INVALID_INPUT;
> > > > > > +    }        
> > > > > 
> > > > > This does not work: test_bit works on unsigned long, while
> > > > > supported_blk_size_bitmask is uint64_t.
> > > > > 
> > > > > Why so funky? what is wrong with:
> > > > > 
> > > > > if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask))
> > > > > 
> > > > > And BTW why cast to int here?      
> > > This became obvious when your suggestion didn't build :(
> > > 
> > > ./../hw/cxl/cxl-mailbox-utils.c: In function ‘cmd_fm_set_dc_region_config’:
> > > /home/jic23/src/qemu/include/qemu/bitops.h:25:39: error: invalid operands to binary << (have ‘long long unsigned int’ and ‘double’)
> > >    25 | #define BIT_ULL(nr)             (1ULL << (nr))
> > >       |                                       ^~ ~~~~
> > > ../../hw/cxl/cxl-mailbox-utils.c:3436:11: note: in expansion of macro ‘BIT_ULL’
> > >  3436 |     if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask)) {
> > >       |           ^~~~~~~
> > > 
> > > Now I look again, this is effectively 2**(log_2(x)) or x. So
> > > if (in->block_sz & region->supporte_blk_size_bitmask)  
> > 
> > it (!(in->block_sz & region->supports_blk_size_bitmask))
> > 
> > I mean.  
> 
> Make sense to me. 
> 
> The only thing is how to detect the violation if the passed in block_sz
> is not power of 2.
> Or who will do the check if not in qemu?
Hi Fan,

I checked the spec on this.  There isn't an explicit statement that the device
should return an error on this. Looks to be impdef. I'd happily see such
a check as a usability improvement though!

I'm just not set up to test this right now so decided to be
risk averse and not trying adding one. 

If you want to send a patch on top I'd be happy to add it.

Jonathan



> 
> Fan
> 
> > 
> >   
> > > Should work as long as we know block_size is a power of 2 (which the specification
> > > says it must be).
> > > 
> > > Anisa?
> > >   
> > > > 
> > > > Change looks fine to me, so I'll prepare an updated set with this
> > > > and the missing semi colon.  Anisa if you can have a look at this
> > > > that would be great. 
> > > > 
> > > > Sorry I seem to have missed Anisa off the cc for this!
> > > > 
> > > > Jonathan
> > > >     
> > >   
> >   
> 

On Mon, Jul 14, 2025 at 06:02:26PM +0100, Jonathan Cameron wrote:
> On Mon, 14 Jul 2025 09:45:31 -0700
> Fan Ni <nifan.cxl@gmail.com> wrote:
> 
> > On Mon, Jul 14, 2025 at 03:16:38PM +0100, Jonathan Cameron wrote:
> > > On Mon, 14 Jul 2025 15:15:12 +0100
> > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > >   
> > > > On Mon, 14 Jul 2025 15:02:18 +0100
> > > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > > >   
> > > > > On Mon, 14 Jul 2025 05:32:19 -0400
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >     
> > > > > > On Wed, Jul 02, 2025 at 05:02:13PM +0100, Jonathan Cameron wrote:      
> > > > > > > From: Anisa Su <anisa.su@samsung.com>
> > > > > > > 
> > > > > > > FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
> > > > > > > 
> > > > > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>
> > > > > > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>      
> > > > > 
> > > > >     
> > > > > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > > > > > index bf1710b251..1fc453f70d 100644
> > > > > > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > > > > > +++ b/hw/cxl/cxl-mailbox-utils.c      
> > > > >     
> > > > > > > +/* CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602) */
> > > > > > > +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> > > > > > > +                                              uint8_t *payload_in,
> > > > > > > +                                              size_t len_in,
> > > > > > > +                                              uint8_t *payload_out,
> > > > > > > +                                              size_t *len_out,
> > > > > > > +                                              CXLCCI *cci)
> > > > > > > +{
> > > > > > > +    struct {
> > > > > > > +        uint8_t reg_id;
> > > > > > > +        uint8_t rsvd[3];
> > > > > > > +        uint64_t block_sz;
> > > > > > > +        uint8_t flags;
> > > > > > > +        uint8_t rsvd2[3];
> > > > > > > +    } QEMU_PACKED *in = (void *)payload_in;
> > > > > > > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > > > > > +    CXLEventDynamicCapacity dcEvent = {};
> > > > > > > +    CXLDCRegion *region = &ct3d->dc.regions[in->reg_id];
> > > > > > > +
> > > > > > > +    /*
> > > > > > > +     * CXL r3.2 7.6.7.6.3: Set DC Region Configuration
> > > > > > > +     * This command shall fail with Unsupported when the Sanitize on Release
> > > > > > > +     * field does not match the region’s configuration... and the device
> > > > > > > +     * does not support reconfiguration of the Sanitize on Release setting.
> > > > > > > +     *
> > > > > > > +     * Currently not reconfigurable, so always fail if sanitize bit (bit 0)
> > > > > > > +     * doesn't match.
> > > > > > > +     */
> > > > > > > +    if ((in->flags & 0x1) != (region->flags & 0x1)) {
> > > > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    if (in->reg_id >= DCD_MAX_NUM_REGION) {
> > > > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    /* Check that no extents are in the region being reconfigured */
> > > > > > > +    if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
> > > > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    /* Check that new block size is supported */
> > > > > > > +    if (!test_bit(BIT((int) log2(in->block_sz)),
> > > > > > > +                  &region->supported_blk_size_bitmask)) {
> > > > > > > +        return CXL_MBOX_INVALID_INPUT;
> > > > > > > +    }        
> > > > > > 
> > > > > > This does not work: test_bit works on unsigned long, while
> > > > > > supported_blk_size_bitmask is uint64_t.
> > > > > > 
> > > > > > Why so funky? what is wrong with:
> > > > > > 
> > > > > > if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask))
> > > > > > 
> > > > > > And BTW why cast to int here?      
> > > > This became obvious when your suggestion didn't build :(
> > > > 
> > > > ./../hw/cxl/cxl-mailbox-utils.c: In function ‘cmd_fm_set_dc_region_config’:
> > > > /home/jic23/src/qemu/include/qemu/bitops.h:25:39: error: invalid operands to binary << (have ‘long long unsigned int’ and ‘double’)
> > > >    25 | #define BIT_ULL(nr)             (1ULL << (nr))
> > > >       |                                       ^~ ~~~~
> > > > ../../hw/cxl/cxl-mailbox-utils.c:3436:11: note: in expansion of macro ‘BIT_ULL’
> > > >  3436 |     if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask)) {
> > > >       |           ^~~~~~~
> > > > 
> > > > Now I look again, this is effectively 2**(log_2(x)) or x. So
> > > > if (in->block_sz & region->supporte_blk_size_bitmask)  
> > > 
> > > it (!(in->block_sz & region->supports_blk_size_bitmask))
> > > 
> > > I mean.  
> > 
> > Make sense to me. 
> > 
> > The only thing is how to detect the violation if the passed in block_sz
> > is not power of 2.
> > Or who will do the check if not in qemu?
> Hi Fan,
> 
> I checked the spec on this.  There isn't an explicit statement that the device
> should return an error on this. Looks to be impdef. I'd happily see such
> a check as a usability improvement though!
> 
> I'm just not set up to test this right now so decided to be
> risk averse and not trying adding one. 
> 
> If you want to send a patch on top I'd be happy to add it.
> 
> Jonathan
> 
> 
Would something like this work?

/* Check that new block size is supported */
if (!is_power_of_2(in->block_sz) ||
    !(in->block_sz & region->supported_blk_size_bitmask)) {
    return CXL_MBOX_INVALID_INPUT;
}

I did not have to add any extra headers to use this helper function in
include/qemu/host-utils.h and it compiles for me.

Anisa
> 
> > 
> > Fan
> > 
> > > 
> > >   
> > > > Should work as long as we know block_size is a power of 2 (which the specification
> > > > says it must be).
> > > > 
> > > > Anisa?
> > > >   
> > > > > 
> > > > > Change looks fine to me, so I'll prepare an updated set with this
> > > > > and the missing semi colon.  Anisa if you can have a look at this
> > > > > that would be great. 
> > > > > 
> > > > > Sorry I seem to have missed Anisa off the cc for this!
> > > > > 
> > > > > Jonathan
> > > > >     
> > > >   
> > >   
> > 
> 

On Mon, 14 Jul 2025 17:21:10 +0000
Anisa Su <anisa.su887@gmail.com> wrote:

> On Mon, Jul 14, 2025 at 06:02:26PM +0100, Jonathan Cameron wrote:
> > On Mon, 14 Jul 2025 09:45:31 -0700
> > Fan Ni <nifan.cxl@gmail.com> wrote:
> >   
> > > On Mon, Jul 14, 2025 at 03:16:38PM +0100, Jonathan Cameron wrote:  
> > > > On Mon, 14 Jul 2025 15:15:12 +0100
> > > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > > >     
> > > > > On Mon, 14 Jul 2025 15:02:18 +0100
> > > > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > > > >     
> > > > > > On Mon, 14 Jul 2025 05:32:19 -0400
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > >       
> > > > > > > On Wed, Jul 02, 2025 at 05:02:13PM +0100, Jonathan Cameron wrote:        
> > > > > > > > From: Anisa Su <anisa.su@samsung.com>
> > > > > > > > 
> > > > > > > > FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
> > > > > > > > 
> > > > > > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>
> > > > > > > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>        
> > > > > > 
> > > > > >       
> > > > > > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > > > > > > index bf1710b251..1fc453f70d 100644
> > > > > > > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > > > > > > +++ b/hw/cxl/cxl-mailbox-utils.c        
> > > > > >       
> > > > > > > > +/* CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration (Opcode 5602) */
> > > > > > > > +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd *cmd,
> > > > > > > > +                                              uint8_t *payload_in,
> > > > > > > > +                                              size_t len_in,
> > > > > > > > +                                              uint8_t *payload_out,
> > > > > > > > +                                              size_t *len_out,
> > > > > > > > +                                              CXLCCI *cci)
> > > > > > > > +{
> > > > > > > > +    struct {
> > > > > > > > +        uint8_t reg_id;
> > > > > > > > +        uint8_t rsvd[3];
> > > > > > > > +        uint64_t block_sz;
> > > > > > > > +        uint8_t flags;
> > > > > > > > +        uint8_t rsvd2[3];
> > > > > > > > +    } QEMU_PACKED *in = (void *)payload_in;
> > > > > > > > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > > > > > > +    CXLEventDynamicCapacity dcEvent = {};
> > > > > > > > +    CXLDCRegion *region = &ct3d->dc.regions[in->reg_id];
> > > > > > > > +
> > > > > > > > +    /*
> > > > > > > > +     * CXL r3.2 7.6.7.6.3: Set DC Region Configuration
> > > > > > > > +     * This command shall fail with Unsupported when the Sanitize on Release
> > > > > > > > +     * field does not match the region’s configuration... and the device
> > > > > > > > +     * does not support reconfiguration of the Sanitize on Release setting.
> > > > > > > > +     *
> > > > > > > > +     * Currently not reconfigurable, so always fail if sanitize bit (bit 0)
> > > > > > > > +     * doesn't match.
> > > > > > > > +     */
> > > > > > > > +    if ((in->flags & 0x1) != (region->flags & 0x1)) {
> > > > > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    if (in->reg_id >= DCD_MAX_NUM_REGION) {
> > > > > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    /* Check that no extents are in the region being reconfigured */
> > > > > > > > +    if (!bitmap_empty(region->blk_bitmap, region->len / region->block_size)) {
> > > > > > > > +        return CXL_MBOX_UNSUPPORTED;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    /* Check that new block size is supported */
> > > > > > > > +    if (!test_bit(BIT((int) log2(in->block_sz)),
> > > > > > > > +                  &region->supported_blk_size_bitmask)) {
> > > > > > > > +        return CXL_MBOX_INVALID_INPUT;
> > > > > > > > +    }          
> > > > > > > 
> > > > > > > This does not work: test_bit works on unsigned long, while
> > > > > > > supported_blk_size_bitmask is uint64_t.
> > > > > > > 
> > > > > > > Why so funky? what is wrong with:
> > > > > > > 
> > > > > > > if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask))
> > > > > > > 
> > > > > > > And BTW why cast to int here?        
> > > > > This became obvious when your suggestion didn't build :(
> > > > > 
> > > > > ./../hw/cxl/cxl-mailbox-utils.c: In function ‘cmd_fm_set_dc_region_config’:
> > > > > /home/jic23/src/qemu/include/qemu/bitops.h:25:39: error: invalid operands to binary << (have ‘long long unsigned int’ and ‘double’)
> > > > >    25 | #define BIT_ULL(nr)             (1ULL << (nr))
> > > > >       |                                       ^~ ~~~~
> > > > > ../../hw/cxl/cxl-mailbox-utils.c:3436:11: note: in expansion of macro ‘BIT_ULL’
> > > > >  3436 |     if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask)) {
> > > > >       |           ^~~~~~~
> > > > > 
> > > > > Now I look again, this is effectively 2**(log_2(x)) or x. So
> > > > > if (in->block_sz & region->supporte_blk_size_bitmask)    
> > > > 
> > > > it (!(in->block_sz & region->supports_blk_size_bitmask))
> > > > 
> > > > I mean.    
> > > 
> > > Make sense to me. 
> > > 
> > > The only thing is how to detect the violation if the passed in block_sz
> > > is not power of 2.
> > > Or who will do the check if not in qemu?  
> > Hi Fan,
> > 
> > I checked the spec on this.  There isn't an explicit statement that the device
> > should return an error on this. Looks to be impdef. I'd happily see such
> > a check as a usability improvement though!
> > 
> > I'm just not set up to test this right now so decided to be
> > risk averse and not trying adding one. 
> > 
> > If you want to send a patch on top I'd be happy to add it.
> > 
> > Jonathan
> > 
> >   
> Would something like this work?
> 
> /* Check that new block size is supported */
> if (!is_power_of_2(in->block_sz) ||
>     !(in->block_sz & region->supported_blk_size_bitmask)) {
>     return CXL_MBOX_INVALID_INPUT;
> }
> 
> I did not have to add any extra headers to use this helper function in
> include/qemu/host-utils.h and it compiles for me.
Nice.  I knew I was too tired today!

I'll roll that in as seems safe enough.
V2 coming shortly.

J
> 
> Anisa
> >   
> > > 
> > > Fan
> > >   
> > > > 
> > > >     
> > > > > Should work as long as we know block_size is a power of 2 (which the specification
> > > > > says it must be).
> > > > > 
> > > > > Anisa?
> > > > >     
> > > > > > 
> > > > > > Change looks fine to me, so I'll prepare an updated set with this
> > > > > > and the missing semi colon.  Anisa if you can have a look at this
> > > > > > that would be great. 
> > > > > > 
> > > > > > Sorry I seem to have missed Anisa off the cc for this!
> > > > > > 
> > > > > > Jonathan
> > > > > >       
> > > > >     
> > > >     
> > >   
> >   
> 
>