Re: [PATCH 7/7] hw/mem/cxl_type3: Add CXL RAS Error Injection Support.

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

Hi Mike,

> > +static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
> > +{
> > +    switch (qmp_err) {
> > +    case CXL_UNCOR_ERROR_TYPE_CACHE_DATA_PARITY:
> > +        return CXL_RAS_UNC_ERR_CACHE_DATA_PARITY;
> > +    case CXL_UNCOR_ERROR_TYPE_CACHE_ADDRESS_PARITY:
> > +        return CXL_RAS_UNC_ERR_CACHE_ADDRESS_PARITY;
> > +    case CXL_UNCOR_ERROR_TYPE_CACHE_BE_PARITY:
> > +        return CXL_RAS_UNC_ERR_CACHE_BE_PARITY;
> > +    case CXL_UNCOR_ERROR_TYPE_CACHE_DATA_ECC:
> > +        return CXL_RAS_UNC_ERR_CACHE_DATA_ECC;
> > +    case CXL_UNCOR_ERROR_TYPE_MEM_DATA_PARITY:
> > +        return CXL_RAS_UNC_ERR_MEM_DATA_PARITY;
> > +    case CXL_UNCOR_ERROR_TYPE_MEM_ADDRESS_PARITY:
> > +        return CXL_RAS_UNC_ERR_MEM_ADDRESS_PARITY;
> > +    case CXL_UNCOR_ERROR_TYPE_MEM_BE_PARITY:
> > +        return CXL_RAS_UNC_ERR_MEM_BE_PARITY;
> > +    case CXL_UNCOR_ERROR_TYPE_MEM_DATA_ECC:
> > +        return CXL_RAS_UNC_ERR_MEM_DATA_ECC;
> > +    case CXL_UNCOR_ERROR_TYPE_REINIT_THRESHOLD:
> > +        return CXL_RAS_UNC_ERR_REINIT_THRESHOLD;
> > +    case CXL_UNCOR_ERROR_TYPE_RSVD_ENCODING:
> > +        return CXL_RAS_UNC_ERR_RSVD_ENCODING;
> > +    case CXL_UNCOR_ERROR_TYPE_POISON_RECEIVED:
> > +        return CXL_RAS_UNC_ERR_POISON_RECEIVED;
> > +    case CXL_UNCOR_ERROR_TYPE_RECEIVER_OVERFLOW:
> > +        return CXL_RAS_UNC_ERR_RECEIVER_OVERFLOW;
> > +    case CXL_UNCOR_ERROR_TYPE_INTERNAL:
> > +        return CXL_RAS_UNC_ERR_INTERNAL;
> > +    case CXL_UNCOR_ERROR_TYPE_CXL_IDE_TX:
> > +        return CXL_RAS_UNC_ERR_CXL_IDE_TX;
> > +    case CXL_UNCOR_ERROR_TYPE_CXL_IDE_RX:
> > +        return CXL_RAS_UNC_ERR_CXL_IDE_RX;
> > +    default:
> > +        return -EINVAL;
> > +    }
> > +}
> > +
> > +static int ct3d_qmp_cor_err_to_cxl(CxlUncorErrorType qmp_err)  
> 
> CxlCorErrorType type  is required.
> 
> Compiler warns here:
> ../hw/mem/cxl_type3.c:1263:44: error: implicit conversion from
> enumeration type 'CxlCorErrorType' (aka 'enum CxlCorErrorType') to
> different enumeration type 'CxlUncorErrorType' (aka 'enum
> CxlUncorErrorType') [-Werror,-Wenum-conversion]
> 
>     cxl_err_type = ct3d_qmp_cor_err_to_cxl(type);
> 
>                    ~~~~~~~~~~~~~~~~~~~~~~~ ^~~~
> 1 error generated.

Yikes. Not sure how I missed that!



> 
> > +{
> > +    switch (qmp_err) {
> > +    case CXL_COR_ERROR_TYPE_CACHE_DATA_ECC:
> > +        return CXL_RAS_COR_ERR_CACHE_DATA_ECC;
> > +    case CXL_COR_ERROR_TYPE_MEM_DATA_ECC:
> > +        return CXL_RAS_COR_ERR_MEM_DATA_ECC;
> > +    case CXL_COR_ERROR_TYPE_CRC_THRESHOLD:
> > +        return CXL_RAS_COR_ERR_CRC_THRESHOLD;
> > +    case CXL_COR_ERROR_TYPE_RETRY_THRESHOLD:
> > +        return CXL_RAS_COR_ERR_RETRY_THRESHOLD;
> > +    case CXL_COR_ERROR_TYPE_CACHE_POISON_RECEIVED:
> > +        return CXL_RAS_COR_ERR_CACHE_POISON_RECEIVED;
> > +    case CXL_COR_ERROR_TYPE_MEM_POISON_RECEIVED:
> > +        return CXL_RAS_COR_ERR_MEM_POISON_RECEIVED;
> > +    case CXL_COR_ERROR_TYPE_PHYSICAL:
> > +        return CXL_RAS_COR_ERR_PHYSICAL;
> > +    default:
> > +        return -EINVAL;
> > +    }
> > +}
> > +
> >  static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> >                             unsigned size)
> >  {
> > @@ -341,6 +402,84 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> >          should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> >          which_hdm = 0;
> >          break;
> > +    case A_CXL_RAS_UNC_ERR_STATUS:
> > +    {
> > +        uint32_t capctrl = ldl_le_p(cache_mem + R_CXL_RAS_ERR_CAP_CTRL);
> > +        uint32_t fe = FIELD_EX32(capctrl, CXL_RAS_ERR_CAP_CTRL, FIRST_ERROR_POINTER);
> > +        CXLError *cxl_err;
> > +        uint32_t unc_err;
> > +
> > +        /*
> > +         * If single bit written that corresponds to the first error
> > +         * pointer being cleared, update the status and header log.
> > +         */
> > +        if (!QTAILQ_EMPTY(&ct3d->error_list)) {
> > +            CXLError *cxl_err = QTAILQ_FIRST(&ct3d->error_list);  
> 
> Is it ok that "CXLError *cxl_err"  definition clobbers previous one above?

It isn't a bug as the external one is only used much later in a QTAILQ_FOREACH()
to build the resulting error status register, but it's certainly inelegant
and there is no need for the internal definition so I'll drop it.
I also moved the assignment to the else leg which is the only place that
specific assignment is used.

Thanks for quick review!  I'll hold off sending a v2 out for a day or two
to let any other early comments come in.

Jonathan