diff mbox series

[v7,24/28] cxl: add region flag for precluding a device memory to be used for dax

Message ID 20241209185429.54054-25-alejandro.lucero-palau@amd.com
State Superseded
Headers show
Series cxl: add type2 device basic support | expand

Commit Message

Lucero Palau, Alejandro Dec. 9, 2024, 6:54 p.m. UTC
From: Alejandro Lucero <alucerop@amd.com>

By definition a type2 cxl device will use the host managed memory for
specific functionality, therefore it should not be available to other
uses. However, a dax interface could be just good enough in some cases.

Add a flag to a cxl region for specifically state to not create a dax
device. Allow a Type2 driver to set that flag at region creation time.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
Reviewed-by: Zhi Wang <zhiw@nvidia.com>
---
 drivers/cxl/core/region.c | 10 +++++++++-
 drivers/cxl/cxl.h         |  3 +++
 drivers/cxl/cxlmem.h      |  3 ++-
 include/cxl/cxl.h         |  3 ++-
 4 files changed, 16 insertions(+), 3 deletions(-)

Comments

Edward Cree Dec. 11, 2024, 2:31 a.m. UTC | #1
On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> By definition a type2 cxl device will use the host managed memory for
> specific functionality, therefore it should not be available to other
> uses. However, a dax interface could be just good enough in some cases.
> 
> Add a flag to a cxl region for specifically state to not create a dax
> device. Allow a Type2 driver to set that flag at region creation time.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Zhi Wang <zhiw@nvidia.com>
> ---
>  drivers/cxl/core/region.c | 10 +++++++++-
>  drivers/cxl/cxl.h         |  3 +++
>  drivers/cxl/cxlmem.h      |  3 ++-
>  include/cxl/cxl.h         |  3 ++-
>  4 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b014f2fab789..b39086356d74 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd,
>   * cxl_region driver.
>   */
>  struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
> -				     struct cxl_endpoint_decoder *cxled)
> +				     struct cxl_endpoint_decoder *cxled,
> +				     bool no_dax)

Won't this break bisectability?  sfc won't build as of this commit
 because it tries to call cxl_create_region with the old signature.
You could do the whole dance of having an interim API during the
 conversion, but seems simpler just to reorder the patches so that
 the no_dax parameter is added first before the caller is introduced.
Alejandro Lucero Palau Dec. 11, 2024, 9:23 a.m. UTC | #2
On 12/11/24 02:31, Edward Cree wrote:
> On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> By definition a type2 cxl device will use the host managed memory for
>> specific functionality, therefore it should not be available to other
>> uses. However, a dax interface could be just good enough in some cases.
>>
>> Add a flag to a cxl region for specifically state to not create a dax
>> device. Allow a Type2 driver to set that flag at region creation time.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Reviewed-by: Zhi Wang <zhiw@nvidia.com>
>> ---
>>   drivers/cxl/core/region.c | 10 +++++++++-
>>   drivers/cxl/cxl.h         |  3 +++
>>   drivers/cxl/cxlmem.h      |  3 ++-
>>   include/cxl/cxl.h         |  3 ++-
>>   4 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index b014f2fab789..b39086356d74 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd,
>>    * cxl_region driver.
>>    */
>>   struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
>> -				     struct cxl_endpoint_decoder *cxled)
>> +				     struct cxl_endpoint_decoder *cxled,
>> +				     bool no_dax)
> Won't this break bisectability?  sfc won't build as of this commit
>   because it tries to call cxl_create_region with the old signature.
> You could do the whole dance of having an interim API during the
>   conversion, but seems simpler just to reorder the patches so that
>   the no_dax parameter is added first before the caller is introduced.


Oh. That's true. I wonder why the robot did not catch this! I thought it 
was building things after each patch in a patchset.

I will change the order for properly using this in the sfc driver.

Thanks!
Simon Horman Dec. 12, 2024, 6:44 p.m. UTC | #3
On Mon, Dec 09, 2024 at 06:54:25PM +0000, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> By definition a type2 cxl device will use the host managed memory for
> specific functionality, therefore it should not be available to other
> uses. However, a dax interface could be just good enough in some cases.
> 
> Add a flag to a cxl region for specifically state to not create a dax
> device. Allow a Type2 driver to set that flag at region creation time.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Zhi Wang <zhiw@nvidia.com>
> ---
>  drivers/cxl/core/region.c | 10 +++++++++-
>  drivers/cxl/cxl.h         |  3 +++
>  drivers/cxl/cxlmem.h      |  3 ++-
>  include/cxl/cxl.h         |  3 ++-
>  4 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b014f2fab789..b39086356d74 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd,
>   * cxl_region driver.
>   */
>  struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
> -				     struct cxl_endpoint_decoder *cxled)
> +				     struct cxl_endpoint_decoder *cxled,
> +				     bool no_dax)

nit: no_dax should be added to the Kernel doc for this function.

Also, I think you need to squash the following patch, which updates
the caller to use pass the extra argument, into this patch. Or otherwise
rework things slightly to avoid breaking bisection.

>  {
>  	struct cxl_region *cxlr;
>  

...
Alejandro Lucero Palau Dec. 13, 2024, 9:47 a.m. UTC | #4
On 12/12/24 18:44, Simon Horman wrote:
> On Mon, Dec 09, 2024 at 06:54:25PM +0000, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> By definition a type2 cxl device will use the host managed memory for
>> specific functionality, therefore it should not be available to other
>> uses. However, a dax interface could be just good enough in some cases.
>>
>> Add a flag to a cxl region for specifically state to not create a dax
>> device. Allow a Type2 driver to set that flag at region creation time.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Reviewed-by: Zhi Wang <zhiw@nvidia.com>
>> ---
>>   drivers/cxl/core/region.c | 10 +++++++++-
>>   drivers/cxl/cxl.h         |  3 +++
>>   drivers/cxl/cxlmem.h      |  3 ++-
>>   include/cxl/cxl.h         |  3 ++-
>>   4 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index b014f2fab789..b39086356d74 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd,
>>    * cxl_region driver.
>>    */
>>   struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
>> -				     struct cxl_endpoint_decoder *cxled)
>> +				     struct cxl_endpoint_decoder *cxled,
>> +				     bool no_dax)
> nit: no_dax should be added to the Kernel doc for this function.


