Re: [RFC] cxl-host: Fix committed check for passthrough decoder

[Fan Ni]

On Fri, Jan 13, 2023 at 09:47:25AM +0000, Jonathan Cameron wrote:

> On Fri, 13 Jan 2023 00:27:55 +0000
> Fan Ni <fan.ni@samsung.com> wrote:
> 
> > For passthrough decoder (a decoder hosted by a cxl component with only
> > one downstream port), its cache_mem_registers field COMMITTED
> > (see spec 2.0 8.2.5.12 - CXL HDM Decoder Capability Structure) will not
> > be set by the current Linux CXL driver. Without the fix, for a cxl
> > topology setup with a single HB and single root port, the memdev read/write
> > requests cannot be passed to the device successfully as the function
> > cxl_hdm_find_target will fail the decoder COMMITTED check and return
> > directly, which causes read/write not being directed to cxl type3 device.
> > 
> > Before the fix, a segfault is observed when trying using cxl memory for
> > htop command through 'numactl --membind' after converting cxl memory
> > into normal RAM.
> 
> We also need to fix that segfault.
With the patch, we do not see the segfault anymore. The segfault was
there before the patch because for a passthrough decoder, we cannot find a
target as the committed field check cannot pass, the read request will
return 0 (in cxl_read_cfmws) which can be used for futher addressing.
With the patch, we skip the committed check for passthrough decoder and
the requests can be passed to the device so the segfault is fixed. Our
concern is that the fix may also let the requests pass for unprogrammed
decoder, which is not allowed in current code.
> 
> 
> > 
> > Detailed steps to reproduce the issue with the cxl setup where there is
> > only one HB and a memdev is directly attached to the only root port of
> > the HB are listed as below,
> > 1. cxl create-region region0
> > 2. ndctl create-namespace -m dax -r region0
> > 3. daxctl reconfigure-device --mode=system-ram --no-online dax0.0
> > 4. daxctl online-memory dax0.0
> > 5. numactl --membind=1 htop
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> 
> Ah. This mess is still going on. I've not been testing with this
> particular combination because the kernel didn't support it.
> The kernel code assumes that the implementation made the choice
> (which is an option in the spec) to not have any HDM decoders
> for the pass through case. As such it never programmed them
> (if you dig back a long way in the region bring patch sets in the
> kernel you'll find some discussion of this). Now I knew that meant
> the configuration didn't 'work' but nothing should be crashing -
> unless you mean that something in linux userspace is trying to
> access the memory and crashing because that fails
> (which is fine as far as I'm concerned ;)
> 
> The work around for QEMU testing so far has been to add another root
> port and put nothing below that. The HDM decoders then have to be
> implemented so the kernel does what we expect.
Do you mean we already have the workaround somewhere or it is what we
have planned? currently the kernel will create a passthrough decoder if
the number of downstream is 1. If we have the workaround, there
should never be a passthrough decoder being created and we should not
see the issue.
> 
> I'm not against a more comprehensive fix.  Two options come to mind.
> 1) Add an option to the host bridge device to tell it not to implement
>    hdm decoders at all. I'm not keen to just automatically drop them
>    because having decoders on a pass through HB is a valid configuration.
> 2) Cheat and cleanly detect a pass through situation and let the accesses
>    through.  I'm not particularly keen on this option though as it
>    will fail to test the code once it's 'fixed' in Linux.  IIRC the spec
>    doesn't say that programming such an HDM decoder is optional.
> 
> I guess we could be a bit naughty with option 1 and flip the logic even
> though it would break backwards compatibility. So default to no HDM decoder.
> I doubt anyone will notice given that's the configuration that would have
> worked.  However I would want to keep the option to enable these decoders
> around.  I can spin up a patch or do you want to do it? My suggestion is 
> option
> 1 with default being no HDM decoder.
> 
> Jonathan
Please feel free to spin up a patch.
> 
> 
> 
> > ---
> >  hw/cxl/cxl-host.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> > index 1adf61231a..5ca0d6fd8f 100644
> > --- a/hw/cxl/cxl-host.c
> > +++ b/hw/cxl/cxl-host.c
> > @@ -107,8 +107,11 @@ static bool cxl_hdm_find_target(uint32_t *cache_mem, 
> > hwaddr addr,
> >      uint32_t target_idx;
> >  
> >      ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL];
> > -    if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
> > -        return false;
> > +
> > +    /* skip the check for passthrough decoder */
> 
> You have a mix of spaces and tabs for indentation. Should all be 4 spaces
> for QEMU code.
> 
> > +   if (FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMIT)
> > +           && !FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
> > +           return false;
> 
> Why is this code specific to a pass through decoder?
> All it's telling us (I think) is no one tried to commit the decoder yet.
> 
> >      }
> >  
> >      ig_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IG);
>