Re: [PATCH 09/14] cxl/mem: Fix CXL DVSEC Range Sizing

From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: linux-cxl@vger.kernel.org, Ben Widawsky <ben.widawsky@intel.com>,
	"Weiny, Ira" <ira.weiny@intel.com>,
	"Schofield, Alison" <alison.schofield@intel.com>,
	Vishal L Verma <vishal.l.verma@intel.com>,
	Ariel.Sibley@microchip.com
Subject: Re: [PATCH 09/14] cxl/mem: Fix CXL DVSEC Range Sizing
Date: Wed, 18 May 2022 10:06:57 -0700	[thread overview]
Message-ID: <CAPcyv4g-7aVgFtOJu-FE_6XHS9PHJBDigk_jQ=UTRs2sj1h3zg@mail.gmail.com> (raw)
In-Reply-To: <20220518174047.00007711@Huawei.com>

On Wed, May 18, 2022 at 9:42 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 12 May 2022 11:15:05 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > Per CXL 2.0 Section 8.1.3.8.4 "DVSEC CXL Range 1 Base Low" there is no
> > way to specify decode sizes smaller than 256M. Fix cxl_dvsec_ranges()
> > and cxl_hdm_decode_init() to account for that default decode range.
>
> This is effectively the same as the discussion on patch 14.
>
> My reading of the spec suggests that size can be 0 and that would mean
> no access is passed on to the hardware.  It's a rather odd corner case
> and would mean the device would only work with HDM decoders and might
> well fail some compliance tests (I haven't checked)
>
> It's not a corner case I care about...
>
> > Note, that this means that any BIOS implementation that sets mem_enable,
> > but not HDM Decoder Capability enable will cause the driver to fail to
> > attach. A later change validates the DVSEC ranges against platform CXL
> > decode (CXL CFMWS) to make a decision about overriding the default DVSEC
> > Range configuration.
>
> >
> > Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/pci.c |   10 +++++++---
> >  drivers/cxl/mem.c      |   10 +---------
> >  2 files changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index f3e59f8b6621..f1c0677a4f52 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -236,7 +236,12 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
> >               if (rc)
> >                       return rc;
> >
> > -             size = (u64)temp << 32;
> > +             /*
> > +              * Per CXL 2.0 Section 8.1.3.8.4 "DVSEC CXL Range 1 Base
> > +              * Low", the minimum decode size is 256MB
> > +              */
> > +             size = SZ_256M;
>
> This is not how I read the spec.  The match is base <= Addr < base + size.
> If size == 0 then there are no matches as base = base + size and so the
> right condition isn't met.  Maybe I'm missing something though...

That's how Ariel reads it too, so I'll just drop this as it doesn't
affect the logic all that much, and hopefully the spec gets cleaned up
to preclude the possibility of sub-256M sized devices.