Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Fan Ni <fan.ni@samsung.com>
To:     Dan Williams <dan.j.williams@intel.com>
CC:     "alison.schofield@intel.com" <alison.schofield@intel.com>,
        "vishal.l.verma@intel.com" <vishal.l.verma@intel.com>,
        "ira.weiny@intel.com" <ira.weiny@intel.com>,
        "bwidawsk@kernel.org" <bwidawsk@kernel.org>,
        "Jonathan.Cameron@huawei.com" <Jonathan.Cameron@huawei.com>,
        "dan.carpenter@oracle.com" <dan.carpenter@oracle.com>,
        "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        Adam Manzanares <a.manzanares@samsung.com>,
        "dave@stgolabs.net" <dave@stgolabs.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cxl/region: Fix memdev reuse check
Thread-Topic: [PATCH] cxl/region: Fix memdev reuse check
Thread-Index: AQHY8u8LeWViEa6a3k+/w2cfyy70na40pOkAgC2eUgA=
Date:   Wed, 7 Dec 2022 00:19:01 +0000
Message-ID: <20221207001852.GA2414318@bgt-140510-bm03>
In-Reply-To: <63699774d1af_18432294b8@dwillia2-xfh.jf.intel.com.notmuch>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <01719DCE08040547B206BD87863253C0@ssi.samsung.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
CMS-TYPE: 301P
References: <CGME20221107212232uscas1p20a000834b76db7a7e459708b06638120@uscas1p2.samsung.com>
        <20221107212153.745993-1-fan.ni@samsung.com>
        <63699774d1af_18432294b8@dwillia2-xfh.jf.intel.com.notmuch>
Precedence: bulk

On Mon, Nov 07, 2022 at 03:40:36PM -0800, Dan Williams wrote:

> Fan Ni wrote:
> > Memdev reuse in a region currently does not iterate over all of the
> > interleave targets. Fix this by using existing iterator for memdev reus=
e
> > check.
>=20
> Not enough detail, what does this actually fix in practice? For example,
> if an end user encountered this bug, what would they see as the
> symptoms?  I could probably figure out, but for bugs I should not have
> to, and more importantly downstream OSV kernel maintainers, who do not
> have the same context as CXL developers, also need that information to
> decide if this is a fix they want to backport into their kernel.
>=20

Hi Dan,
Thanks for the feedback. Here are more details about the patch, and
I will refine the patch.

cxlmd_target =3D cxled_to_memdev(cxled_target);
if (cxlmd_target =3D=3D cxlmd) {
	dev_dbg(&cxlr->dev,
			"%s already specified at position %d via: %s\n",
			dev_name(&cxlmd->dev), pos,
			dev_name(&cxled_target->cxld.dev));
	return -EBUSY;
}

Before the patch, the check of whether or not a memdev has already been
used as a target for the region (above code piece) will always be skipped.
Given a memdev with more than one HDM decoder, an interleaved region can be
created that maps multiple HPAs to the same DPA. According to CXL spec 3.0
8.1.3.8.4, "Aliasing (mapping more than one Host Physical Address (HPA) to =
a
single Device Physical Address) is forbidden."

The CXL specification allows a device to have more than one HDM decoder
("The number of decoders implemented by a component are enumerated via the =
CXL
 HDM Decoder Capability register (see Section 8.2.4.19.1"). If a CXL device
 has multiple HDM decoders the current code allows to create memory regions
 that map multiple HPAs to a single DPA.

Fan

> >=20
> > Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> >  drivers/cxl/core/region.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >=20
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index f9ae5ad284ff..c7152b4bd9eb 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1226,7 +1226,7 @@ static int cxl_region_attach(struct cxl_region *c=
xlr,
> >  		struct cxl_endpoint_decoder *cxled_target;
> >  		struct cxl_memdev *cxlmd_target;
> > =20
> > -		cxled_target =3D p->targets[pos];
> > +		cxled_target =3D p->targets[i];
> >  		if (!cxled_target)
> >  			continue;
> > =20
> > --=20
> > 2.25.1
>=20
> =