From: Fan Ni <fan.ni@samsung.com>

Per cxl spec 3.0, add dynamic capacity region representative based on
Table 8-126 and extend the cxl type3 device definition to include dc region
information. Also, based on info in 8.2.9.8.9.1, add 'Get Dynamic Capacity
Configuration' mailbox support.

Note: decode_len of a dc region is aligned to 256*MiB, need to be divided by
256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
mailbox command.

Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
 hw/cxl/cxl-mailbox-utils.c  | 80 +++++++++++++++++++++++++++++++++++++
 hw/mem/cxl_type3.c          |  6 +++
 include/hw/cxl/cxl_device.h | 17 ++++++++
 3 files changed, 103 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 8eceedfa87..f80dd6474f 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -80,6 +80,8 @@ enum {
         #define GET_POISON_LIST        0x0
         #define INJECT_POISON          0x1
         #define CLEAR_POISON           0x2
+    DCD_CONFIG  = 0x48,
+        #define GET_DC_CONFIG          0x0
     PHYSICAL_SWITCH = 0x51,
         #define IDENTIFY_SWITCH_DEVICE      0x0
         #define GET_PHYSICAL_PORT_STATE     0x1
@@ -1210,6 +1212,74 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+/*
+ * CXL r3.0 section 8.2.9.8.9.1: Get Dynamic Capacity Configuration
+ * (Opcode: 4800h)
+ */
+static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
+                                             uint8_t *payload_in,
+                                             size_t len_in,
+                                             uint8_t *payload_out,
+                                             size_t *len_out,
+                                             CXLCCI *cci)
+{
+    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+    struct get_dyn_cap_config_in_pl {
+        uint8_t region_cnt;
+        uint8_t start_region_id;
+    } QEMU_PACKED;
+
+    struct get_dyn_cap_config_out_pl {
+        uint8_t num_regions;
+        uint8_t rsvd1[7];
+        struct {
+            uint64_t base;
+            uint64_t decode_len;
+            uint64_t region_len;
+            uint64_t block_size;
+            uint32_t dsmadhandle;
+            uint8_t flags;
+            uint8_t rsvd2[3];
+        } QEMU_PACKED records[];
+    } QEMU_PACKED;
+
+    struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
+    struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
+    uint16_t record_count = 0, i;
+    uint16_t out_pl_len;
+    uint8_t start_region_id = in->start_region_id;
+
+    if (start_region_id >= ct3d->dc.num_regions) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
+            in->region_cnt);
+
+    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
+    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
+
+    memset(out, 0, out_pl_len);
+    out->num_regions = record_count;
+    for (i = 0; i < record_count; i++) {
+        stq_le_p(&out->records[i].base,
+                ct3d->dc.regions[start_region_id + i].base);
+        stq_le_p(&out->records[i].decode_len,
+                ct3d->dc.regions[start_region_id + i].decode_len /
+                CXL_CAPACITY_MULTIPLIER);
+        stq_le_p(&out->records[i].region_len,
+                ct3d->dc.regions[start_region_id + i].len);
+        stq_le_p(&out->records[i].block_size,
+                ct3d->dc.regions[start_region_id + i].block_size);
+        stl_le_p(&out->records[i].dsmadhandle,
+                ct3d->dc.regions[start_region_id + i].dsmadhandle);
+        out->records[i].flags = ct3d->dc.regions[start_region_id + i].flags;
+    }
+
+    *len_out = out_pl_len;
+    return CXL_MBOX_SUCCESS;
+}
+
 #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
 #define IMMEDIATE_DATA_CHANGE (1 << 2)
 #define IMMEDIATE_POLICY_CHANGE (1 << 3)
@@ -1254,6 +1324,11 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = {
         cmd_media_clear_poison, 72, 0 },
 };
 
