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