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

[Jonathan Cameron]

On Wed, 20 Jul 2022 13:23:10 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> 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):

Huh. This looks to be one of those cases where a bunch of different
bugs ended up with something that appears to work.

> 
> > +/* 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" ?

Spot on.
> 
> > +    } else {
> > +        target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO];  
> 
> Was this (or the other one) intended to be ...LIST_HI ?
>
Also correct.
 
> > +        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.

The two registers each have 4 target port numbers stored
in them. This was supposed to get the right one.  Tests weren't
exercising greater than 4 as that requires a very flat hierarchy
and doesn't exercise some other paths as can't interleave so
wide at multiple levels. Oh well, clearly need
an extra test.

> I would
> recommend using extract32() for this rather than raw shift
> and mask operations -- it's generally easier to write and
> to review.

Will do.

Unfortunately I've run into an issue testing with
a single host bridge and 8 root ports. Need to pin down if it
is a kernel or qemu issue before sending out this fix.  I'm away for the
next week though so may have to get back to this after returning.

Sorry for the delay.

Thanks,

Jonathan

> 
> thanks
> -- PMM