Yes, I'll do.


>
> Also, I think you need to squash the following patch, which updates
> the caller to use pass the extra argument, into this patch. Or otherwise
> rework things slightly to avoid breaking bisection.


Correct. Ed raised this concern as well, and I'll change the patches 
order in v8 for avoiding the problem.

Thanks!


>>   {
>>   	struct cxl_region *cxlr;
>>   
> ...
>
Simon Horman Dec. 13, 2024, 10:23 a.m. UTC | #5
On Fri, Dec 13, 2024 at 09:47:42AM +0000, Alejandro Lucero Palau wrote:
> 
> On 12/12/24 18:44, Simon Horman wrote:
> > On Mon, Dec 09, 2024 at 06:54:25PM +0000, alejandro.lucero-palau@amd.com wrote:
> > > From: Alejandro Lucero <alucerop@amd.com>
> > > 
> > > By definition a type2 cxl device will use the host managed memory for
> > > specific functionality, therefore it should not be available to other
> > > uses. However, a dax interface could be just good enough in some cases.
> > > 
> > > Add a flag to a cxl region for specifically state to not create a dax
> > > device. Allow a Type2 driver to set that flag at region creation time.
> > > 
> > > Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> > > Reviewed-by: Zhi Wang <zhiw@nvidia.com>
> > > ---
> > >   drivers/cxl/core/region.c | 10 +++++++++-
> > >   drivers/cxl/cxl.h         |  3 +++
> > >   drivers/cxl/cxlmem.h      |  3 ++-
> > >   include/cxl/cxl.h         |  3 ++-
> > >   4 files changed, 16 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > > index b014f2fab789..b39086356d74 100644
> > > --- a/drivers/cxl/core/region.c
> > > +++ b/drivers/cxl/core/region.c
> > > @@ -3562,7 +3562,8 @@ __construct_new_region(struct cxl_root_decoder *cxlrd,
> > >    * cxl_region driver.
> > >    */
> > >   struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
> > > -				     struct cxl_endpoint_decoder *cxled)
> > > +				     struct cxl_endpoint_decoder *cxled,
> > > +				     bool no_dax)
> > nit: no_dax should be added to the Kernel doc for this function.
> 
> 
> Yes, I'll do.
> 
> 
> > 
> > Also, I think you need to squash the following patch, which updates
> > the caller to use pass the extra argument, into this patch. Or otherwise
> > rework things slightly to avoid breaking bisection.
> 
> 
> Correct. Ed raised this concern as well, and I'll change the patches order
> in v8 for avoiding the problem.

Thanks, and sorry for missing that Ed had already raised this.

Likewise, thanks for responding to my other feedback on this patchset.
diff mbox series

Patch

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index b014f2fab789..b39086356d74 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3562,7 +3562,8 @@  __construct_new_region(struct cxl_root_decoder *cxlrd,
  * cxl_region driver.
  */
 struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
-				     struct cxl_endpoint_decoder *cxled)
+				     struct cxl_endpoint_decoder *cxled,
+				     bool no_dax)
 {
 	struct cxl_region *cxlr;
 
@@ -3578,6 +3579,10 @@  struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
 		drop_region(cxlr);
 		return ERR_PTR(-ENODEV);
 	}
+
+	if (no_dax)
+		set_bit(CXL_REGION_F_NO_DAX, &cxlr->flags);
+
 	return cxlr;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_create_region, "CXL");
@@ -3713,6 +3718,9 @@  static int cxl_region_probe(struct device *dev)
 	if (rc)
 		return rc;
 
+	if (test_bit(CXL_REGION_F_NO_DAX, &cxlr->flags))
+		return 0;
+
 	switch (cxlr->mode) {
 	case CXL_DECODER_PMEM:
 		return devm_cxl_add_pmem_region(cxlr);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 57d6dda3fb4a..cc9e3d859fa6 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -521,6 +521,9 @@  struct cxl_region_params {
  */
 #define CXL_REGION_F_NEEDS_RESET 1
 
+/* Allow Type2 drivers to specify if a dax region should not be created. */
+#define CXL_REGION_F_NO_DAX 2
+
 /**
  * struct cxl_region - CXL region
  * @dev: This region's device
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 9d874f1cb3bf..712f25f494e0 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -875,5 +875,6 @@  struct seq_file;
 struct dentry *cxl_debugfs_create_dir(const char *dir);
 void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
 struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
-				     struct cxl_endpoint_decoder *cxled);
+				     struct cxl_endpoint_decoder *cxled,
+				     bool no_dax);
 #endif /* __CXL_MEM_H__ */
diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
index e0ea5b801a2e..14be26358f9c 100644
--- a/include/cxl/cxl.h
+++ b/include/cxl/cxl.h
@@ -61,7 +61,8 @@  struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_memdev *cxlmd,
 					     resource_size_t max);
 int cxl_dpa_free(struct cxl_endpoint_decoder *cxled);
 struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
-				     struct cxl_endpoint_decoder *cxled);
+				     struct cxl_endpoint_decoder *cxled,
+				     bool no_dax);
 
 int cxl_accel_region_detach(struct cxl_endpoint_decoder *cxled);
 #endif