Re: [PULL v2 35/86] cxl/cxl-host: Add memops for CFMWS region.

[Peter Maydell]

On Mon, 16 May 2022 at 21:52, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Jonathan Cameron <jonathan.cameron@huawei.com>
>
> These memops perform interleave decoding, walking down the
> CXL topology from CFMWS described host interleave
> decoder via CXL host bridge HDM decoders, through the CXL
> root ports and finally call CXL type 3 specific read and write
> functions.
>
> Note that, whilst functional the current implementation does
> not support:
> * switches
> * multiple HDM decoders at a given level.
> * unaligned accesses across the interleave boundaries

Hi; Coverity reports a bug in this code (CID 1488873):

> +/* TODO: support, multiple hdm decoders */
> +static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
> +                                uint8_t *target)
> +{
> +    uint32_t ctrl;
> +    uint32_t ig_enc;
> +    uint32_t iw_enc;
> +    uint32_t target_reg;

target_reg is 32 bits...

> +    uint32_t target_idx;
> +
> +    ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL];
> +    if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
> +        return false;
> +    }
> +
> +    ig_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IG);
> +    iw_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW);
> +    target_idx = (addr / cxl_decode_ig(ig_enc)) % (1 << iw_enc);
> +
> +    if (target_idx > 4) {

...in this half of the if() target_idx is at least 5...

> +        target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO];
> +        target_reg >>= target_idx * 8;

...but 5 * 8 is 40, so this shift will always be undefined
behaviour. Was the if() condition intended to be "< 4" ?

> +    } else {
> +        target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO];

Was this (or the other one) intended to be ...LIST_HI ?

> +        target_reg >>= (target_idx - 4) * 8;

Not noticed by Coverity, but in this half of the if(),
target_idx is 4 or less, so (target_idx - 4) is in most
cases going to be negative, which isn't a valid shift amount.
This also suggests the if() condition is wrong.

> +    }
> +    *target = target_reg & 0xff;
> +
> +    return true;
> +}

What's the intended behaviour here ?

The code appears to be attempting to extract a particular
subfield from one or other of the cache_mem[] values. I would
recommend using extract32() for this rather than raw shift
and mask operations -- it's generally easier to write and
to review.

thanks
-- PMM