| 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: <2a91a62a-dcd3-7862-95a6-57f0cfd0b759@amd.com>
Date: Mon, 21 Oct 2024 10:47:10 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Dan Williams <dan.j.williams@...el.com>, Huang Ying
<ying.huang@...el.com>, Dave Jiang <dave.jiang@...el.com>
Cc: linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
Gregory Price <gourry@...rry.net>, Davidlohr Bueso <dave@...olabs.net>,
Jonathan Cameron <jonathan.cameron@...wei.com>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
Ben Cheatham <benjamin.cheatham@....com>
Subject: Re: [PATCH 4/5] cxl: Set type of region to that of the first endpoint
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@...el.com>
>> Reviewed-by: Gregory Price <gourry@...rry.net>
>> 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>
>> Cc: Ben Cheatham <benjamin.cheatham@....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
Powered by blists - more mailing lists