diff mbox series

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

Message ID 167571659666.587790.1381783105886436293.stgit@djiang5-mobl3.local (mailing list archive)
State Handled Elsewhere
Headers show
Series cxl: Add support for QTG ID retrieval for CXL subsystem | expand

Commit Message

Dave Jiang Feb. 6, 2023, 8:49 p.m. UTC
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

Comments

Jonathan Cameron Feb. 9, 2023, 11:58 a.m. UTC | #1
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.


> +
> +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...

> +		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.


> +{
> +	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);

> +
> +	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))

> +			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)

> +
> +		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.

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/.
> 
>
Dave Jiang Feb. 9, 2023, 10:57 p.m. UTC | #2
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/.
>>
>>
>
Lukas Wunner Feb. 11, 2023, 10:18 a.m. UTC | #3
On Thu, Feb 09, 2023 at 03:57:32PM -0700, Dave Jiang wrote:
> 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.
[...]
> > 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.

Yes just iterating over the CDAT entries and directly calling the
appropriate parser function for the entry seems more straightforward.


> > 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.
[...]
> 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;

I think this is backwards.  I'd suggest creating a struct for each
CDAT entry which includes the header.  The kernel switched to
-std=gnu11 a while ago, so you should be able to use an unnamed field
for the header:

struct cdat_dsmas {
	struct cdat_entry_header;
	u8 dsmad_handle;
	u8 flags;
	u8 reserved[2];
	__le64 dpa_base;
	__le64 dpa_length;
}

Note that in my commit "cxl/pci: Handle truncated CDAT entries",
I'm only verifying that the number of bytes received via DOE
matches the length field in the cdat_entry_header.  I do not
verify in cxl_cdat_read_table() whether that length is correct
for the specific CDAT structure.  I think that's the job of
the function parsing that particular structure type.

In other words, at the top of your DSMAS parsing function,
you need to check:

	struct cdat_dsmas dsmas;

	if (dsmas->length != sizeof(*dsmas)) {
		dev_err(...);
		return -EINVAL;
	}


Note how the check is simplified by the header being part of
struct cdat_dsmas.  If the header wasn't part of struct cdat_dsmas,
an addition would be needed here.

Thanks,

Lukas
Jonathan Cameron Feb. 14, 2023, 1:17 p.m. UTC | #4
On Sat, 11 Feb 2023 11:18:33 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> On Thu, Feb 09, 2023 at 03:57:32PM -0700, Dave Jiang wrote:
> > 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.  
> [...]
> > > 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.  
> 
> Yes just iterating over the CDAT entries and directly calling the
> appropriate parser function for the entry seems more straightforward.
> 
> 
> > > 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.  
> [...]
> > 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;  
> 
> I think this is backwards.  I'd suggest creating a struct for each
> CDAT entry which includes the header.  The kernel switched to
> -std=gnu11 a while ago, so you should be able to use an unnamed field
> for the header:
> 
> struct cdat_dsmas {
> 	struct cdat_entry_header;
> 	u8 dsmad_handle;
> 	u8 flags;
> 	u8 reserved[2];
> 	__le64 dpa_base;
> 	__le64 dpa_length;
> }

This is indeed better given we always know the type before accessing.

The above trick might be useful for any code that treats it as
generic entries though a straight forwards check might be easier
and is already present in Lukas' latest code.

> 
> Note that in my commit "cxl/pci: Handle truncated CDAT entries",
> I'm only verifying that the number of bytes received via DOE
> matches the length field in the cdat_entry_header.  I do not
> verify in cxl_cdat_read_table() whether that length is correct
> for the specific CDAT structure.  I think that's the job of
> the function parsing that particular structure type.
> 
> In other words, at the top of your DSMAS parsing function,
> you need to check:
> 
> 	struct cdat_dsmas dsmas;
> 
> 	if (dsmas->length != sizeof(*dsmas)) {
> 		dev_err(...);
> 		return -EINVAL;
> 	}
> 
> 
> Note how the check is simplified by the header being part of
> struct cdat_dsmas.  If the header wasn't part of struct cdat_dsmas,
> an addition would be needed here.
> 
> Thanks,
> 
> Lukas
>
Dave Jiang Feb. 14, 2023, 8:36 p.m. UTC | #5
On 2/11/23 3:18 AM, Lukas Wunner wrote:
> On Thu, Feb 09, 2023 at 03:57:32PM -0700, Dave Jiang wrote:
>> 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.
> [...]
>>> 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.
> 
> Yes just iterating over the CDAT entries and directly calling the
> appropriate parser function for the entry seems more straightforward.
> 
> 
>>> 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.
> [...]
>> 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;
> 
> I think this is backwards.  I'd suggest creating a struct for each
> CDAT entry which includes the header.  The kernel switched to
> -std=gnu11 a while ago, so you should be able to use an unnamed field
> for the header:
> 
> struct cdat_dsmas {
> 	struct cdat_entry_header;
> 	u8 dsmad_handle;
> 	u8 flags;
> 	u8 reserved[2];
> 	__le64 dpa_base;
> 	__le64 dpa_length;
> }

In file included from drivers/cxl/pci.c:14:
drivers/cxl/cxlpci.h:109:33: warning: declaration does not declare anything
   109 |         struct cdat_entry_header;
       |                                 ^

Does not seem to be happy about the unamed field.


> 
> Note that in my commit "cxl/pci: Handle truncated CDAT entries",
> I'm only verifying that the number of bytes received via DOE
> matches the length field in the cdat_entry_header.  I do not
> verify in cxl_cdat_read_table() whether that length is correct
> for the specific CDAT structure.  I think that's the job of
> the function parsing that particular structure type.
> 
> In other words, at the top of your DSMAS parsing function,
> you need to check:
> 
> 	struct cdat_dsmas dsmas;
> 
> 	if (dsmas->length != sizeof(*dsmas)) {
> 		dev_err(...);
> 		return -EINVAL;
> 	}
> 
> 
> Note how the check is simplified by the header being part of
> struct cdat_dsmas.  If the header wasn't part of struct cdat_dsmas,
> an addition would be needed here.
> 
> Thanks,
> 
> Lukas
diff mbox series

Patch

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;
+}
+
+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)
+		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)
+{
+	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));
+
+	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) {
+			rc = call_handler(proc, &entry);
+			if (rc)
+				return rc;
+		}
+
+		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;
+	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/.