Re: [PATCH v2 13/17] hw/cxl: Add support for device sanitation

[Hyeonggon Yoo]

On Tue, Oct 24, 2023 at 1:14 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> From: Davidlohr Bueso <dave@stgolabs.net>
>
> Make use of the background operations through the sanitize command, per CXL
> 3.0 specs. Traditionally run times can be rather long, depending on the
> size of the media.
>
> Estimate times based on:
>          https://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  include/hw/cxl/cxl_device.h |  17 +++++
>  hw/cxl/cxl-mailbox-utils.c  | 140 ++++++++++++++++++++++++++++++++++++
>  hw/mem/cxl_type3.c          |  10 +++
>  3 files changed, 167 insertions(+)
>
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 2a813cdddd..70aca9024c 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -343,6 +343,23 @@ 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 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;
> +}
> +#define cxl_dev_disable_media(cxlds)                    \
> +        do { __toggle_media((cxlds), 0x3); } while (0)
> +#define cxl_dev_enable_media(cxlds)                     \
> +        do { __toggle_media((cxlds), 0x1); } while (0)

Before this patch, it is assumed that "Media Status" and "Mailbox
Interface Ready" were always 1,
thus mdev_reg_read() always returns 1 for both of them regardless of
register values.

I think changes like below are needed as now the assumption is broken?
Please note that it's only build-tested :)

Thanks,
Hyeonggon

diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index 61a3c4dc2e..b6ada2fd6a 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,13 @@ 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 61b7f897f7..61f8f83ddf 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -351,9 +351,9 @@ REG64(CXL_MEM_DEV_STS, 0)

 static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val)
 {
-    uint64_t dev_status_reg;
+    uint64_t dev_status_reg = cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS];

-    dev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
+    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)                    \