diff mbox series

[v10,16/26] cxl: define a driver interface for DPA allocation

Message ID 20250205151950.25268-17-alucerop@amd.com (mailing list archive)
State Not Applicable
Headers show
Series cxl: add type2 device basic support | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Alejandro Lucero Palau Feb. 5, 2025, 3:19 p.m. UTC 
From: Alejandro Lucero <alucerop@amd.com>

Region creation involves finding available DPA (device-physical-address)
capacity to map into HPA (host-physical-address) space. Define an API,
cxl_request_dpa(), that tries to allocate the DPA memory the driver
requires to operate. The memory requested should not be bigger than the
max available HPA obtained previously with cxl_get_hpa_freespace.

Based on https://lore.kernel.org/linux-cxl/168592158743.1948938.7622563891193802610.stgit@dwillia2-xfh.jf.intel.com/

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/hdm.c | 83 ++++++++++++++++++++++++++++++++++++++++++
 include/cxl/cxl.h      |  4 ++
 2 files changed, 87 insertions(+)

Comments

kernel test robot Feb. 6, 2025, 7:11 p.m. UTC | #1 
Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 6cd498f39cf5e1475b567eb67a6fcf1ca3d67d46]

url:    https://github.com/intel-lab-lkp/linux/commits/alucerop-amd-com/cxl-make-memdev-creation-type-agnostic/20250205-233651
base:   6cd498f39cf5e1475b567eb67a6fcf1ca3d67d46
patch link:    https://lore.kernel.org/r/20250205151950.25268-17-alucerop%40amd.com
patch subject: [PATCH v10 16/26] cxl: define a driver interface for DPA allocation
config: i386-buildonly-randconfig-006-20250206 (https://download.01.org/0day-ci/archive/20250207/202502070213.8GNIAg8A-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250207/202502070213.8GNIAg8A-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502070213.8GNIAg8A-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/cxl/core/hdm.c:743: warning: Function parameter or struct member 'alloc' not described in 'cxl_request_dpa'
>> drivers/cxl/core/hdm.c:743: warning: Excess function parameter 'min' description in 'cxl_request_dpa'


vim +743 drivers/cxl/core/hdm.c

   722	
   723	/**
   724	 * cxl_request_dpa - search and reserve DPA given input constraints
   725	 * @cxlmd: memdev with an endpoint port with available decoders
   726	 * @is_ram: DPA operation mode (ram vs pmem)
   727	 * @min: the minimum amount of capacity the call needs
   728	 *
   729	 * Given that a region needs to allocate from limited HPA capacity it
   730	 * may be the case that a device has more mappable DPA capacity than
   731	 * available HPA. So, the expectation is that @min is a driver known
   732	 * value for how much capacity is needed, and @max is the limit of
   733	 * how much HPA space is available for a new region.
   734	 *
   735	 * Returns a pinned cxl_decoder with at least @min bytes of capacity
   736	 * reserved, or an error pointer. The caller is also expected to own the
   737	 * lifetime of the memdev registration associated with the endpoint to
   738	 * pin the decoder registered as well.
   739	 */
   740	struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_memdev *cxlmd,
   741						     bool is_ram,
   742						     resource_size_t alloc)
 > 743	{
   744		struct cxl_port *endpoint = cxlmd->endpoint;
   745		struct cxl_endpoint_decoder *cxled;
   746		enum cxl_partition_mode mode;
   747		struct device *cxled_dev;
   748		int rc;
   749	
   750		if (!IS_ALIGNED(alloc, SZ_256M))
   751			return ERR_PTR(-EINVAL);
   752	
   753		down_read(&cxl_dpa_rwsem);
   754		cxled_dev = device_find_child(&endpoint->dev, NULL, find_free_decoder);
   755		up_read(&cxl_dpa_rwsem);
   756	
   757		if (!cxled_dev)
   758			return ERR_PTR(-ENXIO);
   759	
   760		cxled = to_cxl_endpoint_decoder(cxled_dev);
   761	
   762		if (!cxled) {
   763			rc = -ENODEV;
   764			goto err;
   765		}
   766	
   767		if (is_ram)
   768			mode = CXL_PARTMODE_RAM;
   769		else
   770			mode = CXL_PARTMODE_PMEM;
   771	
   772		rc = cxl_dpa_set_part(cxled, mode);
   773		if (rc)
   774			goto err;
   775	
   776		rc = cxl_dpa_alloc(cxled, alloc);
   777		if (rc)
   778			goto err;
   779	
   780		return cxled;
   781	err:
   782		put_device(cxled_dev);
   783		return ERR_PTR(rc);
   784	}
   785	EXPORT_SYMBOL_NS_GPL(cxl_request_dpa, "CXL");
   786
