Re: [RFC 5/7] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Thu, 11 May 2023 17:56:40 +0000
Fan Ni <fan.ni@samsung.com> wrote:

> From: Fan Ni <nifan@outlook.com>
> 
> Per CXL spec 3.0, we implemented the two mailbox commands:
> Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.8.9.3, and
> Release Dynamic Capacity Response (Opcode 4803h) 8.2.9.8.9.4.
> 
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> ---
>  hw/cxl/cxl-mailbox-utils.c  | 223 ++++++++++++++++++++++++++++++++++++
>  include/hw/cxl/cxl_device.h |   3 +-
>  2 files changed, 225 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index ed2ac154cb..7212934627 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -84,6 +84,8 @@ enum {
>  	DCD_CONFIG = 0x48, /*8.2.9.8.9*/
>  		#define GET_DC_REGION_CONFIG   0x0
>  		#define GET_DYN_CAP_EXT_LIST   0x1
> +		#define ADD_DYN_CAP_RSP        0x2
> +		#define RELEASE_DYN_CAP        0x3
>      PHYSICAL_SWITCH = 0x51
>          #define IDENTIFY_SWITCH_DEVICE      0x0
>  };
> @@ -1069,6 +1071,221 @@ static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(struct cxl_cmd *cmd,
>  	return CXL_MBOX_SUCCESS;
>  }
>  
> +static inline int test_bits(const unsigned long *addr, int nr, int size)

Not obvious what this does from the name.  Please add some docs.

> +{
> +	unsigned long res = find_next_zero_bit(addr, size + nr, nr);
> +
> +	if (res >= nr + size)
> +		return 1;
> +	else
> +		return 0;
> +}
> +
> +static uint8_t find_region_id(struct CXLType3Dev *dev, uint64_t dpa

Operates only on things in dev->dc, so perhaps pass that instead.

> +		, uint64_t len)

comma on previous line.

