Re: [PULL v2 38/53] hw/cxl: Support 4 HDM decoders at all levels of topology

[Jonathan Cameron]

On Thu, 19 Oct 2023 13:31:05 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Thu, 5 Oct 2023 at 04:45, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Support these decoders in CXL host bridges (pxb-cxl), CXL Switch USP
> > and CXL Type 3 end points.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >  
> 
> 
> 
> > -/* TODO: Support multiple HDM decoders and DPA skip */
> >  static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t 
> > *dpa)
> >  {
> >      int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
> >      uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers;
> > -    uint64_t decoder_base, decoder_size, hpa_offset;
> > -    uint32_t hdm0_ctrl;
> > -    int ig, iw;
> > -    int i = 0;
> > +    unsigned int hdm_count;
> > +    uint32_t cap;
> > +    uint64_t dpa_base = 0;
> > +    int i;
> >
> > -    decoder_base =
> > -        (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] << 
> > 32) |
> > -                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc]);
> > -    if ((uint64_t)host_addr < decoder_base) {
> > -        return false;
> > +    cap = ldl_le_p(cache_mem + R_CXL_HDM_DECODER_CAPABILITY);
> > +    hdm_count = cxl_decoder_count_dec(FIELD_EX32(cap,
> > +                                                 
> > CXL_HDM_DECODER_CAPABILITY,
> > +                                                 DECODER_COUNT));
> > +
> > +    for (i = 0; i < hdm_count; i++) {
> > +        uint64_t decoder_base, decoder_size, hpa_offset, skip;
> > +        uint32_t hdm_ctrl, low, high;
> > +        int ig, iw;
> > +
> > +        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_LO + i * 
> > hdm_inc);
> > +        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_HI + i * 
> > hdm_inc);
> > +        decoder_base = ((uint64_t)high << 32) | (low & 0xf0000000);
> > +
> > +        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_LO + i * 
> > hdm_inc);
> > +        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_HI + i * 
> > hdm_inc);
> > +        decoder_size = ((uint64_t)high << 32) | (low & 0xf0000000);
> > +
> > +        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_DPA_SKIP_LO +
> > +                       i * hdm_inc);
> > +        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_DPA_SKIP_HI +
> > +                        i * hdm_inc);
> > +        skip = ((uint64_t)high << 32) | (low & 0xf0000000);
> > +        dpa_base += skip;
> > +
> > +        hpa_offset = (uint64_t)host_addr - decoder_base;
> > +
> > +        hdm_ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + i * 
> > hdm_inc);
> > +        iw = FIELD_EX32(hdm_ctrl, CXL_HDM_DECODER0_CTRL, IW);
> > +        ig = FIELD_EX32(hdm_ctrl, CXL_HDM_DECODER0_CTRL, IG);
> > +        if (!FIELD_EX32(hdm_ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
> > +            return false;
> > +        }
> > +        if (((uint64_t)host_addr < decoder_base) ||
> > +            (hpa_offset >= decoder_size)) {
> > +            dpa_base += decoder_size /
> > +                cxl_interleave_ways_dec(iw, &error_fatal);  
> 
> I noticed this because of a Coverity false-positive, but should
> this really be using error_fatal? It looks like a guest program
> writing bogus values to registers could trip this, and generally
> we don't like to let the guest make QEMU exit.

I have on my list adding a bunch of verification to the HDM decoder
commit path that will make this harder to hit, but in theory not prevent
it unless we make a decision to paper over the problem. I'll aim to do
that sooner rather than later.

Changing this after commit is specifically called out as 'undefined behavior' 
8.2.4.19.13
"Reprogramming the decoder while the Commit bit is set results in undefined
behavior".  We should probably choose the behavior to not be fatal but we do
want to scream very very loudly if a guest does this as it indicates something
that may well bring down a real system.

I get your point on not letting a guest do nasty things though so perhaps
we should user error_warn instead?

Making it die here was deliberate as I really really want to know if someone
hits it and they tend to tell me if QEMU quits on them.
I take your point though about it not being a good idea when looking at
it from a broader point of view!

Jonathan


> 
> thanks
> -- PMM
>