[PATCH v2 0/4] A Followup for "QEMU: CXL mailbox rework and features (Part 1)"

[PATCH v2 0/4] A Followup for "QEMU: CXL mailbox rework and features (Part 1)"

[Hyeonggon Yoo]

v1: https://lore.kernel.org/qemu-devel/20231127105830.2104954-1-42.hyeyoo@gmail.com

Changes from v1:
    - Added patch 1 that fixes a build failure in Jonathan's tree.
    - Added patch 3, as (partially) suggested by Davidlohr Buseo.
      One difference is that I dropped sanitize_running(), because
      cxl_dev_media_diabled() is enough for checking if the media is
      disabled (which implies sanitation is in progress)
    - Added patch 4 that dicards all event logs during sanitation

    Thanks everyone for giving feedbacks!

This is a fixup for the recent patch series "QEMU: CXL mailbox rework and
features (Part 1)" [1].

I don't mind if patch 1 is squashed into the problematic patch, as the
patch is not mainlined yet. This is based on Jonathan
Cameron's git tree (https://gitlab.com/jic23/qemu/-/tree/cxl-2023-11-02)

Sequence of Patches:

   1. Fix build error when CXL is not enabled, because of mismatching
      definition in cxl_type3_stubs.c
 
   2. Make mdev_reg_read() actually read registers, instead of
      returning a dummy value. This fixes Media Status being incorrectly
      read as "Enabled" while sanitation is in progress.

   3. Introduce cxl_dev_media_disabled() and replace sanitize_running()
      with it. Also add an assert() to check the media is correctly disabled
      during sanitation. (Now enabling when already enabled, or vice versa
      raises an assert failure.)

   4. Drop all event records during sanitation, as per spec.

[1] https://lore.kernel.org/linux-cxl/20231023160806.13206-1-Jonathan.Cameron@huawei.com

Hyeonggon Yoo (4):
  hw/cxl: fix build error in cxl_type3_stubs.c
  hw/cxl/device: read from register values in mdev_reg_read()
  hw/cxl/mbox: replace sanitize_running() with cxl_dev_media_disabled()
  hw/cxl/events: discard all event records during sanitation

 hw/cxl/cxl-device-utils.c   | 17 +++++++++++------
 hw/cxl/cxl-events.c         | 13 +++++++++++++
 hw/cxl/cxl-mailbox-utils.c  |  7 +++++--
 hw/mem/cxl_type3.c          |  4 ++--
 hw/mem/cxl_type3_stubs.c    |  4 ++--
 include/hw/cxl/cxl_device.h | 16 ++++++++++------
 6 files changed, 43 insertions(+), 18 deletions(-)

-- 
2.39.3

[PATCH v2 1/4] hw/cxl: fix build error in cxl_type3_stubs.c

[Hyeonggon Yoo]

Fix build errors in cxl_type3_stubs.c due to a the incorrect definition
of the qmp_cxl_{add,release}_dynamic_capacity functions.

Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 hw/mem/cxl_type3_stubs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
index 1b54ec028c..d913b11b4d 100644
--- a/hw/mem/cxl_type3_stubs.c
+++ b/hw/mem/cxl_type3_stubs.c
@@ -68,14 +68,14 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
     error_setg(errp, "CXL Type 3 support is not compiled in");
 }
 
