Re: [Qemu PATCH v2 7/9] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
On Fri, Sep 08, 2023 at 01:00:16PM +, J?rgen Hansen wrote:
> On 7/25/23 20:39, Fan Ni wrote:
> > From: Fan Ni
> >
> > Per CXL spec 3.0, two mailbox commands are implemented:
> > Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.8.9.3, and
> > Release Dynamic Capacity (Opcode 4803h) 8.2.9.8.9.4.
> >
> > Signed-off-by: Fan Ni
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 253
> > include/hw/cxl/cxl_device.h | 3 +-
> > 2 files changed, 255 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 3d25a9697e..1e4944da95 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -84,6 +84,8 @@ enum {
> > DCD_CONFIG = 0x48, /*r3.0: 8.2.9.8.9*/
> > #define GET_DC_CONFIG 0x0
> > #define GET_DYN_CAP_EXT_LIST 0x1
> > +#define ADD_DYN_CAP_RSP0x2
> > +#define RELEASE_DYN_CAP0x3
> > PHYSICAL_SWITCH = 0x51
> > #define IDENTIFY_SWITCH_DEVICE 0x0
> > };
> > @@ -1086,6 +1088,251 @@ static CXLRetCode
> > cmd_dcd_get_dyn_cap_ext_list(struct cxl_cmd *cmd,
> > return CXL_MBOX_SUCCESS;
> > }
> >
> > +/*
> > + * Check whether the bits at addr between [nr, nr+size) are all set,
> > + * return 1 if all 1s, else return 0
> > + */
> > +static inline int test_bits(const unsigned long *addr, int nr, int size)
> > +{
> > +unsigned long res = find_next_zero_bit(addr, size + nr, nr);
> > +
> > +return (res >= nr + size) ? 1 : 0;
> > +}
> > +
> > +/*
> > + * Find dynamic capacity region id based on dpa range [dpa, dpa+len)
> > + */
> > +static uint8_t find_region_id(struct CXLType3Dev *dev, uint64_t dpa,
> > +uint64_t len)
> > +{
> > +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 void insert_extent_to_extent_list(CXLDCDExtentList *list, uint64_t
> > dpa,
> > +uint64_t len, uint8_t *tag, uint16_t shared_seq)
> > +{
> > +CXLDCD_Extent *extent;
> > +extent = g_new0(CXLDCD_Extent, 1);
> > +extent->start_dpa = dpa;
> > +extent->len = len;
> > +if (tag) {
> > +memcpy(extent->tag, tag, 0x10);
> > +} else {
> > +memset(extent->tag, 0, 0x10);
> > +}
> > +extent->shared_seq = shared_seq;
> > +
> > +QTAILQ_INSERT_TAIL(list, extent, node);
> > +}
> > +
> > +typedef struct updated_dc_extent_list_in_pl {
> > +uint32_t num_entries_updated;
> > +uint8_t rsvd[4];
> > +struct { /* r3.0: Table 8-130 */
> > +uint64_t start_dpa;
> > +uint64_t len;
> > +uint8_t rsvd[8];
> > +} QEMU_PACKED updated_entries[];
> > +} QEMU_PACKED updated_dc_extent_list_in_pl;
> > +
> > +/*
> > + * The function only check the input extent list against itself.
> > + */
> > +static CXLRetCode detect_malformed_extent_list(CXLType3Dev *dev,
> > +const updated_dc_extent_list_in_pl *in)
> > +{
> > +unsigned long *blk_bitmap;
> > +uint64_t min_block_size = dev->dc.regions[0].block_size;
> > +struct CXLDCD_Region *region = >dc.regions[0];
> > +uint32_t i;
> > +uint64_t dpa, len;
> > +uint8_t rid;
> > +CXLRetCode ret;
> > +
> > +for (i = 1; i < dev->dc.num_regions; i++) {
> > +region = >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);
> > +
> > +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) {
> > +ret = CXL_MBOX_INVALID_PA;
> > +goto out;
> > +}
> > +
> > +region = >dc.regions[rid];
> > +if (dpa % region->block_size || len % region->block_size) {
> > +ret = CXL_MBOX_INVALID_EXTENT_LIST;
> > +goto out;
> > +}
>
> Hi,
>
> The bitmap uses the dc region 0 base address as the baseline, so when
> checking the dpa against the bitmap it needs to be adjusted for that
> before the bitmap checks, e.g.,
>
> +dpa -= dev->dc.regions[0].base;
>
> Thanks,
> Jorgen
Make sense. Will fix. Thanks.
Fan
>
> > +/* 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)) {
> > +ret = CXL_MBOX_INVALID_EXTENT_LIST;
> > +goto out;
> > +}
Re: [Qemu PATCH v2 7/9] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
On 7/25/23 20:39, Fan Ni wrote:
> From: Fan Ni
>
> Per CXL spec 3.0, two mailbox commands are implemented:
> Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.8.9.3, and
> Release Dynamic Capacity (Opcode 4803h) 8.2.9.8.9.4.
>
> Signed-off-by: Fan Ni
> ---
> hw/cxl/cxl-mailbox-utils.c | 253
> include/hw/cxl/cxl_device.h | 3 +-
> 2 files changed, 255 insertions(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 3d25a9697e..1e4944da95 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -84,6 +84,8 @@ enum {
> DCD_CONFIG = 0x48, /*r3.0: 8.2.9.8.9*/
> #define GET_DC_CONFIG 0x0
> #define GET_DYN_CAP_EXT_LIST 0x1
> +#define ADD_DYN_CAP_RSP0x2
> +#define RELEASE_DYN_CAP0x3
> PHYSICAL_SWITCH = 0x51
> #define IDENTIFY_SWITCH_DEVICE 0x0
> };
> @@ -1086,6 +1088,251 @@ static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(struct
> cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * Check whether the bits at addr between [nr, nr+size) are all set,
> + * return 1 if all 1s, else return 0
> + */
> +static inline int test_bits(const unsigned long *addr, int nr, int size)
> +{
> +unsigned long res = find_next_zero_bit(addr, size + nr, nr);
> +
> +return (res >= nr + size) ? 1 : 0;
> +}
> +
> +/*
> + * Find dynamic capacity region id based on dpa range [dpa, dpa+len)
> + */
> +static uint8_t find_region_id(struct CXLType3Dev *dev, uint64_t dpa,
> +uint64_t len)
> +{
> +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 void insert_extent_to_extent_list(CXLDCDExtentList *list, uint64_t
> dpa,
> +uint64_t len, uint8_t *tag, uint16_t shared_seq)
> +{
> +CXLDCD_Extent *extent;
> +extent = g_new0(CXLDCD_Extent, 1);
> +extent->start_dpa = dpa;
> +extent->len = len;
> +if (tag) {
> +memcpy(extent->tag, tag, 0x10);
> +} else {
> +memset(extent->tag, 0, 0x10);
> +}
> +extent->shared_seq = shared_seq;
> +
> +QTAILQ_INSERT_TAIL(list, extent, node);
> +}
> +
> +typedef struct updated_dc_extent_list_in_pl {
> +uint32_t num_entries_updated;
> +uint8_t rsvd[4];
> +struct { /* r3.0: Table 8-130 */
> +uint64_t start_dpa;
> +uint64_t len;
> +uint8_t rsvd[8];
> +} QEMU_PACKED updated_entries[];
> +} QEMU_PACKED updated_dc_extent_list_in_pl;
> +
> +/*
> + * The function only check the input extent list against itself.
> + */
> +static CXLRetCode detect_malformed_extent_list(CXLType3Dev *dev,
> +const updated_dc_extent_list_in_pl *in)
> +{
> +unsigned long *blk_bitmap;
> +uint64_t min_block_size = dev->dc.regions[0].block_size;
> +struct CXLDCD_Region *region = >dc.regions[0];
> +uint32_t i;
> +uint64_t dpa, len;
> +uint8_t rid;
> +CXLRetCode ret;
> +
> +for (i = 1; i < dev->dc.num_regions; i++) {
> +region = >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);
> +
> +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) {
> +ret = CXL_MBOX_INVALID_PA;
> +goto out;
> +}
> +
> +region = >dc.regions[rid];
> +if (dpa % region->block_size || len % region->block_size) {
> +ret = CXL_MBOX_INVALID_EXTENT_LIST;
> +goto out;
> +}
Hi,
The bitmap uses the dc region 0 base address as the baseline, so when
checking the dpa against the bitmap it needs to be adjusted for that
before the bitmap checks, e.g.,
+dpa -= dev->dc.regions[0].base;
Thanks,
Jorgen
> +/* 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)) {
> +ret = CXL_MBOX_INVALID_EXTENT_LIST;
> +goto out;
> +}
> +bitmap_set(blk_bitmap, dpa / min_block_size, len / min_block_size);
> + }
> +
> +ret = CXL_MBOX_SUCCESS;
> +
> +out:
> +g_free(blk_bitmap);
> +return ret;
> +}
> +
> +/*
> + * cxl spec 3.0: 8.2.9.8.9.3
> + * Add Dynamic Capacity Response (opcode 4802h)
> + * Assume an extent is added only after the response is processed
> successfully
> + * TODO: for better
Re: [Qemu PATCH v2 7/9] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
On Tue, 25 Jul 2023 18:39:56 +
Fan Ni wrote:
> From: Fan Ni
>
> Per CXL spec 3.0, two mailbox commands are implemented:
> Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.8.9.3, and
> Release Dynamic Capacity (Opcode 4803h) 8.2.9.8.9.4.
>
> Signed-off-by: Fan Ni
I'm reviewing these backwards mostly because I'm tidying them up in that
order (makes rebases easier). Hence some of this only makes sense in light of
patch 9 comments!
In my rebase of this, I've made some changes that are non trivial
so definitely want you to look at them.
I also left what I think is a nasty bug. If we get extents
added next to each other they aren't fused, so a release extent that
covers more than one will fail. Far as I can tell that's a valid
if weird corner cases.
Jonathan
> ---
> hw/cxl/cxl-mailbox-utils.c | 253
> include/hw/cxl/cxl_device.h | 3 +-
> 2 files changed, 255 insertions(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 3d25a9697e..1e4944da95 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -84,6 +84,8 @@ enum {
> DCD_CONFIG = 0x48, /*r3.0: 8.2.9.8.9*/
> #define GET_DC_CONFIG 0x0
> #define GET_DYN_CAP_EXT_LIST 0x1
> +#define ADD_DYN_CAP_RSP0x2
> +#define RELEASE_DYN_CAP0x3
> PHYSICAL_SWITCH = 0x51
> #define IDENTIFY_SWITCH_DEVICE 0x0
> };
> @@ -1086,6 +1088,251 @@ static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(struct
> cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> +/*
> + * Check whether the bits at addr between [nr, nr+size) are all set,
> + * return 1 if all 1s, else return 0
> + */
> +static inline int test_bits(const unsigned long *addr, int nr, int size)
bool
Also, let the compiler make decisions on inlining.
Hmm. Documentation lacking on find_next_zero_bit() but I think it returns
the size if there aren't any so this should be fine.
> +{
> +unsigned long res = find_next_zero_bit(addr, size + nr, nr);
> +
> +return (res >= nr + size) ? 1 : 0;
> +}
> +
> +/*
> + * Find dynamic capacity region id based on dpa range [dpa, dpa+len)
> + */
> +static uint8_t find_region_id(struct CXLType3Dev *dev, uint64_t dpa,
> +uint64_t len)
> +{
> +int8_t i = dev->dc.num_regions - 1;
> +
> +while (i > 0 && dpa < dev->dc.regions[i].base) {
> +i--;
> +}
This is another search function, similar to the one I factored out when applying
patch 9. I'll pull that function back here instead of having it in patch 9
+ rename it to be more in keeping with functions in this file.
The handling is a little different (NULL or the region pointer, and that also
simplifies the code around where it is used a little.
> +
> +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 void insert_extent_to_extent_list(CXLDCDExtentList *list, uint64_t
> dpa,
> +uint64_t len, uint8_t *tag, uint16_t shared_seq)
> +{
> +CXLDCD_Extent *extent;
> +extent = g_new0(CXLDCD_Extent, 1);
> +extent->start_dpa = dpa;
> +extent->len = len;
> +if (tag) {
> +memcpy(extent->tag, tag, 0x10);
> +} else {
> +memset(extent->tag, 0, 0x10);
> +}
> +extent->shared_seq = shared_seq;
> +
> +QTAILQ_INSERT_TAIL(list, extent, node);
> +}
> +
Added a reference to both Table 8-129 and Table 8-131 here
> +typedef struct updated_dc_extent_list_in_pl {
> +uint32_t num_entries_updated;
> +uint8_t rsvd[4];
> +struct { /* r3.0: Table 8-130 */
I reformatted this and added name of table. Makes it easier
to find in CXL rN.0
> +uint64_t start_dpa;
> +uint64_t len;
> +uint8_t rsvd[8];
> +} QEMU_PACKED updated_entries[];
> +} QEMU_PACKED updated_dc_extent_list_in_pl;
> +
> +/*
> + * The function only check the input extent list against itself.
I haven't added any info here yet, but feels like this function needs
comments on what those checks are doing.
> + */
> +static CXLRetCode detect_malformed_extent_list(CXLType3Dev *dev,
prefixed with cxl_ to keep everything that is easy to do namespaced.
> +const updated_dc_extent_list_in_pl *in)
> +{
> +unsigned long *blk_bitmap;
> +uint64_t min_block_size = dev->dc.regions[0].block_size;
If we use UINT64_MAX then anything seen will be less than it and
we can just loop over all regions.
> +struct CXLDCD_Region *region = >dc.regions[0];
> +uint32_t i;
> +uint64_t dpa, len;
> +uint8_t rid;
> +CXLRetCode ret;
> +
> +for (i = 1; i < dev->dc.num_regions; i++) {
> +region = >dc.regions[i];
> +if (min_block_size > region->block_size) {
> +min_block_size = region->block_size;
> +}
min_block_size = MIN(min_block_size, region->block_size);
> +
[Qemu PATCH v2 7/9] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response
From: Fan Ni
Per CXL spec 3.0, two mailbox commands are implemented:
Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.8.9.3, and
Release Dynamic Capacity (Opcode 4803h) 8.2.9.8.9.4.
Signed-off-by: Fan Ni
---
hw/cxl/cxl-mailbox-utils.c | 253
include/hw/cxl/cxl_device.h | 3 +-
2 files changed, 255 insertions(+), 1 deletion(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 3d25a9697e..1e4944da95 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -84,6 +84,8 @@ enum {
DCD_CONFIG = 0x48, /*r3.0: 8.2.9.8.9*/
#define GET_DC_CONFIG 0x0
#define GET_DYN_CAP_EXT_LIST 0x1
+#define ADD_DYN_CAP_RSP0x2
+#define RELEASE_DYN_CAP0x3
PHYSICAL_SWITCH = 0x51
#define IDENTIFY_SWITCH_DEVICE 0x0
};
@@ -1086,6 +1088,251 @@ static CXLRetCode cmd_dcd_get_dyn_cap_ext_list(struct
cxl_cmd *cmd,
return CXL_MBOX_SUCCESS;
}
+/*
+ * Check whether the bits at addr between [nr, nr+size) are all set,
+ * return 1 if all 1s, else return 0
+ */
+static inline int test_bits(const unsigned long *addr, int nr, int size)
+{
+unsigned long res = find_next_zero_bit(addr, size + nr, nr);
+
+return (res >= nr + size) ? 1 : 0;
+}
+
+/*
+ * Find dynamic capacity region id based on dpa range [dpa, dpa+len)
+ */
+static uint8_t find_region_id(struct CXLType3Dev *dev, uint64_t dpa,
+uint64_t len)
+{
+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 void insert_extent_to_extent_list(CXLDCDExtentList *list, uint64_t dpa,
+uint64_t len, uint8_t *tag, uint16_t shared_seq)
+{
+CXLDCD_Extent *extent;
+extent = g_new0(CXLDCD_Extent, 1);
+extent->start_dpa = dpa;
+extent->len = len;
+if (tag) {
+memcpy(extent->tag, tag, 0x10);
+} else {
+memset(extent->tag, 0, 0x10);
+}
+extent->shared_seq = shared_seq;
+
+QTAILQ_INSERT_TAIL(list, extent, node);
+}
+
+typedef struct updated_dc_extent_list_in_pl {
+uint32_t num_entries_updated;
+uint8_t rsvd[4];
+struct { /* r3.0: Table 8-130 */
+uint64_t start_dpa;
+uint64_t len;
+uint8_t rsvd[8];
+} QEMU_PACKED updated_entries[];
+} QEMU_PACKED updated_dc_extent_list_in_pl;
+
+/*
+ * The function only check the input extent list against itself.
+ */
+static CXLRetCode detect_malformed_extent_list(CXLType3Dev *dev,
+const updated_dc_extent_list_in_pl *in)
+{
+unsigned long *blk_bitmap;
+uint64_t min_block_size = dev->dc.regions[0].block_size;
+struct CXLDCD_Region *region = >dc.regions[0];
+uint32_t i;
+uint64_t dpa, len;
+uint8_t rid;
+CXLRetCode ret;
+
+for (i = 1; i < dev->dc.num_regions; i++) {
+region = >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);
+
+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) {
+ret = CXL_MBOX_INVALID_PA;
+goto out;
+}
+
+region = >dc.regions[rid];
+if (dpa % region->block_size || len % region->block_size) {
+ret = CXL_MBOX_INVALID_EXTENT_LIST;
+goto out;
+}
+/* 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))
{
+ret = CXL_MBOX_INVALID_EXTENT_LIST;
+goto out;
+}
+bitmap_set(blk_bitmap, dpa / min_block_size, len / min_block_size);
+ }
+
+ret = CXL_MBOX_SUCCESS;
+
+out:
+g_free(blk_bitmap);
+return ret;
+}
+
+/*
+ * cxl spec 3.0: 8.2.9.8.9.3
+ * Add Dynamic Capacity Response (opcode 4802h)
+ * Assume an extent is added only after the response is processed successfully
+ * TODO: for better extent list validation, a better solution would be
+ * maintaining a pending extent list and use it to verify the extent list in
+ * the response.
+ */
+static CXLRetCode cmd_dcd_add_dyn_cap_rsp(struct cxl_cmd *cmd,
+CXLDeviceState *cxl_dstate, uint16_t *len_unused)
+{
+updated_dc_extent_list_in_pl *in = (void *)cmd->payload;
+struct CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev,
+cxl_dstate);
+CXLDCDExtentList *extent_list = >dc.extents;
+CXLDCD_Extent *ent;
+uint32_t i;
+uint64_t dpa, len;
+CXLRetCode ret;
+
+if
