On Thu, 21 Jul 2022 09:29:46 -0700
Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> Jonathan Cameron wrote:
> > On Thu, 14 Jul 2022 17:00:41 -0700
> > Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> >
> >
> > Hi Dan,
> >
> > I'm low on time unfortunately and will be OoO for next week,
> > But whilst fixing a bug in QEMU, I set up a test to exercise
> > the high port target register on the hb with
> >
> > CFMWS interleave ways = 1
> > hb with 8 rp with a type3 device connected to each.
> >
> > The resulting interleave granularity isn't what I'd expect to see.
> > Setting region interleave to 1k (which happens to match the CFMWS)
> > I'm getting 1k for the CFMWS, 2k for the hb and 256 bytes for the type3
> > devices. Which is crazy... Now there may be another bug lurking
> > in QEMU so this might not be a kernel issue at all.
>
> Potentially, I will note that there seems to be a QEMU issue, that I
> have not had time to dig into, that is preventing region creation
> compared to other environments, maybe this is it...
>
Fwiw, proposed QEMU fix is this. One of those where I'm not sure how
it gave the impression of working previously.
>From 64c6566601c782e91eafe48eb28711e4bb85e8d0 Mon Sep 17 00:00:00 2001
From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
Date: Thu, 21 Jul 2022 13:29:59 +0100
Subject: [PATCH] hw/cxl: Fix wrong query of target ports.
Two issues in this code:
1) Check on which register to look in was inverted.
2) Both branches read the _LO register.
Whilst here moved to extract32() rather than hand rolling
the field extraction as simpler and hopefully less error prone.
Fixes Coverity CID: 1488873
Reported-by: Peter Maydell <peter.maydell@xxxxxxxxxx>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
---
hw/cxl/cxl-host.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index faa68ef038..1adf61231a 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -104,7 +104,6 @@ static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
uint32_t ctrl;
uint32_t ig_enc;
uint32_t iw_enc;
- uint32_t target_reg;
uint32_t target_idx;
ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL];
@@ -116,14 +115,13 @@ static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
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) {
- target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO];
- target_reg >>= target_idx * 8;
+ if (target_idx < 4) {
+ *target = extract32(cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO],
+ target_idx * 8, 8);
} else {
- target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO];
- target_reg >>= (target_idx - 4) * 8;
+ *target = extract32(cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_HI],
+ (target_idx - 4) * 8, 8);
}
- *target = target_reg & 0xff;
return true;
}
--
2.32.0
> > For this special case we should be ignoring the CFMWS IG
> > as it's irrelevant if we aren't interleaving at that level.
> > We also know we don't have any address bits used for interleave
> > decoding until the HB.
>
> ...but I am certain that the drvier implementation is not accounting for
> the freedom to specify any granularity when the CFMWS interleave is x1.
> Will craft an incremental fix.
