| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <85a73846-4d8d-4f28-b748-24904b749bca@amd.com>
Date: Wed, 2 Oct 2024 16:15:42 -0500
From: Ben Cheatham <benjamin.cheatham@....com>
To: Huang Ying <ying.huang@...el.com>
CC: <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Jonathan
Cameron" <Jonathan.Cameron@...wei.com>, Davidlohr Bueso <dave@...olabs.net>,
Alison Schofield <alison.schofield@...el.com>, Vishal Verma
<vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>, Alejandro Lucero
<alucerop@....com>, Dan Williams <dan.j.williams@...el.com>, Dave Jiang
<dave.jiang@...el.com>
Subject: Re: [RFC 4/5] cxl: Set type of region to that of the first endpoint
On 9/24/24 9:46 PM, Huang Ying wrote:
> The type of region is hard-coded as type 3 expander now, because this
> is the only supported device type. As a preparation to support type 2
> accelerators, we set the type of region to that of the first endpoint.
> Then, we will check 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.
>
> The target type of endpoint devices comes from expander/accelerator
> device drivers via struct cxl_dev_state.
>
> Signed-off-by: "Huang, Ying" <ying.huang@...el.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Cc: Dan Williams <dan.j.williams@...el.com>
> Cc: Davidlohr Bueso <dave@...olabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@...wei.com>
> Cc: Dave Jiang <dave.jiang@...el.com>
> Cc: Alison Schofield <alison.schofield@...el.com>
> Cc: Vishal Verma <vishal.l.verma@...el.com>
> Cc: Ira Weiny <ira.weiny@...el.com>
> Cc: Alejandro Lucero <alucerop@....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 2dee78e9b90c..5633b7316cb3 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;
You can drop this imo. It doesn't change anything functionally considering
the break is immediately followed by a return.
Thanks,
Ben
> }
> return -ENXIO;
> }
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index cec7d08b6f44..9c68ec445128 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);
> @@ -2482,7 +2485,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.
> @@ -2492,8 +2494,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;
> @@ -2504,7 +2505,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);
> @@ -2576,7 +2577,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 99398c868d82..2a2d2c483654 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,
> CXL_DECODER_ACCEL = 2,
> CXL_DECODER_EXPANDER = 3,
> };
Powered by blists - more mailing lists