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.hyeyoo@gmail.com>
---
hw/cxl/cxl-device-utils.c | 17 +++++++++++------
include/hw/cxl/cxl_device.h | 4 +++-
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index 61a3c4dc2e..f1111eb20f 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->mbox_reg_state64[R_CXL_MEM_DEV_STS];
}
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->mbox_reg_state64[R_CXL_MEM_DEV_STS] = memdev_status_reg;
+}
void cxl_device_register_init_t3(CXLType3Dev *ct3d)
{
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index befb5f884b..873e6d6ab1 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -353,7 +353,9 @@ 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);
+ dev_status_reg = cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS];
+ 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;
}
#define cxl_dev_disable_media(cxlds) \
--
2.39.1On 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.hyeyoo@gmail.com>
Looks good, thanks.
Reviewed-by: Davidlohr Bueso <dave@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)
On Mon, 27 Nov 2023 12:27:02 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:
> 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.hyeyoo@gmail.com>
>
> Looks good, thanks.
>
> Reviewed-by: Davidlohr Bueso <dave@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).
This one you've lost me on. Arm64 and x86 both little endian.
If you mean generally harden the code we haven't fixed up for
big endian systems then fair enough - that indeed needs doing.
Tricky to be sure we got it all right though unless we have a big
endian arch to test on...
Jonathan
On Thu, 30 Nov 2023, Jonathan Cameron wrote: >If you mean generally harden the code we haven't fixed up for >big endian systems then fair enough - that indeed needs doing. Yes, that was a braino, sorry for the confusion. >Tricky to be sure we got it all right though unless we have a big >endian arch to test on... Indeed.
On Tue, Nov 28, 2023 at 5:27 AM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> 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.hyeyoo@gmail.com>
>
> Looks good, thanks.
>
> Reviewed-by: Davidlohr Bueso <dave@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).
All of them make sense to me. I will adjust, thanks!
But I'm not confident enough to write a single description for all the
changes so will
split it into a few patches. May I add your Suggested-by (or
Signed-off-by) in v2
as it will contain some part of your idea and code?
> ----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)
On Tue, 28 Nov 2023, Hyeonggon Yoo wrote: >All of them make sense to me. I will adjust, thanks! > >But I'm not confident enough to write a single description for all the >changes so will >split it into a few patches. May I add your Suggested-by (or >Signed-off-by) in v2 >as it will contain some part of your idea and code? Sure, feel free to split as you see fit.
© 2016 - 2026 Red Hat, Inc.