* [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup.
@ 2024-01-24 12:40 Jonathan Cameron
2024-01-24 12:40 ` [PATCH 01/12] cxl/cdat: Handle cdat table build errors Jonathan Cameron
` (11 more replies)
0 siblings, 12 replies; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
This set includes the majority of fixes I currently have queued
up for the CXL emulation. Most have been posted and reviewed
before so this is just rebases of those.
I've included the _STA fix as that's fairly trivial
and didn't seem worth a separate posting. Also included
dropping some pointless g_malloc0() failure handling that
came up in review of Ira's "cxl/cdat: Handle cdat table build errors".
Note the [... qemu] marking above is because we currently use the
linux-cxl@vger.kernel.org list to ensure CXL people notice the
patches and difficulty of distinguishing QEMU vs Kernel patches
was causing some grief for tracking in patchwork.
Hopefully this is unobtrusive enough to not annoy QEMU reviewers!
Davidlohr Bueso (1):
hw/cxl/mbox: Remove dead code
Hyeonggon Yoo (1):
hw/cxl/device: read from register values in mdev_reg_read()
Ira Weiny (2):
cxl/cdat: Handle cdat table build errors
cxl/cdat: Fix header sum value in CDAT checksum
Jonathan Cameron (6):
hw/mem/cxl_type3: Drop handling of failure of g_malloc0()
hw/pci-bridge/cxl_upstream: Drop g_malloc0() failure handling
hw/mem/cxl_type3: Fix potential divide by zero reported by coverity
tests/acpi: Allow update of DSDT.cxl
hw/i386: Fix _STA return value for ACPI0017
tests/acpi: Update DSDT.cxl to reflect change _STA return value.
Li Zhijian (2):
hw/cxl: Pass CXLComponentState to cache_mem_ops
hw/cxl: Pass NULL for a NULL MemoryRegionOps
include/hw/cxl/cxl_device.h | 9 ++++--
hw/cxl/cxl-cdat.c | 11 +++++--
hw/cxl/cxl-component-utils.c | 4 +--
hw/cxl/cxl-device-utils.c | 17 ++++++----
hw/cxl/cxl-mailbox-utils.c | 43 +++++++++----------------
hw/i386/acpi-build.c | 2 +-
hw/mem/cxl_type3.c | 60 ++++++++---------------------------
hw/pci-bridge/cxl_upstream.c | 6 ----
tests/data/acpi/q35/DSDT.cxl | Bin 9713 -> 9714 bytes
9 files changed, 57 insertions(+), 95 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 01/12] cxl/cdat: Handle cdat table build errors
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 21:04 ` fan
2024-01-24 12:40 ` [PATCH 02/12] hw/mem/cxl_type3: Drop handling of failure of g_malloc0() Jonathan Cameron
` (10 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
From: Ira Weiny <ira.weiny@intel.com>
The callback for building CDAT tables may return negative error codes.
This was previously unhandled and will result in potentially huge
allocations later on in ct3_build_cdat()
Detect the negative error code and defer cdat building.
Fixes: f5ee7413d592 ("hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange")
Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
hw/cxl/cxl-cdat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 639a2db3e1..24829cf242 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -63,7 +63,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
cdat->private);
- if (!cdat->built_buf_len) {
+ if (cdat->built_buf_len <= 0) {
/* Build later as not all data available yet */
cdat->to_update = true;
return;
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 02/12] hw/mem/cxl_type3: Drop handling of failure of g_malloc0()
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
2024-01-24 12:40 ` [PATCH 01/12] cxl/cdat: Handle cdat table build errors Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 21:10 ` fan
2024-01-24 12:40 ` [PATCH 03/12] hw/pci-bridge/cxl_upstream: Drop g_malloc0() failure handling Jonathan Cameron
` (9 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
As g_malloc0 will just exit QEMU on failure there is no point
in checking for it failing.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
hw/mem/cxl_type3.c | 52 +++++++---------------------------------------
1 file changed, 7 insertions(+), 45 deletions(-)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 52647b4ac7..1b92a065a3 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -42,9 +42,9 @@ enum {
CT3_CDAT_NUM_ENTRIES
};
-static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
- int dsmad_handle, MemoryRegion *mr,
- bool is_pmem, uint64_t dpa_base)
+static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
+ int dsmad_handle, MemoryRegion *mr,
+ bool is_pmem, uint64_t dpa_base)
{
g_autofree CDATDsmas *dsmas = NULL;
g_autofree CDATDslbis *dslbis0 = NULL;
@@ -54,9 +54,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
g_autofree CDATDsemts *dsemts = NULL;
dsmas = g_malloc(sizeof(*dsmas));
- if (!dsmas) {
- return -ENOMEM;
- }
*dsmas = (CDATDsmas) {
.header = {
.type = CDAT_TYPE_DSMAS,
@@ -70,9 +67,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
/* For now, no memory side cache, plausiblish numbers */
dslbis0 = g_malloc(sizeof(*dslbis0));
- if (!dslbis0) {
- return -ENOMEM;
- }
*dslbis0 = (CDATDslbis) {
.header = {
.type = CDAT_TYPE_DSLBIS,
@@ -86,9 +80,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
};
dslbis1 = g_malloc(sizeof(*dslbis1));
- if (!dslbis1) {
- return -ENOMEM;
- }
*dslbis1 = (CDATDslbis) {
.header = {
.type = CDAT_TYPE_DSLBIS,
@@ -102,9 +93,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
};
dslbis2 = g_malloc(sizeof(*dslbis2));
- if (!dslbis2) {
- return -ENOMEM;
- }
*dslbis2 = (CDATDslbis) {
.header = {
.type = CDAT_TYPE_DSLBIS,
@@ -118,9 +106,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
};
dslbis3 = g_malloc(sizeof(*dslbis3));
- if (!dslbis3) {
- return -ENOMEM;
- }
*dslbis3 = (CDATDslbis) {
.header = {
.type = CDAT_TYPE_DSLBIS,
@@ -134,9 +119,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
};
dsemts = g_malloc(sizeof(*dsemts));
- if (!dsemts) {
- return -ENOMEM;
- }
*dsemts = (CDATDsemts) {
.header = {
.type = CDAT_TYPE_DSEMTS,
@@ -159,8 +141,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
cdat_table[CT3_CDAT_DSLBIS2] = g_steal_pointer(&dslbis2);
cdat_table[CT3_CDAT_DSLBIS3] = g_steal_pointer(&dslbis3);
cdat_table[CT3_CDAT_DSEMTS] = g_steal_pointer(&dsemts);
-
- return 0;
}
static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
@@ -171,7 +151,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
int dsmad_handle = 0;
int cur_ent = 0;
int len = 0;
- int rc, i;
if (!ct3d->hostpmem && !ct3d->hostvmem) {
return 0;
@@ -194,27 +173,18 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
}
table = g_malloc0(len * sizeof(*table));
- if (!table) {
- return -ENOMEM;
- }
/* Now fill them in */
if (volatile_mr) {
- rc = ct3_build_cdat_entries_for_mr(table, dsmad_handle++, volatile_mr,
- false, 0);
- if (rc < 0) {
- return rc;
- }
+ ct3_build_cdat_entries_for_mr(table, dsmad_handle++, volatile_mr,
+ false, 0);
cur_ent = CT3_CDAT_NUM_ENTRIES;
}
if (nonvolatile_mr) {
uint64_t base = volatile_mr ? memory_region_size(volatile_mr) : 0;
- rc = ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
- nonvolatile_mr, true, base);
- if (rc < 0) {
- goto error_cleanup;
- }
+ ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
+ nonvolatile_mr, true, base);
cur_ent += CT3_CDAT_NUM_ENTRIES;
}
assert(len == cur_ent);
@@ -222,11 +192,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
*cdat_table = g_steal_pointer(&table);
return len;
-error_cleanup:
- for (i = 0; i < cur_ent; i++) {
- g_free(table[i]);
- }
- return rc;
}
static void ct3_free_cdat_table(CDATSubHeader **cdat_table, int num, void *priv)
@@ -1168,9 +1133,6 @@ void qmp_cxl_inject_uncorrectable_errors(const char *path,
}
cxl_err = g_malloc0(sizeof(*cxl_err));
- if (!cxl_err) {
- return;
- }
cxl_err->type = cxl_err_code;
while (header && header_count < 32) {
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 03/12] hw/pci-bridge/cxl_upstream: Drop g_malloc0() failure handling
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
2024-01-24 12:40 ` [PATCH 01/12] cxl/cdat: Handle cdat table build errors Jonathan Cameron
2024-01-24 12:40 ` [PATCH 02/12] hw/mem/cxl_type3: Drop handling of failure of g_malloc0() Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 21:11 ` fan
2024-01-24 12:40 ` [PATCH 04/12] cxl/cdat: Fix header sum value in CDAT checksum Jonathan Cameron
` (8 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
As a failure of g_malloc0() will result in QEMU exiting, it
won't return a NULL to check. As such, drop the incorrect handling
of such NULL returns in the cdat table building code.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
hw/pci-bridge/cxl_upstream.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
index 36737189c6..d5341b530f 100644
--- a/hw/pci-bridge/cxl_upstream.c
+++ b/hw/pci-bridge/cxl_upstream.c
@@ -228,9 +228,6 @@ static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
sslbis_size = sizeof(CDATSslbis) + sizeof(*sslbis_latency->sslbe) * count;
sslbis_latency = g_malloc(sslbis_size);
- if (!sslbis_latency) {
- return -ENOMEM;
- }
*sslbis_latency = (CDATSslbis) {
.sslbis_header = {
.header = {
@@ -251,9 +248,6 @@ static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
}
sslbis_bandwidth = g_malloc(sslbis_size);
- if (!sslbis_bandwidth) {
- return 0;
- }
*sslbis_bandwidth = (CDATSslbis) {
.sslbis_header = {
.header = {
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 04/12] cxl/cdat: Fix header sum value in CDAT checksum
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
` (2 preceding siblings ...)
2024-01-24 12:40 ` [PATCH 03/12] hw/pci-bridge/cxl_upstream: Drop g_malloc0() failure handling Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 12:40 ` [PATCH 05/12] hw/cxl/mbox: Remove dead code Jonathan Cameron
` (7 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
From: Ira Weiny <ira.weiny@intel.com>
The addition of the DCD support for CXL type-3 devices extended the CDAT
table large enough that the checksum being returned was incorrect.[1]
This was because the checksum value was using the header length field
rather than each of the 4 bytes of the length field. This was
previously not seen because the length of the CDAT data was less than
256 thus resulting in an equivalent checksum value.
Properly calculate the checksum for the CDAT header.
[1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b40707d7@intel.com/
Fixes: aba578bdace5 ("hw/cxl/cdat: CXL CDAT Data Object Exchange implementation")
Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Ammended buf naming to hdr_buf to avoid shadow issue with an existing buf variable
with more limited scope.
---
hw/cxl/cxl-cdat.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 24829cf242..2fea975671 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -49,6 +49,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
g_autofree CDATTableHeader *cdat_header = NULL;
g_autofree CDATEntry *cdat_st = NULL;
uint8_t sum = 0;
+ uint8_t *hdr_buf;
int ent, i;
/* Use default table if fopen == NULL */
@@ -95,8 +96,12 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
/* For now, no runtime updates */
cdat_header->sequence = 0;
cdat_header->length += sizeof(CDATTableHeader);
- sum += cdat_header->revision + cdat_header->sequence +
- cdat_header->length;
+
+ hdr_buf = (uint8_t *)cdat_header;
+ for (i = 0; i < sizeof(*cdat_header); i++) {
+ sum += hdr_buf[i];
+ }
+
/* Sum of all bytes including checksum must be 0 */
cdat_header->checksum = ~sum + 1;
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 05/12] hw/cxl/mbox: Remove dead code
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
` (3 preceding siblings ...)
2024-01-24 12:40 ` [PATCH 04/12] cxl/cdat: Fix header sum value in CDAT checksum Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 23:41 ` fan
2024-01-24 12:40 ` [PATCH 06/12] hw/cxl/device: read from register values in mdev_reg_read() Jonathan Cameron
` (6 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
From: Davidlohr Bueso <dave@stgolabs.net>
Two functions were reported to have dead code, remove the bogus
branches altogether, as well as a misplaced qemu_log call.
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
hw/cxl/cxl-mailbox-utils.c | 43 +++++++++++++-------------------------
1 file changed, 15 insertions(+), 28 deletions(-)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 6eff56fb1b..dc42850767 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1001,15 +1001,8 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
cxl_dev_disable_media(&ct3d->cxl_dstate);
- if (secs > 2) {
- /* sanitize when done */
- return CXL_MBOX_BG_STARTED;
- } else {
- __do_sanitization(ct3d);
- cxl_dev_enable_media(&ct3d->cxl_dstate);
-
- return CXL_MBOX_SUCCESS;
- }
+ /* sanitize when done */
+ return CXL_MBOX_BG_STARTED;
}
static CXLRetCode cmd_get_security_state(const struct cxl_cmd *cmd,
@@ -1387,27 +1380,21 @@ static void bg_timercb(void *opaque)
cci->bg.complete_pct = 100;
cci->bg.ret_code = ret;
- if (ret == CXL_MBOX_SUCCESS) {
- switch (cci->bg.opcode) {
- case 0x4400: /* sanitize */
- {
- CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
-
- __do_sanitization(ct3d);
- cxl_dev_enable_media(&ct3d->cxl_dstate);
- }
+ switch (cci->bg.opcode) {
+ case 0x4400: /* sanitize */
+ {
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
+
+ __do_sanitization(ct3d);
+ cxl_dev_enable_media(&ct3d->cxl_dstate);
+ }
+ break;
+ case 0x4304: /* TODO: scan media */
+ break;
+ default:
+ __builtin_unreachable();
break;
- case 0x4304: /* TODO: scan media */
- break;
- default:
- __builtin_unreachable();
- break;
- }
}
-
- qemu_log("Background command %04xh finished: %s\n",
- cci->bg.opcode,
- ret == CXL_MBOX_SUCCESS ? "success" : "aborted");
} else {
/* estimate only */
cci->bg.complete_pct = 100 * now / total_time;
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 06/12] hw/cxl/device: read from register values in mdev_reg_read()
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
` (4 preceding siblings ...)
2024-01-24 12:40 ` [PATCH 05/12] hw/cxl/mbox: Remove dead code Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 12:40 ` [PATCH 07/12] hw/cxl: Pass CXLComponentState to cache_mem_ops Jonathan Cameron
` (5 subsequent siblings)
11 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
From: Hyeonggon Yoo <42.hyeyoo@gmail.com>
In the current mdev_reg_read() implementation, it consistently returns
that the Media Status is Ready (01b). This was fine until commit
25a52959f99d ("hw/cxl: Add support for device sanitation") because the
media was presumed to be ready.
However, as per the CXL 3.0 spec "8.2.9.8.5.1 Sanitize (Opcode 4400h)",
during sanitation, the Media State should be set to Disabled (11b). The
mentioned commit correctly sets it to Disabled, but mdev_reg_read()
still returns Media Status as Ready.
To address this, update mdev_reg_read() to read register values instead
of returning dummy values.
Note that __toggle_media() managed to not only write something
that no one read, it did it to the wrong register storage and
so changed the reported mailbox size which was definitely not
the intent. That gets fixed as a side effect of allocating
separate state storage for this register.
Fixes: commit 25a52959f99d ("hw/cxl: Add support for device sanitation")
Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Patch description expanded to cover the overwrite bug fixed almost
by accident in this patch.
---
include/hw/cxl/cxl_device.h | 9 +++++++--
hw/cxl/cxl-device-utils.c | 17 +++++++++++------
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index befb5f884b..31d2afcd3d 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -202,6 +202,9 @@ typedef struct cxl_device_state {
};
};
+ /* Stash the memory device status value */
+ uint64_t memdev_status;
+
struct {
bool set;
uint64_t last_set;
@@ -353,8 +356,10 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val)
{
uint64_t dev_status_reg;
- dev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
- cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg;
+ dev_status_reg = cxl_dstate->memdev_status;
+ dev_status_reg = FIELD_DP64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS,
+ val);
+ cxl_dstate->memdev_status = dev_status_reg;
}
#define cxl_dev_disable_media(cxlds) \
do { __toggle_media((cxlds), 0x3); } while (0)
diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index 61a3c4dc2e..40b619ffd9 100644
--- a/hw/cxl/cxl-device-utils.c
+++ b/hw/cxl/cxl-device-utils.c
@@ -229,12 +229,9 @@ static void mailbox_reg_write(void *opaque, hwaddr offset, uint64_t value,
static uint64_t mdev_reg_read(void *opaque, hwaddr offset, unsigned size)
{
- uint64_t retval = 0;
-
- retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MEDIA_STATUS, 1);
- retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MBOX_READY, 1);
+ CXLDeviceState *cxl_dstate = opaque;
- return retval;
+ return cxl_dstate->memdev_status;
}
static void ro_reg_write(void *opaque, hwaddr offset, uint64_t value,
@@ -371,7 +368,15 @@ static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
cxl_dstate->mbox_msi_n = msi_n;
}
-static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) { }
+static void memdev_reg_init_common(CXLDeviceState *cxl_dstate)
+{
+ uint64_t memdev_status_reg;
+
+ memdev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, 1);
+ memdev_status_reg = FIELD_DP64(memdev_status_reg, CXL_MEM_DEV_STS,
+ MBOX_READY, 1);
+ cxl_dstate->memdev_status = memdev_status_reg;
+}
void cxl_device_register_init_t3(CXLType3Dev *ct3d)
{
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 07/12] hw/cxl: Pass CXLComponentState to cache_mem_ops
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
` (5 preceding siblings ...)
2024-01-24 12:40 ` [PATCH 06/12] hw/cxl/device: read from register values in mdev_reg_read() Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 23:46 ` fan
2024-01-24 12:40 ` [PATCH 08/12] hw/cxl: Pass NULL for a NULL MemoryRegionOps Jonathan Cameron
` (4 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
From: Li Zhijian <lizhijian@fujitsu.com>
cache_mem_ops.{read,write}() interprets opaque as
CXLComponentState(cxl_cstate) instead of ComponentRegisters(cregs).
Fortunately, cregs is the first member of cxl_cstate, so their values are
the same.
Fixes: 9e58f52d3f8 ("hw/cxl/component: Introduce CXL components (8.1.x, 8.2.5)")
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
hw/cxl/cxl-component-utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index 29d477492b..9dfde6c0b3 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -199,7 +199,7 @@ void cxl_component_register_block_init(Object *obj,
/* io registers controls link which we don't care about in QEMU */
memory_region_init_io(&cregs->io, obj, NULL, cregs, ".io",
CXL2_COMPONENT_IO_REGION_SIZE);
- memory_region_init_io(&cregs->cache_mem, obj, &cache_mem_ops, cregs,
+ memory_region_init_io(&cregs->cache_mem, obj, &cache_mem_ops, cxl_cstate,
".cache_mem", CXL2_COMPONENT_CM_REGION_SIZE);
memory_region_add_subregion(&cregs->component_registers, 0, &cregs->io);
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 08/12] hw/cxl: Pass NULL for a NULL MemoryRegionOps
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
` (6 preceding siblings ...)
2024-01-24 12:40 ` [PATCH 07/12] hw/cxl: Pass CXLComponentState to cache_mem_ops Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 23:51 ` fan
2024-01-24 12:40 ` [PATCH 09/12] hw/mem/cxl_type3: Fix potential divide by zero reported by coverity Jonathan Cameron
` (3 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
From: Li Zhijian <lizhijian@fujitsu.com>
a NULL parameter is enough for a NULL MemoryRegionOps
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
hw/cxl/cxl-component-utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index 9dfde6c0b3..5ddd47ed8d 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -197,7 +197,7 @@ void cxl_component_register_block_init(Object *obj,
CXL2_COMPONENT_BLOCK_SIZE);
/* io registers controls link which we don't care about in QEMU */
- memory_region_init_io(&cregs->io, obj, NULL, cregs, ".io",
+ memory_region_init_io(&cregs->io, obj, NULL, NULL, ".io",
CXL2_COMPONENT_IO_REGION_SIZE);
memory_region_init_io(&cregs->cache_mem, obj, &cache_mem_ops, cxl_cstate,
".cache_mem", CXL2_COMPONENT_CM_REGION_SIZE);
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 09/12] hw/mem/cxl_type3: Fix potential divide by zero reported by coverity
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
` (7 preceding siblings ...)
2024-01-24 12:40 ` [PATCH 08/12] hw/cxl: Pass NULL for a NULL MemoryRegionOps Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 23:53 ` fan
2024-01-24 12:40 ` [PATCH 10/12] tests/acpi: Allow update of DSDT.cxl Jonathan Cameron
` (2 subsequent siblings)
11 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
Fixes Coverity ID 1522368.
Currently error_fatal is set if interleave_ways_dec() is going to return 0
but we should handle that zero return explicitly.
Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Note this is a stop gap until a more complex HDM decoder verification series.
---
hw/mem/cxl_type3.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 1b92a065a3..24211703c6 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -794,8 +794,12 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
}
if (((uint64_t)host_addr < decoder_base) ||
(hpa_offset >= decoder_size)) {
- dpa_base += decoder_size /
- cxl_interleave_ways_dec(iw, &error_fatal);
+ int decoded_iw = cxl_interleave_ways_dec(iw, &error_fatal);
+
+ if (decoded_iw == 0)
+ return false;
+
+ dpa_base += decoder_size / decoded_iw;
continue;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 10/12] tests/acpi: Allow update of DSDT.cxl
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
` (8 preceding siblings ...)
2024-01-24 12:40 ` [PATCH 09/12] hw/mem/cxl_type3: Fix potential divide by zero reported by coverity Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 12:40 ` [PATCH 11/12] hw/i386: Fix _STA return value for ACPI0017 Jonathan Cameron
2024-01-24 12:41 ` [PATCH 12/12] tests/acpi: Update DSDT.cxl to reflect change _STA return value Jonathan Cameron
11 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
The _STA value returned currently indicates the ACPI0017 device
is not enabled. Whilst this isn't a real device, setting _STA
like this may prevent an OS from enumerating it correctly and
hence from parsing the CEDT table.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..9ce0f596cc 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/DSDT.cxl",
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 11/12] hw/i386: Fix _STA return value for ACPI0017
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
` (9 preceding siblings ...)
2024-01-24 12:40 ` [PATCH 10/12] tests/acpi: Allow update of DSDT.cxl Jonathan Cameron
@ 2024-01-24 12:40 ` Jonathan Cameron
2024-01-24 12:41 ` [PATCH 12/12] tests/acpi: Update DSDT.cxl to reflect change _STA return value Jonathan Cameron
11 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:40 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
Found whilst testing a series for the linux kernel that actually
bothers to check if enabled is set. 0xB is the option used
for vast majority of DSDT entries in QEMU.
It is a little odd for a device that doesn't really exist and
is simply a hook to tell the OS there is a CEDT table but 0xB
seems a reasonable choice and avoids need to special case
this device in the OS.
Means:
* Device present.
* Device enabled and decoding it's resources.
* Not shown in UI
* Functioning properly
* No battery (on this device!)
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
hw/i386/acpi-build.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index edc979379c..e3e9afd376 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1415,7 +1415,7 @@ static void build_acpi0017(Aml *table)
aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0017")));
method = aml_method("_STA", 0, AML_NOTSERIALIZED);
- aml_append(method, aml_return(aml_int(0x01)));
+ aml_append(method, aml_return(aml_int(0x0B)));
aml_append(dev, method);
build_cxl_dsm_method(dev);
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 12/12] tests/acpi: Update DSDT.cxl to reflect change _STA return value.
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
` (10 preceding siblings ...)
2024-01-24 12:40 ` [PATCH 11/12] hw/i386: Fix _STA return value for ACPI0017 Jonathan Cameron
@ 2024-01-24 12:41 ` Jonathan Cameron
11 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-24 12:41 UTC (permalink / raw
To: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin
Cc: Ira Weiny, Huai-Cheng Kuo, Dave Jiang, Peter Maydell,
Davidlohr Bueso, Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi,
linuxarm, Philippe Mathieu-Daudé
_STA will now return 0xB (in common with most other devices)
rather than not setting the bits to indicate this fake device
has not been enabled, and self tests haven't passed.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 1 -
tests/data/acpi/q35/DSDT.cxl | Bin 9713 -> 9714 bytes
2 files changed, 1 deletion(-)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 9ce0f596cc..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,2 +1 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/DSDT.cxl",
diff --git a/tests/data/acpi/q35/DSDT.cxl b/tests/data/acpi/q35/DSDT.cxl
index 145301c52af9a17242bb306c210f8a7e0f01b827..afcdc0d0ba8e41bb70ac20a78dcc8562ca0cb74b 100644
GIT binary patch
delta 64
zcmez9{mGllCD<k8lPUuP<Nb|X6BX6{S>uD9;-j0~S)C(%d^zGhJY9GlodY}#3=GW~
UL^$JvLmU~FaB*)wsA#|f0Fwq1RsaA1
delta 63
zcmez5{n4AtCD<k8qbdUf<M)kR6BX6`SmT48;-j0~Se+w$d^zGhJY9GlodY}#3=GW~
TL^$GuLmU~FFm67iXutvhh3yhn
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 01/12] cxl/cdat: Handle cdat table build errors
2024-01-24 12:40 ` [PATCH 01/12] cxl/cdat: Handle cdat table build errors Jonathan Cameron
@ 2024-01-24 21:04 ` fan
0 siblings, 0 replies; 21+ messages in thread
From: fan @ 2024-01-24 21:04 UTC (permalink / raw
To: Jonathan Cameron
Cc: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin, Ira Weiny,
Huai-Cheng Kuo, Dave Jiang, Peter Maydell, Davidlohr Bueso,
Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi, linuxarm,
Philippe Mathieu-Daudé
On Wed, Jan 24, 2024 at 12:40:49PM +0000, Jonathan Cameron wrote:
> From: Ira Weiny <ira.weiny@intel.com>
>
> The callback for building CDAT tables may return negative error codes.
> This was previously unhandled and will result in potentially huge
> allocations later on in ct3_build_cdat()
>
> Detect the negative error code and defer cdat building.
>
> Fixes: f5ee7413d592 ("hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange")
> Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
Reviewed-by: Fan Ni <fan.ni@samsung.com>
> hw/cxl/cxl-cdat.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index 639a2db3e1..24829cf242 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -63,7 +63,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
> cdat->private);
>
> - if (!cdat->built_buf_len) {
> + if (cdat->built_buf_len <= 0) {
> /* Build later as not all data available yet */
> cdat->to_update = true;
> return;
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 02/12] hw/mem/cxl_type3: Drop handling of failure of g_malloc0()
2024-01-24 12:40 ` [PATCH 02/12] hw/mem/cxl_type3: Drop handling of failure of g_malloc0() Jonathan Cameron
@ 2024-01-24 21:10 ` fan
0 siblings, 0 replies; 21+ messages in thread
From: fan @ 2024-01-24 21:10 UTC (permalink / raw
To: Jonathan Cameron
Cc: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin, Ira Weiny,
Huai-Cheng Kuo, Dave Jiang, Peter Maydell, Davidlohr Bueso,
Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi, linuxarm,
Philippe Mathieu-Daudé
On Wed, Jan 24, 2024 at 12:40:50PM +0000, Jonathan Cameron wrote:
> As g_malloc0 will just exit QEMU on failure there is no point
> in checking for it failing.
The change is also related to g_malloc. So we may want to also mention it in
the comments like " As g_malloc and g_malloc0 will just .... ". Other
than that, LGTM.
Reviewed-by: Fan Ni <fan.ni@samsung.com>
Fan
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> hw/mem/cxl_type3.c | 52 +++++++---------------------------------------
> 1 file changed, 7 insertions(+), 45 deletions(-)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 52647b4ac7..1b92a065a3 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -42,9 +42,9 @@ enum {
> CT3_CDAT_NUM_ENTRIES
> };
>
> -static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> - int dsmad_handle, MemoryRegion *mr,
> - bool is_pmem, uint64_t dpa_base)
> +static void ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> + int dsmad_handle, MemoryRegion *mr,
> + bool is_pmem, uint64_t dpa_base)
> {
> g_autofree CDATDsmas *dsmas = NULL;
> g_autofree CDATDslbis *dslbis0 = NULL;
> @@ -54,9 +54,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> g_autofree CDATDsemts *dsemts = NULL;
>
> dsmas = g_malloc(sizeof(*dsmas));
> - if (!dsmas) {
> - return -ENOMEM;
> - }
> *dsmas = (CDATDsmas) {
> .header = {
> .type = CDAT_TYPE_DSMAS,
> @@ -70,9 +67,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
>
> /* For now, no memory side cache, plausiblish numbers */
> dslbis0 = g_malloc(sizeof(*dslbis0));
> - if (!dslbis0) {
> - return -ENOMEM;
> - }
> *dslbis0 = (CDATDslbis) {
> .header = {
> .type = CDAT_TYPE_DSLBIS,
> @@ -86,9 +80,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> };
>
> dslbis1 = g_malloc(sizeof(*dslbis1));
> - if (!dslbis1) {
> - return -ENOMEM;
> - }
> *dslbis1 = (CDATDslbis) {
> .header = {
> .type = CDAT_TYPE_DSLBIS,
> @@ -102,9 +93,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> };
>
> dslbis2 = g_malloc(sizeof(*dslbis2));
> - if (!dslbis2) {
> - return -ENOMEM;
> - }
> *dslbis2 = (CDATDslbis) {
> .header = {
> .type = CDAT_TYPE_DSLBIS,
> @@ -118,9 +106,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> };
>
> dslbis3 = g_malloc(sizeof(*dslbis3));
> - if (!dslbis3) {
> - return -ENOMEM;
> - }
> *dslbis3 = (CDATDslbis) {
> .header = {
> .type = CDAT_TYPE_DSLBIS,
> @@ -134,9 +119,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> };
>
> dsemts = g_malloc(sizeof(*dsemts));
> - if (!dsemts) {
> - return -ENOMEM;
> - }
> *dsemts = (CDATDsemts) {
> .header = {
> .type = CDAT_TYPE_DSEMTS,
> @@ -159,8 +141,6 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> cdat_table[CT3_CDAT_DSLBIS2] = g_steal_pointer(&dslbis2);
> cdat_table[CT3_CDAT_DSLBIS3] = g_steal_pointer(&dslbis3);
> cdat_table[CT3_CDAT_DSEMTS] = g_steal_pointer(&dsemts);
> -
> - return 0;
> }
>
> static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> @@ -171,7 +151,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> int dsmad_handle = 0;
> int cur_ent = 0;
> int len = 0;
> - int rc, i;
>
> if (!ct3d->hostpmem && !ct3d->hostvmem) {
> return 0;
> @@ -194,27 +173,18 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> }
>
> table = g_malloc0(len * sizeof(*table));
> - if (!table) {
> - return -ENOMEM;
> - }
>
> /* Now fill them in */
> if (volatile_mr) {
> - rc = ct3_build_cdat_entries_for_mr(table, dsmad_handle++, volatile_mr,
> - false, 0);
> - if (rc < 0) {
> - return rc;
> - }
> + ct3_build_cdat_entries_for_mr(table, dsmad_handle++, volatile_mr,
> + false, 0);
> cur_ent = CT3_CDAT_NUM_ENTRIES;
> }
>
> if (nonvolatile_mr) {
> uint64_t base = volatile_mr ? memory_region_size(volatile_mr) : 0;
> - rc = ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
> - nonvolatile_mr, true, base);
> - if (rc < 0) {
> - goto error_cleanup;
> - }
> + ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
> + nonvolatile_mr, true, base);
> cur_ent += CT3_CDAT_NUM_ENTRIES;
> }
> assert(len == cur_ent);
> @@ -222,11 +192,6 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> *cdat_table = g_steal_pointer(&table);
>
> return len;
> -error_cleanup:
> - for (i = 0; i < cur_ent; i++) {
> - g_free(table[i]);
> - }
> - return rc;
> }
>
> static void ct3_free_cdat_table(CDATSubHeader **cdat_table, int num, void *priv)
> @@ -1168,9 +1133,6 @@ void qmp_cxl_inject_uncorrectable_errors(const char *path,
> }
>
> cxl_err = g_malloc0(sizeof(*cxl_err));
> - if (!cxl_err) {
> - return;
> - }
>
> cxl_err->type = cxl_err_code;
> while (header && header_count < 32) {
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 03/12] hw/pci-bridge/cxl_upstream: Drop g_malloc0() failure handling
2024-01-24 12:40 ` [PATCH 03/12] hw/pci-bridge/cxl_upstream: Drop g_malloc0() failure handling Jonathan Cameron
@ 2024-01-24 21:11 ` fan
0 siblings, 0 replies; 21+ messages in thread
From: fan @ 2024-01-24 21:11 UTC (permalink / raw
To: Jonathan Cameron
Cc: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin, Ira Weiny,
Huai-Cheng Kuo, Dave Jiang, Peter Maydell, Davidlohr Bueso,
Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi, linuxarm,
Philippe Mathieu-Daudé
On Wed, Jan 24, 2024 at 12:40:51PM +0000, Jonathan Cameron wrote:
> As a failure of g_malloc0() will result in QEMU exiting, it
s/g_malloc0/g_malloc/
Fan
> won't return a NULL to check. As such, drop the incorrect handling
> of such NULL returns in the cdat table building code.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> hw/pci-bridge/cxl_upstream.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
> index 36737189c6..d5341b530f 100644
> --- a/hw/pci-bridge/cxl_upstream.c
> +++ b/hw/pci-bridge/cxl_upstream.c
> @@ -228,9 +228,6 @@ static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
>
> sslbis_size = sizeof(CDATSslbis) + sizeof(*sslbis_latency->sslbe) * count;
> sslbis_latency = g_malloc(sslbis_size);
> - if (!sslbis_latency) {
> - return -ENOMEM;
> - }
> *sslbis_latency = (CDATSslbis) {
> .sslbis_header = {
> .header = {
> @@ -251,9 +248,6 @@ static int build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
> }
>
> sslbis_bandwidth = g_malloc(sslbis_size);
> - if (!sslbis_bandwidth) {
> - return 0;
> - }
> *sslbis_bandwidth = (CDATSslbis) {
> .sslbis_header = {
> .header = {
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 05/12] hw/cxl/mbox: Remove dead code
2024-01-24 12:40 ` [PATCH 05/12] hw/cxl/mbox: Remove dead code Jonathan Cameron
@ 2024-01-24 23:41 ` fan
0 siblings, 0 replies; 21+ messages in thread
From: fan @ 2024-01-24 23:41 UTC (permalink / raw
To: Jonathan Cameron
Cc: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin, Ira Weiny,
Huai-Cheng Kuo, Dave Jiang, Peter Maydell, Davidlohr Bueso,
Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi, linuxarm,
Philippe Mathieu-Daudé
On Wed, Jan 24, 2024 at 12:40:53PM +0000, Jonathan Cameron wrote:
> From: Davidlohr Bueso <dave@stgolabs.net>
>
> Two functions were reported to have dead code, remove the bogus
> branches altogether, as well as a misplaced qemu_log call.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
> ---
> hw/cxl/cxl-mailbox-utils.c | 43 +++++++++++++-------------------------
> 1 file changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 6eff56fb1b..dc42850767 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1001,15 +1001,8 @@ static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
>
> cxl_dev_disable_media(&ct3d->cxl_dstate);
>
> - if (secs > 2) {
> - /* sanitize when done */
> - return CXL_MBOX_BG_STARTED;
> - } else {
> - __do_sanitization(ct3d);
> - cxl_dev_enable_media(&ct3d->cxl_dstate);
> -
> - return CXL_MBOX_SUCCESS;
> - }
> + /* sanitize when done */
> + return CXL_MBOX_BG_STARTED;
> }
>
> static CXLRetCode cmd_get_security_state(const struct cxl_cmd *cmd,
> @@ -1387,27 +1380,21 @@ static void bg_timercb(void *opaque)
>
> cci->bg.complete_pct = 100;
> cci->bg.ret_code = ret;
> - if (ret == CXL_MBOX_SUCCESS) {
> - switch (cci->bg.opcode) {
> - case 0x4400: /* sanitize */
> - {
> - CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> -
> - __do_sanitization(ct3d);
> - cxl_dev_enable_media(&ct3d->cxl_dstate);
> - }
> + switch (cci->bg.opcode) {
> + case 0x4400: /* sanitize */
> + {
> + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +
> + __do_sanitization(ct3d);
> + cxl_dev_enable_media(&ct3d->cxl_dstate);
> + }
> + break;
> + case 0x4304: /* TODO: scan media */
> + break;
> + default:
> + __builtin_unreachable();
> break;
> - case 0x4304: /* TODO: scan media */
> - break;
> - default:
> - __builtin_unreachable();
> - break;
> - }
> }
> -
> - qemu_log("Background command %04xh finished: %s\n",
> - cci->bg.opcode,
> - ret == CXL_MBOX_SUCCESS ? "success" : "aborted");
> } else {
> /* estimate only */
> cci->bg.complete_pct = 100 * now / total_time;
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 07/12] hw/cxl: Pass CXLComponentState to cache_mem_ops
2024-01-24 12:40 ` [PATCH 07/12] hw/cxl: Pass CXLComponentState to cache_mem_ops Jonathan Cameron
@ 2024-01-24 23:46 ` fan
0 siblings, 0 replies; 21+ messages in thread
From: fan @ 2024-01-24 23:46 UTC (permalink / raw
To: Jonathan Cameron
Cc: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin, Ira Weiny,
Huai-Cheng Kuo, Dave Jiang, Peter Maydell, Davidlohr Bueso,
Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi, linuxarm,
Philippe Mathieu-Daudé
On Wed, Jan 24, 2024 at 12:40:55PM +0000, Jonathan Cameron wrote:
> From: Li Zhijian <lizhijian@fujitsu.com>
>
> cache_mem_ops.{read,write}() interprets opaque as
> CXLComponentState(cxl_cstate) instead of ComponentRegisters(cregs).
>
> Fortunately, cregs is the first member of cxl_cstate, so their values are
> the same.
>
> Fixes: 9e58f52d3f8 ("hw/cxl/component: Introduce CXL components (8.1.x, 8.2.5)")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
> ---
> hw/cxl/cxl-component-utils.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index 29d477492b..9dfde6c0b3 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -199,7 +199,7 @@ void cxl_component_register_block_init(Object *obj,
> /* io registers controls link which we don't care about in QEMU */
> memory_region_init_io(&cregs->io, obj, NULL, cregs, ".io",
> CXL2_COMPONENT_IO_REGION_SIZE);
> - memory_region_init_io(&cregs->cache_mem, obj, &cache_mem_ops, cregs,
> + memory_region_init_io(&cregs->cache_mem, obj, &cache_mem_ops, cxl_cstate,
> ".cache_mem", CXL2_COMPONENT_CM_REGION_SIZE);
>
> memory_region_add_subregion(&cregs->component_registers, 0, &cregs->io);
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 08/12] hw/cxl: Pass NULL for a NULL MemoryRegionOps
2024-01-24 12:40 ` [PATCH 08/12] hw/cxl: Pass NULL for a NULL MemoryRegionOps Jonathan Cameron
@ 2024-01-24 23:51 ` fan
0 siblings, 0 replies; 21+ messages in thread
From: fan @ 2024-01-24 23:51 UTC (permalink / raw
To: Jonathan Cameron
Cc: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin, Ira Weiny,
Huai-Cheng Kuo, Dave Jiang, Peter Maydell, Davidlohr Bueso,
Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi, linuxarm,
Philippe Mathieu-Daudé
On Wed, Jan 24, 2024 at 12:40:56PM +0000, Jonathan Cameron wrote:
> From: Li Zhijian <lizhijian@fujitsu.com>
>
> a NULL parameter is enough for a NULL MemoryRegionOps
>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
Reviewed-by: Fan Ni <fan.ni@samsung.com>
> hw/cxl/cxl-component-utils.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index 9dfde6c0b3..5ddd47ed8d 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -197,7 +197,7 @@ void cxl_component_register_block_init(Object *obj,
> CXL2_COMPONENT_BLOCK_SIZE);
>
> /* io registers controls link which we don't care about in QEMU */
> - memory_region_init_io(&cregs->io, obj, NULL, cregs, ".io",
> + memory_region_init_io(&cregs->io, obj, NULL, NULL, ".io",
> CXL2_COMPONENT_IO_REGION_SIZE);
> memory_region_init_io(&cregs->cache_mem, obj, &cache_mem_ops, cxl_cstate,
> ".cache_mem", CXL2_COMPONENT_CM_REGION_SIZE);
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 09/12] hw/mem/cxl_type3: Fix potential divide by zero reported by coverity
2024-01-24 12:40 ` [PATCH 09/12] hw/mem/cxl_type3: Fix potential divide by zero reported by coverity Jonathan Cameron
@ 2024-01-24 23:53 ` fan
2024-01-26 11:57 ` Jonathan Cameron
0 siblings, 1 reply; 21+ messages in thread
From: fan @ 2024-01-24 23:53 UTC (permalink / raw
To: Jonathan Cameron
Cc: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin, Ira Weiny,
Huai-Cheng Kuo, Dave Jiang, Peter Maydell, Davidlohr Bueso,
Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi, linuxarm,
Philippe Mathieu-Daudé
On Wed, Jan 24, 2024 at 12:40:57PM +0000, Jonathan Cameron wrote:
> Fixes Coverity ID 1522368.
>
> Currently error_fatal is set if interleave_ways_dec() is going to return 0
> but we should handle that zero return explicitly.
>
> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
> ---
> Note this is a stop gap until a more complex HDM decoder verification series.
> ---
> hw/mem/cxl_type3.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 1b92a065a3..24211703c6 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -794,8 +794,12 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
> }
> if (((uint64_t)host_addr < decoder_base) ||
> (hpa_offset >= decoder_size)) {
> - dpa_base += decoder_size /
> - cxl_interleave_ways_dec(iw, &error_fatal);
> + int decoded_iw = cxl_interleave_ways_dec(iw, &error_fatal);
> +
> + if (decoded_iw == 0)
> + return false;
> +
> + dpa_base += decoder_size / decoded_iw;
> continue;
> }
>
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 09/12] hw/mem/cxl_type3: Fix potential divide by zero reported by coverity
2024-01-24 23:53 ` fan
@ 2024-01-26 11:57 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2024-01-26 11:57 UTC (permalink / raw
To: fan
Cc: qemu-devel, linux-cxl, Fan Ni, Michael Tsirkin, Ira Weiny,
Huai-Cheng Kuo, Dave Jiang, Peter Maydell, Davidlohr Bueso,
Hyeonggon Yoo, Li Zhijian, Stefan Hajnoczi, linuxarm,
Philippe Mathieu-Daudé
On Wed, 24 Jan 2024 15:53:58 -0800
fan <nifan.cxl@gmail.com> wrote:
> On Wed, Jan 24, 2024 at 12:40:57PM +0000, Jonathan Cameron wrote:
> > Fixes Coverity ID 1522368.
> >
> > Currently error_fatal is set if interleave_ways_dec() is going to return 0
> > but we should handle that zero return explicitly.
> >
> > Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
>
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
>
> > ---
> > Note this is a stop gap until a more complex HDM decoder verification series.
> > ---
> > hw/mem/cxl_type3.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 1b92a065a3..24211703c6 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -794,8 +794,12 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
> > }
> > if (((uint64_t)host_addr < decoder_base) ||
> > (hpa_offset >= decoder_size)) {
> > - dpa_base += decoder_size /
> > - cxl_interleave_ways_dec(iw, &error_fatal);
> > + int decoded_iw = cxl_interleave_ways_dec(iw, &error_fatal);
> > +
> > + if (decoded_iw == 0)
> > + return false;
I've obviously been lax in running checkpatch.
Brackets needed for qemu style here. I'll add them for v2
Jonathan
> > +
> > + dpa_base += decoder_size / decoded_iw;
> > continue;
> > }
> >
> > --
> > 2.39.2
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2024-01-26 11:57 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-24 12:40 [PATCH 00/12 qemu] CXL emulation fixes and minor cleanup Jonathan Cameron
2024-01-24 12:40 ` [PATCH 01/12] cxl/cdat: Handle cdat table build errors Jonathan Cameron
2024-01-24 21:04 ` fan
2024-01-24 12:40 ` [PATCH 02/12] hw/mem/cxl_type3: Drop handling of failure of g_malloc0() Jonathan Cameron
2024-01-24 21:10 ` fan
2024-01-24 12:40 ` [PATCH 03/12] hw/pci-bridge/cxl_upstream: Drop g_malloc0() failure handling Jonathan Cameron
2024-01-24 21:11 ` fan
2024-01-24 12:40 ` [PATCH 04/12] cxl/cdat: Fix header sum value in CDAT checksum Jonathan Cameron
2024-01-24 12:40 ` [PATCH 05/12] hw/cxl/mbox: Remove dead code Jonathan Cameron
2024-01-24 23:41 ` fan
2024-01-24 12:40 ` [PATCH 06/12] hw/cxl/device: read from register values in mdev_reg_read() Jonathan Cameron
2024-01-24 12:40 ` [PATCH 07/12] hw/cxl: Pass CXLComponentState to cache_mem_ops Jonathan Cameron
2024-01-24 23:46 ` fan
2024-01-24 12:40 ` [PATCH 08/12] hw/cxl: Pass NULL for a NULL MemoryRegionOps Jonathan Cameron
2024-01-24 23:51 ` fan
2024-01-24 12:40 ` [PATCH 09/12] hw/mem/cxl_type3: Fix potential divide by zero reported by coverity Jonathan Cameron
2024-01-24 23:53 ` fan
2024-01-26 11:57 ` Jonathan Cameron
2024-01-24 12:40 ` [PATCH 10/12] tests/acpi: Allow update of DSDT.cxl Jonathan Cameron
2024-01-24 12:40 ` [PATCH 11/12] hw/i386: Fix _STA return value for ACPI0017 Jonathan Cameron
2024-01-24 12:41 ` [PATCH 12/12] tests/acpi: Update DSDT.cxl to reflect change _STA return value Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).