Re: [PATCH v4 cxl-2.0-doe 2/3] CXL Data Object Exchange implementation
On Wed, 31 Mar 2021 12:36:58 -0400
Chris Browy wrote:
> From: hchkuo
Again, must have a description, plus a sign off from Chris if Chris is
the person sending the patch out.
>
> Signed-off-by: hchkuo
Please split this into 2 patches. Add the DOE + CDAT in first patch, and
compliance in the second. That will make them easier to review.
Also break out the test data as another separate patch.
A few other things inline, particularly around multiple instances of
this device causing interesting issues given static non const data.
> ---
> hw/cxl/cxl-cdat.c | 220 +
> hw/cxl/meson.build | 1 +
> hw/mem/cxl_type3.c | 200 +++
> include/hw/cxl/cxl_cdat.h | 149
> include/hw/cxl/cxl_compliance.h | 297
>
> include/hw/cxl/cxl_component.h | 7 +
> include/hw/cxl/cxl_device.h | 4 +
> include/hw/cxl/cxl_pci.h| 2 +
> tests/data/cdat/cdat.dat| Bin 0 -> 148 bytes
> 9 files changed, 880 insertions(+)
> create mode 100644 hw/cxl/cxl-cdat.c
> create mode 100644 include/hw/cxl/cxl_cdat.h
> create mode 100644 include/hw/cxl/cxl_compliance.h
> create mode 100644 tests/data/cdat/cdat.dat
>
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> new file mode 100644
> index 000..fa54506
> --- /dev/null
> +++ b/hw/cxl/cxl-cdat.c
> @@ -0,0 +1,220 @@
> +/*
> + * CXL CDAT Structure
> + *
> + * Copyright (C) 2021 Avery Design Systems, Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/pci/pci.h"
> +#include "hw/cxl/cxl.h"
> +#include "qapi/error.h"
> +
> +static struct cdat_dsmas dsmas = {
> +.header = {
> +.type = CDAT_TYPE_DSMAS,
> +.length = sizeof(dsmas),
> +},
> +.DSMADhandle = 0,
> +.flags = 0,
> +.DPA_base = 0,
> +.DPA_length = 0,
From a testing point of view would be helpful if you put some
plausible values in these :)
> +};
> +
> +static struct cdat_dslbis dslbis = {
> +.header = {
> +.type = CDAT_TYPE_DSLBIS,
> +.length = sizeof(dslbis),
> +},
> +.handle = 0,
> +.flags = 0,
> +.data_type = 0,
> +.entry_base_unit = 0,
> +};
> +
> +static struct cdat_dsmscis dsmscis = {
> +.header = {
> +.type = CDAT_TYPE_DSMSCIS,
> +.length = sizeof(dsmscis),
> +},
> +.DSMAS_handle = 0,
> +.memory_side_cache_size = 0,
> +.cache_attributes = 0,
> +};
> +
> +static struct cdat_dsis dsis = {
> +.header = {
> +.type = CDAT_TYPE_DSIS,
> +.length = sizeof(dsis),
> +},
> +.flags = 0,
> +.handle = 0,
> +};
> +
> +static struct cdat_dsemts dsemts = {
> +.header = {
> +.type = CDAT_TYPE_DSEMTS,
> +.length = sizeof(dsemts),
> +},
> +.DSMAS_handle = 0,
> +.EFI_memory_type_attr = 0,
> +.DPA_offset = 0,
> +.DPA_length = 0,
> +};
> +
> +struct cdat_sslbis {
> +struct cdat_sslbis_header sslbis_header;
> +struct cdat_sslbe sslbe[];
> +};
> +
> +static struct cdat_sslbis sslbis = {
> +.sslbis_header = {
> +.header = {
> +.type = CDAT_TYPE_SSLBIS,
> +.length = sizeof(sslbis.sslbis_header) +
> + sizeof(struct cdat_sslbe) * 2,
> +},
> +.data_type = 0,
> +.entry_base_unit = 0,
> +},
> +.sslbe[0] = {
> +.port_x_id = 0,
> +.port_y_id = 0,
> +.latency_bandwidth = 0,
> +},
> +.sslbe[1] = {
> +.port_x_id = 0,
> +.port_y_id = 0,
> +.latency_bandwidth = 0,
> +},
> +};
> +
> +static void *cdat_table[] = {
> +(void *) ,
You should never need to cast to a (void *) as the c spec never requires
it.
> +(void *) ,
> +(void *) ,
> +(void *) ,
> +(void *) ,
> +(void *) ,
> +};
If I instantiate two CXL mem devices, They will currently share this
table. That is definitely not a good idea as an architecture as they may
well have different parameters. You need to ensure each instance has it's
own copy. Or if you want them to just be constant values and not handle
that complexity, then make them const and ensure the code is happy to
work with that.
> +
> +static void cdat_len_check(struct cdat_sub_header *hdr, Error **errp)
> +{
> +assert(hdr->length);
> +assert(hdr->reserved == 0);
> +
> +switch (hdr->type) {
> +case CDAT_TYPE_DSMAS:
> +assert(hdr->length == sizeof(struct cdat_dsmas));
> +break;
> +case CDAT_TYPE_DSLBIS:
> +assert(hdr->length == sizeof(struct cdat_dslbis));
> +break;
> +case CDAT_TYPE_DSMSCIS:
> +assert(hdr->length == sizeof(struct cdat_dsmscis));
> +break;
> +case CDAT_TYPE_DSIS:
> +assert(hdr->length == sizeof(struct cdat_dsis));
> +break;
> +
[PATCH v4 cxl-2.0-doe 2/3] CXL Data Object Exchange implementation
From: hchkuo
Signed-off-by: hchkuo
---
hw/cxl/cxl-cdat.c | 220 +
hw/cxl/meson.build | 1 +
hw/mem/cxl_type3.c | 200 +++
include/hw/cxl/cxl_cdat.h | 149
include/hw/cxl/cxl_compliance.h | 297
include/hw/cxl/cxl_component.h | 7 +
include/hw/cxl/cxl_device.h | 4 +
include/hw/cxl/cxl_pci.h| 2 +
tests/data/cdat/cdat.dat| Bin 0 -> 148 bytes
9 files changed, 880 insertions(+)
create mode 100644 hw/cxl/cxl-cdat.c
create mode 100644 include/hw/cxl/cxl_cdat.h
create mode 100644 include/hw/cxl/cxl_compliance.h
create mode 100644 tests/data/cdat/cdat.dat
diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
new file mode 100644
index 000..fa54506
--- /dev/null
+++ b/hw/cxl/cxl-cdat.c
@@ -0,0 +1,220 @@
+/*
+ * CXL CDAT Structure
+ *
+ * Copyright (C) 2021 Avery Design Systems, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "hw/cxl/cxl.h"
+#include "qapi/error.h"
+
+static struct cdat_dsmas dsmas = {
+.header = {
+.type = CDAT_TYPE_DSMAS,
+.length = sizeof(dsmas),
+},
+.DSMADhandle = 0,
+.flags = 0,
+.DPA_base = 0,
+.DPA_length = 0,
+};
+
+static struct cdat_dslbis dslbis = {
+.header = {
+.type = CDAT_TYPE_DSLBIS,
+.length = sizeof(dslbis),
+},
+.handle = 0,
+.flags = 0,
+.data_type = 0,
+.entry_base_unit = 0,
+};
+
+static struct cdat_dsmscis dsmscis = {
+.header = {
+.type = CDAT_TYPE_DSMSCIS,
+.length = sizeof(dsmscis),
+},
+.DSMAS_handle = 0,
+.memory_side_cache_size = 0,
+.cache_attributes = 0,
+};
+
+static struct cdat_dsis dsis = {
+.header = {
+.type = CDAT_TYPE_DSIS,
+.length = sizeof(dsis),
+},
+.flags = 0,
+.handle = 0,
+};
+
+static struct cdat_dsemts dsemts = {
+.header = {
+.type = CDAT_TYPE_DSEMTS,
+.length = sizeof(dsemts),
+},
+.DSMAS_handle = 0,
+.EFI_memory_type_attr = 0,
+.DPA_offset = 0,
+.DPA_length = 0,
+};
+
+struct cdat_sslbis {
+struct cdat_sslbis_header sslbis_header;
+struct cdat_sslbe sslbe[];
+};
+
+static struct cdat_sslbis sslbis = {
+.sslbis_header = {
+.header = {
+.type = CDAT_TYPE_SSLBIS,
+.length = sizeof(sslbis.sslbis_header) +
+ sizeof(struct cdat_sslbe) * 2,
+},
+.data_type = 0,
+.entry_base_unit = 0,
+},
+.sslbe[0] = {
+.port_x_id = 0,
+.port_y_id = 0,
+.latency_bandwidth = 0,
+},
+.sslbe[1] = {
+.port_x_id = 0,
+.port_y_id = 0,
+.latency_bandwidth = 0,
+},
+};
+
+static void *cdat_table[] = {
+(void *) ,
+(void *) ,
+(void *) ,
+(void *) ,
+(void *) ,
+(void *) ,
+};
+
+static void cdat_len_check(struct cdat_sub_header *hdr, Error **errp)
+{
+assert(hdr->length);
+assert(hdr->reserved == 0);
+
+switch (hdr->type) {
+case CDAT_TYPE_DSMAS:
+assert(hdr->length == sizeof(struct cdat_dsmas));
+break;
+case CDAT_TYPE_DSLBIS:
+assert(hdr->length == sizeof(struct cdat_dslbis));
+break;
+case CDAT_TYPE_DSMSCIS:
+assert(hdr->length == sizeof(struct cdat_dsmscis));
+break;
+case CDAT_TYPE_DSIS:
+assert(hdr->length == sizeof(struct cdat_dsis));
+break;
+case CDAT_TYPE_DSEMTS:
+assert(hdr->length == sizeof(struct cdat_dsemts));
+break;
+case CDAT_TYPE_SSLBIS:
+assert(hdr->length >= sizeof(struct cdat_sslbis_header));
+assert((hdr->length - sizeof(struct cdat_sslbis_header)) %
+ sizeof(struct cdat_sslbe) == 0);
+break;
+default:
+error_setg(errp, "Type %d is reserved", hdr->type);
+}
+}
+
+void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp)
+{
+CDATObject *cdat = _cstate->cdat;
+CDATEntry cdat_st[1024];
+uint8_t sum = 0, *buf;
+int i = 0, ent = 0, file_size = 0;
+struct cdat_sub_header *hdr;
+struct cdat_table_header *cdat_header;
+FILE *fp;
+
+fp = fopen(cdat->filename, "r");
+
+if (fp) {
+/* Read CDAT file and create its cache */
+fseek(fp, 0, SEEK_END);
+file_size = ftell(fp);
+fseek(fp, 0, SEEK_SET);
+cdat->buf = g_malloc0(file_size);
+
+if (fread(cdat->buf, file_size, 1, fp) == 0) {
+error_setg(errp, "File read failed");
+}
+
+fclose(fp);
+
+/* Set CDAT header, ent = 0 */
+cdat_st[ent].base = cdat->buf;
+cdat_st[ent].length = sizeof(struct cdat_table_header);
+ent++;
+while (i < cdat_st[0].length) {
+sum +=
