| Message ID | 20240729084611.502889-3-ying.huang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: Preparation of type2 accelerators support | expand |
On Mon, Jul 29, 2024 at 04:46:10PM +0800, Ying Huang wrote: > Now, the target type of region is hard-coded to HOSTONLYMEM, because > only type3 expanders are supported. To support type2 accelerators, > set the target type of region root decoder with that of the root > decoder. Hi Ying, If the target type of a region is always the same as it's root decoder, (is it?) why do we store it as an attribute of the region. Can we look it up when needed? A bit more below - > > Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Cc: Davidlohr Bueso <dave@stgolabs.net> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Alison Schofield <alison.schofield@intel.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Alejandro Lucero <alucerop@amd.com> > --- > drivers/cxl/core/region.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 21ad5f242875..9a483c8a32fd 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2545,7 +2545,8 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, > return ERR_PTR(-EBUSY); > } > > - return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM); > + return devm_cxl_add_region(cxlrd, id, mode, > + cxlrd->cxlsd.cxld.target_type); > } Passing the 'cxlrd' and then a piece of the cxlrd (.target_type) looks redundant. -- Alison > > static ssize_t create_pmem_region_store(struct device *dev, > -- > 2.39.2 >
Alison Schofield <alison.schofield@intel.com> writes: > On Mon, Jul 29, 2024 at 04:46:10PM +0800, Ying Huang wrote: >> Now, the target type of region is hard-coded to HOSTONLYMEM, because >> only type3 expanders are supported. To support type2 accelerators, >> set the target type of region root decoder with that of the root >> decoder. > > Hi Ying, > > If the target type of a region is always the same as it's root decoder, > (is it?) IIUC, it is. Do you know when they may be different? > why do we store it as an attribute of the region. Can we look > it up when needed? Yes. This is possible via to_cxl_root_decoder(). It's just a little inconvenient. > A bit more below - > >> >> Signed-off-by: "Huang, Ying" <ying.huang@intel.com> >> Suggested-by: Dan Williams <dan.j.williams@intel.com> >> Cc: Davidlohr Bueso <dave@stgolabs.net> >> Cc: Jonathan Cameron <jonathan.cameron@huawei.com> >> Cc: Dave Jiang <dave.jiang@intel.com> >> Cc: Alison Schofield <alison.schofield@intel.com> >> Cc: Vishal Verma <vishal.l.verma@intel.com> >> Cc: Ira Weiny <ira.weiny@intel.com> >> Cc: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/cxl/core/region.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index 21ad5f242875..9a483c8a32fd 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -2545,7 +2545,8 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, >> return ERR_PTR(-EBUSY); >> } >> >> - return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM); >> + return devm_cxl_add_region(cxlrd, id, mode, >> + cxlrd->cxlsd.cxld.target_type); >> } > > Passing the 'cxlrd' and then a piece of the cxlrd (.target_type) looks > redundant. Yes. We can remove the parameter. Will change this if we still need cxlr->type. Thanks! -- Best Regards, Huang, Ying > > -- Alison > >> >> static ssize_t create_pmem_region_store(struct device *dev, >> -- >> 2.39.2 >>
On Thu, 01 Aug 2024 14:28:55 +0800 "Huang, Ying" <ying.huang@intel.com> wrote: > Alison Schofield <alison.schofield@intel.com> writes: > > > On Mon, Jul 29, 2024 at 04:46:10PM +0800, Ying Huang wrote: > >> Now, the target type of region is hard-coded to HOSTONLYMEM, because > >> only type3 expanders are supported. To support type2 accelerators, > >> set the target type of region root decoder with that of the root > >> decoder. > > > > Hi Ying, > > > > If the target type of a region is always the same as it's root decoder, > > (is it?) > > IIUC, it is. Do you know when they may be different? Root decoder (CFMW I think) allows both and target device only one. More likely when it's HDM-DB / HDM-H though I think than HDM-D / HDM-H. A host would do this because it has simple address routing (maybe a single root complex) and doesn't want to pay the HPA space cost of providing separate regions, so decisions on protocol etc derived from RC HDM decoder programming, not the fixed bit in in the host. Note for those not so buried in CXL terms, HDM-DB includes back invalidate memory devices (for P2P or hardware coherent sharing), HDM-H is simpler memory devices that don't support back invalidate. Jonathan > > > why do we store it as an attribute of the region. Can we look > > it up when needed? > > Yes. This is possible via to_cxl_root_decoder(). It's just > a little inconvenient. > > > A bit more below - > > > >> > >> Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > >> Suggested-by: Dan Williams <dan.j.williams@intel.com> > >> Cc: Davidlohr Bueso <dave@stgolabs.net> > >> Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > >> Cc: Dave Jiang <dave.jiang@intel.com> > >> Cc: Alison Schofield <alison.schofield@intel.com> > >> Cc: Vishal Verma <vishal.l.verma@intel.com> > >> Cc: Ira Weiny <ira.weiny@intel.com> > >> Cc: Alejandro Lucero <alucerop@amd.com> > >> --- > >> drivers/cxl/core/region.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > >> index 21ad5f242875..9a483c8a32fd 100644 > >> --- a/drivers/cxl/core/region.c > >> +++ b/drivers/cxl/core/region.c > >> @@ -2545,7 +2545,8 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, > >> return ERR_PTR(-EBUSY); > >> } > >> > >> - return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM); > >> + return devm_cxl_add_region(cxlrd, id, mode, > >> + cxlrd->cxlsd.cxld.target_type); > >> } > > > > Passing the 'cxlrd' and then a piece of the cxlrd (.target_type) looks > > redundant. > > Yes. We can remove the parameter. Will change this if we still need > cxlr->type. Thanks! > > -- > Best Regards, > Huang, Ying > > > > > -- Alison > > > >> > >> static ssize_t create_pmem_region_store(struct device *dev, > >> -- > >> 2.39.2 > >> > >
On Mon, Jul 29, 2024 at 04:46:10PM +0800, Huang Ying wrote: > Now, the target type of region is hard-coded to HOSTONLYMEM, because > only type3 expanders are supported. To support type2 accelerators, > set the target type of region root decoder with that of the root > decoder. > > Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Cc: Davidlohr Bueso <dave@stgolabs.net> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Alison Schofield <alison.schofield@intel.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Alejandro Lucero <alucerop@amd.com> > --- Reviewed-by: Fan Ni <fan.ni@samsung.com> > drivers/cxl/core/region.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 21ad5f242875..9a483c8a32fd 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2545,7 +2545,8 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, > return ERR_PTR(-EBUSY); > } > > - return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM); > + return devm_cxl_add_region(cxlrd, id, mode, > + cxlrd->cxlsd.cxld.target_type); > } > > static ssize_t create_pmem_region_store(struct device *dev, > -- > 2.39.2 >
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 21ad5f242875..9a483c8a32fd 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -2545,7 +2545,8 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, return ERR_PTR(-EBUSY); } - return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM); + return devm_cxl_add_region(cxlrd, id, mode, + cxlrd->cxlsd.cxld.target_type); } static ssize_t create_pmem_region_store(struct device *dev,
Now, the target type of region is hard-coded to HOSTONLYMEM, because only type3 expanders are supported. To support type2 accelerators, set the target type of region root decoder with that of the root decoder. Signed-off-by: "Huang, Ying" <ying.huang@intel.com> Suggested-by: Dan Williams <dan.j.williams@intel.com> Cc: Davidlohr Bueso <dave@stgolabs.net> Cc: Jonathan Cameron <jonathan.cameron@huawei.com> Cc: Dave Jiang <dave.jiang@intel.com> Cc: Alison Schofield <alison.schofield@intel.com> Cc: Vishal Verma <vishal.l.verma@intel.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Alejandro Lucero <alucerop@amd.com> --- drivers/cxl/core/region.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)