Re: [PATCH v2 00/28] CXL PMEM Region Provisioning

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

On Thu, 21 Jul 2022 09:29:46 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Thu, 14 Jul 2022 17:00:41 -0700
> > Dan Williams <dan.j.williams@intel.com> 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@huawei.com>
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@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 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.