-void qmp_cxl_add_dynamic_capacity(const char *path,
+void qmp_cxl_add_dynamic_capacity(const char *path, uint8_t region_id,
                                   CXLDCExtentRecordList  *records,
                                   Error **errp)
 {
     error_setg(errp, "CXL Type 3 support is not compiled in");
 }
 
-void qmp_cxl_release_dynamic_capacity(const char *path,
+void qmp_cxl_release_dynamic_capacity(const char *path, uint8_t region_id,
                                       CXLDCExtentRecordList  *records,
                                       Error **errp)
 {
-- 
2.39.3

Re: [PATCH v2 1/4] hw/cxl: fix build error in cxl_type3_stubs.c

[Jonathan Cameron via]

On Fri, 22 Dec 2023 18:00:48 +0900
Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:

> Fix build errors in cxl_type3_stubs.c due to a the incorrect definition
> of the qmp_cxl_{add,release}_dynamic_capacity functions.
> 
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>

Fan, this one needs squashing into your
hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
patch in the DCD series. I'll do that in my tree, but just wanted to
make sure you noticed this so we don't end up reintroducing it again by
accident!

Thanks Hyeonggon,

Jonathan

> ---
>  hw/mem/cxl_type3_stubs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
> index 1b54ec028c..d913b11b4d 100644
> --- a/hw/mem/cxl_type3_stubs.c
> +++ b/hw/mem/cxl_type3_stubs.c
> @@ -68,14 +68,14 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
>      error_setg(errp, "CXL Type 3 support is not compiled in");
>  }
>  
> -void qmp_cxl_add_dynamic_capacity(const char *path,
> +void qmp_cxl_add_dynamic_capacity(const char *path, uint8_t region_id,
>                                    CXLDCExtentRecordList  *records,
>                                    Error **errp)
>  {
>      error_setg(errp, "CXL Type 3 support is not compiled in");
>  }
>  
> -void qmp_cxl_release_dynamic_capacity(const char *path,
> +void qmp_cxl_release_dynamic_capacity(const char *path, uint8_t region_id,
>                                        CXLDCExtentRecordList  *records,
>                                        Error **errp)
>  {

Re: [PATCH v2 1/4] hw/cxl: fix build error in cxl_type3_stubs.c

[fan]

On Tue, Jan 09, 2024 at 05:40:26PM +0000, Jonathan Cameron wrote:
> On Fri, 22 Dec 2023 18:00:48 +0900
> Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
> 
> > Fix build errors in cxl_type3_stubs.c due to a the incorrect definition
> > of the qmp_cxl_{add,release}_dynamic_capacity functions.
> > 
> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> 
> Fan, this one needs squashing into your
> hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
> patch in the DCD series. I'll do that in my tree, but just wanted to
> make sure you noticed this so we don't end up reintroducing it again by
> accident!
> 
> Thanks Hyeonggon,
> 
> Jonathan

Hi Jonathan,
Thanks for the notice. I missed this series. 

I checked the last patch set I sent out in Nov., 2023, there is no issue
there.

https://lore.kernel.org/linux-cxl/20231107180907.553451-9-nifan.cxl@gmail.com/

So the following patch is actually not needed, maybe Hyeonggon's work
was based on some of your local branch which does not have the latest DCD
work.

Thanks,
Fan

> 
> > ---
> >  hw/mem/cxl_type3_stubs.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
> > index 1b54ec028c..d913b11b4d 100644
> > --- a/hw/mem/cxl_type3_stubs.c
> > +++ b/hw/mem/cxl_type3_stubs.c
> > @@ -68,14 +68,14 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
> >      error_setg(errp, "CXL Type 3 support is not compiled in");
> >  }
> >  
> > -void qmp_cxl_add_dynamic_capacity(const char *path,
> > +void qmp_cxl_add_dynamic_capacity(const char *path, uint8_t region_id,
> >                                    CXLDCExtentRecordList  *records,
> >                                    Error **errp)
> >  {
> >      error_setg(errp, "CXL Type 3 support is not compiled in");
> >  }
> >  
> > -void qmp_cxl_release_dynamic_capacity(const char *path,
> > +void qmp_cxl_release_dynamic_capacity(const char *path, uint8_t region_id,
> >                                        CXLDCExtentRecordList  *records,
> >                                        Error **errp)
> >  {
> 

[PATCH v2 2/4] hw/cxl/device: read from register values in mdev_reg_read()

[Hyeonggon Yoo]

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")
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
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 29de298117..ba3f80e6e7 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 b2cb280e16..b318d94b36 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -408,7 +408,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.3

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

[Jonathan Cameron via]

On Fri, 22 Dec 2023 18:00:49 +0900
Hyeonggon Yoo <42.hyeyoo@gmail.com> 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")
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>

I've applied this one to my tree.  (I'll push that out in a day or two after
tidying up some other outstanding stuff). 

Sometime in next week or so I'll send out a set bundling together various
fixes and cleanup with the intent for getting it applied.

Thanks,

Jonathan

> ---
>  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 29de298117..ba3f80e6e7 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 b2cb280e16..b318d94b36 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -408,7 +408,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)                    \

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

[Jonathan Cameron via]

On Tue, 9 Jan 2024 17:45:50 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 22 Dec 2023 18:00:49 +0900
> Hyeonggon Yoo <42.hyeyoo@gmail.com> 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")
> > Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>  
> 
> I've applied this one to my tree.  (I'll push that out in a day or two after
> tidying up some other outstanding stuff). 
> 
> Sometime in next week or so I'll send out a set bundling together various
> fixes and cleanup with the intent for getting it applied.

I've changed how this works because what this is doing as presented is
overwriting the mailbox capability register.  mbox_reg_state64 is as the
name should have made obvious to reviewers such as me, the mailbox registers!

Anyhow, I'll put an alternative fix in place.

Jonathan

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  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 29de298117..ba3f80e6e7 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 b2cb280e16..b318d94b36 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -408,7 +408,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)                    \  
> 
> 

[PATCH v2 3/4] hw/cxl/mbox: replace sanitize_running() with cxl_dev_media_disabled()

[Hyeonggon Yoo]

The spec states that reads/writes should have no effect and a part of
commands should be ignored when the media is disabled, not when the
sanitize command is running.

Introduce cxl_dev_media_disabled() to check if the media is disabled and
replace sanitize_running() with it.

Make sure that the media has been correctly disabled during sanitation
by adding an assert to __toggle_media(). Now, enabling when already
enabled or vice versa results in an assert() failure.

Suggested-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 hw/cxl/cxl-mailbox-utils.c  |  6 ++++--
 hw/mem/cxl_type3.c          |  4 ++--
 include/hw/cxl/cxl_device.h | 11 ++++++-----
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 11ec8b648b..efeb5f8174 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -2248,6 +2248,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
     int ret;
     const struct cxl_cmd *cxl_cmd;
     opcode_handler h;
+    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->intf)->cxl_dstate;
+
 
     *len_out = 0;
     cxl_cmd = &cci->cxl_cmd_set[set][cmd];
@@ -2268,8 +2270,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 while the media is disabled */
+    if (cxl_dev_media_disabled(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 85fc08f118..ecb525a608 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1286,7 +1286,7 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
         return MEMTX_ERROR;
     }
 
-    if (sanitize_running(&ct3d->cci)) {
+    if (cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
         qemu_guest_getrandom_nofail(data, size);
         return MEMTX_OK;
     }
@@ -1314,7 +1314,7 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
         return MEMTX_ERROR;
     }
 
-    if (sanitize_running(&ct3d->cci)) {
+    if (cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
         return MEMTX_OK;
     }
 
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index b318d94b36..5618061ebe 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -411,6 +411,7 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int 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);
+    assert(dev_status_reg != cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS]);
     cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg;
 }
 #define cxl_dev_disable_media(cxlds)                    \
@@ -418,14 +419,14 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val)
 #define cxl_dev_enable_media(cxlds)                     \
         do { __toggle_media((cxlds), 0x1); } while (0)
 