Simon Horman Feb. 7, 2025, 1:46 p.m. UTC | #2 
On Wed, Feb 05, 2025 at 03:19:40PM +0000, alucerop@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Region creation involves finding available DPA (device-physical-address)
> capacity to map into HPA (host-physical-address) space. Define an API,
> cxl_request_dpa(), that tries to allocate the DPA memory the driver
> requires to operate. The memory requested should not be bigger than the
> max available HPA obtained previously with cxl_get_hpa_freespace.
> 
> Based on https://lore.kernel.org/linux-cxl/168592158743.1948938.7622563891193802610.stgit@dwillia2-xfh.jf.intel.com/
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/hdm.c | 83 ++++++++++++++++++++++++++++++++++++++++++
>  include/cxl/cxl.h      |  4 ++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index af025da81fa2..cec2c7dcaf3a 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -3,6 +3,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/device.h>
>  #include <linux/delay.h>
> +#include <cxl/cxl.h>

Hi Alejandro,

I think that linux/range.h should be included in cxl.h, or if not here.
This is because on allmodconfigs for both arm and arm64 I see:

In file included from drivers/cxl/core/hdm.c:6:
./include/cxl/cxl.h:67:16: error: field has incomplete type 'struct range'
   67 |                 struct range range;
      |                              ^
./include/linux/memory_hotplug.h:247:8: note: forward declaration of 'struct range'
  247 | struct range arch_get_mappable_range(void);
      |        ^
1 error generated.

...
Alejandro Lucero Palau Feb. 17, 2025, 2:08 p.m. UTC | #3 
On 2/7/25 13:46, Simon Horman wrote:
> On Wed, Feb 05, 2025 at 03:19:40PM +0000, alucerop@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Region creation involves finding available DPA (device-physical-address)
>> capacity to map into HPA (host-physical-address) space. Define an API,
>> cxl_request_dpa(), that tries to allocate the DPA memory the driver
>> requires to operate. The memory requested should not be bigger than the
>> max available HPA obtained previously with cxl_get_hpa_freespace.
>>
>> Based on https://lore.kernel.org/linux-cxl/168592158743.1948938.7622563891193802610.stgit@dwillia2-xfh.jf.intel.com/
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/hdm.c | 83 ++++++++++++++++++++++++++++++++++++++++++
>>   include/cxl/cxl.h      |  4 ++
>>   2 files changed, 87 insertions(+)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index af025da81fa2..cec2c7dcaf3a 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -3,6 +3,7 @@
>>   #include <linux/seq_file.h>
>>   #include <linux/device.h>
>>   #include <linux/delay.h>
>> +#include <cxl/cxl.h>
> Hi Alejandro,


Hi Simon,


> I think that linux/range.h should be included in cxl.h, or if not here.
> This is because on allmodconfigs for both arm and arm64 I see:
>
> In file included from drivers/cxl/core/hdm.c:6:
> ./include/cxl/cxl.h:67:16: error: field has incomplete type 'struct range'
>     67 |                 struct range range;
>        |                              ^
> ./include/linux/memory_hotplug.h:247:8: note: forward declaration of 'struct range'
>    247 | struct range arch_get_mappable_range(void);
>        |        ^
> 1 error generated.
>
> ...


I do not understand then why the robot does not trigger an issue when 
building this code for those archs.

