[RESEND PATCH 0/2] hw/cxl: Fix decoder commit and uncommit handling

[RESEND PATCH 0/2] hw/cxl: Fix decoder commit and uncommit handling

[Jonathan Cameron]

Resending because all but patch 1 seem to have failed to make it out of
our network.

Issue reported in discussion of:
https://lore.kernel.org/all/20230228224014.1402545-1-fan.ni@samsung.com/

The committed bit for HDM decoders is expected reset when commit transitions
from 1->0.  Whilst looking at that it was noticed that hardware was resetting
the commit bit which is not an option allowed by the CXL spec.

In common with many other areas the code did not take into account
big endian architectures, so fix that whilst we are here.

Note testing this exposed a kernel bug around repeated attempts to clear
a decoder out of order. That's been reported but not yet fixed.

Jonathan Cameron (2):
  hw/cxl: Fix endian handling for decoder commit.
  hw/cxl: Fix incorrect reset of commit and associated clearing of
    committed.

 hw/cxl/cxl-component-utils.c | 14 ++++++++------
 hw/mem/cxl_type3.c           | 28 +++++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 9 deletions(-)

-- 
2.37.2

[RESEND PATCH 1/2] hw/cxl: Fix endian handling for decoder commit.

[Jonathan Cameron]

Not a real problem yet as all supported architectures are
little endian, but continue to tidy these up when touching
code for other reasons.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-component-utils.c | 10 ++++------
 hw/mem/cxl_type3.c           |  9 ++++++---
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index b665d4f565..a3e6cf75cf 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -47,14 +47,12 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
         break;
     }
 
-    memory_region_transaction_begin();
-    stl_le_p((uint8_t *)cache_mem + offset, value);
     if (should_commit) {
-        ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
-        ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0);
-        ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
+        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
+        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
+        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
     }
-    memory_region_transaction_commit();
+    stl_le_p((uint8_t *)cache_mem + offset, value);
 }
 
 static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index abe60b362c..846089ccda 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -314,14 +314,17 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
 {
     ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
     uint32_t *cache_mem = cregs->cache_mem_registers;
+    uint32_t ctrl;
 
     assert(which == 0);
 
+    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
     /* TODO: Sanity checks that the decoder is possible */
-    ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
-    ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0);
+    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
+    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
+    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
 
-    ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
+    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
 }
 
 static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
-- 
2.37.2

Re: [RESEND PATCH 1/2] hw/cxl: Fix endian handling for decoder commit.

[Philippe Mathieu-Daudé]

On 22/3/23 11:27, Jonathan Cameron via wrote:
> Not a real problem yet as all supported architectures are
> little endian, but continue to tidy these up when touching
> code for other reasons.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   hw/cxl/cxl-component-utils.c | 10 ++++------
>   hw/mem/cxl_type3.c           |  9 ++++++---
>   2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index b665d4f565..a3e6cf75cf 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -47,14 +47,12 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
>           break;
>       }
>   
> -    memory_region_transaction_begin();
> -    stl_le_p((uint8_t *)cache_mem + offset, value);
>       if (should_commit) {
> -        ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
> -        ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, ERR, 0);
> -        ARRAY_FIELD_DP32(cache_mem, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
> +        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
> +        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
> +        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
>       }
> -    memory_region_transaction_commit();

Indeed the memory_region_transaction guard seems pointless here,
but it is a different change, so should go in a preliminary patch IMHO.

Conditional to this patch being split:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    stl_le_p((uint8_t *)cache_mem + offset, value);
>   }

[RESEND PATCH 2/2] hw/cxl: Fix incorrect reset of commit and associated clearing of committed.

[Jonathan Cameron]

The hardware clearing the commit bit is not spec compliant.
Clearing of committed bit when commit is cleared is not specifically
stated in the CXL spec, but is the expected (and simplest) permitted
behaviour so use that for QEMU emulation.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-component-utils.c |  6 +++++-
 hw/mem/cxl_type3.c           | 21 ++++++++++++++++++++-
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index a3e6cf75cf..378f1082ce 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -38,19 +38,23 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
     ComponentRegisters *cregs = &cxl_cstate->crb;
     uint32_t *cache_mem = cregs->cache_mem_registers;
     bool should_commit = false;
