On 5/11/23 12:56, Fan Ni wrote:
> From: Fan Ni <ni...@outlook.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.
>
> Signed-off-by: Fan Ni <fan...@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 68 +++++++++++++++++++++++++++++++++++++
> include/hw/cxl/cxl_device.h | 16 +++++++++
> 2 files changed, 84 insertions(+)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 7ff4fbdf22..61c77e52d8 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -81,6 +81,8 @@ enum {
> #define GET_POISON_LIST 0x0
> #define INJECT_POISON 0x1
> #define CLEAR_POISON 0x2
> + DCD_CONFIG = 0x48, /*8.2.9.8.9*/
> + #define GET_DC_REGION_CONFIG 0x0
> PHYSICAL_SWITCH = 0x51
> #define IDENTIFY_SWITCH_DEVICE 0x0
> };
> @@ -935,6 +937,70 @@ static CXLRetCode cmd_media_clear_poison(struct cxl_cmd
> *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * cxl spec 3.0: 8.2.9.8.9.2
> + * Get Dynamic Capacity Configuration
> + **/
> +static CXLRetCode cmd_dcd_get_dyn_cap_config(struct cxl_cmd *cmd,
> + CXLDeviceState *cxl_dstate,
> + uint16_t *len)
> +{
> + 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[];
Could you declare CXLDCD_Region as QEMU_PACKED and use it here instead of
re-defining the region structure?
> + } QEMU_PACKED;
> +
> + struct get_dyn_cap_config_in_pl *in = (void *)cmd->payload;
> + struct get_dyn_cap_config_out_pl *out = (void *)cmd->payload;
> + struct CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev,
> cxl_dstate);
> + uint16_t record_count = 0, i = 0;
> + uint16_t out_pl_len;
> +
> + if (in->start_region_id >= ct3d->dc.num_regions)
> + record_count = 0;
> + else if (ct3d->dc.num_regions - in->start_region_id < in->region_cnt)
> + record_count = ct3d->dc.num_regions - in->start_region_id;
> + else
> + record_count = 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 < record_count; i++) {
> + stq_le_p(&out->records[i].base,
> + ct3d->dc.regions[in->start_region_id+i].base);
> + stq_le_p(&out->records[i].decode_len,
> + ct3d->dc.regions[in->start_region_id+i].decode_len);
> + stq_le_p(&out->records[i].region_len,
> + ct3d->dc.regions[in->start_region_id+i].len);
> + stq_le_p(&out->records[i].block_size,
> + ct3d->dc.regions[in->start_region_id+i].block_size);
> + stl_le_p(&out->records[i].dsmadhandle,
> + ct3d->dc.regions[in->start_region_id+i].dsmadhandle);
> + out->records[i].flags
> + = ct3d->dc.regions[in->start_region_id+i].flags;
In this loop your reading from 'in' and writing to 'out' where in and out both
point to the same payload buffer. It works because of the structure layouts but
feels like a bug waiting to happen. Perhaps saving start_region to a local
variable
and using that for the loop?
-Nathan
> + }
> +
> + *len = 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)
> @@ -973,6 +1039,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
> cmd_media_inject_poison, 8, 0 },
> [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON",
> cmd_media_clear_poison, 72, 0 },
> + [DCD_CONFIG][GET_DC_REGION_CONFIG] = { "DCD_GET_DC_REGION_CONFIG",
> + cmd_dcd_get_dyn_cap_config, 2, 0 },
> };
>
> static struct cxl_cmd cxl_cmd_set_sw[256][256] = {
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index e285369693..8a04e53e90 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -383,6 +383,17 @@ typedef struct CXLPoison {
> typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> #define CXL_POISON_LIST_LIMIT 256
>
> +#define DCD_MAX_REGION_NUM 8
> +
> +typedef struct CXLDCD_Region {
> + uint64_t base;
> + uint64_t decode_len; /* in multiples of 256MB */
> + uint64_t len;
> + uint64_t block_size;
> + uint32_t dsmadhandle;
> + uint8_t flags;
> +} CXLDCD_Region;
> +
> struct CXLType3Dev {
> /* Private */
> PCIDevice parent_obj;
> @@ -414,6 +425,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; // 1-8
> + struct CXLDCD_Region regions[DCD_MAX_REGION_NUM];
> + } dc;
> };
>
> #define TYPE_CXL_TYPE3 "cxl-type3"