-static inline bool scan_media_running(CXLCCI *cci)
+static inline bool cxl_dev_media_disabled(CXLDeviceState *cxl_dstate)
 {
-    return !!cci->bg.runtime && cci->bg.opcode == 0x4304;
+    uint64_t dev_status_reg = 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 bool sanitize_running(CXLCCI *cci)
+static inline bool scan_media_running(CXLCCI *cci)
 {
-    return !!cci->bg.runtime && cci->bg.opcode == 0x4400;
+    return !!cci->bg.runtime && cci->bg.opcode == 0x4304;
 }
 
 typedef struct CXLError {
-- 
2.39.3

Re: [PATCH v2 3/4] hw/cxl/mbox: replace sanitize_running() with cxl_dev_media_disabled()

[Jonathan Cameron via]

On Fri, 22 Dec 2023 18:00:50 +0900
Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:

> The spec states that reads/writes should have no effect and a part of
> commands should be ignored when the media is disabled, not when the
> sanitize command is running.qq
> 
> Introduce cxl_dev_media_disabled() to check if the media is disabled and
> replace sanitize_running() with it.
> 
> Make sure that the media has been correctly disabled during sanitation
> by adding an assert to __toggle_media(). Now, enabling when already
> enabled or vice versa results in an assert() failure.
> 
> Suggested-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>

This applies to

hw/cxl: Add get scan media capabilities cmd support.

Should I just squash it with that patch in my tree?
For now I'm holding it immediately on top of that, but I'm not keen to
send messy code upstream unless there is a good reason to retain the
history.

If you are doing this sort of fix series in future, please call out
what they fix explicitly.  Can't use fixes tags as the commit ids
are unstable, but can mention the patch to make my life easier!

Thanks,

Jonathan


> ---
>  hw/cxl/cxl-mailbox-utils.c  |  6 ++++--
>  hw/mem/cxl_type3.c          |  4 ++--
>  include/hw/cxl/cxl_device.h | 11 ++++++-----
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 11ec8b648b..efeb5f8174 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -2248,6 +2248,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
>      int ret;
>      const struct cxl_cmd *cxl_cmd;
>      opcode_handler h;
> +    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->intf)->cxl_dstate;
> +
>  
>      *len_out = 0;
>      cxl_cmd = &cci->cxl_cmd_set[set][cmd];
> @@ -2268,8 +2270,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 while the media is disabled */
> +    if (cxl_dev_media_disabled(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 85fc08f118..ecb525a608 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1286,7 +1286,7 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
>          return MEMTX_ERROR;
>      }
>  
> -    if (sanitize_running(&ct3d->cci)) {
> +    if (cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
>          qemu_guest_getrandom_nofail(data, size);
>          return MEMTX_OK;
>      }
> @@ -1314,7 +1314,7 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
>          return MEMTX_ERROR;
>      }
>  
> -    if (sanitize_running(&ct3d->cci)) {
> +    if (cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
>          return MEMTX_OK;
>      }
>  
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index b318d94b36..5618061ebe 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -411,6 +411,7 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int 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);
> +    assert(dev_status_reg != cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS]);
>      cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg;
>  }
>  #define cxl_dev_disable_media(cxlds)                    \
> @@ -418,14 +419,14 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val)
>  #define cxl_dev_enable_media(cxlds)                     \
>          do { __toggle_media((cxlds), 0x1); } while (0)
>  
> -static inline bool scan_media_running(CXLCCI *cci)
> +static inline bool cxl_dev_media_disabled(CXLDeviceState *cxl_dstate)
>  {
> -    return !!cci->bg.runtime && cci->bg.opcode == 0x4304;
> +    uint64_t dev_status_reg = 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 bool sanitize_running(CXLCCI *cci)
> +static inline bool scan_media_running(CXLCCI *cci)
>  {
> -    return !!cci->bg.runtime && cci->bg.opcode == 0x4400;
> +    return !!cci->bg.runtime && cci->bg.opcode == 0x4304;
>  }
>  
>  typedef struct CXLError {

Re: [PATCH v2 3/4] hw/cxl/mbox: replace sanitize_running() with cxl_dev_media_disabled()

[Hyeonggon Yoo]

On Tue, Jan 9, 2024 at 12:54 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Fri, 22 Dec 2023 18:00:50 +0900
> Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
>
> > The spec states that reads/writes should have no effect and a part of
> > commands should be ignored when the media is disabled, not when the
> > sanitize command is running.qq
> >
> > Introduce cxl_dev_media_disabled() to check if the media is disabled and
> > replace sanitize_running() with it.
> >
> > Make sure that the media has been correctly disabled during sanitation
> > by adding an assert to __toggle_media(). Now, enabling when already
> > enabled or vice versa results in an assert() failure.
> >
> > Suggested-by: Davidlohr Bueso <dave@stgolabs.net>
> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
>
> This applies to
>
> hw/cxl: Add get scan media capabilities cmd support.
>
> Should I just squash it with that patch in my tree?
> For now I'm holding it immediately on top of that, but I'm not keen to
> send messy code upstream unless there is a good reason to retain the
> history.

Oh, while the diff looks like the patch touches scan_media_running(), it's not.

The proper Fixes: tag will be:
Fixes: d77176724422 ("hw/cxl: Add support for device sanitation")

> If you are doing this sort of fix series in future, please call out
> what they fix explicitly.  Can't use fixes tags as the commit ids
> are unstable, but can mention the patch to make my life easier!

Okay, next time I will either add the Fixes tag or add a comment on
what it fixes.

By the way I guess your latest, public branch is still cxl-2023-11-02, right?
https://gitlab.com/jic23/qemu/-/tree/cxl-2023-11-02

I assume you adjusted my v2 series, but please let me know if you prefer
sending v3 against your latest tree.

Thanks,
Hyeonggon

Re: [PATCH v2 3/4] hw/cxl/mbox: replace sanitize_running() with cxl_dev_media_disabled()

[Jonathan Cameron via]

On Sun, 21 Jan 2024 21:50:00 -0500
Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:

> On Tue, Jan 9, 2024 at 12:54 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Fri, 22 Dec 2023 18:00:50 +0900
> > Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
> >  
> > > The spec states that reads/writes should have no effect and a part of
> > > commands should be ignored when the media is disabled, not when the
> > > sanitize command is running.qq
> > >
> > > Introduce cxl_dev_media_disabled() to check if the media is disabled and
> > > replace sanitize_running() with it.
> > >
> > > Make sure that the media has been correctly disabled during sanitation
> > > by adding an assert to __toggle_media(). Now, enabling when already
> > > enabled or vice versa results in an assert() failure.
> > >
> > > Suggested-by: Davidlohr Bueso <dave@stgolabs.net>
> > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>  
> >
> > This applies to
> >
> > hw/cxl: Add get scan media capabilities cmd support.
> >
> > Should I just squash it with that patch in my tree?
> > For now I'm holding it immediately on top of that, but I'm not keen to
> > send messy code upstream unless there is a good reason to retain the
> > history.  
> 
> Oh, while the diff looks like the patch touches scan_media_running(), it's not.
> 
> The proper Fixes: tag will be:
> Fixes: d77176724422 ("hw/cxl: Add support for device sanitation")
> 
> > If you are doing this sort of fix series in future, please call out
> > what they fix explicitly.  Can't use fixes tags as the commit ids
> > are unstable, but can mention the patch to make my life easier!  
> 
> Okay, next time I will either add the Fixes tag or add a comment on
> what it fixes.
> 
> By the way I guess your latest, public branch is still cxl-2023-11-02, right?
> https://gitlab.com/jic23/qemu/-/tree/cxl-2023-11-02
> 
> I assume you adjusted my v2 series, but please let me know if you prefer
> sending v3 against your latest tree.
> 
> Thanks,
> Hyeonggon

Side note, in it's current form this breaks the switch-cci support in upstream
QEMU.  I've finally gotten back to getting ready to look at MMPT support and
ran into a crash as a result.  Needs protection with checked object_dynamic_cast()
to make sure we have a type3 device.  I'll update the patch in my tree.

Thanks,

Jonathan

> 

[PATCH v2 4/4] hw/cxl/events: discard all event records during sanitation

[Hyeonggon Yoo]

Per spec 8.2.9.9.5.1 Sanitize (Opcode 4400h), sanitize command should
delete all event logs. Introduce cxl_discard_all_event_logs() and call
this in __do_sanitization().

Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 hw/cxl/cxl-events.c         | 13 +++++++++++++
 hw/cxl/cxl-mailbox-utils.c  |  1 +
 include/hw/cxl/cxl_device.h |  1 +
 3 files changed, 15 insertions(+)

diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
index bee6dfaf14..837b18ab47 100644
--- a/hw/cxl/cxl-events.c
+++ b/hw/cxl/cxl-events.c
@@ -141,6 +141,19 @@ bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
     return cxl_event_count(log) == 1;
 }
 
+void cxl_discard_all_event_records(CXLDeviceState *cxlds)
+{
+    CXLEventLogType log_type;
+    CXLEventLog *log;
+
+    for (log_type = 0; log_type < CXL_EVENT_TYPE_MAX; log_type++) {
+        log = &cxlds->event_logs[log_type];
+        while (!cxl_event_empty(log)) {
+            cxl_event_delete_head(cxlds, log_type, log);
+        }
+    }
+}
+
 CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
                                  uint8_t log_type, int max_recs,
                                  size_t *len)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index efeb5f8174..2ade351d82 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1150,6 +1150,7 @@ static void __do_sanitization(CXLType3Dev *ct3d)
             memset(lsa, 0, memory_region_size(mr));
         }
     }