+    bool should_uncommit = false;
 
     switch (offset) {
     case A_CXL_HDM_DECODER0_CTRL:
         should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
+        should_uncommit = !should_commit;
         break;
     default:
         break;
     }
 
     if (should_commit) {
-        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
         value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
         value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
+    } else if (should_uncommit) {
+        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
+        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
     }
     stl_le_p((uint8_t *)cache_mem + offset, value);
 }
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 846089ccda..9598d584ac 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -320,13 +320,28 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
 
     ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
     /* TODO: Sanity checks that the decoder is possible */
-    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
     ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
     ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
 
     stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
 }
 
+static void hdm_decoder_uncommit(CXLType3Dev *ct3d, int which)
+{
+    ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
+    uint32_t *cache_mem = cregs->cache_mem_registers;
+    uint32_t ctrl;
+
+    assert(which == 0);
+
+    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
+
+    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
+    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
+
+    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
+}
+
 static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
 {
     switch (qmp_err) {
@@ -395,6 +410,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
     CXLType3Dev *ct3d = container_of(cxl_cstate, CXLType3Dev, cxl_cstate);
     uint32_t *cache_mem = cregs->cache_mem_registers;
     bool should_commit = false;
+    bool should_uncommit = false;
     int which_hdm = -1;
 
     assert(size == 4);
@@ -403,6 +419,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
     switch (offset) {
     case A_CXL_HDM_DECODER0_CTRL:
         should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
+        should_uncommit = !should_commit;
         which_hdm = 0;
         break;
     case A_CXL_RAS_UNC_ERR_STATUS:
@@ -489,6 +506,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
     stl_le_p((uint8_t *)cache_mem + offset, value);
     if (should_commit) {
         hdm_decoder_commit(ct3d, which_hdm);
+    } else if (should_uncommit) {
+        hdm_decoder_uncommit(ct3d, which_hdm);
     }
 }
 
-- 
2.37.2

[RESEND PATCH 2/2] hw/cxl: Fix incorrect reset of commit and associated clearing of committed.

[Jonathan Cameron]

The hardware clearing the commit bit is not spec compliant.
Clearing of committed bit when commit is cleared is not specifically
stated in the CXL spec, but is the expected (and simplest) permitted
behaviour so use that for QEMU emulation.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-component-utils.c |  6 +++++-
 hw/mem/cxl_type3.c           | 21 ++++++++++++++++++++-
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index a3e6cf75cf..378f1082ce 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -38,19 +38,23 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
     ComponentRegisters *cregs = &cxl_cstate->crb;
     uint32_t *cache_mem = cregs->cache_mem_registers;
     bool should_commit = false;
+    bool should_uncommit = false;
 
     switch (offset) {
     case A_CXL_HDM_DECODER0_CTRL:
         should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
+        should_uncommit = !should_commit;
         break;
     default:
         break;
     }
 
     if (should_commit) {
-        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
         value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
         value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
+    } else if (should_uncommit) {
+        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
+        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
     }
     stl_le_p((uint8_t *)cache_mem + offset, value);
 }
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 846089ccda..9598d584ac 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -320,13 +320,28 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
 
     ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
     /* TODO: Sanity checks that the decoder is possible */
-    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
     ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
     ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
 
     stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
 }
 