And where does that second struct range reference in memory_hotplug.h 
come from? Is that related to cxl.h?
Simon Horman Feb. 18, 2025, 1:34 p.m. UTC | #4 
On Mon, Feb 17, 2025 at 02:08:28PM +0000, Alejandro Lucero Palau wrote:
> 
> On 2/7/25 13:46, Simon Horman wrote:
> > On Wed, Feb 05, 2025 at 03:19:40PM +0000, alucerop@amd.com wrote:
> > > From: Alejandro Lucero <alucerop@amd.com>
> > > 
> > > Region creation involves finding available DPA (device-physical-address)
> > > capacity to map into HPA (host-physical-address) space. Define an API,
> > > cxl_request_dpa(), that tries to allocate the DPA memory the driver
> > > requires to operate. The memory requested should not be bigger than the
> > > max available HPA obtained previously with cxl_get_hpa_freespace.
> > > 
> > > Based on https://lore.kernel.org/linux-cxl/168592158743.1948938.7622563891193802610.stgit@dwillia2-xfh.jf.intel.com/
> > > 
> > > Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> > > ---
> > >   drivers/cxl/core/hdm.c | 83 ++++++++++++++++++++++++++++++++++++++++++
> > >   include/cxl/cxl.h      |  4 ++
> > >   2 files changed, 87 insertions(+)
> > > 
> > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > index af025da81fa2..cec2c7dcaf3a 100644
> > > --- a/drivers/cxl/core/hdm.c
> > > +++ b/drivers/cxl/core/hdm.c
> > > @@ -3,6 +3,7 @@
> > >   #include <linux/seq_file.h>
> > >   #include <linux/device.h>
> > >   #include <linux/delay.h>
> > > +#include <cxl/cxl.h>
> > Hi Alejandro,
> 
> 
> Hi Simon,
> 
> 
> > I think that linux/range.h should be included in cxl.h, or if not here.
> > This is because on allmodconfigs for both arm and arm64 I see:
> > 
> > In file included from drivers/cxl/core/hdm.c:6:
> > ./include/cxl/cxl.h:67:16: error: field has incomplete type 'struct range'
> >     67 |                 struct range range;
> >        |                              ^
> > ./include/linux/memory_hotplug.h:247:8: note: forward declaration of 'struct range'
> >    247 | struct range arch_get_mappable_range(void);
> >        |        ^
> > 1 error generated.
> > 
> > ...
> 
> 
> I do not understand then why the robot does not trigger an issue when
> building this code for those archs.
> 
> And where does that second struct range reference in memory_hotplug.h come
> from? Is that related to cxl.h?

Thanks, let me try to reproduce this again.
Simon Horman Feb. 18, 2025, 2:09 p.m. UTC | #5 
On Tue, Feb 18, 2025 at 01:34:59PM +0000, Simon Horman wrote:
> On Mon, Feb 17, 2025 at 02:08:28PM +0000, Alejandro Lucero Palau wrote:
> > 
> > On 2/7/25 13:46, Simon Horman wrote:
> > > On Wed, Feb 05, 2025 at 03:19:40PM +0000, alucerop@amd.com wrote:
> > > > From: Alejandro Lucero <alucerop@amd.com>
> > > > 
> > > > Region creation involves finding available DPA (device-physical-address)
> > > > capacity to map into HPA (host-physical-address) space. Define an API,
> > > > cxl_request_dpa(), that tries to allocate the DPA memory the driver
> > > > requires to operate. The memory requested should not be bigger than the
> > > > max available HPA obtained previously with cxl_get_hpa_freespace.
> > > > 
> > > > Based on https://lore.kernel.org/linux-cxl/168592158743.1948938.7622563891193802610.stgit@dwillia2-xfh.jf.intel.com/
> > > > 
> > > > Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> > > > ---
> > > >   drivers/cxl/core/hdm.c | 83 ++++++++++++++++++++++++++++++++++++++++++
> > > >   include/cxl/cxl.h      |  4 ++
> > > >   2 files changed, 87 insertions(+)
> > > > 
> > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > > index af025da81fa2..cec2c7dcaf3a 100644
> > > > --- a/drivers/cxl/core/hdm.c
> > > > +++ b/drivers/cxl/core/hdm.c
> > > > @@ -3,6 +3,7 @@
> > > >   #include <linux/seq_file.h>
> > > >   #include <linux/device.h>
> > > >   #include <linux/delay.h>
> > > > +#include <cxl/cxl.h>
> > > Hi Alejandro,
> > 
> > 
> > Hi Simon,
> > 
> > 
> > > I think that linux/range.h should be included in cxl.h, or if not here.
> > > This is because on allmodconfigs for both arm and arm64 I see:
> > > 
> > > In file included from drivers/cxl/core/hdm.c:6:
> > > ./include/cxl/cxl.h:67:16: error: field has incomplete type 'struct range'
> > >     67 |                 struct range range;
> > >        |                              ^
> > > ./include/linux/memory_hotplug.h:247:8: note: forward declaration of 'struct range'
> > >    247 | struct range arch_get_mappable_range(void);
> > >        |        ^
> > > 1 error generated.
> > > 
> > > ...
> > 
> > 
> > I do not understand then why the robot does not trigger an issue when
> > building this code for those archs.
> > 
> > And where does that second struct range reference in memory_hotplug.h come
> > from? Is that related to cxl.h?
> 
> Thanks, let me try to reproduce this again.

Hi Alejandro,

I tried testing this with an allmodconfig build for arm64 [*].

And this time I see this manifesting slightly differently.  I can follow-up
on why it is different (probably I messed something up when first reporting
the issue). But I am certainly seeing an issue there today.

...
  CC [M]  drivers/cxl/core/hdm.o
In file included from drivers/cxl/core/hdm.c:6:
./include/cxl/cxl.h:67:30: error: field 'range' has incomplete type
   67 |                 struct range range;
      |                              ^~~~~