+    cxl_discard_all_event_records(&ct3d->cxl_dstate);
 }
 
 /*
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 5618061ebe..8f05dd9beb 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -604,6 +604,7 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
                                  size_t *len);
 CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
                                    CXLClearEventPayload *pl);
+void cxl_discard_all_event_records(CXLDeviceState *cxlds);
 
 void cxl_event_irq_assert(CXLType3Dev *ct3d);
 
-- 
2.39.3

Re: [PATCH v2 4/4] hw/cxl/events: discard all event records during sanitation

[Davidlohr Bueso]

On Fri, 22 Dec 2023, Hyeonggon Yoo wrote:

>Per spec 8.2.9.9.5.1 Sanitize (Opcode 4400h), sanitize command should
>delete all event logs. Introduce cxl_discard_all_event_logs() and call
>this in __do_sanitization().

lgtm

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

>Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
>---
> hw/cxl/cxl-events.c         | 13 +++++++++++++
> hw/cxl/cxl-mailbox-utils.c  |  1 +
> include/hw/cxl/cxl_device.h |  1 +
> 3 files changed, 15 insertions(+)
>
>diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
>index bee6dfaf14..837b18ab47 100644
>--- a/hw/cxl/cxl-events.c
>+++ b/hw/cxl/cxl-events.c
>@@ -141,6 +141,19 @@ bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
>     return cxl_event_count(log) == 1;
> }
>
>+void cxl_discard_all_event_records(CXLDeviceState *cxlds)
>+{
>+    CXLEventLogType log_type;
>+    CXLEventLog *log;
>+
>+    for (log_type = 0; log_type < CXL_EVENT_TYPE_MAX; log_type++) {
>+        log = &cxlds->event_logs[log_type];
>+        while (!cxl_event_empty(log)) {
>+            cxl_event_delete_head(cxlds, log_type, log);
>+        }
>+    }
>+}
>+
> CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
>                                  uint8_t log_type, int max_recs,
>                                  size_t *len)
>diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>index efeb5f8174..2ade351d82 100644
>--- a/hw/cxl/cxl-mailbox-utils.c
>+++ b/hw/cxl/cxl-mailbox-utils.c
>@@ -1150,6 +1150,7 @@ static void __do_sanitization(CXLType3Dev *ct3d)
>             memset(lsa, 0, memory_region_size(mr));
>         }
>     }
>+    cxl_discard_all_event_records(&ct3d->cxl_dstate);
> }
>
> /*
>diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
>index 5618061ebe..8f05dd9beb 100644
>--- a/include/hw/cxl/cxl_device.h
>+++ b/include/hw/cxl/cxl_device.h
>@@ -604,6 +604,7 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
>                                  size_t *len);
> CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
>                                    CXLClearEventPayload *pl);
>+void cxl_discard_all_event_records(CXLDeviceState *cxlds);
>
> void cxl_event_irq_assert(CXLType3Dev *ct3d);
>
>-- 
>2.39.3
>

Re: [PATCH v2 4/4] hw/cxl/events: discard all event records during sanitation

[Jonathan Cameron via]

On Mon, 1 Jan 2024 13:38:28 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Fri, 22 Dec 2023, Hyeonggon Yoo wrote:
> 
> >Per spec 8.2.9.9.5.1 Sanitize (Opcode 4400h), sanitize command should
> >delete all event logs. Introduce cxl_discard_all_event_logs() and call
> >this in __do_sanitization().  
> 
> lgtm
> 
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

I'll carry this on my tree on top of where I put the previous patch.
Seems to apply there at least :)

If it makes sense to remix these patches a bit to provide cleaner
history I'll do so once these are ready to send for Michael to look
at.

Jonathan

> 
> >Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> >---
> > hw/cxl/cxl-events.c         | 13 +++++++++++++
> > hw/cxl/cxl-mailbox-utils.c  |  1 +
> > include/hw/cxl/cxl_device.h |  1 +
> > 3 files changed, 15 insertions(+)
> >
> >diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> >index bee6dfaf14..837b18ab47 100644
> >--- a/hw/cxl/cxl-events.c
> >+++ b/hw/cxl/cxl-events.c
> >@@ -141,6 +141,19 @@ bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
> >     return cxl_event_count(log) == 1;
> > }
> >
> >+void cxl_discard_all_event_records(CXLDeviceState *cxlds)
> >+{
> >+    CXLEventLogType log_type;
> >+    CXLEventLog *log;
> >+
> >+    for (log_type = 0; log_type < CXL_EVENT_TYPE_MAX; log_type++) {
> >+        log = &cxlds->event_logs[log_type];
> >+        while (!cxl_event_empty(log)) {
> >+            cxl_event_delete_head(cxlds, log_type, log);
> >+        }
> >+    }
> >+}
> >+
> > CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
> >                                  uint8_t log_type, int max_recs,
> >                                  size_t *len)
> >diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> >index efeb5f8174..2ade351d82 100644
> >--- a/hw/cxl/cxl-mailbox-utils.c
> >+++ b/hw/cxl/cxl-mailbox-utils.c
> >@@ -1150,6 +1150,7 @@ static void __do_sanitization(CXLType3Dev *ct3d)
> >             memset(lsa, 0, memory_region_size(mr));
> >         }
> >     }
> >+    cxl_discard_all_event_records(&ct3d->cxl_dstate);
> > }
> >
> > /*
> >diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> >index 5618061ebe..8f05dd9beb 100644
> >--- a/include/hw/cxl/cxl_device.h
> >+++ b/include/hw/cxl/cxl_device.h
> >@@ -604,6 +604,7 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
> >                                  size_t *len);
> > CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
> >                                    CXLClearEventPayload *pl);
> >+void cxl_discard_all_event_records(CXLDeviceState *cxlds);
> >
> > void cxl_event_irq_assert(CXLType3Dev *ct3d);
> >
> >-- 
> >2.39.3
> >