| 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: <9a18b887-dbff-d2d4-1446-8f327fd9777f@amd.com>
Date: Mon, 20 Jan 2025 17:15:45 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Dan Williams <dan.j.williams@...el.com>, alejandro.lucero-palau@....com,
linux-cxl@...r.kernel.org, netdev@...r.kernel.org, edward.cree@....com,
davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v9 13/27] cxl: prepare memdev creation for type2
On 1/18/25 02:27, Dan Williams wrote:
> alejandro.lucero-palau@ wrote:
>> From: Alejandro Lucero <alucerop@....com>
>>
>> Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device when
>> creating a memdev leading to problems when obtaining cxl_memdev_state
>> references from a CXL_DEVTYPE_DEVMEM type. This last device type is
>> managed by a specific vendor driver and does not need same sysfs files
>> since not userspace intervention is expected.
>>
>> Create a new cxl_mem device type with no attributes for Type2.
>>
>> Avoid debugfs files relying on existence of cxl_memdev_state.
>>
>> Make devm_cxl_add_memdev accesible from a accel driver.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>> ---
>> drivers/cxl/core/cdat.c | 3 +++
>> drivers/cxl/core/memdev.c | 14 ++++++++++++--
>> drivers/cxl/core/region.c | 3 ++-
>> drivers/cxl/cxlmem.h | 2 --
>> drivers/cxl/mem.c | 25 +++++++++++++++++++------
>> include/cxl/cxl.h | 2 ++
>> 6 files changed, 38 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index 8153f8d83a16..c57bc83e79ee 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -577,6 +577,9 @@ static struct cxl_dpa_perf *cxled_get_dpa_perf(struct cxl_endpoint_decoder *cxle
>> struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>> struct cxl_dpa_perf *perf;
>>
>> + if (!mds)
>> + return ERR_PTR(-EINVAL);
>> +
> The references to @mds in cxled_get_dpa_perf() are gone
> after the DPA partition changes.
OK
>
> If an accelerator has a CDAT it will get qos_class information for free.
> If it does not have a CDAT then I wonder how it is telling the BIOS the
> memory type for its CXL.mem?
AFAIK, this is not mandatory, and the BIOS will look at CXL DVSEC for
finding out. So no perf data is mandatory.
FWIW, our device will export a CDAT via PCI option rom since we have to
avoid the BIOS doing things we do not want to like testing the memory as
some BIOS seem to do in some debug/test mode, and to advertise this flag
we discussed in v1/RFC for the kernel doing nothing with that memory
when found in the HMAT table.
>> switch (mode) {
>> case CXL_DECODER_RAM:
>> perf = &mds->ram_perf;
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 836db4a462b3..f91feca586dd 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -547,9 +547,16 @@ static const struct device_type cxl_memdev_type = {
>> .groups = cxl_memdev_attribute_groups,
>> };
>>
>> +static const struct device_type cxl_accel_memdev_type = {
>> + .name = "cxl_accel_memdev",
>> + .release = cxl_memdev_release,
>> + .devnode = cxl_memdev_devnode,
>> +};
>> +
>> bool is_cxl_memdev(const struct device *dev)
>> {
>> - return dev->type == &cxl_memdev_type;
>> + return (dev->type == &cxl_memdev_type ||
>> + dev->type == &cxl_accel_memdev_type);
> At this point the game is over in terms of future code that trips over
> the assumption that all "is_cxl_memdev() == true" devices are created
> equal.
Yep.
>> }
>> EXPORT_SYMBOL_NS_GPL(is_cxl_memdev, "CXL");
>>
>> @@ -660,7 +667,10 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>> dev->parent = cxlds->dev;
>> dev->bus = &cxl_bus_type;
>> dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
>> - dev->type = &cxl_memdev_type;
>> + if (cxlds->type == CXL_DEVTYPE_DEVMEM)
>> + dev->type = &cxl_accel_memdev_type;
>> + else
>> + dev->type = &cxl_memdev_type;
>> device_set_pm_not_required(dev);
>> INIT_WORK(&cxlmd->detach_work, detach_memdev);
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index d77899650798..967132b49832 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -1948,7 +1948,8 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>> return -EINVAL;
>> }
>>
>> - cxl_region_perf_data_calculate(cxlr, cxled);
>> + if (cxlr->type == CXL_DECODER_HOSTONLYMEM)
>> + cxl_region_perf_data_calculate(cxlr, cxled);
> Per-above no need to worry about @mds reference in this path.
I'll adapt it.
>
>> if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>> int i;
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index 4c1c53c29544..360d3728f492 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -87,8 +87,6 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>> return is_cxl_memdev(port->uport_dev);
>> }
>>
>> -struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>> - struct cxl_dev_state *cxlds);
>> int devm_cxl_sanitize_setup_notifier(struct device *host,
>> struct cxl_memdev *cxlmd);
>> struct cxl_memdev_state;
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 2f03a4d5606e..93106a43990b 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -130,12 +130,18 @@ static int cxl_mem_probe(struct device *dev)
>> dentry = cxl_debugfs_create_dir(dev_name(dev));
>> debugfs_create_devm_seqfile(dev, "dpamem", dentry, cxl_mem_dpa_show);
>>
>> - if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
>> - debugfs_create_file("inject_poison", 0200, dentry, cxlmd,
>> - &cxl_poison_inject_fops);
>> - if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds))
>> - debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
>> - &cxl_poison_clear_fops);
>> + /*
>> + * Avoid poison debugfs files for Type2 devices as they rely on
>> + * cxl_memdev_state.
>> + */
>> + if (mds) {
>> + if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
>> + debugfs_create_file("inject_poison", 0200, dentry, cxlmd,
>> + &cxl_poison_inject_fops);
>> + if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds))
>> + debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
>> + &cxl_poison_clear_fops);
>> + }
>>
>> rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
>> if (rc)
>> @@ -219,6 +225,13 @@ static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n)
>> struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>>
>> + /*
>> + * Avoid poison sysfs files for Type2 devices as they rely on
>> + * cxl_memdev_state.
>> + */
>> + if (!mds)
>> + return 0;
> It turns out that nothing in the poison handling path requires 'struct
> cxl_memdev_state'. So rather than key this rejection off of @mds ==
> NULL, I would prefer to find a way to make this optional relative to
> data read from 'struct cxl_mailbox', or 'struct cxl_dev_state'.
> Conceptually there is nothing stopping a CXL accelertor from supporting
> poison management commands on its mailbox, it's just unlikely.
Right. This is the easiest path for the initial type2 support SFC
requires, but I agree it should cover other options as well.
I'll work on it for v10 and if not a major work, I'll do it.
Thanks
Powered by blists - more mailing lists