| Message ID | 168193568543.1178687.3067575213689202382.stgit@djiang5-mobl3 |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Add support for QTG ID retrieval for CXL subsystem | expand |
On Wed, 19 Apr 2023 13:21:25 -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> > A few minor things inline. More than possible you addressed them in earlier versions though. Jonathan > --- > v2: > - Use local headers to handle LE instead of ACPI header > - Reduce complexity of parser function. (Jonathan) > - Directly access header type. (Jonathan) > - Simplify header ptr math. (Jonathan) > - Move parsed counter to the correct location. (Jonathan) > - Add LE to host conversion for entry length > --- > drivers/cxl/core/Makefile | 1 > drivers/cxl/core/cdat.c | 100 +++++++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxlpci.h | 29 +++++++++++++ > 3 files changed, 130 insertions(+) > create mode 100644 drivers/cxl/core/cdat.c > > diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile > index ca4ae31d8f57..867a8014b462 100644 > --- a/drivers/cxl/core/Makefile > +++ b/drivers/cxl/core/Makefile > @@ -12,5 +12,6 @@ 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_TRACING) += trace.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..210f4499bddb > --- /dev/null > +++ b/drivers/cxl/core/cdat.c > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */ > +#include "cxlpci.h" > +#include "cxl.h" > + > +static bool has_handler(struct cdat_subtable_proc *proc) Even though they are static, I'd add a cxl_ or cdat_ prefix to these to make it clear they are local. > +{ > + return proc->handler; > +} > + > +static int call_handler(struct cdat_subtable_proc *proc, > + struct cdat_subtable_entry *ent) > +{ > + if (has_handler(proc)) Do we need to check this again? It's checked in the parse_entries code well before this point. Also, if moving to checking it once, then is it worth the little wrapper functions? > + return proc->handler(ent->hdr, proc->arg); > + return -EINVAL; > +} > + > +static bool cdat_is_subtable_match(struct cdat_subtable_entry *ent) > +{ > + return ent->hdr->type == ent->type; > +} > + > +static int cdat_table_parse_entries(enum cdat_type type, > + struct cdat_header *table_header, > + struct cdat_subtable_proc *proc) > +{ > + unsigned long table_end, entry_len; > + struct cdat_subtable_entry entry; > + int count = 0; > + int rc; > + > + if (!has_handler(proc)) > + return -EINVAL; > + > + table_end = (unsigned long)table_header + table_header->length; > + > + if (type >= CDAT_TYPE_RESERVED) > + return -EINVAL; > + > + entry.type = type; > + entry.hdr = (struct cdat_entry_header *)(table_header + 1); > + > + while ((unsigned long)entry.hdr < table_end) { > + entry_len = le16_to_cpu(entry.hdr->length); > + > + if ((unsigned long)entry.hdr + entry_len > table_end) > + return -EINVAL; > + > + if (entry_len == 0) > + return -EINVAL; > + > + if (cdat_is_subtable_match(&entry)) { > + rc = call_handler(proc, &entry); > + if (rc) > + return rc; > + count++; > + } > + > + entry.hdr = (struct cdat_entry_header *)((unsigned long)entry.hdr + entry_len); > + } > + > + return count; > +} ... > +int cdat_table_parse_sslbis(struct cdat_header *table, > + cdat_tbl_entry_handler handler, void *arg) Feels like these ones should take a typed arg. Sure you'll loose that again to use the generic handling code, but at this level we can do it I think. > +{ > + struct cdat_subtable_proc proc = { > + .handler = handler, > + .arg = arg, > + }; > + > + return cdat_table_parse_entries(CDAT_TYPE_SSLBIS, table, &proc); > +}
On 4/20/23 2:41 AM, Jonathan Cameron wrote: > On Wed, 19 Apr 2023 13:21:25 -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> >> > A few minor things inline. More than possible you addressed them > in earlier versions though. > > Jonathan > >> --- >> v2: >> - Use local headers to handle LE instead of ACPI header >> - Reduce complexity of parser function. (Jonathan) >> - Directly access header type. (Jonathan) >> - Simplify header ptr math. (Jonathan) >> - Move parsed counter to the correct location. (Jonathan) >> - Add LE to host conversion for entry length >> --- >> drivers/cxl/core/Makefile | 1 >> drivers/cxl/core/cdat.c | 100 +++++++++++++++++++++++++++++++++++++++++++++ >> drivers/cxl/cxlpci.h | 29 +++++++++++++ >> 3 files changed, 130 insertions(+) >> create mode 100644 drivers/cxl/core/cdat.c >> >> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile >> index ca4ae31d8f57..867a8014b462 100644 >> --- a/drivers/cxl/core/Makefile >> +++ b/drivers/cxl/core/Makefile >> @@ -12,5 +12,6 @@ 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_TRACING) += trace.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..210f4499bddb >> --- /dev/null >> +++ b/drivers/cxl/core/cdat.c >> @@ -0,0 +1,100 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */ >> +#include "cxlpci.h" >> +#include "cxl.h" >> + >> +static bool has_handler(struct cdat_subtable_proc *proc) > > Even though they are static, I'd add a cxl_ or cdat_ prefix > to these to make it clear they are local. Ok I'll change to cdat_* > >> +{ >> + return proc->handler; >> +} >> + >> +static int call_handler(struct cdat_subtable_proc *proc, >> + struct cdat_subtable_entry *ent) >> +{ >> + if (has_handler(proc)) > > Do we need to check this again? It's checked in the parse_entries code > well before this point. > > Also, if moving to checking it once, then is it worth the > little wrapper functions? Ok I'll call it directly and remove the wrapper. > > >> + return proc->handler(ent->hdr, proc->arg); >> + return -EINVAL; >> +} >> + >> +static bool cdat_is_subtable_match(struct cdat_subtable_entry *ent) >> +{ >> + return ent->hdr->type == ent->type; >> +} >> + >> +static int cdat_table_parse_entries(enum cdat_type type, >> + struct cdat_header *table_header, >> + struct cdat_subtable_proc *proc) >> +{ >> + unsigned long table_end, entry_len; >> + struct cdat_subtable_entry entry; >> + int count = 0; >> + int rc; >> + >> + if (!has_handler(proc)) >> + return -EINVAL; >> + >> + table_end = (unsigned long)table_header + table_header->length; >> + >> + if (type >= CDAT_TYPE_RESERVED) >> + return -EINVAL; >> + >> + entry.type = type; >> + entry.hdr = (struct cdat_entry_header *)(table_header + 1); >> + >> + while ((unsigned long)entry.hdr < table_end) { >> + entry_len = le16_to_cpu(entry.hdr->length); >> + >> + if ((unsigned long)entry.hdr + entry_len > table_end) >> + return -EINVAL; >> + >> + if (entry_len == 0) >> + return -EINVAL; >> + >> + if (cdat_is_subtable_match(&entry)) { >> + rc = call_handler(proc, &entry); >> + if (rc) >> + return rc; >> + count++; >> + } >> + >> + entry.hdr = (struct cdat_entry_header *)((unsigned long)entry.hdr + entry_len); >> + } >> + >> + return count; >> +} > > ... > >> +int cdat_table_parse_sslbis(struct cdat_header *table, >> + cdat_tbl_entry_handler handler, void *arg) > > Feels like these ones should take a typed arg. Sure you'll loose > that again to use the generic handling code, but at this level we can > do it I think. while DSMAS and DSLBIS takes a list_head, SSLBIS takes an xarray. I can create a union. > >> +{ >> + struct cdat_subtable_proc proc = { >> + .handler = handler, >> + .arg = arg, >> + }; >> + >> + return cdat_table_parse_entries(CDAT_TYPE_SSLBIS, table, &proc); >> +} >
> >> +int cdat_table_parse_sslbis(struct cdat_header *table, > >> + cdat_tbl_entry_handler handler, void *arg) > > > > Feels like these ones should take a typed arg. Sure you'll loose > > that again to use the generic handling code, but at this level we can > > do it I think. > > while DSMAS and DSLBIS takes a list_head, SSLBIS takes an xarray. I can > create a union. I don't understand why, If you drop the macro usage introduced in a later patch you can just have each one take the right thing. That macro isn't a huge saving anyway. Jonathan > > > > >> +{ > >> + struct cdat_subtable_proc proc = { > >> + .handler = handler, > >> + .arg = arg, > >> + }; > >> + > >> + return cdat_table_parse_entries(CDAT_TYPE_SSLBIS, table, &proc); > >> +} > > >
On 4/21/23 9:06 AM, Jonathan Cameron wrote: > >>>> +int cdat_table_parse_sslbis(struct cdat_header *table, >>>> + cdat_tbl_entry_handler handler, void *arg) >>> >>> Feels like these ones should take a typed arg. Sure you'll loose >>> that again to use the generic handling code, but at this level we can >>> do it I think. >> >> while DSMAS and DSLBIS takes a list_head, SSLBIS takes an xarray. I can >> create a union. > > I don't understand why, If you drop the macro usage introduced in > a later patch you can just have each one take the right thing. > That macro isn't a huge saving anyway. Oh I think I understand where you are trying to get at. Ok I'll update. > > Jonathan > >> >>> >>>> +{ >>>> + struct cdat_subtable_proc proc = { >>>> + .handler = handler, >>>> + .arg = arg, >>>> + }; >>>> + >>>> + return cdat_table_parse_entries(CDAT_TYPE_SSLBIS, table, &proc); >>>> +} >>> >> >
Dave Jiang 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. It seems a shame that CDAT is so ACPI-like, but can't reuse the ACPI table parsing infrastructure. Can this not be achieved by modifying some of the helpers helpers in drivers/acpi/tables.c to take a passed in @table_header?
On 4/24/23 3:33 PM, Dan Williams wrote: > Dave Jiang 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. > > It seems a shame that CDAT is so ACPI-like, but can't reuse the ACPI > table parsing infrastructure. Can this not be achieved by modifying some > of the helpers helpers in drivers/acpi/tables.c to take a passed in > @table_header? Rafael, Do you have any issues with adding some endieness support in drivers/acpi/tables.c in order to support CDAT parsing by BE hosts? To start off with something like below? diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 7b4680da57d7..e63e2daf151d 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -287,6 +287,12 @@ acpi_get_subtable_type(char *id) return ACPI_SUBTABLE_COMMON; } +static unsigned long __init_or_acpilib +acpi_table_get_length(struct acpi_table_header *hdr) +{ + return le32_to_cpu((__force __le32)hdr->length); +} + static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc) { return proc->handler || proc->handler_arg; @@ -337,7 +343,8 @@ static int __init_or_acpilib acpi_parse_entries_array( int errs = 0; int i; - table_end = (unsigned long)table_header + table_header->length; + table_end = (unsigned long)table_header + + acpi_table_get_length(table_header); /* Parse all entries looking for a match. */
Dave Jiang wrote: > > > On 4/24/23 3:33 PM, Dan Williams wrote: > > Dave Jiang 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. > > > > It seems a shame that CDAT is so ACPI-like, but can't reuse the ACPI > > table parsing infrastructure. Can this not be achieved by modifying some > > of the helpers helpers in drivers/acpi/tables.c to take a passed in > > @table_header? > > Rafael, > Do you have any issues with adding some endieness support in > drivers/acpi/tables.c in order to support CDAT parsing by BE hosts? To > start off with something like below? Some additional background, recall that CDAT is an ACPI-like data structure that lives on endpoint CXL devices to describe the access characteristics of the device's memory similar to SRAT+HMAT for host-memory. Unlike ACPI that is guaranteed to be deployed on a little-endian host-cpu, a big-endian host might also encounter CXL endpoints. This reuse ends up at ~50 lines and duplication ends up at ~100 lines. Not a huge win, but a win nonetheless. > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 7b4680da57d7..e63e2daf151d 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -287,6 +287,12 @@ acpi_get_subtable_type(char *id) > return ACPI_SUBTABLE_COMMON; > } > > +static unsigned long __init_or_acpilib > +acpi_table_get_length(struct acpi_table_header *hdr) > +{ > + return le32_to_cpu((__force __le32)hdr->length); > +} > + > static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc) > { > return proc->handler || proc->handler_arg; > @@ -337,7 +343,8 @@ static int __init_or_acpilib acpi_parse_entries_array( > int errs = 0; > int i; > > - table_end = (unsigned long)table_header + table_header->length; > + table_end = (unsigned long)table_header + > + acpi_table_get_length(table_header); > > /* Parse all entries looking for a match. */ >
diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile index ca4ae31d8f57..867a8014b462 100644 --- a/drivers/cxl/core/Makefile +++ b/drivers/cxl/core/Makefile @@ -12,5 +12,6 @@ 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_TRACING) += trace.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..210f4499bddb --- /dev/null +++ b/drivers/cxl/core/cdat.c @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */ +#include "cxlpci.h" +#include "cxl.h" + +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 (has_handler(proc)) + return proc->handler(ent->hdr, proc->arg); + return -EINVAL; +} + +static bool cdat_is_subtable_match(struct cdat_subtable_entry *ent) +{ + return ent->hdr->type == ent->type; +} + +static int cdat_table_parse_entries(enum cdat_type type, + struct cdat_header *table_header, + struct cdat_subtable_proc *proc) +{ + unsigned long table_end, entry_len; + struct cdat_subtable_entry entry; + int count = 0; + int rc; + + if (!has_handler(proc)) + return -EINVAL; + + table_end = (unsigned long)table_header + table_header->length; + + if (type >= CDAT_TYPE_RESERVED) + return -EINVAL; + + entry.type = type; + entry.hdr = (struct cdat_entry_header *)(table_header + 1); + + while ((unsigned long)entry.hdr < table_end) { + entry_len = le16_to_cpu(entry.hdr->length); + + if ((unsigned long)entry.hdr + entry_len > table_end) + return -EINVAL; + + if (entry_len == 0) + return -EINVAL; + + if (cdat_is_subtable_match(&entry)) { + rc = call_handler(proc, &entry); + if (rc) + return rc; + count++; + } + + entry.hdr = (struct cdat_entry_header *)((unsigned long)entry.hdr + entry_len); + } + + return count; +} + +int cdat_table_parse_dsmas(struct cdat_header *table, + cdat_tbl_entry_handler handler, void *arg) +{ + struct cdat_subtable_proc proc = { + .handler = handler, + .arg = arg, + }; + + return cdat_table_parse_entries(CDAT_TYPE_DSMAS, table, &proc); +} +EXPORT_SYMBOL_NS_GPL(cdat_table_parse_dsmas, CXL); + +int cdat_table_parse_dslbis(struct cdat_header *table, + cdat_tbl_entry_handler handler, void *arg) +{ + struct cdat_subtable_proc proc = { + .handler = handler, + .arg = arg, + }; + + return cdat_table_parse_entries(CDAT_TYPE_DSLBIS, table, &proc); +} +EXPORT_SYMBOL_NS_GPL(cdat_table_parse_dslbis, CXL); + +int cdat_table_parse_sslbis(struct cdat_header *table, + cdat_tbl_entry_handler handler, void *arg) +{ + struct cdat_subtable_proc proc = { + .handler = handler, + .arg = arg, + }; + + return cdat_table_parse_entries(CDAT_TYPE_SSLBIS, table, &proc); +} +EXPORT_SYMBOL_NS_GPL(cdat_table_parse_sslbis, CXL); diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h index 0465ef963cd6..45e2f2bf5ef8 100644 --- a/drivers/cxl/cxlpci.h +++ b/drivers/cxl/cxlpci.h @@ -76,12 +76,34 @@ struct cdat_header { __le32 sequence; } __packed; +enum cdat_type { + CDAT_TYPE_DSMAS = 0, + CDAT_TYPE_DSLBIS, + CDAT_TYPE_DSMSCIS, + CDAT_TYPE_DSIS, + CDAT_TYPE_DSEMTS, + CDAT_TYPE_SSLBIS, + CDAT_TYPE_RESERVED +}; + struct cdat_entry_header { u8 type; u8 reserved; __le16 length; } __packed; +typedef int (*cdat_tbl_entry_handler)(struct cdat_entry_header *header, void *arg); + +struct cdat_subtable_proc { + cdat_tbl_entry_handler handler; + void *arg; +}; + +struct cdat_subtable_entry { + struct cdat_entry_header *hdr; + enum cdat_type type; +}; + int devm_cxl_port_enumerate_dports(struct cxl_port *port); struct cxl_dev_state; int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm, @@ -90,4 +112,11 @@ void read_cdat_data(struct cxl_port *port); void cxl_cor_error_detected(struct pci_dev *pdev); pci_ers_result_t cxl_error_detected(struct pci_dev *pdev, pci_channel_state_t state); + +#define cdat_table_parse(x) \ +int cdat_table_parse_##x(struct cdat_header *table, \ + cdat_tbl_entry_handler handler, void *arg) +cdat_table_parse(dsmas); +cdat_table_parse(dslbis); +cdat_table_parse(sslbis); #endif /* __CXL_PCI_H__ */
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> --- v2: - Use local headers to handle LE instead of ACPI header - Reduce complexity of parser function. (Jonathan) - Directly access header type. (Jonathan) - Simplify header ptr math. (Jonathan) - Move parsed counter to the correct location. (Jonathan) - Add LE to host conversion for entry length --- drivers/cxl/core/Makefile | 1 drivers/cxl/core/cdat.c | 100 +++++++++++++++++++++++++++++++++++++++++++++ drivers/cxl/cxlpci.h | 29 +++++++++++++ 3 files changed, 130 insertions(+) create mode 100644 drivers/cxl/core/cdat.c