Re: [PATCH 04/18] cxl: Add common helpers for cdat parsing

[Dave Jiang <dave.jiang@intel.com>]

On 2/9/23 4:58 AM, Jonathan Cameron wrote:
> On Mon, 06 Feb 2023 13:49:58 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add helper functions to parse the CDAT table and provide a callback to
>> parse the sub-table. Helpers are provided for DSMAS and DSLBIS sub-table
>> parsing. The code is patterned after the ACPI table parsing helpers.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/Makefile |    1
>>   drivers/cxl/core/cdat.c   |   98 +++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/cxl/core/cdat.h   |   15 +++++++
>>   drivers/cxl/cxl.h         |    9 ++++
>>   4 files changed, 123 insertions(+)
>>   create mode 100644 drivers/cxl/core/cdat.c
>>   create mode 100644 drivers/cxl/core/cdat.h
>>
>> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
>> index 79c7257f4107..438ce27faf77 100644
>> --- a/drivers/cxl/core/Makefile
>> +++ b/drivers/cxl/core/Makefile
>> @@ -10,4 +10,5 @@ cxl_core-y += memdev.o
>>   cxl_core-y += mbox.o
>>   cxl_core-y += pci.o
>>   cxl_core-y += hdm.o
>> +cxl_core-y += cdat.o
>>   cxl_core-$(CONFIG_CXL_REGION) += region.o
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> new file mode 100644
>> index 000000000000..be09c8a690f5
>> --- /dev/null
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -0,0 +1,98 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
>> +#include "cxl.h"
>> +#include "cdat.h"
>> +
>> +static u8 cdat_get_subtable_entry_type(struct cdat_subtable_entry *entry)
>> +{
>> +	return entry->hdr->type;
>> +}
> 
> Are these all worthwhile given the resulting function name is longer
> than accessing it directly.  If aim is to move the details of the
> struct cdat_subtable_entry away from being exposed at caller, then
> fair enough, but if that is the plan I'd expect to see something about
> that in the patch description.
> 
> Feels like some premature abstraction, but I don't feel particularly
> strongly about this.

I'll drop them. The code was adapted from ACPI table parsing code. But 
we can simplify for our usages.
> 
> 
>> +
>> +static u16 cdat_get_subtable_entry_length(struct cdat_subtable_entry *entry)
>> +{
>> +	return entry->hdr->length;
>> +}
>> +
>> +static bool has_handler(struct cdat_subtable_proc *proc)
>> +{
>> +	return proc->handler;
>> +}
>> +
>> +static int call_handler(struct cdat_subtable_proc *proc,
>> +			struct cdat_subtable_entry *ent)
>> +{
>> +	if (proc->handler)
> 
> Use your wrapper...

ok
> 
>> +		return proc->handler(ent->hdr, proc->arg);
>> +	return -EINVAL;
>> +}
>> +
>> +static int cdat_table_parse_entries(enum acpi_cdat_type type,
>> +				    struct acpi_table_cdat *table_header,
>> +				    struct cdat_subtable_proc *proc,
>> +				    unsigned int max_entries)
> 
> Documentation needed.  max_entries wasn't what I was expecting.
> I would have expected it to be a cap on number of entries of
> matching type, whereas it seems to be number of entries of any type.
> 
> Also, max_entries == 0 non obvious parameter value.

