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
>