Current kernel hard-codes the type of region to type 3 expander now,
because this is the only supported device type. As a preparation to
support type 2 accelerators, the patch sets the type of region to that
of the first endpoint. Then, the patch checks whether the type of
region is same as the type of other endpoints of the region. Because
what we really need is to make sure the type of all endpoints of a
region is same. And, the patch lets expander/accelerator device
drivers specify the target type of endpoint devices via struct
cxl_dev_state.
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Cc: 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>
Cc: Ben Cheatham <benjamin.cheatham@amd.com>
---
drivers/cxl/acpi.c | 1 -
drivers/cxl/core/hdm.c | 28 +++++++++++++---------------
drivers/cxl/core/port.c | 2 ++
drivers/cxl/core/region.c | 13 +++++++------
drivers/cxl/cxl.h | 1 +
5 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 21486e471305..29c2a44b122c 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -382,7 +382,6 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
cxld = &cxlrd->cxlsd.cxld;
cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
- cxld->target_type = CXL_DECODER_EXPANDER;
cxld->hpa_range = (struct range) {
.start = cfmws->base_hpa,
.end = cfmws->base_hpa + cfmws->window_size - 1,
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 478fb6691759..c9accf8be71f 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -841,18 +841,25 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
.end = base + size - 1,
};
+ if (cxled) {
+ struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+ struct cxl_dev_state *cxlds = cxlmd->cxlds;
+
+ if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
+ cxld->target_type = CXL_DECODER_EXPANDER;
+ else
+ cxld->target_type = CXL_DECODER_ACCEL;
+ }
+
/* decoders are enabled if committed */
if (committed) {
cxld->flags |= CXL_DECODER_F_ENABLE;
if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
cxld->flags |= CXL_DECODER_F_LOCK;
- if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) {
- cxld->target_type = CXL_DECODER_EXPANDER;
+ if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl))
cxld->coherence = CXL_DECODER_HOSTONLYCOH;
- } else {
- cxld->target_type = CXL_DECODER_ACCEL;
+ else
cxld->coherence = CXL_DECODER_DEVCOH;
- }
guard(rwsem_write)(&cxl_region_rwsem);
if (cxld->id != cxl_num_decoders_committed(port)) {
@@ -874,21 +881,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
struct cxl_dev_state *cxlds = cxlmd->cxlds;
- /*
- * Default by devtype until a device arrives that needs
- * more precision.
- */
- if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
- cxld->target_type = CXL_DECODER_EXPANDER;
- else
- cxld->target_type = CXL_DECODER_ACCEL;
if (cxlds->coherence == CXL_DEVCOH_HOSTONLY)
cxld->coherence = CXL_DECODER_HOSTONLYCOH;
else
cxld->coherence = CXL_DECODER_DEVCOH;
} else {
- /* To be overridden by region type/coherence at commit time */
- cxld->target_type = CXL_DECODER_EXPANDER;
+ /* To be overridden by region coherence at commit time */
cxld->coherence = CXL_DECODER_HOSTONLYCOH;
}
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 9ebbffcea26a..d1bc6aed6509 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev,
return sysfs_emit(buf, "accelerator\n");
case CXL_DECODER_EXPANDER:
return sysfs_emit(buf, "expander\n");
+ default:
+ break;
}
return -ENXIO;
}
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 21b877d8582f..d709738ada61 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
return -ENXIO;
}
- if (cxled->cxld.target_type != cxlr->type) {
+ /* Set the type of region to that of the first endpoint */
+ if (cxlr->type == CXL_DECODER_INVALID) {
+ cxlr->type = cxled->cxld.target_type;
+ } else if (cxled->cxld.target_type != cxlr->type) {
dev_dbg(&cxlr->dev, "%s:%s type mismatch: %d vs %d\n",
dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
cxled->cxld.target_type, cxlr->type);
@@ -2476,7 +2479,6 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb,
* @cxlrd: root decoder
* @id: memregion id to create, or memregion_free() on failure
* @mode: mode for the endpoint decoders of this region
- * @type: select whether this is an expander or accelerator (type-2 or type-3)
*
* This is the second step of region initialization. Regions exist within an
* address space which is mapped by a @cxlrd.
@@ -2486,8 +2488,7 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb,
*/
static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
int id,
- enum cxl_decoder_mode mode,
- enum cxl_decoder_type type)
+ enum cxl_decoder_mode mode)
{
struct cxl_port *port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);
struct cxl_region *cxlr;
@@ -2498,7 +2499,7 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
if (IS_ERR(cxlr))
return cxlr;
cxlr->mode = mode;
- cxlr->type = type;
+ cxlr->type = CXL_DECODER_INVALID;
dev = &cxlr->dev;
rc = dev_set_name(dev, "region%d", id);
@@ -2562,7 +2563,7 @@ 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_EXPANDER);
+ return devm_cxl_add_region(cxlrd, id, mode);
}
static ssize_t create_pmem_region_store(struct device *dev,
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 1927a1849d82..f4cbe5c292ea 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -324,6 +324,7 @@ resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
#define CXL_DECODER_F_MASK GENMASK(5, 0)
enum cxl_decoder_type {
+ CXL_DECODER_INVALID = 0,
CXL_DECODER_ACCEL = 2,
CXL_DECODER_EXPANDER = 3,
};
--
2.39.2Huang Ying wrote:
> Current kernel hard-codes the type of region to type 3 expander now,
> because this is the only supported device type. As a preparation to
> support type 2 accelerators, the patch sets the type of region to that
> of the first endpoint. Then, the patch checks whether the type of
> region is same as the type of other endpoints of the region. Because
> what we really need is to make sure the type of all endpoints of a
> region is same. And, the patch lets expander/accelerator device
> drivers specify the target type of endpoint devices via struct
> cxl_dev_state.
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Cc: 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>
> Cc: Ben Cheatham <benjamin.cheatham@amd.com>
> ---
> drivers/cxl/acpi.c | 1 -
> drivers/cxl/core/hdm.c | 28 +++++++++++++---------------
> drivers/cxl/core/port.c | 2 ++
> drivers/cxl/core/region.c | 13 +++++++------
> drivers/cxl/cxl.h | 1 +
> 5 files changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 21486e471305..29c2a44b122c 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -382,7 +382,6 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>
> cxld = &cxlrd->cxlsd.cxld;
> cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
> - cxld->target_type = CXL_DECODER_EXPANDER;
> cxld->hpa_range = (struct range) {
> .start = cfmws->base_hpa,
> .end = cfmws->base_hpa + cfmws->window_size - 1,
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 478fb6691759..c9accf8be71f 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -841,18 +841,25 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> .end = base + size - 1,
> };
>
> + if (cxled) {
> + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> + struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> + if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
> + cxld->target_type = CXL_DECODER_EXPANDER;
> + else
> + cxld->target_type = CXL_DECODER_ACCEL;
This looks broken there is no way to know the target type of the decoder
from the cxl_dev_state. An "accelerator" can have HDM and an "expander"
can have HDM-DB.
> + }
> +
> /* decoders are enabled if committed */
> if (committed) {
> cxld->flags |= CXL_DECODER_F_ENABLE;
> if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> cxld->flags |= CXL_DECODER_F_LOCK;
> - if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) {
> - cxld->target_type = CXL_DECODER_EXPANDER;
> + if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl))
> cxld->coherence = CXL_DECODER_HOSTONLYCOH;
> - } else {
> - cxld->target_type = CXL_DECODER_ACCEL;
> + else
> cxld->coherence = CXL_DECODER_DEVCOH;
> - }
>
> guard(rwsem_write)(&cxl_region_rwsem);
> if (cxld->id != cxl_num_decoders_committed(port)) {
> @@ -874,21 +881,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> struct cxl_dev_state *cxlds = cxlmd->cxlds;
>
> - /*
> - * Default by devtype until a device arrives that needs
> - * more precision.
> - */
> - if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
> - cxld->target_type = CXL_DECODER_EXPANDER;
> - else
> - cxld->target_type = CXL_DECODER_ACCEL;
> if (cxlds->coherence == CXL_DEVCOH_HOSTONLY)
> cxld->coherence = CXL_DECODER_HOSTONLYCOH;
> else
> cxld->coherence = CXL_DECODER_DEVCOH;
> } else {
> - /* To be overridden by region type/coherence at commit time */
> - cxld->target_type = CXL_DECODER_EXPANDER;
> + /* To be overridden by region coherence at commit time */
> cxld->coherence = CXL_DECODER_HOSTONLYCOH;
> }
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 9ebbffcea26a..d1bc6aed6509 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev,
> return sysfs_emit(buf, "accelerator\n");
> case CXL_DECODER_EXPANDER:
> return sysfs_emit(buf, "expander\n");
> + default:
> + break;
> }
> return -ENXIO;
> }
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21b877d8582f..d709738ada61 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> return -ENXIO;
> }
>
> - if (cxled->cxld.target_type != cxlr->type) {
> + /* Set the type of region to that of the first endpoint */
> + if (cxlr->type == CXL_DECODER_INVALID) {
> + cxlr->type = cxled->cxld.target_type;
> + } else if (cxled->cxld.target_type != cxlr->type) {
No, the type of the region is determined by the caller and should be
gated by the region capability. For type-2 region creation I doubt
userspace is going to be creating those vs the accelerator so this all
seems backwards to me.
On 10/17/24 23:33, Dan Williams wrote:
> Huang Ying wrote:
>> Current kernel hard-codes the type of region to type 3 expander now,
>> because this is the only supported device type. As a preparation to
>> support type 2 accelerators, the patch sets the type of region to that
>> of the first endpoint. Then, the patch checks whether the type of
>> region is same as the type of other endpoints of the region. Because
>> what we really need is to make sure the type of all endpoints of a
>> region is same. And, the patch lets expander/accelerator device
>> drivers specify the target type of endpoint devices via struct
>> cxl_dev_state.
>>
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Reviewed-by: Gregory Price <gourry@gourry.net>
>> Cc: 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>
>> Cc: Ben Cheatham <benjamin.cheatham@amd.com>
>> ---
>> drivers/cxl/acpi.c | 1 -
>> drivers/cxl/core/hdm.c | 28 +++++++++++++---------------
>> drivers/cxl/core/port.c | 2 ++
>> drivers/cxl/core/region.c | 13 +++++++------
>> drivers/cxl/cxl.h | 1 +
>> 5 files changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>> index 21486e471305..29c2a44b122c 100644
>> --- a/drivers/cxl/acpi.c
>> +++ b/drivers/cxl/acpi.c
>> @@ -382,7 +382,6 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>>
>> cxld = &cxlrd->cxlsd.cxld;
>> cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
>> - cxld->target_type = CXL_DECODER_EXPANDER;
>> cxld->hpa_range = (struct range) {
>> .start = cfmws->base_hpa,
>> .end = cfmws->base_hpa + cfmws->window_size - 1,
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 478fb6691759..c9accf8be71f 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -841,18 +841,25 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>> .end = base + size - 1,
>> };
>>
>> + if (cxled) {
>> + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>> + struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +
>> + if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
>> + cxld->target_type = CXL_DECODER_EXPANDER;
>> + else
>> + cxld->target_type = CXL_DECODER_ACCEL;
> This looks broken there is no way to know the target type of the decoder
> from the cxl_dev_state. An "accelerator" can have HDM and an "expander"
> can have HDM-DB.
>
>> + }
>> +
>> /* decoders are enabled if committed */
>> if (committed) {
>> cxld->flags |= CXL_DECODER_F_ENABLE;
>> if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
>> cxld->flags |= CXL_DECODER_F_LOCK;
>> - if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) {
>> - cxld->target_type = CXL_DECODER_EXPANDER;
>> + if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl))
>> cxld->coherence = CXL_DECODER_HOSTONLYCOH;
>> - } else {
>> - cxld->target_type = CXL_DECODER_ACCEL;
>> + else
>> cxld->coherence = CXL_DECODER_DEVCOH;
>> - }
>>
>> guard(rwsem_write)(&cxl_region_rwsem);
>> if (cxld->id != cxl_num_decoders_committed(port)) {
>> @@ -874,21 +881,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>> struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>
>> - /*
>> - * Default by devtype until a device arrives that needs
>> - * more precision.
>> - */
>> - if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
>> - cxld->target_type = CXL_DECODER_EXPANDER;
>> - else
>> - cxld->target_type = CXL_DECODER_ACCEL;
>> if (cxlds->coherence == CXL_DEVCOH_HOSTONLY)
>> cxld->coherence = CXL_DECODER_HOSTONLYCOH;
>> else
>> cxld->coherence = CXL_DECODER_DEVCOH;
>> } else {
>> - /* To be overridden by region type/coherence at commit time */
>> - cxld->target_type = CXL_DECODER_EXPANDER;
>> + /* To be overridden by region coherence at commit time */
>> cxld->coherence = CXL_DECODER_HOSTONLYCOH;
>> }
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 9ebbffcea26a..d1bc6aed6509 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev,
>> return sysfs_emit(buf, "accelerator\n");
>> case CXL_DECODER_EXPANDER:
>> return sysfs_emit(buf, "expander\n");
>> + default:
>> + break;
>> }
>> return -ENXIO;
>> }
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 21b877d8582f..d709738ada61 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>> return -ENXIO;
>> }
>>
>> - if (cxled->cxld.target_type != cxlr->type) {
>> + /* Set the type of region to that of the first endpoint */
>> + if (cxlr->type == CXL_DECODER_INVALID) {
>> + cxlr->type = cxled->cxld.target_type;
>> + } else if (cxled->cxld.target_type != cxlr->type) {
> No, the type of the region is determined by the caller and should be
> gated by the region capability. For type-2 region creation I doubt
> userspace is going to be creating those vs the accelerator so this all
> seems backwards to me.
FWIW, path 19 of last type support patchset does add the type to be set
at region creation time by the caller:
https://lore.kernel.org/linux-cxl/4ce8cc04-71fd-424a-9831-86f89fcd7d2f@amd.com/T/#m420ea86f7b9193519e3226c377612ad3ea546633
Dan Williams <dan.j.williams@intel.com> writes:
> Huang Ying wrote:
[snip]
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 21b877d8582f..d709738ada61 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>> return -ENXIO;
>> }
>>
>> - if (cxled->cxld.target_type != cxlr->type) {
>> + /* Set the type of region to that of the first endpoint */
>> + if (cxlr->type == CXL_DECODER_INVALID) {
>> + cxlr->type = cxled->cxld.target_type;
>> + } else if (cxled->cxld.target_type != cxlr->type) {
>
> No, the type of the region is determined by the caller and should be
> gated by the region capability. For type-2 region creation I doubt
> userspace is going to be creating those vs the accelerator so this all
> seems backwards to me.
How do we determine the type of the endpoint? Specify it in type2/type3
device driver?
If so, we will specify the type of both the endpoint and the region in
type2/type3 device driver. Then, why not only specify the type of the
endpoint? The type of region can be determined from the type of the
endpoint.
--
Best Regards,
Huang, Ying
Huang, Ying wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > Huang Ying wrote:
>
> [snip]
>
> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >> index 21b877d8582f..d709738ada61 100644
> >> --- a/drivers/cxl/core/region.c
> >> +++ b/drivers/cxl/core/region.c
> >> @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >> return -ENXIO;
> >> }
> >>
> >> - if (cxled->cxld.target_type != cxlr->type) {
> >> + /* Set the type of region to that of the first endpoint */
> >> + if (cxlr->type == CXL_DECODER_INVALID) {
> >> + cxlr->type = cxled->cxld.target_type;
> >> + } else if (cxled->cxld.target_type != cxlr->type) {
> >
> > No, the type of the region is determined by the caller and should be
> > gated by the region capability. For type-2 region creation I doubt
> > userspace is going to be creating those vs the accelerator so this all
> > seems backwards to me.
>
> How do we determine the type of the endpoint? Specify it in type2/type3
> device driver?
Why does the endpoint type matter? Memory expansion can be supported by
HDM-D[B], and an accelerator could have one or more HDM-H decoders.
> If so, we will specify the type of both the endpoint and the region in
> type2/type3 device driver. Then, why not only specify the type of the
> endpoint? The type of region can be determined from the type of the
> endpoint.
Because CXL HDM protocol is per decoder not per device.
Dan Williams <dan.j.williams@intel.com> writes:
> Huang, Ying wrote:
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>> > Huang Ying wrote:
>>
>> [snip]
>>
>> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> >> index 21b877d8582f..d709738ada61 100644
>> >> --- a/drivers/cxl/core/region.c
>> >> +++ b/drivers/cxl/core/region.c
>> >> @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>> >> return -ENXIO;
>> >> }
>> >>
>> >> - if (cxled->cxld.target_type != cxlr->type) {
>> >> + /* Set the type of region to that of the first endpoint */
>> >> + if (cxlr->type == CXL_DECODER_INVALID) {
>> >> + cxlr->type = cxled->cxld.target_type;
>> >> + } else if (cxled->cxld.target_type != cxlr->type) {
>> >
>> > No, the type of the region is determined by the caller and should be
>> > gated by the region capability. For type-2 region creation I doubt
>> > userspace is going to be creating those vs the accelerator so this all
>> > seems backwards to me.
>>
>> How do we determine the type of the endpoint? Specify it in type2/type3
>> device driver?
>
> Why does the endpoint type matter? Memory expansion can be supported by
> HDM-D[B], and an accelerator could have one or more HDM-H decoders.
>
>> If so, we will specify the type of both the endpoint and the region in
>> type2/type3 device driver. Then, why not only specify the type of the
>> endpoint? The type of region can be determined from the type of the
>> endpoint.
>
> Because CXL HDM protocol is per decoder not per device.
Sorry for confusing. When I said endpoint, I wanted to say "endpoint
decoder" actually. IIUC, the coherence type of region should be same as
that of all endpoint decoders participating the region. If we specify
the coherence type of the endpoint decoders, the coherence type of the
region should be same, so we don't need to specify it again? We need to
check root decoder capability as you pointed out.
Best Regards,
Huang, Ying
© 2016 - 2024 Red Hat, Inc.