| 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: <bae25f0e-26c9-d051-731d-c87260cf120f@amd.com>
Date: Wed, 4 Dec 2024 08:58:36 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Fan Ni <nifan.cxl@...il.com>, alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
dan.j.williams@...el.com, martin.habets@...inx.com, edward.cree@....com,
davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v6 04/28] cxl/pci: add check for validating capabilities
On 12/3/24 22:55, Fan Ni wrote:
> On Mon, Dec 02, 2024 at 05:11:58PM +0000, alejandro.lucero-palau@....com wrote:
>> From: Alejandro Lucero <alucerop@....com>
>>
>> During CXL device initialization supported capabilities by the device
>> are discovered. Type3 and Type2 devices have different mandatory
>> capabilities and a Type2 expects a specific set including optional
>> capabilities.
>>
>> Add a function for checking expected capabilities against those found
>> during initialization and allow those mandatory/expected capabilities to
>> be a subset of the capabilities found.
>>
>> Rely on this function for validating capabilities instead of when CXL
>> regs are probed.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>> ---
>> drivers/cxl/core/pci.c | 16 ++++++++++++++++
>> drivers/cxl/core/regs.c | 9 ---------
>> drivers/cxl/pci.c | 24 ++++++++++++++++++++++++
>> include/cxl/cxl.h | 3 +++
>> 4 files changed, 43 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 7114d632be04..a85b96eebfd3 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -8,6 +8,7 @@
>> #include <linux/pci.h>
>> #include <linux/pci-doe.h>
>> #include <linux/aer.h>
>> +#include <cxl/cxl.h>
>> #include <cxlpci.h>
>> #include <cxlmem.h>
>> #include <cxl.h>
>> @@ -1055,3 +1056,18 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c)
>>
>> return 0;
>> }
>> +
>> +bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, unsigned long *expected_caps,
>> + unsigned long *current_caps)
> It seems "current_caps" will always be cxlds->capabilities in this
> series, and used only for the error message. Do we expect a case where
> these two can be different? If not, I think we can get rid of it and
> just use cxlds->capabilities directly in the function and in the error
> message below.
They are the same because current_caps is the way the client obtains the
information, being a pointer to a capabilities bitmap allocated by the
client.
The point being, the client can not access the capabilities from the
cxlds as it is opaque for him on purpose.
If the capabilities check returns error, the client can show/compare
those discovered by the CXL core code and those expected.
I hope this clarifies it.
Thanks
> Fan
>> +{
>> +
>> + if (current_caps)
>> + bitmap_copy(current_caps, cxlds->capabilities, CXL_MAX_CAPS);
>> +
>> + dev_dbg(cxlds->dev, "Checking cxlds caps 0x%08lx vs expected caps 0x%08lx\n",
>> + *cxlds->capabilities, *expected_caps);
>> +
>> + /* Checking a minimum of mandatory/expected capabilities */
>> + return bitmap_subset(expected_caps, cxlds->capabilities, CXL_MAX_CAPS);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_pci_check_caps, CXL);
>> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
>> index fe835f6df866..70378bb80b33 100644
>> --- a/drivers/cxl/core/regs.c
>> +++ b/drivers/cxl/core/regs.c
>> @@ -444,15 +444,6 @@ static int cxl_probe_regs(struct cxl_register_map *map, unsigned long *caps)
>> case CXL_REGLOC_RBI_MEMDEV:
>> dev_map = &map->device_map;
>> cxl_probe_device_regs(host, base, dev_map, caps);
>> - if (!dev_map->status.valid || !dev_map->mbox.valid ||
>> - !dev_map->memdev.valid) {
>> - dev_err(host, "registers not found: %s%s%s\n",
>> - !dev_map->status.valid ? "status " : "",
>> - !dev_map->mbox.valid ? "mbox " : "",
>> - !dev_map->memdev.valid ? "memdev " : "");
>> - return -ENXIO;
>> - }
>> -
>> dev_dbg(host, "Probing device registers...\n");
>> break;
>> default:
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index f6071bde437b..822030843b2f 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -903,6 +903,8 @@ __ATTRIBUTE_GROUPS(cxl_rcd);
>> static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> {
>> struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
>> + DECLARE_BITMAP(expected, CXL_MAX_CAPS);
>> + DECLARE_BITMAP(found, CXL_MAX_CAPS);
>> struct cxl_memdev_state *mds;
>> struct cxl_dev_state *cxlds;
>> struct cxl_register_map map;
>> @@ -964,6 +966,28 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> if (rc)
>> dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
>>
>> + bitmap_clear(expected, 0, CXL_MAX_CAPS);
>> +
>> + /*
>> + * These are the mandatory capabilities for a Type3 device.
>> + * Only checking capabilities used by current Linux drivers.
>> + */
>> + bitmap_set(expected, CXL_DEV_CAP_HDM, 1);
>> + bitmap_set(expected, CXL_DEV_CAP_DEV_STATUS, 1);
>> + bitmap_set(expected, CXL_DEV_CAP_MAILBOX_PRIMARY, 1);
>> + bitmap_set(expected, CXL_DEV_CAP_DEV_STATUS, 1);
>> +
>> + /*
>> + * Checking mandatory caps are there as, at least, a subset of those
>> + * found.
>> + */
>> + if (!cxl_pci_check_caps(cxlds, expected, found)) {
>> + dev_err(&pdev->dev,
>> + "Expected mandatory capabilities not found: (%08lx - %08lx)\n",
>> + *expected, *found);
>> + return -ENXIO;
>> + }
>> +
>> rc = cxl_pci_type3_init_mailbox(cxlds);
>> if (rc)
>> return rc;
>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>> index f656fcd4945f..05f06bfd2c29 100644
>> --- a/include/cxl/cxl.h
>> +++ b/include/cxl/cxl.h
>> @@ -37,4 +37,7 @@ void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec);
>> void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial);
>> int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>> enum cxl_resource);
>> +bool cxl_pci_check_caps(struct cxl_dev_state *cxlds,
>> + unsigned long *expected_caps,
>> + unsigned long *current_caps);
>> #endif
>> --
>> 2.17.1
>>
Powered by blists - more mailing lists