Received-SPF: pass (google.com: domain of linux-kernel+bounces-128827-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 2 Apr 2024 16:24:50 -0700
From: Ira Weiny <ira.weiny@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>, <ira.weiny@intel.com>
CC: Dave Jiang <dave.jiang@intel.com>, Fan Ni <fan.ni@samsung.com>, "Navneet
 Singh" <navneet.singh@intel.com>, Dan Williams <dan.j.williams@intel.com>,
	Davidlohr Bueso <dave@stgolabs.net>, Alison Schofield
	<alison.schofield@intel.com>, Vishal Verma <vishal.l.verma@intel.com>,
	<linux-btrfs@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 02/26] cxl/core: Separate region mode from decoder mode
Message-ID: <660c93c2b2623_8a7d0294af@iweiny-mobl.notmuch>
References: <20240324-dcd-type2-upstream-v1-0-b7b00d623625@intel.com>
 <20240324-dcd-type2-upstream-v1-2-b7b00d623625@intel.com>
 <20240325162010.000036cc@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240325162010.000036cc@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?yKvOB+zzaXBFJ7Qa1Z4uNEka0kRtKCcHQRzM0uhXrTWbmi5Yj+NeGqdsmBOe?=
 =?us-ascii?Q?fro12SXGFB2fkyzI61e6YIGflxktps3w3rh2QjrETz7VzXC0ri1/sOM3H+JF?=
 =?us-ascii?Q?iMBkncF+ADBEA0cTeMl+jcpVcmxzjFJKh+3KYqUKqp+4ztWGT8mwx2jsz+/J?=
 =?us-ascii?Q?/4oWlZbY1uFz9/IdlyBZLwWo97l4RkRUIUFMUBC9iFZo2a18fApi/mlLpOAb?=
 =?us-ascii?Q?2diOb6Q+hw2MlEQCFO7JMFENV1QVkp+iQnYsl9JAFnF2zXuTMqvqxluFWWfJ?=
 =?us-ascii?Q?reyHI1rbU2r3kIrywB77WJkk1ChDTeE2UiqxydLwB0Yk5VSxcB9MhQJoJKkP?=
 =?us-ascii?Q?B/XegpbMqWqWk/0xK/NSOqOcfrU4QxZLGxTKfGV5u/dKAC6iqFLb9OO2pUTX?=
 =?us-ascii?Q?CuyaBN+iZIqylkzZbefC78gFWr8Vn0xpqEB/EJBWyk9PTWrRITe8i/ZIx70e?=
 =?us-ascii?Q?OJMlNq11XJ0QdpQBHFAbBjSK3u7XSr/Y7mDuXujWwU6mC/HrjdaaaOgOpFrI?=
 =?us-ascii?Q?vqAiDpp1zedIPdxWx160LqXOTulLPuvwYgyR4sOlqAXAYeZxW9oFGUGMC3Du?=
 =?us-ascii?Q?AaVC/2tsNQYiu2yJ6XtaoPczdH5Fcg6OlplDEF5KI8wbvZPRYX+BpOSMfVjN?=
 =?us-ascii?Q?+6IvkYTVp9Aw3/4s11/SfJ4aKw8IAmhgSoOsRfT/aW7ZYmNnf0z49LjjTuqG?=
 =?us-ascii?Q?HN+aGESid86u+8eWDBs9KjsBijxuTuRUdxPtqBwViJaTZREjMY6R6GJ1wiwQ?=
 =?us-ascii?Q?AacXcqrHufo74slXT8ZfAQgO1Pr0xCVuL/rMKxycEnLt44vc4WHNOxiKwmVS?=
 =?us-ascii?Q?z22DEEnZQbreW5a4M8A35bZLeJ+5DoRAqjSuKXjRGaiUsb1A6yejH/nlTfKz?=
 =?us-ascii?Q?f1DPgitOHYLjeg0iFhR1S2UGFUKBzdDtNaMBjqUBrRZ5Bv1OZRMEXEmL9bnX?=
 =?us-ascii?Q?nsrT8hhlw4S1DrweFKLLstCtE1iprR27ET/dFtTiI6HVduMoW6Dizz/7xYb4?=
 =?us-ascii?Q?VQL+hbF0m5aKBqplYHZKV2FG+3K3/Deqbhs2PszmZ05epZJ3cc3KLqx9aaBP?=
 =?us-ascii?Q?fTp3C2tJGKBD4KcwZSb8absMlesw6Dosisf7hcHqKEoI4T9TF0Qk0EcVBQOM?=
 =?us-ascii?Q?Ffp7oHNmYcrh+aHNg3BDiYx09iFbDOlQxo3Ccut2kY6j9LROeWJcEQP15rIW?=
 =?us-ascii?Q?TG0v/lJ9kximyEVZA2PVClnEyR4e/P7YW+5X2hxqkGwnthAvRynC2uMnPQlc?=
 =?us-ascii?Q?HFM+KtzwQi28MurxEAdy+l8gIJgV8Jw8Tdo6YqxDH2a6qBCX5K/JSFBIEY/t?=
 =?us-ascii?Q?KUbMlZeEf006ZnCEkcef4jrR4szfk0eqsf/YTJRewTmtGNOpeNcangxZnDYZ?=
 =?us-ascii?Q?ptvG9Kbig67KTwOm2Rs4b8ebGnw067BBe7mRHPwyzzZFU0yjm4sGDZiOJPrh?=
 =?us-ascii?Q?fx9RdKay3DCyoD40Bwe7yZ7NzvIWdKCyhCa66011YYmENzqogSMWxciDcJ6/?=
 =?us-ascii?Q?zU/SwRDtKkaqWpBSugo5gc46WL7nCmB2zwmZ9lNwkprY2RGPmqKFqalga4O8?=
 =?us-ascii?Q?WgAD0M9zgjsTRwlRe7H1l5qecN3HxMqe+qYosSy7?=
