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);
>       }
>   }
>