Received-SPF: pass (google.com: domain of linux-kernel+bounces-167949-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 3 May 2024 10:09:41 -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 07/26] cxl/port: Add dynamic capacity size support to
 endpoint decoders
Message-ID: <66351a55a38db_e1f58294ce@iweiny-mobl.notmuch>
References: <20240324-dcd-type2-upstream-v1-0-b7b00d623625@intel.com>
 <20240324-dcd-type2-upstream-v1-7-b7b00d623625@intel.com>
 <20240405145444.0000437f@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240405145444.0000437f@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ZMxCtQYcKyr4fWxOzysT90fbvXVhB+j7YaUcF7qwVBD9q/ipbBjBLpfeHdVY?=
 =?us-ascii?Q?dWesQmdP6ShefFY2nmDvWo2mXEq2eOvloVMADuZMc6Ahw37w7Cj4brqxx+3B?=
 =?us-ascii?Q?b3t81MD7oIO5BvzKyn31Go+mq6Qp7UJcvLvNBXESLPh2sKFOwDFgxM4OhAiJ?=
 =?us-ascii?Q?mYZ1lORquAKH+PsnFcbhByu+PRGlOyWkdX0vG4NCP+YM9KIxM2EB3035UILO?=
 =?us-ascii?Q?AAZ6CTVgkKf+41DbE2PvY4Gn6RKAwZiYF8ax1oiK4gbJvHIAdbMbQJr+izJE?=
 =?us-ascii?Q?NVXhVPFqRWNknmfE+U1icdKLEPyrsmOQUXWb9W9wEbjGvdESymmNLt5B36Nq?=
 =?us-ascii?Q?/t4StBuucjR0KLh/x9sS67RedmcJPj6H25T7whegJNPezCZDSAkXCd/bDix9?=
 =?us-ascii?Q?DactpJ2DNxRn3F5GNOIdF3p5hF3s//z7znSvP85+gIsThi1TnfBvtyFreRFT?=
 =?us-ascii?Q?5fH9Qh7RymiSLiBqfjAUr7Z5G1M7uCoQmzx0FoGqJKnJM3eNTNF8f+LVcLxf?=
 =?us-ascii?Q?sSsR58E8vFUCYCdfbiyrHN5M20s7fK+1F0e02CEEv6MbpwLjieiJQ2K4Ux+2?=
 =?us-ascii?Q?2SMvntnOp7h9Zb9MQX+0BQr3eCClWO8Gdlh6ZysHkTCbVpdVYAQO/tYdDzYq?=
 =?us-ascii?Q?NPU5ogyNNAfHDSIblrmhpoF9hwWsQjgy+/peFVow3NSJd57mb0z9NB4xf8Tw?=
 =?us-ascii?Q?vavpr8m1j3GWMPN1R1uGiC6BTsKN/zM+uv3OmlwbrwfkKOUQqtLqX3Z5HJBf?=
 =?us-ascii?Q?E/jVp7ew6r6Qtnv+b/329jgzFLZ7tn9yYhCEiBE3S7xmmztcfX7DlNCEIf6u?=
 =?us-ascii?Q?Ih/MZGPd85xSXoZFm+tt/aESfYJiNfzhfq45zedhKyY6w9bjBG/s2iHsJlmR?=
 =?us-ascii?Q?HHGtUCJ4YVfQr9dXh1aOBvg+RnT799eCZqgrPTRUc7Lhi/Ek0HxRkZLe7Lv5?=
 =?us-ascii?Q?OV7Cs5vjBLxTtJp9bCFRVm1OuQjNwM8udfG1BHYLVJAG2HK9PQWH0brpQ+mf?=
 =?us-ascii?Q?u9bscy/fmQox4226Wx6T19zacKWPW6jORnXH1Ku4u4zSJC1Bi/gcmhnBnHC9?=
 =?us-ascii?Q?tdhefJEuFhH8vPrw+qrDnrHF71IeNFI1KWb/ZZlHRuslYRRwurHcDTxuFpKG?=
 =?us-ascii?Q?3ttXXwNF3J70lnjdO/Q2c6N5uOdhoBhyhBV6RPkOkq6sTe6HHQ/aXVS8ibMR?=
 =?us-ascii?Q?piBko3fzIx8k2DN405iBTs7BqipkjJKoPiaU5/5K8cjlT04cgYSa9qOKl2/h?=
 =?us-ascii?Q?naTuXm5JfzR+Du5KS/+tIwWwjRGm18y9aPdx76/57smJovZVKrbYq2z6+XOp?=
 =?us-ascii?Q?e014cdeBcxZU+FFp533/DtLiGiOxg76Mjil2nv5Bn2UneZDdSfD0T1sQwDWi?=
 =?us-ascii?Q?EA+iUWozEWcdz/37CaoIbe+LK4pfHKWVaGK3mK+cTxnZ32TKSL59IWwWhT6A?=
 =?us-ascii?Q?bRIHSCfwPMJ2GIWNVDnJVL3gq6qorIwKJtDNAg3QoODfORi9CcL3Ia1p7CEy?=
 =?us-ascii?Q?0AOJdccU3VDY7JoGLFNudxpOhIIG0SvL3BytZR08so2TSphW0Wuwp7L31MLF?=
 =?us-ascii?Q?L79xNAA8si84aQOh2fQ5AGPwyHnvLi7OmF2ap2O7?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9129fd06-5f3b-4212-996c-08dc6b93d536
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2024 17:09:46.3273
 (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: dpUa400hEndPvqg3qvjm1U7YAehy/hsRjXV25r9JlW6EykjrF9ho02zzDXoDsOiltfoQqCH8aeIf8d4VJ2jgMA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4710
X-OriginatorOrg: intel.com

Jonathan Cameron wrote:
> On Sun, 24 Mar 2024 16:18:10 -0700
> ira.weiny@intel.com wrote:
> 
> > From: Navneet Singh <navneet.singh@intel.com>
> > 
> > To support Dynamic Capacity Devices (DCD) endpoint decoders will need to
> > map DC partitions (regions).  In addition to assigning the size of the
> > DC partition, the decoder must assign any skip value from the previous
> > decoder.  This must be done within a contiguous DPA space.
> > 
> > Two complications arise with Dynamic Capacity regions which did not
> > exist with Ram and PMEM partitions.  First, gaps in the DPA space can
> > exist between and around the DC Regions.  Second, the Linux resource
> > tree does not allow a resource to be marked across existing nodes within
> > a tree.
> > 
> > For clarity, below is an example of an 60GB device with 10GB of RAM,
> > 10GB of PMEM and 10GB for each of 2 DC Regions.  The desired CXL mapping
> > is 5GB of RAM, 5GB of PMEM, and all 10GB of DC1.
> > 
> >      DPA RANGE
> >      (dpa_res)
> > 0GB        10GB       20GB       30GB       40GB       50GB       60GB
> > |----------|----------|----------|----------|----------|----------|
> > 
> > RAM         PMEM                  DC0                   DC1
> >  (ram_res)  (pmem_res)            (dc_res[0])           (dc_res[1])
> > |----------|----------|   <gap>  |----------|   <gap>  |----------|
> > 
> >  RAM        PMEM                                        DC1
> > |XXXXX|----|XXXXX|----|----------|----------|----------|XXXXXXXXXX|
> > 0GB   5GB  10GB  15GB 20GB       30GB       40GB       50GB       60GB
> 
> 
> To add another corner to the example, maybe map only part of DC1?

Maybe.  See below.


[snip]

> > @@ -500,6 +617,21 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
> >  	else
> >  		free_pmem_start = cxlds->pmem_res.start;
> >  
> > +	/*
> > +	 * Limit each decoder to a single DC region to map memory with
> > +	 * different DSMAS entry.

This prevents more than 1 region per DC partition (region).

> > +	 */
> > +	dc_index = dc_mode_to_region_index(cxled->mode);
> > +	if (dc_index >= 0) {
> > +		if (cxlds->dc_res[dc_index].child) {
> > +			dev_err(dev, "Cannot allocate DPA from DC Region: %d\n",
> > +				dc_index);
> > +			rc = -EINVAL;
> > +			goto out;
> > +		}
> > +		free_dc_start = cxlds->dc_res[dc_index].start;
> > +	}
> > +
> >  	if (cxled->mode == CXL_DECODER_RAM) {
> >  		start = free_ram_start;
> >  		avail = cxlds->ram_res.end - start + 1;
> > @@ -521,12 +653,38 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
> >  		else
> >  			skip_end = start - 1;
> >  		skip = skip_end - skip_start + 1;
> > +	} else if (cxl_decoder_mode_is_dc(cxled->mode)) {
> > +		resource_size_t skip_start, skip_end;
> > +
> > +		start = free_dc_start;
> > +		avail = cxlds->dc_res[dc_index].end - start + 1;
> > +		if ((resource_size(&cxlds->pmem_res) == 0) || !cxlds->pmem_res.child)
> > +			skip_start = free_ram_start;
> > +		else
> > +			skip_start = free_pmem_start;
> > +		/*
> > +		 * If any dc region is already mapped, then that allocation
> > +		 * already handled the RAM and PMEM skip.  Check for DC region
> > +		 * skip.
> > +		 */
> > +		for (int i = dc_index - 1; i >= 0 ; i--) {
> > +			if (cxlds->dc_res[i].child) {
> > +				skip_start = cxlds->dc_res[i].child->end + 1;
> > +				break;
> > +			}
> > +		}
> > +
> > +		skip_end = start - 1;
> > +		skip = skip_end - skip_start + 1;
> 
> I notice in the pmem equivalent there is a case for part of the region already mapped.
> Can that not happen for a DC region as well?

See above check.  Each DC region (partition) was to be associated with a
single DSMAS entry.  I'm unclear now why that decision was made.

It does not seem hard to add this though.  Do we really need that ability
considering dax devices are likely going to be the main boundry for users
of a DC region?

Ira