X-MS-Exchange-CrossTenant-Network-Message-Id: cb8f66da-0a57-43b9-8dd2-08dc536c1bba
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Apr 2024 23:24:56.9395
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1SjFn+BBoKX7VstM+wL860/oDQrKJmfEn3wI6yQQ7bq52AUMhgVugsuorLB0oCn2cKQVqA8r5sjr8I3eICKAKw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6477
X-OriginatorOrg: intel.com

Jonathan Cameron wrote:
> On Sun, 24 Mar 2024 16:18:05 -0700
> ira.weiny@intel.com wrote:

[snip]

> > 
> > ---
> > Changes for v1
> > <none>
> > ---
> >  drivers/cxl/core/region.c | 77 +++++++++++++++++++++++++++++++++++------------
> >  drivers/cxl/cxl.h         | 26 ++++++++++++++--
> >  2 files changed, 81 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 4c7fd2d5cccb..1723d17f121e 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> 
> 
> > @@ -2800,6 +2814,24 @@ static int match_region_by_range(struct device *dev, void *data)
> >  	return rc;
> >  }
> >  
> > +static enum cxl_region_mode
> > +cxl_decoder_to_region_mode(enum cxl_decoder_mode mode)
> > +{
> > +	switch (mode) {
> > +	case CXL_DECODER_NONE:
> > +		return CXL_REGION_NONE;
> > +	case CXL_DECODER_RAM:
> > +		return CXL_REGION_RAM;
> > +	case CXL_DECODER_PMEM:
> > +		return CXL_REGION_PMEM;
> > +	case CXL_DECODER_MIXED:
> > +	default:
> > +		return CXL_REGION_MIXED;
> > +	}
> > +
> 
> Dead code.

Fixed thanks.

> 
> > +	return CXL_REGION_MIXED;
> > +}
> > +
> >  /* Establish an empty region covering the given HPA range */
> >  static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> >  					   struct cxl_endpoint_decoder *cxled)
> > @@ -2808,12 +2840,17 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> >  	struct cxl_port *port = cxlrd_to_port(cxlrd);
> >  	struct range *hpa = &cxled->cxld.hpa_range;
> >  	struct cxl_region_params *p;
> > +	enum cxl_region_mode mode;
> >  	struct cxl_region *cxlr;
> >  	struct resource *res;
> >  	int rc;
> >  
> > +	if (cxled->mode == CXL_DECODER_DEAD)
> > +		return ERR_PTR(-EINVAL);
> 
> Not a bad thing necessarily, but why do we now need this and didn't before?

Ah.  Because in devm_cxl_add_region() the mode of CXL_DECODER_DEAD used to
return -EINVAL.

There is no logical equivalent to decoder dead in the region mode (regions
don't need it).  So this correctly flags the error based on the decoder
mode rather than introduce a mode for regions which does not make sense.

I'll update the commit message because that is hard to see.

> 
> > +
> > +	mode = cxl_decoder_to_region_mode(cxled->mode);
> >  	do {
> > -		cxlr = __create_region(cxlrd, cxled->mode,
> > +		cxlr = __create_region(cxlrd, mode,
> >  				       atomic_read(&cxlrd->region_id));
> >  	} while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
> 
> 
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 003feebab79b..9a0cce1e6fca 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> 
> 
> >  /*
> >   * Track whether this decoder is reserved for region autodiscovery, or
> >   * free for userspace provisioning.
> > @@ -511,7 +532,8 @@ struct cxl_region_params {
> >   * struct cxl_region - CXL region
> >   * @dev: This region's device
> >   * @id: This region's id. Id is globally unique across all regions
> > - * @mode: Endpoint decoder allocation / access mode
> > + * @mode: Region mode which defines which endpoint decoder mode the region is
> mode or potentially modes?
> 
> If region is mixed, I guess that means endpoint could be pmem or ram in theory?
> Don't think anyone has implemented anything yet, but is the potential there?

Yes the potential is there.  The endpoint decoder is set to
CXL_DECODER_MIXED in __cxl_dpa_reserve().  But I am unclear how that will
ever be executed except if the BIOS sets up a decoder to span ram/pmem.
In this case the rest of the stack is not going to work and will complain
about mixed mode.

Ok Dan clued me in.  Check out cxl_dpa_alloc().  Spanning partitions is
not allowed.

So the comment is targeted toward the 'normal' case even though the region
could be created incorrectly via BIOS.

Ira