...

[*] This is with patches 1 - 16 of this series applied on top of next-20250205.

    I am using the GCC 14.2.0 toolchain from [1] to cross compile on x86_64.
    Like this:

    PATH=/tmp/gcc-14.2.0-nolibc/aarch64-linux/bin:$PATH
    ARCH=arm64 CROSS_COMPILE=aarch64-linux- make allmodconfig
    ARCH=arm64 CROSS_COMPILE=aarch64-linux- make

[1] https://mirrors.edge.kernel.org/pub/tools/crosstool/
diff mbox series

Patch

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index af025da81fa2..cec2c7dcaf3a 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -3,6 +3,7 @@ 
 #include <linux/seq_file.h>
 #include <linux/device.h>
 #include <linux/delay.h>
+#include <cxl/cxl.h>
 
 #include "cxlmem.h"
 #include "core.h"
@@ -587,6 +588,7 @@  int cxl_dpa_free(struct cxl_endpoint_decoder *cxled)
 	up_write(&cxl_dpa_rwsem);
 	return rc;
 }
+EXPORT_SYMBOL_NS_GPL(cxl_dpa_free, "CXL");
 
 int cxl_dpa_set_part(struct cxl_endpoint_decoder *cxled,
 		     enum cxl_partition_mode mode)
@@ -701,6 +703,87 @@  int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
 	return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
 }
 
+static int find_free_decoder(struct device *dev, const void *data)
+{
+	struct cxl_endpoint_decoder *cxled;
+	struct cxl_port *port;
+
+	if (!is_endpoint_decoder(dev))
+		return 0;
+
+	cxled = to_cxl_endpoint_decoder(dev);
+	port = cxled_to_port(cxled);
+
+	if (cxled->cxld.id != port->hdm_end + 1)
+		return 0;
+
+	return 1;
+}
+
+/**
+ * cxl_request_dpa - search and reserve DPA given input constraints
+ * @cxlmd: memdev with an endpoint port with available decoders
+ * @is_ram: DPA operation mode (ram vs pmem)
+ * @min: the minimum amount of capacity the call needs
+ *
+ * Given that a region needs to allocate from limited HPA capacity it
+ * may be the case that a device has more mappable DPA capacity than
+ * available HPA. So, the expectation is that @min is a driver known
+ * value for how much capacity is needed, and @max is the limit of
+ * how much HPA space is available for a new region.
+ *
+ * Returns a pinned cxl_decoder with at least @min bytes of capacity
+ * reserved, or an error pointer. The caller is also expected to own the
+ * lifetime of the memdev registration associated with the endpoint to
+ * pin the decoder registered as well.
+ */
+struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_memdev *cxlmd,
+					     bool is_ram,
+					     resource_size_t alloc)
+{
+	struct cxl_port *endpoint = cxlmd->endpoint;
+	struct cxl_endpoint_decoder *cxled;
+	enum cxl_partition_mode mode;
+	struct device *cxled_dev;
+	int rc;
+
+	if (!IS_ALIGNED(alloc, SZ_256M))
+		return ERR_PTR(-EINVAL);
+
+	down_read(&cxl_dpa_rwsem);
+	cxled_dev = device_find_child(&endpoint->dev, NULL, find_free_decoder);
+	up_read(&cxl_dpa_rwsem);
+
+	if (!cxled_dev)
+		return ERR_PTR(-ENXIO);
+
+	cxled = to_cxl_endpoint_decoder(cxled_dev);
+
+	if (!cxled) {
+		rc = -ENODEV;
+		goto err;
+	}
+
+	if (is_ram)
+		mode = CXL_PARTMODE_RAM;
+	else
+		mode = CXL_PARTMODE_PMEM;
+
+	rc = cxl_dpa_set_part(cxled, mode);
+	if (rc)
+		goto err;
+
+	rc = cxl_dpa_alloc(cxled, alloc);
+	if (rc)
+		goto err;
+
+	return cxled;
+err:
+	put_device(cxled_dev);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_request_dpa, "CXL");
+
 static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)
 {
 	u16 eig;
diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
index 3b72dc7ce8cf..3fa390b10089 100644
--- a/include/cxl/cxl.h
+++ b/include/cxl/cxl.h
@@ -90,4 +90,8 @@  struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_memdev *cxlmd,
 					       unsigned long flags,
 					       resource_size_t *max);
 void cxl_put_root_decoder(struct cxl_root_decoder *cxlrd);
+struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_memdev *cxlmd,
+					     bool is_ram,
+					     resource_size_t alloc);
+int cxl_dpa_free(struct cxl_endpoint_decoder *cxled);
 #endif