On Tue, Nov 28, 2023 at 5:27 AM Davidlohr Bueso <d...@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.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).
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)