> +{
> +	int8_t i = dev->dc.num_regions-1;
> +
> +	while (i > 0 && dpa < dev->dc.regions[i].base)
> +		i--;
> +
> +	if (dpa < dev->dc.regions[i].base
> +			|| dpa + len > dev->dc.regions[i].base + dev->dc.regions[i].len)
> +		return dev->dc.num_regions;
> +
> +	return i;
> +}
> +
> +static CXLRetCode detect_malformed_extent_list(CXLType3Dev *dev, void *data)
> +{
> +	struct updated_dc_extent_list_in_pl {
This is same as used in next function.  Pull it out of the functions and
use that type rather than mapping via a void *data pointer.  

> +		uint32_t num_entries_updated;
> +		uint8_t rsvd[4];
> +		struct {
> +			uint64_t start_dpa;
> +			uint64_t len;
> +			uint8_t rsvd[8];
> +		} QEMU_PACKED updated_entries[];
> +	} QEMU_PACKED;
> +
> +	struct updated_dc_extent_list_in_pl *in = data;
> +	unsigned long *blk_bitmap;
> +	uint64_t min_block_size = dev->dc.regions[0].block_size;
> +	struct CXLDCD_Region *region = &dev->dc.regions[0];
> +	uint32_t i;
> +	uint64_t dpa, len;
> +	uint8_t rid;
> +
> +	for (i = 1; i < dev->dc.num_regions; i++) {
> +		region = &dev->dc.regions[i];
> +		if (min_block_size > region->block_size)
> +			min_block_size = region->block_size;
> +	}
> +	blk_bitmap = bitmap_new((region->len + region->base
> +				- dev->dc.regions[0].base) / min_block_size);
> +	g_assert(blk_bitmap);

Abort in bitmap_new() anyway so no need for this.  Most qemu allocations
don't need to be checked as they fail hard anyway so we never get to the checks.

> +	bitmap_zero(blk_bitmap, (region->len + region->base
> +				- dev->dc.regions[0].base) / min_block_size);

bitmap_new() seems to use a g_malloc0 internally so no need to zero again here
I think.

> +
> +	for (i = 0; i < in->num_entries_updated; i++) {
> +		dpa = in->updated_entries[i].start_dpa;
> +		len = in->updated_entries[i].len;
> +
> +		rid = find_region_id(dev, dpa, len);
> +		if (rid == dev->dc.num_regions) {

Use a goto and single cleanup path having set ret or similar to
the particular issue.

> +			g_free(blk_bitmap);
> +			return CXL_MBOX_INVALID_PA;
> +		}
> +		region = &dev->dc.regions[rid];
> +		if (dpa % region->block_size || len % region->block_size) {
> +			g_free(blk_bitmap);
> +			return CXL_MBOX_INVALID_EXTENT_LIST;
goto from here as well.. etc.

> +		}
> +		/* the dpa range already covered by some other extents in the list */
> +		if (test_bits(blk_bitmap, dpa/min_block_size, len/min_block_size)) {
> +			g_free(blk_bitmap);
> +			return CXL_MBOX_INVALID_EXTENT_LIST;
> +		}
> +		bitmap_set(blk_bitmap, dpa/min_block_size, len/min_block_size);
> +	}
> +
> +	g_free(blk_bitmap);
> +	return CXL_MBOX_SUCCESS;
> +}
> +
> +/*
> + * cxl spec 3.0: 8.2.9.8.9.3
> + * Add Dynamic Capacity Response (opcode 4802h)
> + * Assuming extent list is updated when a extent is added, when receiving
> + * the response, verify and ensure the extent is utilized by the host, and
> + * update extent list  as needed.

Double space in middle of sentence

> + **/
> +static CXLRetCode cmd_dcd_add_dyn_cap_rsp(struct cxl_cmd *cmd,
> +		CXLDeviceState *cxl_dstate,
> +		uint16_t *len_unused)
> +{
> +	struct add_dyn_cap_ext_list_in_pl {
> +		uint32_t num_entries_updated;
> +		uint8_t rsvd[4];
> +		struct {
> +			uint64_t start_dpa;
> +			uint64_t len;
> +			uint8_t rsvd[8];
> +		} QEMU_PACKED updated_entries[];

These extent list entries keep turning up in the code. Pull that out of here
to be a general 'Extent list element' or similar.

> +	} QEMU_PACKED;
> +
> +	struct add_dyn_cap_ext_list_in_pl *in = (void *)cmd->payload;
> +	struct CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> +	CXLDCDExtentList *extent_list = &ct3d->dc.extents;
> +	CXLDCD_Extent *ent;
> +	uint32_t i;
> +	uint64_t dpa, len;
> +	CXLRetCode rs;
> +
> +	if (in->num_entries_updated == 0)
> +		return CXL_MBOX_SUCCESS;
> +
> +	rs = detect_malformed_extent_list(ct3d, in);
> +	if (rs != CXL_MBOX_SUCCESS)
> +		return rs;
> +
> +	for (i = 0; i < in->num_entries_updated; i++) {
> +		dpa = in->updated_entries[i].start_dpa;
> +		len = in->updated_entries[i].len;
> +
> +		/* Todo: check following
> +		 * One or more of the updated extent lists contain Starting DPA
> +		 * or Lengths that are out of range of a current extent list
> +		 * maintained by the device.
> +		 **/
> +
> +		QTAILQ_FOREACH(ent, extent_list, node) {

Add some comments here.  Is this a repeated entry test?

> +			if (ent->start_dpa == dpa && ent->len == len)
> +				return CXL_MBOX_INVALID_PA;
> +			if (ent->start_dpa <= dpa
> +				&& dpa + len <= ent->start_dpa + ent->len) {

Comment needed on this one. Why is it shrinking an existing entry?

> +				ent->start_dpa = dpa;
> +				ent->len = len;
> +				break;

If you break here I think it will only result in return CXL_MBOX_SUCCESS so
might as well do that here.

> +			} else if ((dpa < ent->start_dpa + ent->len
> +				&& dpa + len > ent->start_dpa + ent->len)

Above is new entry would contain existing one.

> +				|| (dpa < ent->start_dpa && dpa + len > ent->start_dpa))
> +				return CXL_MBOX_INVALID_EXTENT_LIST;
> +		}
> +		// a new extent added

Why?  This function generally need more documentation.

> +		if (!ent) {
> +			ent = g_new0(CXLDCD_Extent, 1);
> +			assert(ent);

No need.  g_new0 will already have blown up before you get here.

> +			ent->start_dpa = dpa;
> +			ent->len = len;
> +			memset(ent->tag, 0, 0x10);
Allocated empty just above, still empty so no need to set it again.
> +			ent->shared_seq = 0;
> +			QTAILQ_INSERT_TAIL(extent_list, ent, node);
> +		}
> +	}
> +
> +	return CXL_MBOX_SUCCESS;
> +}
> +
> +/*
> + * Spec 3.0: 8.2.9.8.9.4
> + * Release Dynamic Capacity (opcode 4803h)
> + **/
> +static CXLRetCode cmd_dcd_release_dcd_capacity(struct cxl_cmd *cmd,
> +		CXLDeviceState *cxl_dstate,
> +		uint16_t *len_unused)
> +{
> +	struct release_dcd_cap_in_pl {
> +		uint32_t num_entries_updated;
> +		uint8_t rsvd[4];
> +		struct {
> +			uint64_t start_dpa;
> +			uint64_t len;
> +			uint8_t rsvd[8];
> +		} QEMU_PACKED updated_entries[];
> +	} QEMU_PACKED;
> +
> +	struct release_dcd_cap_in_pl *in = (void *)cmd->payload;
> +	struct CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> +	CXLDCDExtentList *extent_list = &ct3d->dc.extents;
> +	CXLDCD_Extent *ent;
> +	uint32_t i;
> +	uint64_t dpa, len;
> +	CXLRetCode rs;
> +
> +	if (in->num_entries_updated == 0)
> +		return CXL_MBOX_INVALID_INPUT;
> +
> +	rs = detect_malformed_extent_list(ct3d, in);
> +	if (rs != CXL_MBOX_SUCCESS)
> +		return rs;
> +
> +		/* Todo: check following
> +		 * One or more of the updated extent lists contain Starting DPA
> +		 * or Lengths that are out of range of a current extent list
> +		 * maintained by the device.
> +		 **/
> +
> +	for (i = 0; i < in->num_entries_updated; i++) {
> +		dpa = in->updated_entries[i].start_dpa;
> +		len = in->updated_entries[i].len;
> +
> +		QTAILQ_FOREACH(ent, extent_list, node) {
> +			if (ent->start_dpa == dpa && ent->len == len)

Do the remove here and comment on 'found entry' not needed.
Note I think you can release partial extents so that will need handling at some point.

> +				break;
> +			else if ((dpa < ent->start_dpa + ent->len
> +				&& dpa + len > ent->start_dpa + ent->len)
> +				|| (dpa < ent->start_dpa && dpa + len > ent->start_dpa))
Comment on this condition and why it's a problem.

> +				return CXL_MBOX_INVALID_EXTENT_LIST;
> +		}
> +		/* found the entry, release it */
> +		if (ent)
> +			QTAILQ_REMOVE(extent_list, ent, node);
> +	}
> +
> +	return CXL_MBOX_SUCCESS;
> +}
> +
>  #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
>  #define IMMEDIATE_DATA_CHANGE (1 << 2)
>  #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> @@ -1112,6 +1329,12 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
>  	[DCD_CONFIG][GET_DYN_CAP_EXT_LIST] = {
>  		"DCD_GET_DYNAMIC_CAPACITY_EXTENT_LIST", cmd_dcd_get_dyn_cap_ext_list,
>  		8, 0 },
> +	[DCD_CONFIG][ADD_DYN_CAP_RSP] = {
> +		"ADD_DCD_DYNAMIC_CAPACITY_RESPONSE", cmd_dcd_add_dyn_cap_rsp,
> +		~0, IMMEDIATE_DATA_CHANGE },
> +	[DCD_CONFIG][RELEASE_DYN_CAP] = {
> +		"RELEASE_DCD_DYNAMIC_CAPACITY", cmd_dcd_release_dcd_capacity,
> +		~0, IMMEDIATE_DATA_CHANGE },
>  };
>  
>  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 20ad5e7411..c0c8fcc24b 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -131,7 +131,8 @@ typedef enum {
>      CXL_MBOX_INCORRECT_PASSPHRASE = 0x14,
>      CXL_MBOX_UNSUPPORTED_MAILBOX = 0x15,
>      CXL_MBOX_INVALID_PAYLOAD_LENGTH = 0x16,
> -    CXL_MBOX_MAX = 0x17
> +	CXL_MBOX_INVALID_EXTENT_LIST = 0x17,

0x1e in the spec.

> +	CXL_MBOX_MAX = 0x18
>  } CXLRetCode;
>  
>  struct cxl_cmd;