+static const struct cxl_cmd cxl_cmd_set_dcd[256][256] = {
+    [DCD_CONFIG][GET_DC_CONFIG] = { "DCD_GET_DC_CONFIG",
+        cmd_dcd_get_dyn_cap_config, 2, 0 },
+};
+
 static const struct cxl_cmd cxl_cmd_set_sw[256][256] = {
     [INFOSTAT][IS_IDENTIFY] = { "IDENTIFY", cmd_infostat_identify, 0, 18 },
     [INFOSTAT][BACKGROUND_OPERATION_STATUS] = { "BACKGROUND_OPERATION_STATUS",
@@ -1465,7 +1540,12 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
 
 void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
 {
+    CXLType3Dev *ct3d = CXL_TYPE3(d);
+
     cxl_copy_cci_commands(cci, cxl_cmd_set);
+    if (ct3d->dc.num_regions) {
+        cxl_copy_cci_commands(cci, cxl_cmd_set_dcd);
+    }
     cci->d = d;
 
     /* No separation for PCI MB as protocol handled in PCI device */
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 7b4d1ee774..6c1ccda159 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1075,6 +1075,12 @@ static void ct3d_reset(DeviceState *dev)
     uint32_t *reg_state = ct3d->cxl_cstate.crb.cache_mem_registers;
     uint32_t *write_msk = ct3d->cxl_cstate.crb.cache_mem_regs_write_mask;
 
+    if (ct3d->dc.num_regions) {
+        ct3d->cxl_dstate.is_dcd = true;
+    } else {
+        ct3d->cxl_dstate.is_dcd = false;
+    }
+
     cxl_component_register_init_common(reg_state, write_msk, CXL2_TYPE3_DEVICE);
     cxl_device_register_init_t3(ct3d);
 
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 4f2ef0b899..334c51fddb 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -235,6 +235,7 @@ typedef struct cxl_device_state {
     uint64_t mem_size;
     uint64_t pmem_size;
     uint64_t vmem_size;
+    bool is_dcd;
 
     const struct cxl_cmd (*cxl_cmd_set)[256];
     CPMUState cpmu[CXL_NUM_CPMU_INSTANCES];
@@ -417,6 +418,17 @@ typedef struct CXLPoison {
 typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
 #define CXL_POISON_LIST_LIMIT 256
 
+#define DCD_MAX_REGION_NUM 8
+
+typedef struct CXLDCDRegion {
+    uint64_t base;
+    uint64_t decode_len; /* aligned to 256*MiB */
+    uint64_t len;
+    uint64_t block_size;
+    uint32_t dsmadhandle;
+    uint8_t flags;
+} CXLDCDRegion;
+
 struct CXLType3Dev {
     /* Private */
     PCIDevice parent_obj;
@@ -453,6 +465,11 @@ struct CXLType3Dev {
     unsigned int poison_list_cnt;
     bool poison_list_overflowed;
     uint64_t poison_list_overflow_ts;
+
+    struct dynamic_capacity {
+        uint8_t num_regions; /* 0-8 regions */
+        CXLDCDRegion regions[DCD_MAX_REGION_NUM];
+    } dc;
 };
 
 #define TYPE_CXL_TYPE3 "cxl-type3"
-- 
2.42.0

> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 4f2ef0b899..334c51fddb 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -235,6 +235,7 @@ typedef struct cxl_device_state {
>      uint64_t mem_size;
>      uint64_t pmem_size;
>      uint64_t vmem_size;
> +    bool is_dcd;
Written but never read, so drop this.

On Tue,  7 Nov 2023 10:07:06 -0800
nifan.cxl@gmail.com wrote:

> From: Fan Ni <fan.ni@samsung.com>
> 
> Per cxl spec 3.0, add dynamic capacity region representative based on
> Table 8-126 and extend the cxl type3 device definition to include dc region
> information. Also, based on info in 8.2.9.8.9.1, add 'Get Dynamic Capacity
> Configuration' mailbox support.
> 
> Note: decode_len of a dc region is aligned to 256*MiB, need to be divided by
> 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> mailbox command.
> 
> Signed-off-by: Fan Ni <fan.ni@samsung.com>

Hi Fan,

I'm looking at how to move these much earlier in my tree on basis that
they should be our main focus for merging in this QEMU cycle.

Whilst I do that rebase, I'm taking a closer look at the code.
I'm targetting rebasing on upstream qemu + the two patch sets I just
sent out:
[PATCH 00/12 qemu] CXL emulation fixes and minor cleanup. 
[PATCH 0/5 qemu] hw/cxl: Update CXL emulation to reflect and reference r3.1

It would be good to document why these commands should be optional (which I think
comes down to the annoying fact that Get Dynamic Capacity Configuration isn't
allowed to return 0 regions, but instead should not be available as a command
if DCD isn't supported.

Note this requires us to carry Gregory's patches to make the CCI command list
constructed at runtime rather than baked in ahead of this set.

So another question is should we jump directly to the r3.1 version of DCD?
I think we probably should as it includes some additions that are necessary
for a bunch of the potential use cases.


> ---
>  hw/cxl/cxl-mailbox-utils.c  | 80 +++++++++++++++++++++++++++++++++++++
>  hw/mem/cxl_type3.c          |  6 +++
>  include/hw/cxl/cxl_device.h | 17 ++++++++
>  3 files changed, 103 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 8eceedfa87..f80dd6474f 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -80,6 +80,8 @@ enum {
>          #define GET_POISON_LIST        0x0
>          #define INJECT_POISON          0x1
>          #define CLEAR_POISON           0x2
> +    DCD_CONFIG  = 0x48,
> +        #define GET_DC_CONFIG          0x0
>      PHYSICAL_SWITCH = 0x51,
>          #define IDENTIFY_SWITCH_DEVICE      0x0
>          #define GET_PHYSICAL_PORT_STATE     0x1
> @@ -1210,6 +1212,74 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +/*
> + * CXL r3.0 section 8.2.9.8.9.1: Get Dynamic Capacity Configuration

As per the patch set I just sent out, I want to standardize on references
to r3.1 because it's all that is easy to get.  However if we decide to r3.0
DCD first the upgrade it later, then clearly these need to stick to r3.0 for
now.

> + * (Opcode: 4800h)
> + */
> +static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> +                                             uint8_t *payload_in,
> +                                             size_t len_in,
> +                                             uint8_t *payload_out,
> +                                             size_t *len_out,
> +                                             CXLCCI *cci)
> +{
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +    struct get_dyn_cap_config_in_pl {
> +        uint8_t region_cnt;
> +        uint8_t start_region_id;
> +    } QEMU_PACKED;
> +
> +    struct get_dyn_cap_config_out_pl {
> +        uint8_t num_regions;
> +        uint8_t rsvd1[7];

This changed in r3.1 (errata? - I haven't checked)
Should be 'regions returned' in first byte.

> +        struct {
> +            uint64_t base;
> +            uint64_t decode_len;
> +            uint64_t region_len;
> +            uint64_t block_size;
> +            uint32_t dsmadhandle;

> +            uint8_t flags;
> +            uint8_t rsvd2[3];
> +        } QEMU_PACKED records[];

There are two fields after this as well.
Total number of supported extents and number of available extents.

That annoyingly means we can't use the structure to tell us where
to find all the fields...


> +    } QEMU_PACKED;
> +
> +    struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
> +    struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
> +    uint16_t record_count = 0, i;

Better to split that on to 2 lines. Never hide setting a value
in the middle of a set of declarations.

> +    uint16_t out_pl_len;
> +    uint8_t start_region_id = in->start_region_id;
> +
> +    if (start_region_id >= ct3d->dc.num_regions) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
> +            in->region_cnt);
> +
> +    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);

For r3.1 + 8 for the two trailing fields.

> +    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> +
> +    memset(out, 0, out_pl_len);

As part of the cci rework we started zeroing the whole mailbox payload space
after copying out the input payload.
https://elixir.bootlin.com/qemu/latest/source/hw/cxl/cxl-device-utils.c#L204

So shouldn't need this (unless we have a bug)

Jonathan

On Wed, Jan 24, 2024 at 02:51:18PM +0000, Jonathan Cameron wrote:
> On Tue,  7 Nov 2023 10:07:06 -0800
> nifan.cxl@gmail.com wrote:
> 
> > From: Fan Ni <fan.ni@samsung.com>
> > 
> > Per cxl spec 3.0, add dynamic capacity region representative based on
> > Table 8-126 and extend the cxl type3 device definition to include dc region
> > information. Also, based on info in 8.2.9.8.9.1, add 'Get Dynamic Capacity
> > Configuration' mailbox support.
> > 
> > Note: decode_len of a dc region is aligned to 256*MiB, need to be divided by
> > 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> > mailbox command.
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> 
> Hi Fan,
> 
> I'm looking at how to move these much earlier in my tree on basis that
> they should be our main focus for merging in this QEMU cycle.
> 
> Whilst I do that rebase, I'm taking a closer look at the code.
> I'm targetting rebasing on upstream qemu + the two patch sets I just
> sent out:
> [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup. 
> [PATCH 0/5 qemu] hw/cxl: Update CXL emulation to reflect and reference r3.1
> 
> It would be good to document why these commands should be optional (which I think
> comes down to the annoying fact that Get Dynamic Capacity Configuration isn't
> allowed to return 0 regions, but instead should not be available as a command
> if DCD isn't supported.
> 
> Note this requires us to carry Gregory's patches to make the CCI command list
> constructed at runtime rather than baked in ahead of this set.
> 
> So another question is should we jump directly to the r3.1 version of DCD?
> I think we probably should as it includes some additions that are necessary
> for a bunch of the potential use cases.
> 

Based on cxl spec r3.1, the get dynamic capacity configuration output
payload (Table 8-164) have 4 extra items after the variable region configuration
structure. That is not allowed by the compiler, should we move the
new-added 4 items before the variable region configuration structures?

Fan

> 
> > ---
> >  hw/cxl/cxl-mailbox-utils.c  | 80 +++++++++++++++++++++++++++++++++++++
> >  hw/mem/cxl_type3.c          |  6 +++
> >  include/hw/cxl/cxl_device.h | 17 ++++++++
> >  3 files changed, 103 insertions(+)
> > 
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 8eceedfa87..f80dd6474f 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -80,6 +80,8 @@ enum {
> >          #define GET_POISON_LIST        0x0
> >          #define INJECT_POISON          0x1
> >          #define CLEAR_POISON           0x2
> > +    DCD_CONFIG  = 0x48,
> > +        #define GET_DC_CONFIG          0x0
> >      PHYSICAL_SWITCH = 0x51,
> >          #define IDENTIFY_SWITCH_DEVICE      0x0
> >          #define GET_PHYSICAL_PORT_STATE     0x1
> > @@ -1210,6 +1212,74 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
> >      return CXL_MBOX_SUCCESS;
> >  }
> >  
> > +/*
> > + * CXL r3.0 section 8.2.9.8.9.1: Get Dynamic Capacity Configuration
> 
> As per the patch set I just sent out, I want to standardize on references
> to r3.1 because it's all that is easy to get.  However if we decide to r3.0
> DCD first the upgrade it later, then clearly these need to stick to r3.0 for
> now.
> 
> > + * (Opcode: 4800h)
> > + */
> > +static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> > +                                             uint8_t *payload_in,
> > +                                             size_t len_in,
> > +                                             uint8_t *payload_out,
> > +                                             size_t *len_out,
> > +                                             CXLCCI *cci)
> > +{
> > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > +    struct get_dyn_cap_config_in_pl {
> > +        uint8_t region_cnt;
> > +        uint8_t start_region_id;
> > +    } QEMU_PACKED;
> > +
> > +    struct get_dyn_cap_config_out_pl {
> > +        uint8_t num_regions;
> > +        uint8_t rsvd1[7];
> 
> This changed in r3.1 (errata? - I haven't checked)
> Should be 'regions returned' in first byte.
> 
> > +        struct {
> > +            uint64_t base;
> > +            uint64_t decode_len;
> > +            uint64_t region_len;
> > +            uint64_t block_size;
> > +            uint32_t dsmadhandle;
> 
> > +            uint8_t flags;
> > +            uint8_t rsvd2[3];
> > +        } QEMU_PACKED records[];
> 
> There are two fields after this as well.
> Total number of supported extents and number of available extents.
> 
> That annoyingly means we can't use the structure to tell us where
> to find all the fields...
> 
> 
> > +    } QEMU_PACKED;
> > +
> > +    struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
> > +    struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
> > +    uint16_t record_count = 0, i;
> 
> Better to split that on to 2 lines. Never hide setting a value
> in the middle of a set of declarations.
> 
> > +    uint16_t out_pl_len;
> > +    uint8_t start_region_id = in->start_region_id;
> > +
> > +    if (start_region_id >= ct3d->dc.num_regions) {
> > +        return CXL_MBOX_INVALID_INPUT;
> > +    }
> > +
> > +    record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
> > +            in->region_cnt);
> > +
> > +    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> 
> For r3.1 + 8 for the two trailing fields.
> 
> > +    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> > +
> > +    memset(out, 0, out_pl_len);
> 
> As part of the cci rework we started zeroing the whole mailbox payload space
> after copying out the input payload.
> https://elixir.bootlin.com/qemu/latest/source/hw/cxl/cxl-device-utils.c#L204
> 
> So shouldn't need this (unless we have a bug)
> 
> Jonathan

On Thu, 1 Feb 2024 11:58:43 -0800
fan <nifan.cxl@gmail.com> wrote:

> On Wed, Jan 24, 2024 at 02:51:18PM +0000, Jonathan Cameron wrote:
> > On Tue,  7 Nov 2023 10:07:06 -0800
> > nifan.cxl@gmail.com wrote:
> >   
> > > From: Fan Ni <fan.ni@samsung.com>
> > > 
> > > Per cxl spec 3.0, add dynamic capacity region representative based on
> > > Table 8-126 and extend the cxl type3 device definition to include dc region
> > > information. Also, based on info in 8.2.9.8.9.1, add 'Get Dynamic Capacity
> > > Configuration' mailbox support.
> > > 
> > > Note: decode_len of a dc region is aligned to 256*MiB, need to be divided by
> > > 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> > > mailbox command.
> > > 
> > > Signed-off-by: Fan Ni <fan.ni@samsung.com>  
> > 
> > Hi Fan,
> > 
> > I'm looking at how to move these much earlier in my tree on basis that
> > they should be our main focus for merging in this QEMU cycle.
> > 
> > Whilst I do that rebase, I'm taking a closer look at the code.
> > I'm targetting rebasing on upstream qemu + the two patch sets I just
> > sent out:
> > [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup. 
> > [PATCH 0/5 qemu] hw/cxl: Update CXL emulation to reflect and reference r3.1
> > 
> > It would be good to document why these commands should be optional (which I think
> > comes down to the annoying fact that Get Dynamic Capacity Configuration isn't
> > allowed to return 0 regions, but instead should not be available as a command
> > if DCD isn't supported.
> > 
> > Note this requires us to carry Gregory's patches to make the CCI command list
> > constructed at runtime rather than baked in ahead of this set.
> > 
> > So another question is should we jump directly to the r3.1 version of DCD?
> > I think we probably should as it includes some additions that are necessary
> > for a bunch of the potential use cases.
> >   
> 
> Based on cxl spec r3.1, the get dynamic capacity configuration output
> payload (Table 8-164) have 4 extra items after the variable region configuration
> structure. That is not allowed by the compiler, should we move the
> new-added 4 items before the variable region configuration structures?

You will just need to manage that size explicitly rather than using a variable
element at the end.  Add some helpers to find the offset in the structure
and it shouldn't be too ugly.

Can't reorganize it just because they made the spec hideous :(

> 
> Fan
> 
> >   
> > > ---
> > >  hw/cxl/cxl-mailbox-utils.c  | 80 +++++++++++++++++++++++++++++++++++++
> > >  hw/mem/cxl_type3.c          |  6 +++
> > >  include/hw/cxl/cxl_device.h | 17 ++++++++
> > >  3 files changed, 103 insertions(+)
> > > 
> > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > index 8eceedfa87..f80dd6474f 100644
> > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > @@ -80,6 +80,8 @@ enum {
> > >          #define GET_POISON_LIST        0x0
> > >          #define INJECT_POISON          0x1
> > >          #define CLEAR_POISON           0x2
> > > +    DCD_CONFIG  = 0x48,
> > > +        #define GET_DC_CONFIG          0x0
> > >      PHYSICAL_SWITCH = 0x51,
> > >          #define IDENTIFY_SWITCH_DEVICE      0x0
> > >          #define GET_PHYSICAL_PORT_STATE     0x1
> > > @@ -1210,6 +1212,74 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
> > >      return CXL_MBOX_SUCCESS;
> > >  }
> > >  
> > > +/*
> > > + * CXL r3.0 section 8.2.9.8.9.1: Get Dynamic Capacity Configuration  
> > 
> > As per the patch set I just sent out, I want to standardize on references
> > to r3.1 because it's all that is easy to get.  However if we decide to r3.0
> > DCD first the upgrade it later, then clearly these need to stick to r3.0 for
> > now.
> >   
> > > + * (Opcode: 4800h)
> > > + */
> > > +static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> > > +                                             uint8_t *payload_in,
> > > +                                             size_t len_in,
> > > +                                             uint8_t *payload_out,
> > > +                                             size_t *len_out,
> > > +                                             CXLCCI *cci)
> > > +{
> > > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > +    struct get_dyn_cap_config_in_pl {
> > > +        uint8_t region_cnt;
> > > +        uint8_t start_region_id;
> > > +    } QEMU_PACKED;
> > > +
> > > +    struct get_dyn_cap_config_out_pl {
> > > +        uint8_t num_regions;
> > > +        uint8_t rsvd1[7];  
> > 
> > This changed in r3.1 (errata? - I haven't checked)
> > Should be 'regions returned' in first byte.
> >   
> > > +        struct {
> > > +            uint64_t base;
> > > +            uint64_t decode_len;
> > > +            uint64_t region_len;
> > > +            uint64_t block_size;
> > > +            uint32_t dsmadhandle;  
> >   
> > > +            uint8_t flags;
> > > +            uint8_t rsvd2[3];
> > > +        } QEMU_PACKED records[];  
> > 
> > There are two fields after this as well.
> > Total number of supported extents and number of available extents.
> > 
> > That annoyingly means we can't use the structure to tell us where
> > to find all the fields...
> > 
> >   
> > > +    } QEMU_PACKED;
> > > +
> > > +    struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
> > > +    struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
> > > +    uint16_t record_count = 0, i;  
> > 
> > Better to split that on to 2 lines. Never hide setting a value
> > in the middle of a set of declarations.
> >   
> > > +    uint16_t out_pl_len;
> > > +    uint8_t start_region_id = in->start_region_id;
> > > +
> > > +    if (start_region_id >= ct3d->dc.num_regions) {
> > > +        return CXL_MBOX_INVALID_INPUT;
> > > +    }
> > > +
> > > +    record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
> > > +            in->region_cnt);
> > > +
> > > +    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);  
> > 
> > For r3.1 + 8 for the two trailing fields.
> >   
> > > +    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> > > +
> > > +    memset(out, 0, out_pl_len);  
> > 
> > As part of the cci rework we started zeroing the whole mailbox payload space
> > after copying out the input payload.
> > https://elixir.bootlin.com/qemu/latest/source/hw/cxl/cxl-device-utils.c#L204
> > 
> > So shouldn't need this (unless we have a bug)
> > 
> > Jonathan

On Wed, Jan 24, 2024 at 02:51:18PM +0000, Jonathan Cameron wrote:
> On Tue,  7 Nov 2023 10:07:06 -0800
> nifan.cxl@gmail.com wrote:
> 
> > From: Fan Ni <fan.ni@samsung.com>
> > 
> > Per cxl spec 3.0, add dynamic capacity region representative based on
> > Table 8-126 and extend the cxl type3 device definition to include dc region
> > information. Also, based on info in 8.2.9.8.9.1, add 'Get Dynamic Capacity
> > Configuration' mailbox support.
> > 
> > Note: decode_len of a dc region is aligned to 256*MiB, need to be divided by
> > 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> > mailbox command.
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> 
> Hi Fan,
> 
> I'm looking at how to move these much earlier in my tree on basis that
> they should be our main focus for merging in this QEMU cycle.
> 
> Whilst I do that rebase, I'm taking a closer look at the code.
> I'm targetting rebasing on upstream qemu + the two patch sets I just
> sent out:
> [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup. 
> [PATCH 0/5 qemu] hw/cxl: Update CXL emulation to reflect and reference r3.1
> 
> It would be good to document why these commands should be optional (which I think
> comes down to the annoying fact that Get Dynamic Capacity Configuration isn't
> allowed to return 0 regions, but instead should not be available as a command
> if DCD isn't supported.
> 
> Note this requires us to carry Gregory's patches to make the CCI command list
> constructed at runtime rather than baked in ahead of this set.
> 
> So another question is should we jump directly to the r3.1 version of DCD?
> I think we probably should as it includes some additions that are necessary
> for a bunch of the potential use cases.
> 

Hi Jonathan,

Thanks for taking time to review the patches. 
I will redo the patches and make them align with cxl spec v3.1. Before
that, I need some clarifications.
As you mentioned above, for the next version, I will use upstream qemu + the
two patchsets you mentioned above as base, that is clear to me.
However, you mentioned Gregory's patches above constructing CCI command list
at runtime, I think you meant we should also include that patchset
before DCD so if DCD is not supported, the Get Dynamic capacity
configuration command will not be available at the first place, am I
right? If so, could you point me to the latest patches of the mentioned
CCI work I should use? I see the CCI rework patches, but not sure if we
should have them all or they are the latest.

Thanks,
Fan

> 
> > ---
> >  hw/cxl/cxl-mailbox-utils.c  | 80 +++++++++++++++++++++++++++++++++++++
> >  hw/mem/cxl_type3.c          |  6 +++
> >  include/hw/cxl/cxl_device.h | 17 ++++++++
> >  3 files changed, 103 insertions(+)
> > 
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 8eceedfa87..f80dd6474f 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -80,6 +80,8 @@ enum {
> >          #define GET_POISON_LIST        0x0
> >          #define INJECT_POISON          0x1
> >          #define CLEAR_POISON           0x2
> > +    DCD_CONFIG  = 0x48,
> > +        #define GET_DC_CONFIG          0x0
> >      PHYSICAL_SWITCH = 0x51,
> >          #define IDENTIFY_SWITCH_DEVICE      0x0
> >          #define GET_PHYSICAL_PORT_STATE     0x1
> > @@ -1210,6 +1212,74 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
> >      return CXL_MBOX_SUCCESS;
> >  }
> >  
> > +/*
> > + * CXL r3.0 section 8.2.9.8.9.1: Get Dynamic Capacity Configuration
> 
> As per the patch set I just sent out, I want to standardize on references
> to r3.1 because it's all that is easy to get.  However if we decide to r3.0
> DCD first the upgrade it later, then clearly these need to stick to r3.0 for
> now.
> 
> > + * (Opcode: 4800h)
> > + */
> > +static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> > +                                             uint8_t *payload_in,
> > +                                             size_t len_in,
> > +                                             uint8_t *payload_out,
> > +                                             size_t *len_out,
> > +                                             CXLCCI *cci)
> > +{
> > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > +    struct get_dyn_cap_config_in_pl {
> > +        uint8_t region_cnt;
> > +        uint8_t start_region_id;
> > +    } QEMU_PACKED;
> > +
> > +    struct get_dyn_cap_config_out_pl {
> > +        uint8_t num_regions;
> > +        uint8_t rsvd1[7];
> 
> This changed in r3.1 (errata? - I haven't checked)
> Should be 'regions returned' in first byte.
> 
> > +        struct {
> > +            uint64_t base;
> > +            uint64_t decode_len;
> > +            uint64_t region_len;
> > +            uint64_t block_size;
> > +            uint32_t dsmadhandle;
> 
> > +            uint8_t flags;
> > +            uint8_t rsvd2[3];
> > +        } QEMU_PACKED records[];
> 
> There are two fields after this as well.
> Total number of supported extents and number of available extents.
> 
> That annoyingly means we can't use the structure to tell us where
> to find all the fields...
> 
> 
> > +    } QEMU_PACKED;
> > +
> > +    struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
> > +    struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
> > +    uint16_t record_count = 0, i;
> 
> Better to split that on to 2 lines. Never hide setting a value
> in the middle of a set of declarations.
> 
> > +    uint16_t out_pl_len;
> > +    uint8_t start_region_id = in->start_region_id;
> > +
> > +    if (start_region_id >= ct3d->dc.num_regions) {
> > +        return CXL_MBOX_INVALID_INPUT;
> > +    }
> > +
> > +    record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
> > +            in->region_cnt);
> > +
> > +    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
> 
> For r3.1 + 8 for the two trailing fields.
> 
> > +    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> > +
> > +    memset(out, 0, out_pl_len);
> 
> As part of the cci rework we started zeroing the whole mailbox payload space
> after copying out the input payload.
> https://elixir.bootlin.com/qemu/latest/source/hw/cxl/cxl-device-utils.c#L204
> 
> So shouldn't need this (unless we have a bug)
> 
> Jonathan

On Mon, 29 Jan 2024 09:32:39 -0800
fan <nifan.cxl@gmail.com> wrote:

> On Wed, Jan 24, 2024 at 02:51:18PM +0000, Jonathan Cameron wrote:
> > On Tue,  7 Nov 2023 10:07:06 -0800
> > nifan.cxl@gmail.com wrote:
> >   
> > > From: Fan Ni <fan.ni@samsung.com>
> > > 
> > > Per cxl spec 3.0, add dynamic capacity region representative based on
> > > Table 8-126 and extend the cxl type3 device definition to include dc region
> > > information. Also, based on info in 8.2.9.8.9.1, add 'Get Dynamic Capacity
> > > Configuration' mailbox support.
> > > 
> > > Note: decode_len of a dc region is aligned to 256*MiB, need to be divided by
> > > 256 * MiB before returned to the host for "Get Dynamic Capacity Configuration"
> > > mailbox command.
> > > 
> > > Signed-off-by: Fan Ni <fan.ni@samsung.com>  
> > 
> > Hi Fan,
> > 
> > I'm looking at how to move these much earlier in my tree on basis that
> > they should be our main focus for merging in this QEMU cycle.
> > 
> > Whilst I do that rebase, I'm taking a closer look at the code.
> > I'm targetting rebasing on upstream qemu + the two patch sets I just
> > sent out:
> > [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup. 
> > [PATCH 0/5 qemu] hw/cxl: Update CXL emulation to reflect and reference r3.1
> > 
> > It would be good to document why these commands should be optional (which I think
> > comes down to the annoying fact that Get Dynamic Capacity Configuration isn't
> > allowed to return 0 regions, but instead should not be available as a command
> > if DCD isn't supported.
> > 
> > Note this requires us to carry Gregory's patches to make the CCI command list
> > constructed at runtime rather than baked in ahead of this set.
> > 
> > So another question is should we jump directly to the r3.1 version of DCD?
> > I think we probably should as it includes some additions that are necessary
> > for a bunch of the potential use cases.
> >   
> 
> Hi Jonathan,
> 
> Thanks for taking time to review the patches. 
> I will redo the patches and make them align with cxl spec v3.1. Before
> that, I need some clarifications.
> As you mentioned above, for the next version, I will use upstream qemu + the
> two patchsets you mentioned above as base, that is clear to me.
> However, you mentioned Gregory's patches above constructing CCI command list
> at runtime, I think you meant we should also include that patchset
> before DCD so if DCD is not supported, the Get Dynamic capacity
> configuration command will not be available at the first place, am I
> right? If so, could you point me to the latest patches of the mentioned
> CCI work I should use? I see the CCI rework patches, but not sure if we
> should have them all or they are the latest.

only the two before DCD in this tree.
https://gitlab.com/jic23/qemu/-/commits/cxl-2024-26-01-draft/?ref_type=heads

hw/cxl/mailbox: change CCI cmd set structure to be a member, not a reference 
hw/cxl/mailbox: interface to add CCI commands to an existing CCI 

There is one more sneaky fix on that tree that isn't related to these that I
put behind the spec version updates because it was a pain to rebase.
So fine to ignore that one.

Everything else ahead of DCD has been sent to the list for a merge hopefully.

Jonathan

> 
> Thanks,
> Fan
> 
> >   
> > > ---
> > >  hw/cxl/cxl-mailbox-utils.c  | 80 +++++++++++++++++++++++++++++++++++++
> > >  hw/mem/cxl_type3.c          |  6 +++
> > >  include/hw/cxl/cxl_device.h | 17 ++++++++
> > >  3 files changed, 103 insertions(+)
> > > 
> > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > index 8eceedfa87..f80dd6474f 100644
> > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > @@ -80,6 +80,8 @@ enum {
> > >          #define GET_POISON_LIST        0x0
> > >          #define INJECT_POISON          0x1
> > >          #define CLEAR_POISON           0x2
> > > +    DCD_CONFIG  = 0x48,
> > > +        #define GET_DC_CONFIG          0x0
> > >      PHYSICAL_SWITCH = 0x51,
> > >          #define IDENTIFY_SWITCH_DEVICE      0x0
> > >          #define GET_PHYSICAL_PORT_STATE     0x1
> > > @@ -1210,6 +1212,74 @@ static CXLRetCode cmd_media_clear_poison(const struct cxl_cmd *cmd,
> > >      return CXL_MBOX_SUCCESS;
> > >  }
> > >  
> > > +/*
> > > + * CXL r3.0 section 8.2.9.8.9.1: Get Dynamic Capacity Configuration  
> > 
> > As per the patch set I just sent out, I want to standardize on references
> > to r3.1 because it's all that is easy to get.  However if we decide to r3.0
> > DCD first the upgrade it later, then clearly these need to stick to r3.0 for
> > now.
> >   
> > > + * (Opcode: 4800h)
> > > + */
> > > +static CXLRetCode cmd_dcd_get_dyn_cap_config(const struct cxl_cmd *cmd,
> > > +                                             uint8_t *payload_in,
> > > +                                             size_t len_in,
> > > +                                             uint8_t *payload_out,
> > > +                                             size_t *len_out,
> > > +                                             CXLCCI *cci)
> > > +{
> > > +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > +    struct get_dyn_cap_config_in_pl {
> > > +        uint8_t region_cnt;
> > > +        uint8_t start_region_id;
> > > +    } QEMU_PACKED;
> > > +
> > > +    struct get_dyn_cap_config_out_pl {
> > > +        uint8_t num_regions;
> > > +        uint8_t rsvd1[7];  
> > 
> > This changed in r3.1 (errata? - I haven't checked)
> > Should be 'regions returned' in first byte.
> >   
> > > +        struct {
> > > +            uint64_t base;
> > > +            uint64_t decode_len;
> > > +            uint64_t region_len;
> > > +            uint64_t block_size;
> > > +            uint32_t dsmadhandle;  
> >   
> > > +            uint8_t flags;
> > > +            uint8_t rsvd2[3];
> > > +        } QEMU_PACKED records[];  
> > 
> > There are two fields after this as well.
> > Total number of supported extents and number of available extents.
> > 
> > That annoyingly means we can't use the structure to tell us where
> > to find all the fields...
> > 
> >   
> > > +    } QEMU_PACKED;
> > > +
> > > +    struct get_dyn_cap_config_in_pl *in = (void *)payload_in;
> > > +    struct get_dyn_cap_config_out_pl *out = (void *)payload_out;
> > > +    uint16_t record_count = 0, i;  
> > 
> > Better to split that on to 2 lines. Never hide setting a value
> > in the middle of a set of declarations.
> >   
> > > +    uint16_t out_pl_len;
> > > +    uint8_t start_region_id = in->start_region_id;
> > > +
> > > +    if (start_region_id >= ct3d->dc.num_regions) {
> > > +        return CXL_MBOX_INVALID_INPUT;
> > > +    }
> > > +
> > > +    record_count = MIN(ct3d->dc.num_regions - in->start_region_id,
> > > +            in->region_cnt);
> > > +
> > > +    out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);  
> > 
> > For r3.1 + 8 for the two trailing fields.
> >   
> > > +    assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> > > +
> > > +    memset(out, 0, out_pl_len);  
> > 
> > As part of the cci rework we started zeroing the whole mailbox payload space
> > after copying out the input payload.
> > https://elixir.bootlin.com/qemu/latest/source/hw/cxl/cxl-device-utils.c#L204
> > 
> > So shouldn't need this (unless we have a bug)
> > 
> > Jonathan