+static void hdm_decoder_uncommit(CXLType3Dev *ct3d, int which)
+{
+    ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
+    uint32_t *cache_mem = cregs->cache_mem_registers;
+    uint32_t ctrl;
+
+    assert(which == 0);
+
+    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
+
+    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
+    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
+
+    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
+}
+
 static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
 {
     switch (qmp_err) {
@@ -395,6 +410,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
     CXLType3Dev *ct3d = container_of(cxl_cstate, CXLType3Dev, cxl_cstate);
     uint32_t *cache_mem = cregs->cache_mem_registers;
     bool should_commit = false;
+    bool should_uncommit = false;
     int which_hdm = -1;
 
     assert(size == 4);
@@ -403,6 +419,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
     switch (offset) {
     case A_CXL_HDM_DECODER0_CTRL:
         should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
+        should_uncommit = !should_commit;
         which_hdm = 0;
         break;
     case A_CXL_RAS_UNC_ERR_STATUS:
@@ -489,6 +506,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
     stl_le_p((uint8_t *)cache_mem + offset, value);
     if (should_commit) {
         hdm_decoder_commit(ct3d, which_hdm);
+    } else if (should_uncommit) {
+        hdm_decoder_uncommit(ct3d, which_hdm);
     }
 }
 
-- 
2.37.2

Re: [RESEND PATCH 2/2] hw/cxl: Fix incorrect reset of commit and associated clearing of committed.

[Fan Ni]

On Wed, Mar 22, 2023 at 10:33:00AM +0000, Jonathan Cameron wrote:
> The hardware clearing the commit bit is not spec compliant.
> Clearing of committed bit when commit is cleared is not specifically
> stated in the CXL spec, but is the expected (and simplest) permitted
> behaviour so use that for QEMU emulation.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>
Tested-by: Fan Ni <fan.ni@samsung.com>


The patch passed the tests as shown in previous mailing list discussion:
https://lore.kernel.org/linux-cxl/640276695c8e8_5b27929473@dwillia2-xfh.jf.intel.com.notmuch/T/#m0afcfc21d68c84c07f2e9e3194f587c2ffa82d6d
The following two topologies are tested:
1. one HB with one root port and a direct attached memdev.
2. one HB with 2 root ports and a memdev is directly attached to a CXL switch
which is attached to one root port.

One minor thing below.

>  hw/cxl/cxl-component-utils.c |  6 +++++-
>  hw/mem/cxl_type3.c           | 21 ++++++++++++++++++++-
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index a3e6cf75cf..378f1082ce 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -38,19 +38,23 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
>      ComponentRegisters *cregs = &cxl_cstate->crb;
>      uint32_t *cache_mem = cregs->cache_mem_registers;
>      bool should_commit = false;
> +    bool should_uncommit = false;
>  
>      switch (offset) {
>      case A_CXL_HDM_DECODER0_CTRL:
>          should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> +        should_uncommit = !should_commit;
This will cause committed always reset if COMMIT is 0, not only
1->0. No issue for now, just point out to make sure it is what we
want.
>          break;
>      default:
>          break;
>      }
>  
>      if (should_commit) {
> -        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
>          value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
>          value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
> +    } else if (should_uncommit) {
> +        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
> +        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
>      }
>      stl_le_p((uint8_t *)cache_mem + offset, value);
>  }
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 846089ccda..9598d584ac 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -320,13 +320,28 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
>  
>      ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
>      /* TODO: Sanity checks that the decoder is possible */
> -    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
>      ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
>      ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
>  
>      stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
>  }
>  
> +static void hdm_decoder_uncommit(CXLType3Dev *ct3d, int which)
> +{
> +    ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
> +    uint32_t *cache_mem = cregs->cache_mem_registers;
> +    uint32_t ctrl;
> +
> +    assert(which == 0);
> +
> +    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
> +
> +    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
> +    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
> +
> +    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
> +}
> +
>  static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
>  {
>      switch (qmp_err) {
> @@ -395,6 +410,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
>      CXLType3Dev *ct3d = container_of(cxl_cstate, CXLType3Dev, cxl_cstate);
>      uint32_t *cache_mem = cregs->cache_mem_registers;
>      bool should_commit = false;
> +    bool should_uncommit = false;
>      int which_hdm = -1;
>  
>      assert(size == 4);
> @@ -403,6 +419,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
>      switch (offset) {
>      case A_CXL_HDM_DECODER0_CTRL:
>          should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> +        should_uncommit = !should_commit;
>          which_hdm = 0;
>          break;
>      case A_CXL_RAS_UNC_ERR_STATUS:
> @@ -489,6 +506,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
>      stl_le_p((uint8_t *)cache_mem + offset, value);
>      if (should_commit) {
>          hdm_decoder_commit(ct3d, which_hdm);
> +    } else if (should_uncommit) {
> +        hdm_decoder_uncommit(ct3d, which_hdm);
>      }
>  }
>  
> -- 
> 2.37.2
> 

Re: [RESEND PATCH 2/2] hw/cxl: Fix incorrect reset of commit and associated clearing of committed.

[Jonathan Cameron]

On Wed, 22 Mar 2023 16:21:26 +0000
Fan Ni <fan.ni@samsung.com> wrote:

> On Wed, Mar 22, 2023 at 10:33:00AM +0000, Jonathan Cameron wrote:
> > The hardware clearing the commit bit is not spec compliant.
> > Clearing of committed bit when commit is cleared is not specifically
> > stated in the CXL spec, but is the expected (and simplest) permitted
> > behaviour so use that for QEMU emulation.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---  
> 
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
> Tested-by: Fan Ni <fan.ni@samsung.com>
> 
> 
> The patch passed the tests as shown in previous mailing list discussion:
> https://lore.kernel.org/linux-cxl/640276695c8e8_5b27929473@dwillia2-xfh.jf.intel.com.notmuch/T/#m0afcfc21d68c84c07f2e9e3194f587c2ffa82d6d
> The following two topologies are tested:
> 1. one HB with one root port and a direct attached memdev.
> 2. one HB with 2 root ports and a memdev is directly attached to a CXL switch
> which is attached to one root port.
> 
> One minor thing below.
> 
> >  hw/cxl/cxl-component-utils.c |  6 +++++-
> >  hw/mem/cxl_type3.c           | 21 ++++++++++++++++++++-
> >  2 files changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index a3e6cf75cf..378f1082ce 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -38,19 +38,23 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
> >      ComponentRegisters *cregs = &cxl_cstate->crb;
> >      uint32_t *cache_mem = cregs->cache_mem_registers;
> >      bool should_commit = false;
> > +    bool should_uncommit = false;
> >  
> >      switch (offset) {
> >      case A_CXL_HDM_DECODER0_CTRL:
> >          should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> > +        should_uncommit = !should_commit;  
> This will cause committed always reset if COMMIT is 0, not only
> 1->0. No issue for now, just point out to make sure it is what we
> want.

True.  However I think it's harmless.

We will want to be a little careful if uncommitting gains other
functionality in future though.

Note that the same potential corner existing on the commit side of things.
Trying to commit a decoder that is already committed will call the code
for commit (also currently harmless)

I'm not particularly keen to introduce additional complexity to separate
out these cases until / if we ever need it.

Jonathan

> >          break;
> >      default:
> >          break;
> >      }
> >  
> >      if (should_commit) {
> > -        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
> >          value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
> >          value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
> > +    } else if (should_uncommit) {
> > +        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
> > +        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
> >      }
> >      stl_le_p((uint8_t *)cache_mem + offset, value);
> >  }
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 846089ccda..9598d584ac 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -320,13 +320,28 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
> >  
> >      ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
> >      /* TODO: Sanity checks that the decoder is possible */
> > -    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
> >      ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
> >      ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
> >  
> >      stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
> >  }
> >  
> > +static void hdm_decoder_uncommit(CXLType3Dev *ct3d, int which)
> > +{
> > +    ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
> > +    uint32_t *cache_mem = cregs->cache_mem_registers;
> > +    uint32_t ctrl;
> > +
> > +    assert(which == 0);
> > +
> > +    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
> > +
> > +    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
> > +    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
> > +
> > +    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
> > +}
> > +
> >  static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
> >  {
> >      switch (qmp_err) {
> > @@ -395,6 +410,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> >      CXLType3Dev *ct3d = container_of(cxl_cstate, CXLType3Dev, cxl_cstate);
> >      uint32_t *cache_mem = cregs->cache_mem_registers;
> >      bool should_commit = false;
> > +    bool should_uncommit = false;
> >      int which_hdm = -1;
> >  
> >      assert(size == 4);
> > @@ -403,6 +419,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> >      switch (offset) {
> >      case A_CXL_HDM_DECODER0_CTRL:
> >          should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> > +        should_uncommit = !should_commit;
> >          which_hdm = 0;
> >          break;
> >      case A_CXL_RAS_UNC_ERR_STATUS:
> > @@ -489,6 +506,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> >      stl_le_p((uint8_t *)cache_mem + offset, value);
> >      if (should_commit) {
> >          hdm_decoder_commit(ct3d, which_hdm);
> > +    } else if (should_uncommit) {
> > +        hdm_decoder_uncommit(ct3d, which_hdm);
> >      }
> >  }
> >  
> > -- 
> > 2.37.2
> >  

Re: [RESEND PATCH 2/2] hw/cxl: Fix incorrect reset of commit and associated clearing of committed.

[Dave Jiang]

On 3/22/23 3:33 AM, Jonathan Cameron wrote:
> The hardware clearing the commit bit is not spec compliant.
> Clearing of committed bit when commit is cleared is not specifically
> stated in the CXL spec, but is the expected (and simplest) permitted
> behaviour so use that for QEMU emulation.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   hw/cxl/cxl-component-utils.c |  6 +++++-
>   hw/mem/cxl_type3.c           | 21 ++++++++++++++++++++-
>   2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index a3e6cf75cf..378f1082ce 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -38,19 +38,23 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
>       ComponentRegisters *cregs = &cxl_cstate->crb;
>       uint32_t *cache_mem = cregs->cache_mem_registers;
>       bool should_commit = false;
> +    bool should_uncommit = false;
>   
>       switch (offset) {
>       case A_CXL_HDM_DECODER0_CTRL:
>           should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> +        should_uncommit = !should_commit;
>           break;
>       default:
>           break;
>       }
>   
>       if (should_commit) {
> -        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
>           value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
>           value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
> +    } else if (should_uncommit) {
> +        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, ERR, 0);
> +        value = FIELD_DP32(value, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
>       }
>       stl_le_p((uint8_t *)cache_mem + offset, value);
>   }
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 846089ccda..9598d584ac 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -320,13 +320,28 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
>   
>       ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
>       /* TODO: Sanity checks that the decoder is possible */
> -    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT, 0);
>       ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
>       ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
>   
>       stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
>   }
>   
> +static void hdm_decoder_uncommit(CXLType3Dev *ct3d, int which)
> +{
> +    ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
> +    uint32_t *cache_mem = cregs->cache_mem_registers;
> +    uint32_t ctrl;
> +
> +    assert(which == 0);
> +
> +    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
> +
> +    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
> +    ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
> +
> +    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
> +}
> +
>   static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
>   {
>       switch (qmp_err) {
> @@ -395,6 +410,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
>       CXLType3Dev *ct3d = container_of(cxl_cstate, CXLType3Dev, cxl_cstate);
>       uint32_t *cache_mem = cregs->cache_mem_registers;
>       bool should_commit = false;
> +    bool should_uncommit = false;
>       int which_hdm = -1;
>   
>       assert(size == 4);
> @@ -403,6 +419,7 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
>       switch (offset) {
>       case A_CXL_HDM_DECODER0_CTRL:
>           should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> +        should_uncommit = !should_commit;
>           which_hdm = 0;
>           break;
>       case A_CXL_RAS_UNC_ERR_STATUS:
> @@ -489,6 +506,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
>       stl_le_p((uint8_t *)cache_mem + offset, value);
>       if (should_commit) {
>           hdm_decoder_commit(ct3d, which_hdm);
> +    } else if (should_uncommit) {
> +        hdm_decoder_uncommit(ct3d, which_hdm);
>       }
>   }
>