On Mon, 27 Nov 2023, Hyeonggon Yoo wrote:
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.
Fixes: commit 25a52959f99d ("hw/cxl: Add support for device sanitation")
Signed-off-by: Hyeonggon Yoo <42.hye...@gmail.com>
Looks good, thanks.
Reviewed-by: Davidlohr Bueso <d...@stgolabs.net>
In addition how about the following to further robustify?
- disallow certain incoming cci cmd when media is disabled
- deal with memory reads/writes when media is disabled
- make __toggle_media() a nop when passed value is already set
- play nice with arm64 uses little endian reads and writes (this
should be extended to all of mbox/cci of course).
----8<-----------------------------
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 6eff56fb1b34..9bc5121215c9 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1314,6 +1314,7 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set,
uint8_t cmd,
int ret;
const struct cxl_cmd *cxl_cmd;
opcode_handler h;
+ CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
*len_out = 0;
cxl_cmd = &cci->cxl_cmd_set[set][cmd];
@@ -1334,8 +1335,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set,
uint8_t cmd,
return CXL_MBOX_BUSY;
}
- /* forbid any selected commands while overwriting */
- if (sanitize_running(cci)) {
+ /* forbid any selected commands when necessary */
+ if (sanitize_running(cci) || cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
if (h == cmd_events_get_records ||
h == cmd_ccls_get_partition_info ||
h == cmd_ccls_set_lsa ||
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 72d93713473d..e0a164fde007 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -899,7 +899,8 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr,
uint64_t *data,
return MEMTX_ERROR;
}
- if (sanitize_running(&ct3d->cci)) {
+ if (sanitize_running(&ct3d->cci) ||
+ cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
qemu_guest_getrandom_nofail(data, size);
return MEMTX_OK;
}
@@ -925,6 +926,11 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr
host_addr, uint64_t data,
return MEMTX_OK;
}
+ /* memory writes to the device will have no effect */
+ if (cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
+ return MEMTX_OK;
+ }
+
return address_space_write(as, dpa_offset, attrs, &data, size);
}
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 873e6d6ab159..007d4169df7c 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -349,14 +349,26 @@ REG64(CXL_MEM_DEV_STS, 0)
FIELD(CXL_MEM_DEV_STS, MBOX_READY, 4, 1)
FIELD(CXL_MEM_DEV_STS, RESET_NEEDED, 5, 3)
+static inline bool cxl_dev_media_disabled(CXLDeviceState *cxl_dstate)
+{
+ uint64_t dev_status_reg;
+
+ dev_status_reg = ldq_le_p(cxl_dstate->mbox_reg_state64 +
R_CXL_MEM_DEV_STS);
+ return FIELD_EX64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == 0x3;
+}
+
static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val)
{
uint64_t dev_status_reg;
- dev_status_reg = cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS];
+ dev_status_reg = ldq_le_p(cxl_dstate->mbox_reg_state64 +
R_CXL_MEM_DEV_STS);
+ if (FIELD_EX64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == val) {
+ return;
+ }
+
dev_status_reg = FIELD_DP64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS,
val);
- cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg;
+ stq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS, dev_status_reg);
}
#define cxl_dev_disable_media(cxlds) \
do { __toggle_media((cxlds), 0x3); } while (0)
