Message ID | 20241118164434.7551-25-alejandro.lucero-palau@amd.com |
---|---|
State | Superseded |
Headers | show |
Series | cxl: add type2 device basic support | expand |
On Mon, 18 Nov 2024 16:44:31 +0000 <alejandro.lucero-palau@amd.com> wrote: Minor comment: maybe no_dax would be better than "avoid dax". > 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> > --- > 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 70549d42c2e3..eff3ad788077 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3558,7 +3558,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 avoid_dax) > { > struct cxl_region *cxlr; > > @@ -3574,6 +3575,10 @@ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, > drop_region(cxlr); > return ERR_PTR(-ENODEV); > } > + > + if (avoid_dax) > + set_bit(CXL_REGION_F_AVOID_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) > case CXL_DECODER_PMEM: > return devm_cxl_add_pmem_region(cxlr); > case CXL_DECODER_RAM: > + if (test_bit(CXL_REGION_F_AVOID_DAX, &cxlr->flags)) > + return 0; > + > /* > * The region can not be manged by CXL if any portion of > * it is already online as 'System RAM' > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 1e0e797b9303..ee3385db5663 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -512,6 +512,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_AVOID_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..cc2e2a295f3d 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 avoid_dax); > #endif /* __CXL_MEM_H__ */ > diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h > index d295af4f5f9e..2a8ebabfc1dd 100644 > --- a/include/cxl/cxl.h > +++ b/include/cxl/cxl.h > @@ -73,7 +73,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 avoid_dax); > > int cxl_accel_region_detach(struct cxl_endpoint_decoder *cxled); > #endif
On 11/19/24 20:39, Zhi Wang wrote: > On Mon, 18 Nov 2024 16:44:31 +0000 > <alejandro.lucero-palau@amd.com> wrote: > > Minor comment: maybe no_dax would be better than "avoid dax". It makes sense. I'll do the change. Thanks >> 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> >> --- >> 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 70549d42c2e3..eff3ad788077 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -3558,7 +3558,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 avoid_dax) >> { >> struct cxl_region *cxlr; >> >> @@ -3574,6 +3575,10 @@ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, >> drop_region(cxlr); >> return ERR_PTR(-ENODEV); >> } >> + >> + if (avoid_dax) >> + set_bit(CXL_REGION_F_AVOID_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) >> case CXL_DECODER_PMEM: >> return devm_cxl_add_pmem_region(cxlr); >> case CXL_DECODER_RAM: >> + if (test_bit(CXL_REGION_F_AVOID_DAX, &cxlr->flags)) >> + return 0; >> + >> /* >> * The region can not be manged by CXL if any portion of >> * it is already online as 'System RAM' >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index 1e0e797b9303..ee3385db5663 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -512,6 +512,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_AVOID_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..cc2e2a295f3d 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 avoid_dax); >> #endif /* __CXL_MEM_H__ */ >> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h >> index d295af4f5f9e..2a8ebabfc1dd 100644 >> --- a/include/cxl/cxl.h >> +++ b/include/cxl/cxl.h >> @@ -73,7 +73,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 avoid_dax); >> >> int cxl_accel_region_detach(struct cxl_endpoint_decoder *cxled); >> #endif
On 11/18/24 10:44 AM, 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> > --- > 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 70549d42c2e3..eff3ad788077 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3558,7 +3558,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 avoid_dax) > { > struct cxl_region *cxlr; > > @@ -3574,6 +3575,10 @@ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, > drop_region(cxlr); > return ERR_PTR(-ENODEV); > } > + > + if (avoid_dax) > + set_bit(CXL_REGION_F_AVOID_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) > case CXL_DECODER_PMEM: > return devm_cxl_add_pmem_region(cxlr); > case CXL_DECODER_RAM: > + if (test_bit(CXL_REGION_F_AVOID_DAX, &cxlr->flags)) > + return 0; I think it's possible for a type 2 device to have pmem as well, and it looks like these are the only two options at the moment, so I would just move this check to before the switch statement. > + > /* > * The region can not be manged by CXL if any portion of > * it is already online as 'System RAM' > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 1e0e797b9303..ee3385db5663 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -512,6 +512,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_AVOID_DAX 2 > + I would like to see flags such that the device could choose the region type (system ram, device-dax, or none). I think that adding the ability for device-dax would add a patch or two, so that may be a good follow up patch. > /** > * struct cxl_region - CXL region > * @dev: This region's device > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index 9d874f1cb3bf..cc2e2a295f3d 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 avoid_dax); > #endif /* __CXL_MEM_H__ */ > diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h > index d295af4f5f9e..2a8ebabfc1dd 100644 > --- a/include/cxl/cxl.h > +++ b/include/cxl/cxl.h > @@ -73,7 +73,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 avoid_dax); > > int cxl_accel_region_detach(struct cxl_endpoint_decoder *cxled); > #endif
On 11/22/24 20:46, Ben Cheatham wrote: > On 11/18/24 10:44 AM, 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> >> --- >> 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 70549d42c2e3..eff3ad788077 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -3558,7 +3558,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 avoid_dax) >> { >> struct cxl_region *cxlr; >> >> @@ -3574,6 +3575,10 @@ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, >> drop_region(cxlr); >> return ERR_PTR(-ENODEV); >> } >> + >> + if (avoid_dax) >> + set_bit(CXL_REGION_F_AVOID_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) >> case CXL_DECODER_PMEM: >> return devm_cxl_add_pmem_region(cxlr); >> case CXL_DECODER_RAM: >> + if (test_bit(CXL_REGION_F_AVOID_DAX, &cxlr->flags)) >> + return 0; > I think it's possible for a type 2 device to have pmem as well, and > it looks like these are the only two options at the moment, so I would > just move this check to before the switch statement. Yes, that was also suggested in previous patchsets and I forgot. I'll do it for v6. >> + >> /* >> * The region can not be manged by CXL if any portion of >> * it is already online as 'System RAM' >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index 1e0e797b9303..ee3385db5663 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -512,6 +512,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_AVOID_DAX 2 >> + > I would like to see flags such that the device could choose the region type > (system ram, device-dax, or none). I think that adding the ability > for device-dax would add a patch or two, so that may be a good follow up > patch. Not sure the system ram option makes sense when this code can be executed. Anyway, as you say, let's leave this for a follow up. Thanks >> /** >> * struct cxl_region - CXL region >> * @dev: This region's device >> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h >> index 9d874f1cb3bf..cc2e2a295f3d 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 avoid_dax); >> #endif /* __CXL_MEM_H__ */ >> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h >> index d295af4f5f9e..2a8ebabfc1dd 100644 >> --- a/include/cxl/cxl.h >> +++ b/include/cxl/cxl.h >> @@ -73,7 +73,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 avoid_dax); >> >> int cxl_accel_region_detach(struct cxl_endpoint_decoder *cxled); >> #endif
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 70549d42c2e3..eff3ad788077 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -3558,7 +3558,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 avoid_dax) { struct cxl_region *cxlr; @@ -3574,6 +3575,10 @@ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, drop_region(cxlr); return ERR_PTR(-ENODEV); } + + if (avoid_dax) + set_bit(CXL_REGION_F_AVOID_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) case CXL_DECODER_PMEM: return devm_cxl_add_pmem_region(cxlr); case CXL_DECODER_RAM: + if (test_bit(CXL_REGION_F_AVOID_DAX, &cxlr->flags)) + return 0; + /* * The region can not be manged by CXL if any portion of * it is already online as 'System RAM' diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 1e0e797b9303..ee3385db5663 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -512,6 +512,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_AVOID_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..cc2e2a295f3d 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 avoid_dax); #endif /* __CXL_MEM_H__ */ diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h index d295af4f5f9e..2a8ebabfc1dd 100644 --- a/include/cxl/cxl.h +++ b/include/cxl/cxl.h @@ -73,7 +73,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 avoid_dax); int cxl_accel_region_detach(struct cxl_endpoint_decoder *cxled); #endif