Re: [PATCH v5 cxl2.0-v3-doe 5/6] cxl/cdat: CXL CDAT Data Object Exchange implementation
On Mon, 26 Apr 2021 13:36:02 -0400
Chris Browy wrote:
> From: hchkuo
>
> The Data Object Exchange implementation of CXL Coherent Device Attribute
> Table (CDAT). This implementation is referring to "Coherent Device
> Attribute Table Specification, Rev. 1.02, Oct. 2020" and "Compute
> Express Link Specification, Rev. 2.0, Oct. 2020"
>
> The CDAT can be specified in two ways. One is to add ",cdat="
> in "-device cxl-type3"'s command option. The file is required to provide
> the whole CDAT table in binary mode. The other is to use the default
> CDAT value created by build_cdat_table in hw/cxl/cxl-cdat.c.
>
> A DOE capability of CDAT is added to hw/mem/cxl_type3.c with capability
> offset 0x190. The config read/write to this capability range can be
> generated in the OS to request the CDAT data.
>
> Signed-off-by: Chris Browy
What is here is mostly fine, but I'm not sure we want to integrate a
default table that doesn't make sense for the device that it is associated
with. So I'd move the table generation to cxl_type3 and make sure it is
'reasonable' for the device in question. Hardcoded is probably fine for now,
but ultimately it still needs to be reasonable even if that emulation becomes
more parameterized than currently.
One other comment on a possible (but incredibly unlikely) issue with
a file containing far more entries than are ever likely to make sense!
Jonathan
> ---
> hw/cxl/cxl-cdat.c | 228 +
> hw/cxl/meson.build | 1 +
> hw/mem/cxl_type3.c | 57 -
> include/hw/cxl/cxl_cdat.h | 149 +
> include/hw/cxl/cxl_component.h | 4 +
> include/hw/cxl/cxl_device.h| 1 +
> include/hw/cxl/cxl_pci.h | 1 +
> 7 files changed, 440 insertions(+), 1 deletion(-)
> create mode 100644 hw/cxl/cxl-cdat.c
> create mode 100644 include/hw/cxl/cxl_cdat.h
>
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> new file mode 100644
> index 00..3b86ecaddf
> --- /dev/null
> +++ b/hw/cxl/cxl-cdat.c
> @@ -0,0 +1,228 @@
> +/*
> + * 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"
> +#include "qemu/error-report.h"
> +
> +static void build_cdat_table(void ***cdat_table, int *len) {
> +struct cdat_dsmas *dsmas = g_malloc(sizeof(struct cdat_dsmas));
> +struct cdat_dslbis *dslbis = g_malloc(sizeof(struct cdat_dslbis));
> +struct cdat_dsmscis *dsmscis = g_malloc(sizeof(struct cdat_dsmscis));
> +struct cdat_dsis *dsis = g_malloc(sizeof(struct cdat_dsis));
> +struct cdat_dsemts *dsemts = g_malloc(sizeof(struct cdat_dsemts));
> +struct cdat_sslbis {
> +struct cdat_sslbis_header sslbis_header;
> +struct cdat_sslbe sslbe[2];
> +};
> +struct cdat_sslbis *sslbis = g_malloc(sizeof(struct cdat_sslbis));
> +void *__cdat_table[] = {
> +dsmas,
> +dslbis,
> +dsmscis,
> +dsis,
> +dsemts,
> +sslbis,
> +};
> +
> +*dsmas = (struct cdat_dsmas){
> +.header = {
> +.type = CDAT_TYPE_DSMAS,
> +.length = sizeof(struct cdat_dsmas),
> +},
> +.DSMADhandle = 0,
> +.flags = 0,
> +.DPA_base = 0,
> +.DPA_length = 0,
> +};
> +*dslbis = (struct cdat_dslbis){
> +.header = {
> +.type = CDAT_TYPE_DSLBIS,
> +.length = sizeof(struct cdat_dslbis),
> +},
> +.handle = 0,
> +.flags = 0,
> +.data_type = 0,
> +.entry_base_unit = 0,
> +};
> +*dsmscis = (struct cdat_dsmscis){
> +.header = {
> +.type = CDAT_TYPE_DSMSCIS,
> +.length = sizeof(struct cdat_dsmscis),
> +},
> +.DSMAS_handle = 0,
> +.memory_side_cache_size = 0,
> +.cache_attributes = 0,
> +};
> +*dsis = (struct cdat_dsis){
> +.header = {
> +.type = CDAT_TYPE_DSIS,
> +.length = sizeof(struct cdat_dsis),
> +},
> +.flags = 0,
> +.handle = 0,
> +};
> +*dsemts = (struct cdat_dsemts){
> +.header = {
> +.type = CDAT_TYPE_DSEMTS,
> +.length = sizeof(struct cdat_dsemts),
> +},
> +.DSMAS_handle = 0,
> +.EFI_memory_type_attr = 0,
> +.DPA_offset = 0,
> +.DPA_length = 0,
> +};
> +*sslbis = (struct cdat_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,
> +},
> +
[PATCH v5 cxl2.0-v3-doe 5/6] cxl/cdat: CXL CDAT Data Object Exchange implementation
From: hchkuo
The Data Object Exchange implementation of CXL Coherent Device Attribute
Table (CDAT). This implementation is referring to "Coherent Device
Attribute Table Specification, Rev. 1.02, Oct. 2020" and "Compute
Express Link Specification, Rev. 2.0, Oct. 2020"
The CDAT can be specified in two ways. One is to add ",cdat="
in "-device cxl-type3"'s command option. The file is required to provide
the whole CDAT table in binary mode. The other is to use the default
CDAT value created by build_cdat_table in hw/cxl/cxl-cdat.c.
A DOE capability of CDAT is added to hw/mem/cxl_type3.c with capability
offset 0x190. The config read/write to this capability range can be
generated in the OS to request the CDAT data.
Signed-off-by: Chris Browy
---
hw/cxl/cxl-cdat.c | 228 +
hw/cxl/meson.build | 1 +
hw/mem/cxl_type3.c | 57 -
include/hw/cxl/cxl_cdat.h | 149 +
include/hw/cxl/cxl_component.h | 4 +
include/hw/cxl/cxl_device.h| 1 +
include/hw/cxl/cxl_pci.h | 1 +
7 files changed, 440 insertions(+), 1 deletion(-)
create mode 100644 hw/cxl/cxl-cdat.c
create mode 100644 include/hw/cxl/cxl_cdat.h
diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
new file mode 100644
index 00..3b86ecaddf
--- /dev/null
+++ b/hw/cxl/cxl-cdat.c
@@ -0,0 +1,228 @@
+/*
+ * 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"
+#include "qemu/error-report.h"
+
+static void build_cdat_table(void ***cdat_table, int *len) {
+struct cdat_dsmas *dsmas = g_malloc(sizeof(struct cdat_dsmas));
+struct cdat_dslbis *dslbis = g_malloc(sizeof(struct cdat_dslbis));
+struct cdat_dsmscis *dsmscis = g_malloc(sizeof(struct cdat_dsmscis));
+struct cdat_dsis *dsis = g_malloc(sizeof(struct cdat_dsis));
+struct cdat_dsemts *dsemts = g_malloc(sizeof(struct cdat_dsemts));
+struct cdat_sslbis {
+struct cdat_sslbis_header sslbis_header;
+struct cdat_sslbe sslbe[2];
+};
+struct cdat_sslbis *sslbis = g_malloc(sizeof(struct cdat_sslbis));
+void *__cdat_table[] = {
+dsmas,
+dslbis,
+dsmscis,
+dsis,
+dsemts,
+sslbis,
+};
+
+*dsmas = (struct cdat_dsmas){
+.header = {
+.type = CDAT_TYPE_DSMAS,
+.length = sizeof(struct cdat_dsmas),
+},
+.DSMADhandle = 0,
+.flags = 0,
+.DPA_base = 0,
+.DPA_length = 0,
+};
+*dslbis = (struct cdat_dslbis){
+.header = {
+.type = CDAT_TYPE_DSLBIS,
+.length = sizeof(struct cdat_dslbis),
+},
+.handle = 0,
+.flags = 0,
+.data_type = 0,
+.entry_base_unit = 0,
+};
+*dsmscis = (struct cdat_dsmscis){
+.header = {
+.type = CDAT_TYPE_DSMSCIS,
+.length = sizeof(struct cdat_dsmscis),
+},
+.DSMAS_handle = 0,
+.memory_side_cache_size = 0,
+.cache_attributes = 0,
+};
+*dsis = (struct cdat_dsis){
+.header = {
+.type = CDAT_TYPE_DSIS,
+.length = sizeof(struct cdat_dsis),
+},
+.flags = 0,
+.handle = 0,
+};
+*dsemts = (struct cdat_dsemts){
+.header = {
+.type = CDAT_TYPE_DSEMTS,
+.length = sizeof(struct cdat_dsemts),
+},
+.DSMAS_handle = 0,
+.EFI_memory_type_attr = 0,
+.DPA_offset = 0,
+.DPA_length = 0,
+};
+*sslbis = (struct cdat_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,
+},
+};
+
+*len = ARRAY_SIZE(__cdat_table);
+*cdat_table = g_malloc0((*len) * sizeof(void *));
+memcpy(*cdat_table, __cdat_table, (*len) * sizeof(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
