Re: [PATCH v1 1/2] hw/cxl/device: read from register values in mdev_reg_read()

[Hyeonggon Yoo]

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)