I'll drop max_entries. Code came from ACPI, but I don't think we need it.
> 
> 
>> +{
>> +	struct cdat_subtable_entry entry;
>> +	unsigned long table_end, entry_len;
>> +	int count = 0;
>> +	int rc;
>> +
>> +	if (!has_handler(proc))
>> +		return -EINVAL;
>> +
>> +	table_end = (unsigned long)table_header + table_header->length;
>> +
>> +	if (type >= ACPI_CDAT_TYPE_RESERVED)
>> +		return -EINVAL;
>> +
>> +	entry.type = type;
>> +	entry.hdr = (struct acpi_cdat_header *)((unsigned long)table_header +
>> +					       sizeof(*table_header));
> 
> Common idiom for this is.
> 
> 	entry.hdr = (struct acpi_cdat_header *)(table_header + 1);
>
ok.


>> +
>> +	while ((unsigned long)entry.hdr < table_end) {
>> +		entry_len = cdat_get_subtable_entry_length(&entry);
>> +
>> +		if ((unsigned long)entry.hdr + entry_len > table_end)
>> +			return -EINVAL;
>> +
>> +		if (max_entries && count >= max_entries)
>> +			break;
>> +
>> +		if (entry_len == 0)
>> +			return -EINVAL;
>> +
>> +		if (cdat_get_subtable_entry_type(&entry) == type) {
> 
> This is a little odd as we set entry.type above == type, but
> the match here is on the value in the one in entry.hdr.
> 
> That's not particularly intuitive. Not sure on what a good solution
> would be though.  Maybe just
> 
> 		if (cdat_is_subtable_match(&entry))
ok

> 
>> +			rc = call_handler(proc, &entry);
>> +			if (rc)
>> +				return rc;
>> +		}
> 
> As above.  Maybe intent, but my initial assumption would have had
> count not incremented unless there was a match. (so put it in this if block
> not below)

right ok.

> 
>> +
>> +		entry.hdr = (struct acpi_cdat_header *)((unsigned long)entry.hdr + entry_len);
>> +		count++;
>> +	}
>> +
>> +	return count;
>> +}
>> +
>> +int cdat_table_parse_dsmas(void *table, cdat_tbl_entry_handler handler, void *arg)
>> +{
>> +	struct acpi_table_cdat *header = (struct acpi_table_cdat *)table;
> 
> Now struct acpi_table_cdata exists, maybe just move to using
> that type for all references.  Will make a mess of the range checking
> efforts the hardening folk are working on as we will index off end of
> it and it doesn't have a variable length array trailing element.
> 
> Random musing follows...
> We could add a variable length element to that struct
> definition and the magic to associate that with the length parameter
> and get range protection if relevant hardening is turned on.
> 
> Structure definition comes (I think) from scripts in acpica so
> would need to push such changes into acpica and I'm not sure
> they will be keen even though it would be good for the kernel
> to have the protections.


Lukas actually noticed that the ACPI data structs are unsuitable for 
kernel usage because it doesn't designate the data as LE. He has created 
local structs that has the correct data type. We can expand on top of 
that for our usages.

https://github.com/l1k/linux/commit/d376a53a45da2fff219799a02f216962123f9fd0

I see what you are saying. But I'm not sure how easily we can do this 
for the CDAT table due to endieness. Is this what you had in mind?

From:
struct cdat_entry_header {
	u8 type;
	u8 reserved;
	__le16 length;
} __packed;

To:
struct cdat_entry_header {
	u8 type;
	u8 reserved;
	__le16 length;
	DECLARE_BOUNDED_ARRAY(u8, body, le16_to_cpu(length));
} __packed;

> 
> https://people.kernel.org/kees/bounded-flexible-arrays-in-c
> for Kees Cook's blog on this stuff.  The last bit needs
> the 'comming soon' part.
> 
>> +	struct cdat_subtable_proc proc = {
>> +		.handler	= handler,
>> +		.arg		= arg,
>> +	};
>> +
>> +	return cdat_table_parse_entries(ACPI_CDAT_TYPE_DSMAS, header, &proc, 0);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cdat_table_parse_dsmas, CXL);
>> +
>> +int cdat_table_parse_dslbis(void *table, cdat_tbl_entry_handler handler, void *arg)
>> +{
>> +	struct acpi_table_cdat *header = (struct acpi_table_cdat *)table;
>> +	struct cdat_subtable_proc proc = {
>> +		.handler	= handler,
>> +		.arg		= arg,
>> +	};
>> +
>> +	return cdat_table_parse_entries(ACPI_CDAT_TYPE_DSLBIS, header, &proc, 0);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cdat_table_parse_dslbis, CXL);
>> diff --git a/drivers/cxl/core/cdat.h b/drivers/cxl/core/cdat.h
>> new file mode 100644
>> index 000000000000..f690325e82a6
>> --- /dev/null
>> +++ b/drivers/cxl/core/cdat.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* Copyright(c) 2023 Intel Corporation. */
>> +#ifndef __CXL_CDAT_H__
>> +#define __CXL_CDAT_H__
>> +
>> +struct cdat_subtable_proc {
>> +	cdat_tbl_entry_handler handler;
>> +	void *arg;
>> +};
>> +
>> +struct cdat_subtable_entry {
>> +	struct acpi_cdat_header *hdr;
>> +	enum acpi_cdat_type type;
>> +};
>> +#endif
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index f558bbfc0332..839a121c1997 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -9,6 +9,7 @@
>>   #include <linux/bitops.h>
>>   #include <linux/log2.h>
>>   #include <linux/io.h>
>> +#include <linux/acpi.h>
>>   
>>   /**
>>    * DOC: cxl objects
>> @@ -697,6 +698,14 @@ static inline struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev)
>>   }
>>   #endif
>>   
>> +typedef int (*cdat_tbl_entry_handler)(struct acpi_cdat_header *header, void *arg);
>> +
>> +u8 cdat_table_checksum(u8 *buffer, u32 length);
>> +int cdat_table_parse_dsmas(void *table, cdat_tbl_entry_handler handler,
>> +			   void *arg);
>> +int cdat_table_parse_dslbis(void *table, cdat_tbl_entry_handler handler,
>> +			    void *arg);
>> +
>>   /*
>>    * Unit test builds overrides this to __weak, find the 'strong' version
>>    * of these symbols in tools/testing/cxl/.
>>
>>
>