| Message ID | 168330797672.2042604.12339416734589446371.stgit@djiang5-mobl3 (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | acpi: Add CDAT parsing support to ACPI tables code | expand |
On Fri, 05 May 2023 10:32:56 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > The CDAT table is very similar to ACPI tables when it comes to sub-table > and entry structures. The helper functions can be also used to parse the > CDAT table. Add support to the helper functions to deal with an external > CDAT table, and also handle the endieness since CDAT can be processed by a > BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse > a CDAT table. > > In order to minimize ACPI code changes, __force is being utilized to deal > with the case of a big endien (BE) host parsing a CDAT. All CDAT data > structure variables are being force casted to __leX as appropriate. Hi Dave, This falls into the annoyance that CDAT doesn't have a standard table header. Whilst I understand that was done deliberately it means some odd things happen in this code. Just how bad is the duplication if we don't do this at all, but instead roll a version for CDAT that doesn't force things through pointers of the wrong types? Otherwise, maybe we need some unions so that the type mashups don't happen. > > Cc: Rafael J. Wysocki <rafael@kernel.org> > Cc: Len Brown <lenb@kernel.org> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/acpi/tables.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- > include/acpi/actbl1.h | 3 +++ > include/linux/acpi.h | 4 ++++ > 3 files changed, 52 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 7b4680da57d7..08486f6df442 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -42,6 +42,7 @@ enum acpi_subtable_type { > ACPI_SUBTABLE_HMAT, > ACPI_SUBTABLE_PRMT, > ACPI_SUBTABLE_CEDT, > + ACPI_SUBTABLE_CDAT, > }; > > struct acpi_subtable_entry { > @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry) > return 0; > case ACPI_SUBTABLE_CEDT: > return entry->hdr->cedt.type; > + case ACPI_SUBTABLE_CDAT: > + return entry->hdr->cdat.type; > } > return 0; > } > @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry) > return entry->hdr->prmt.length; > case ACPI_SUBTABLE_CEDT: > return entry->hdr->cedt.length; > + case ACPI_SUBTABLE_CDAT: > + return le16_to_cpu((__force __le16)entry->hdr->cdat.length); > } > return 0; > } > @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry) > return sizeof(entry->hdr->prmt); > case ACPI_SUBTABLE_CEDT: > return sizeof(entry->hdr->cedt); > + case ACPI_SUBTABLE_CDAT: > + return sizeof(entry->hdr->cdat); > } > return 0; > } > @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id) > return ACPI_SUBTABLE_PRMT; > if (strncmp(id, ACPI_SIG_CEDT, 4) == 0) > return ACPI_SUBTABLE_CEDT; > + if (strncmp(id, ACPI_SIG_CDAT, 4) == 0) > + return ACPI_SUBTABLE_CDAT; I'm not super keen on inventing a SIG when the CDAT 'table' doesn't actually have one. > return ACPI_SUBTABLE_COMMON; > } > > +static unsigned long __init_or_acpilib > +acpi_table_get_length(enum acpi_subtable_type type, > + struct acpi_table_header *hdr) I don't like parsing in an acpi_table_header type here when it may not be one. I think this length decision needs to be pushed up a level to where we can see if we have a CDAT table or not. > +{ > + if (type == ACPI_SUBTABLE_CDAT) > + return le32_to_cpu( > + (__force __le32)((struct acpi_table_cdat *)hdr)->length); Perhaps a local variable in here somewhere would make it more readable. __le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length; return le32_to_cpu(length)? > + > + return hdr->length; > +} > + > static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc) > { > return proc->handler || proc->handler_arg; > @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array( > int proc_num, unsigned int max_entries) > { > struct acpi_subtable_entry entry; > + enum acpi_subtable_type type; > unsigned long table_end, subtable_len, entry_len; > int count = 0; > int errs = 0; > int i; > > - table_end = (unsigned long)table_header + table_header->length; > + type = acpi_get_subtable_type(id); > + table_end = (unsigned long)table_header + > + acpi_table_get_length(type, table_header); As above, I don't like carrying CDAT which doesn't have an acpi_table_header section around as that type of pointer. > > /* Parse all entries looking for a match. */ > > - entry.type = acpi_get_subtable_type(id); > + entry.type = type; > entry.hdr = (union acpi_subtable_headers *) > ((unsigned long)table_header + table_size); > subtable_len = acpi_get_subtable_header_length(&entry); > @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, > handler, max_entries); > } > > +int acpi_table_parse_cdat(enum acpi_cdat_type type, > + acpi_tbl_entry_handler_arg handler_arg, void *arg, > + struct acpi_table_cdat *table_header) > +{ > + struct acpi_subtable_proc proc = { > + .id = type, > + .handler_arg = handler_arg, > + .arg = arg, > + }; > + > + if (!table_header) > + return -EINVAL; > + > + return acpi_parse_entries_array(ACPI_SIG_CDAT, > + sizeof(struct acpi_table_cdat), > + (struct acpi_table_header *)table_header, > + &proc, 1, 0); > +} > +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL); > + > /** > * acpi_table_parse - find table with @id, run @handler on it > * @id: table id to find > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h > index 81b9e794424d..3119be093cfe 100644 > --- a/include/acpi/actbl1.h > +++ b/include/acpi/actbl1.h > @@ -66,6 +66,9 @@ > #define ACPI_SIG_IEIT "IEIT" > #endif > > +/* External to ACPI */ > +#define ACPI_SIG_CDAT "CDAT" /* Coherent Device Attribute Table */ Worse that that, fictional signature :) It's the nameof the 'table', but it's not a signature as it's never used as they are in ACPI and doesn't appear anywhere in the table. > + > /* > * All tables must be byte-packed to match the ACPI specification, since > * the tables are provided by the system BIOS. > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index efff750f326d..4c3dfe7587e9 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -135,6 +135,7 @@ union acpi_subtable_headers { > struct acpi_hmat_structure hmat; > struct acpi_prmt_module_header prmt; > struct acpi_cedt_header cedt; > + struct acpi_cdat_header cdat; > }; > > typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table); > @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id, > > int acpi_parse_mcfg (struct acpi_table_header *header); > void acpi_table_print_madt_entry (struct acpi_subtable_header *madt); > +int acpi_table_parse_cdat(enum acpi_cdat_type type, > + acpi_tbl_entry_handler_arg handler, void *arg, > + struct acpi_table_cdat *table_header); How did we end up with an 'acpi_' table that isn't in ACPI? (I'm not looking as I fear I might be responsible :) Should perhaps consider renaming all the CDAT entries so it doesn't looks like they are. > > /* the following numa functions are architecture-dependent */ > void acpi_numa_slit_init (struct acpi_table_slit *slit); > > >
On 5/12/23 4:58 AM, Jonathan Cameron wrote: > On Fri, 05 May 2023 10:32:56 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> The CDAT table is very similar to ACPI tables when it comes to sub-table >> and entry structures. The helper functions can be also used to parse the >> CDAT table. Add support to the helper functions to deal with an external >> CDAT table, and also handle the endieness since CDAT can be processed by a >> BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse >> a CDAT table. >> >> In order to minimize ACPI code changes, __force is being utilized to deal >> with the case of a big endien (BE) host parsing a CDAT. All CDAT data >> structure variables are being force casted to __leX as appropriate. > > Hi Dave, > > This falls into the annoyance that CDAT doesn't have a standard table header. > Whilst I understand that was done deliberately it means some odd things happen > in this code. > > Just how bad is the duplication if we don't do this at all, but instead roll > a version for CDAT that doesn't force things through pointers of the wrong types? 130 lines I believe. https://lore.kernel.org/linux-cxl/168193568543.1178687.3067575213689202382.stgit@djiang5-mobl3/ Dan, can we please make a decision on which way we want to go with this? > > Otherwise, maybe we need some unions so that the type mashups don't happen. > >> >> Cc: Rafael J. Wysocki <rafael@kernel.org> >> Cc: Len Brown <lenb@kernel.org> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> drivers/acpi/tables.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- >> include/acpi/actbl1.h | 3 +++ >> include/linux/acpi.h | 4 ++++ >> 3 files changed, 52 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >> index 7b4680da57d7..08486f6df442 100644 >> --- a/drivers/acpi/tables.c >> +++ b/drivers/acpi/tables.c >> @@ -42,6 +42,7 @@ enum acpi_subtable_type { >> ACPI_SUBTABLE_HMAT, >> ACPI_SUBTABLE_PRMT, >> ACPI_SUBTABLE_CEDT, >> + ACPI_SUBTABLE_CDAT, >> }; >> >> struct acpi_subtable_entry { >> @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry) >> return 0; >> case ACPI_SUBTABLE_CEDT: >> return entry->hdr->cedt.type; >> + case ACPI_SUBTABLE_CDAT: >> + return entry->hdr->cdat.type; >> } >> return 0; >> } >> @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry) >> return entry->hdr->prmt.length; >> case ACPI_SUBTABLE_CEDT: >> return entry->hdr->cedt.length; >> + case ACPI_SUBTABLE_CDAT: >> + return le16_to_cpu((__force __le16)entry->hdr->cdat.length); >> } >> return 0; >> } >> @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry) >> return sizeof(entry->hdr->prmt); >> case ACPI_SUBTABLE_CEDT: >> return sizeof(entry->hdr->cedt); >> + case ACPI_SUBTABLE_CDAT: >> + return sizeof(entry->hdr->cdat); >> } >> return 0; >> } >> @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id) >> return ACPI_SUBTABLE_PRMT; >> if (strncmp(id, ACPI_SIG_CEDT, 4) == 0) >> return ACPI_SUBTABLE_CEDT; >> + if (strncmp(id, ACPI_SIG_CDAT, 4) == 0) >> + return ACPI_SUBTABLE_CDAT; > > I'm not super keen on inventing a SIG when the CDAT 'table' > doesn't actually have one. I wasn't sure how else to deal with this. Additional parameter to indicate it's CDAT? > >> return ACPI_SUBTABLE_COMMON; >> } >> >> +static unsigned long __init_or_acpilib >> +acpi_table_get_length(enum acpi_subtable_type type, >> + struct acpi_table_header *hdr) > > I don't like parsing in an acpi_table_header type here when it may not be one. > I think this length decision needs to be pushed up a level to where we can see > if we have a CDAT table or not. You mean have the caller pass in the table size directly? > > >> +{ >> + if (type == ACPI_SUBTABLE_CDAT) >> + return le32_to_cpu( >> + (__force __le32)((struct acpi_table_cdat *)hdr)->length); > > Perhaps a local variable in here somewhere would make it more readable. > __le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length; > > return le32_to_cpu(length)? > > >> + >> + return hdr->length; >> +} >> + >> static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc) >> { >> return proc->handler || proc->handler_arg; >> @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array( >> int proc_num, unsigned int max_entries) >> { >> struct acpi_subtable_entry entry; >> + enum acpi_subtable_type type; >> unsigned long table_end, subtable_len, entry_len; >> int count = 0; >> int errs = 0; >> int i; >> >> - table_end = (unsigned long)table_header + table_header->length; >> + type = acpi_get_subtable_type(id); >> + table_end = (unsigned long)table_header + >> + acpi_table_get_length(type, table_header); > As above, I don't like carrying CDAT which doesn't have an acpi_table_header > section around as that type of pointer. > >> >> /* Parse all entries looking for a match. */ >> >> - entry.type = acpi_get_subtable_type(id); >> + entry.type = type; >> entry.hdr = (union acpi_subtable_headers *) >> ((unsigned long)table_header + table_size); >> subtable_len = acpi_get_subtable_header_length(&entry); >> @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, >> handler, max_entries); >> } >> >> +int acpi_table_parse_cdat(enum acpi_cdat_type type, >> + acpi_tbl_entry_handler_arg handler_arg, void *arg, >> + struct acpi_table_cdat *table_header) >> +{ >> + struct acpi_subtable_proc proc = { >> + .id = type, >> + .handler_arg = handler_arg, >> + .arg = arg, >> + }; >> + >> + if (!table_header) >> + return -EINVAL; >> + >> + return acpi_parse_entries_array(ACPI_SIG_CDAT, >> + sizeof(struct acpi_table_cdat), >> + (struct acpi_table_header *)table_header, >> + &proc, 1, 0); >> +} >> +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL); >> + >> /** >> * acpi_table_parse - find table with @id, run @handler on it >> * @id: table id to find >> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h >> index 81b9e794424d..3119be093cfe 100644 >> --- a/include/acpi/actbl1.h >> +++ b/include/acpi/actbl1.h >> @@ -66,6 +66,9 @@ >> #define ACPI_SIG_IEIT "IEIT" >> #endif >> >> +/* External to ACPI */ >> +#define ACPI_SIG_CDAT "CDAT" /* Coherent Device Attribute Table */ > > Worse that that, fictional signature :) > It's the nameof the 'table', but it's not a signature as it's never > used as they are in ACPI and doesn't appear anywhere in the table. > >> + >> /* >> * All tables must be byte-packed to match the ACPI specification, since >> * the tables are provided by the system BIOS. >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index efff750f326d..4c3dfe7587e9 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -135,6 +135,7 @@ union acpi_subtable_headers { >> struct acpi_hmat_structure hmat; >> struct acpi_prmt_module_header prmt; >> struct acpi_cedt_header cedt; >> + struct acpi_cdat_header cdat; >> }; >> >> typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table); >> @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id, >> >> int acpi_parse_mcfg (struct acpi_table_header *header); >> void acpi_table_print_madt_entry (struct acpi_subtable_header *madt); >> +int acpi_table_parse_cdat(enum acpi_cdat_type type, >> + acpi_tbl_entry_handler_arg handler, void *arg, >> + struct acpi_table_cdat *table_header); > How did we end up with an 'acpi_' table that isn't in ACPI? > (I'm not looking as I fear I might be responsible :) git blame says Bob. You are off the hook. :) > Should perhaps consider renaming all the CDAT entries so it doesn't looks like they > are. Sure I can do that. > >> >> /* the following numa functions are architecture-dependent */ >> void acpi_numa_slit_init (struct acpi_table_slit *slit); >> >> >> >
Jonathan Cameron wrote: > On Fri, 05 May 2023 10:32:56 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > > > The CDAT table is very similar to ACPI tables when it comes to sub-table > > and entry structures. The helper functions can be also used to parse the > > CDAT table. Add support to the helper functions to deal with an external > > CDAT table, and also handle the endieness since CDAT can be processed by a > > BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse > > a CDAT table. > > > > In order to minimize ACPI code changes, __force is being utilized to deal > > with the case of a big endien (BE) host parsing a CDAT. All CDAT data > > structure variables are being force casted to __leX as appropriate. > > Hi Dave, > > This falls into the annoyance that CDAT doesn't have a standard table header. > Whilst I understand that was done deliberately it means some odd things happen > in this code. > > Just how bad is the duplication if we don't do this at all, but instead roll > a version for CDAT that doesn't force things through pointers of the wrong types? Yes, this was the question before sending this out. The savings is on the order of ~100 lines which is not amazing, but was enough for me to say lets keep going with this idea. The other observation is that the ACPICA project is doing something similar for offering disassembly of CDAT buffers within the existing ACPICA tooling vs building independent infrastructure. So that was another weight on the scale with proceeding with the code reuse for me. The only thing I don't like about the result is still seeing acpi_/ACPI_ prefixes. I think these entry points and symbol names should be cdat_/CDAT_ where possible, more below. ...and as I read to the end of the feedback on this one it seems you have the same reaction. > > Otherwise, maybe we need some unions so that the type mashups don't happen. > > > > > Cc: Rafael J. Wysocki <rafael@kernel.org> > > Cc: Len Brown <lenb@kernel.org> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > > drivers/acpi/tables.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- > > include/acpi/actbl1.h | 3 +++ > > include/linux/acpi.h | 4 ++++ > > 3 files changed, 52 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > > index 7b4680da57d7..08486f6df442 100644 > > --- a/drivers/acpi/tables.c > > +++ b/drivers/acpi/tables.c > > @@ -42,6 +42,7 @@ enum acpi_subtable_type { > > ACPI_SUBTABLE_HMAT, > > ACPI_SUBTABLE_PRMT, > > ACPI_SUBTABLE_CEDT, > > + ACPI_SUBTABLE_CDAT, To your point about ACPI_SIG_CDAT I also think this should be named differently, like CDAT_SUBTABLE, just to make it clear that this is a special case and not another ACPI table. > > }; > > > > struct acpi_subtable_entry { > > @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry) > > return 0; > > case ACPI_SUBTABLE_CEDT: > > return entry->hdr->cedt.type; > > + case ACPI_SUBTABLE_CDAT: > > + return entry->hdr->cdat.type; > > } > > return 0; > > } > > @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry) > > return entry->hdr->prmt.length; > > case ACPI_SUBTABLE_CEDT: > > return entry->hdr->cedt.length; > > + case ACPI_SUBTABLE_CDAT: > > + return le16_to_cpu((__force __le16)entry->hdr->cdat.length); > > } > > return 0; > > } > > @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry) > > return sizeof(entry->hdr->prmt); > > case ACPI_SUBTABLE_CEDT: > > return sizeof(entry->hdr->cedt); > > + case ACPI_SUBTABLE_CDAT: > > + return sizeof(entry->hdr->cdat); > > } > > return 0; > > } > > @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id) > > return ACPI_SUBTABLE_PRMT; > > if (strncmp(id, ACPI_SIG_CEDT, 4) == 0) > > return ACPI_SUBTABLE_CEDT; > > + if (strncmp(id, ACPI_SIG_CDAT, 4) == 0) > > + return ACPI_SUBTABLE_CDAT; > > I'm not super keen on inventing a SIG when the CDAT 'table' > doesn't actually have one. Agree, I think CDAT_SIG makes it clearer that CDAT is not in the traditional ACPI namespace. > > > return ACPI_SUBTABLE_COMMON; > > } > > > > +static unsigned long __init_or_acpilib > > +acpi_table_get_length(enum acpi_subtable_type type, > > + struct acpi_table_header *hdr) > > I don't like parsing in an acpi_table_header type here when it may not be one. > I think this length decision needs to be pushed up a level to where we can see > if we have a CDAT table or not. > > > > +{ > > + if (type == ACPI_SUBTABLE_CDAT) > > + return le32_to_cpu( > > + (__force __le32)((struct acpi_table_cdat *)hdr)->length); > > Perhaps a local variable in here somewhere would make it more readable. > __le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length; > > return le32_to_cpu(length)? > > > > + > > + return hdr->length; > > +} > > + > > static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc) > > { > > return proc->handler || proc->handler_arg; > > @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array( > > int proc_num, unsigned int max_entries) > > { > > struct acpi_subtable_entry entry; > > + enum acpi_subtable_type type; > > unsigned long table_end, subtable_len, entry_len; > > int count = 0; > > int errs = 0; > > int i; > > > > - table_end = (unsigned long)table_header + table_header->length; > > + type = acpi_get_subtable_type(id); > > + table_end = (unsigned long)table_header + > > + acpi_table_get_length(type, table_header); > As above, I don't like carrying CDAT which doesn't have an acpi_table_header > section around as that type of pointer. > > > > > /* Parse all entries looking for a match. */ > > > > - entry.type = acpi_get_subtable_type(id); > > + entry.type = type; > > entry.hdr = (union acpi_subtable_headers *) > > ((unsigned long)table_header + table_size); > > subtable_len = acpi_get_subtable_header_length(&entry); > > @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, > > handler, max_entries); > > } > > > > +int acpi_table_parse_cdat(enum acpi_cdat_type type, > > + acpi_tbl_entry_handler_arg handler_arg, void *arg, > > + struct acpi_table_cdat *table_header) > > +{ > > + struct acpi_subtable_proc proc = { > > + .id = type, > > + .handler_arg = handler_arg, > > + .arg = arg, > > + }; > > + > > + if (!table_header) > > + return -EINVAL; > > + > > + return acpi_parse_entries_array(ACPI_SIG_CDAT, > > + sizeof(struct acpi_table_cdat), > > + (struct acpi_table_header *)table_header, > > + &proc, 1, 0); > > +} > > +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL); > > + > > /** > > * acpi_table_parse - find table with @id, run @handler on it > > * @id: table id to find > > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h > > index 81b9e794424d..3119be093cfe 100644 > > --- a/include/acpi/actbl1.h > > +++ b/include/acpi/actbl1.h > > @@ -66,6 +66,9 @@ > > #define ACPI_SIG_IEIT "IEIT" > > #endif > > > > +/* External to ACPI */ > > +#define ACPI_SIG_CDAT "CDAT" /* Coherent Device Attribute Table */ > > Worse that that, fictional signature :) > It's the nameof the 'table', but it's not a signature as it's never > used as they are in ACPI and doesn't appear anywhere in the table. > > > + > > /* > > * All tables must be byte-packed to match the ACPI specification, since > > * the tables are provided by the system BIOS. > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index efff750f326d..4c3dfe7587e9 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -135,6 +135,7 @@ union acpi_subtable_headers { > > struct acpi_hmat_structure hmat; > > struct acpi_prmt_module_header prmt; > > struct acpi_cedt_header cedt; > > + struct acpi_cdat_header cdat; > > }; > > > > typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table); > > @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id, > > > > int acpi_parse_mcfg (struct acpi_table_header *header); > > void acpi_table_print_madt_entry (struct acpi_subtable_header *madt); > > +int acpi_table_parse_cdat(enum acpi_cdat_type type, > > + acpi_tbl_entry_handler_arg handler, void *arg, > > + struct acpi_table_cdat *table_header); > How did we end up with an 'acpi_' table that isn't in ACPI? > (I'm not looking as I fear I might be responsible :) > Should perhaps consider renaming all the CDAT entries so it doesn't looks like they > are. > > > > > /* the following numa functions are architecture-dependent */ > > void acpi_numa_slit_init (struct acpi_table_slit *slit); > > > > > > >
On Fri, 12 May 2023 08:24:21 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > On 5/12/23 4:58 AM, Jonathan Cameron wrote: > > On Fri, 05 May 2023 10:32:56 -0700 > > Dave Jiang <dave.jiang@intel.com> wrote: > > > >> The CDAT table is very similar to ACPI tables when it comes to sub-table > >> and entry structures. The helper functions can be also used to parse the > >> CDAT table. Add support to the helper functions to deal with an external > >> CDAT table, and also handle the endieness since CDAT can be processed by a > >> BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse > >> a CDAT table. > >> > >> In order to minimize ACPI code changes, __force is being utilized to deal > >> with the case of a big endien (BE) host parsing a CDAT. All CDAT data > >> structure variables are being force casted to __leX as appropriate. > > > > Hi Dave, > > > > This falls into the annoyance that CDAT doesn't have a standard table header. > > Whilst I understand that was done deliberately it means some odd things happen > > in this code. > > > > Just how bad is the duplication if we don't do this at all, but instead roll > > a version for CDAT that doesn't force things through pointers of the wrong types? > > 130 lines I believe. > https://lore.kernel.org/linux-cxl/168193568543.1178687.3067575213689202382.stgit@djiang5-mobl3/ > > Dan, can we please make a decision on which way we want to go with this? > > > > > Otherwise, maybe we need some unions so that the type mashups don't happen. > > > >> > >> Cc: Rafael J. Wysocki <rafael@kernel.org> > >> Cc: Len Brown <lenb@kernel.org> > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > >> --- > >> drivers/acpi/tables.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- > >> include/acpi/actbl1.h | 3 +++ > >> include/linux/acpi.h | 4 ++++ > >> 3 files changed, 52 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > >> index 7b4680da57d7..08486f6df442 100644 > >> --- a/drivers/acpi/tables.c > >> +++ b/drivers/acpi/tables.c > >> @@ -42,6 +42,7 @@ enum acpi_subtable_type { > >> ACPI_SUBTABLE_HMAT, > >> ACPI_SUBTABLE_PRMT, > >> ACPI_SUBTABLE_CEDT, > >> + ACPI_SUBTABLE_CDAT, > >> }; > >> > >> struct acpi_subtable_entry { > >> @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry) > >> return 0; > >> case ACPI_SUBTABLE_CEDT: > >> return entry->hdr->cedt.type; > >> + case ACPI_SUBTABLE_CDAT: > >> + return entry->hdr->cdat.type; > >> } > >> return 0; > >> } > >> @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry) > >> return entry->hdr->prmt.length; > >> case ACPI_SUBTABLE_CEDT: > >> return entry->hdr->cedt.length; > >> + case ACPI_SUBTABLE_CDAT: > >> + return le16_to_cpu((__force __le16)entry->hdr->cdat.length); > >> } > >> return 0; > >> } > >> @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry) > >> return sizeof(entry->hdr->prmt); > >> case ACPI_SUBTABLE_CEDT: > >> return sizeof(entry->hdr->cedt); > >> + case ACPI_SUBTABLE_CDAT: > >> + return sizeof(entry->hdr->cdat); > >> } > >> return 0; > >> } > >> @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id) > >> return ACPI_SUBTABLE_PRMT; > >> if (strncmp(id, ACPI_SIG_CEDT, 4) == 0) > >> return ACPI_SUBTABLE_CEDT; > >> + if (strncmp(id, ACPI_SIG_CDAT, 4) == 0) > >> + return ACPI_SUBTABLE_CDAT; > > > > I'm not super keen on inventing a SIG when the CDAT 'table' > > doesn't actually have one. > > I wasn't sure how else to deal with this. Additional parameter to > indicate it's CDAT? No idea. I poke holes, not provide solutions (well not on Friday at going home time anyway) :) > > > > >> return ACPI_SUBTABLE_COMMON; > >> } > >> > >> +static unsigned long __init_or_acpilib > >> +acpi_table_get_length(enum acpi_subtable_type type, > >> + struct acpi_table_header *hdr) > > > > I don't like parsing in an acpi_table_header type here when it may not be one. > > I think this length decision needs to be pushed up a level to where we can see > > if we have a CDAT table or not. > > You mean have the caller pass in the table size directly? > Ah, I wrongly thought the acpi_table_header was assigned one function up but nope, its' many layers above. union maybe? > > > > > >> +{ > >> + if (type == ACPI_SUBTABLE_CDAT) > >> + return le32_to_cpu( > >> + (__force __le32)((struct acpi_table_cdat *)hdr)->length); > > > > Perhaps a local variable in here somewhere would make it more readable. > > __le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length; > > > > return le32_to_cpu(length)? > > > > > >> + > >> + return hdr->length; > >> +} > >> + > >> static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc) > >> { > >> return proc->handler || proc->handler_arg; > >> @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array( > >> int proc_num, unsigned int max_entries) > >> { > >> struct acpi_subtable_entry entry; > >> + enum acpi_subtable_type type; > >> unsigned long table_end, subtable_len, entry_len; > >> int count = 0; > >> int errs = 0; > >> int i; > >> > >> - table_end = (unsigned long)table_header + table_header->length; > >> + type = acpi_get_subtable_type(id); > >> + table_end = (unsigned long)table_header + > >> + acpi_table_get_length(type, table_header); > > As above, I don't like carrying CDAT which doesn't have an acpi_table_header > > section around as that type of pointer. > > > >> > >> /* Parse all entries looking for a match. */ > >> > >> - entry.type = acpi_get_subtable_type(id); > >> + entry.type = type; > >> entry.hdr = (union acpi_subtable_headers *) > >> ((unsigned long)table_header + table_size); > >> subtable_len = acpi_get_subtable_header_length(&entry); > >> @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, > >> handler, max_entries); > >> } > >> > >> +int acpi_table_parse_cdat(enum acpi_cdat_type type, > >> + acpi_tbl_entry_handler_arg handler_arg, void *arg, > >> + struct acpi_table_cdat *table_header) > >> +{ > >> + struct acpi_subtable_proc proc = { > >> + .id = type, > >> + .handler_arg = handler_arg, > >> + .arg = arg, > >> + }; > >> + > >> + if (!table_header) > >> + return -EINVAL; > >> + > >> + return acpi_parse_entries_array(ACPI_SIG_CDAT, > >> + sizeof(struct acpi_table_cdat), > >> + (struct acpi_table_header *)table_header, > >> + &proc, 1, 0); > >> +} > >> +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL); > >> +
On Fri, 12 May 2023 09:14:15 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > Jonathan Cameron wrote: > > On Fri, 05 May 2023 10:32:56 -0700 > > Dave Jiang <dave.jiang@intel.com> wrote: > > > > > The CDAT table is very similar to ACPI tables when it comes to sub-table > > > and entry structures. The helper functions can be also used to parse the > > > CDAT table. Add support to the helper functions to deal with an external > > > CDAT table, and also handle the endieness since CDAT can be processed by a > > > BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse > > > a CDAT table. > > > > > > In order to minimize ACPI code changes, __force is being utilized to deal > > > with the case of a big endien (BE) host parsing a CDAT. All CDAT data > > > structure variables are being force casted to __leX as appropriate. > > > > Hi Dave, > > > > This falls into the annoyance that CDAT doesn't have a standard table header. > > Whilst I understand that was done deliberately it means some odd things happen > > in this code. > > > > Just how bad is the duplication if we don't do this at all, but instead roll > > a version for CDAT that doesn't force things through pointers of the wrong types? > > Yes, this was the question before sending this out. The savings is on > the order of ~100 lines which is not amazing, but was enough for me to > say lets keep going with this idea. > > The other observation is that the ACPICA project is doing something > similar for offering disassembly of CDAT buffers within the existing > ACPICA tooling vs building independent infrastructure. So that was > another weight on the scale with proceeding with the code reuse for me. Great. I'd missed that. I took a look at that ages ago and decided it was too hard to solve and ran away / wrote CDAT tables in a hex editor instead for a bit. > > The only thing I don't like about the result is still seeing acpi_/ACPI_ > prefixes. I think these entry points and symbol names should be > cdat_/CDAT_ where possible, more below. > > ...and as I read to the end of the feedback on this one it seems you > have the same reaction. > > > > > Otherwise, maybe we need some unions so that the type mashups don't happen. A union or two would get rid of the type confusion - or at least make it deliberate at the cost of annoyingly noisy patch or maybe some wrappers. > > > > > > > > Cc: Rafael J. Wysocki <rafael@kernel.org> > > > Cc: Len Brown <lenb@kernel.org> > > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > > --- > > > drivers/acpi/tables.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- > > > include/acpi/actbl1.h | 3 +++ > > > include/linux/acpi.h | 4 ++++ > > > 3 files changed, 52 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > > > index 7b4680da57d7..08486f6df442 100644 > > > --- a/drivers/acpi/tables.c > > > +++ b/drivers/acpi/tables.c > > > @@ -42,6 +42,7 @@ enum acpi_subtable_type { > > > ACPI_SUBTABLE_HMAT, > > > ACPI_SUBTABLE_PRMT, > > > ACPI_SUBTABLE_CEDT, > > > + ACPI_SUBTABLE_CDAT, > > To your point about ACPI_SIG_CDAT I also think this should be named > differently, like CDAT_SUBTABLE, just to make it clear that this is a > special case and not another ACPI table. That works for me. > > > > }; > > > > > > struct acpi_subtable_entry { > > > @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry) > > > return 0; > > > case ACPI_SUBTABLE_CEDT: > > > return entry->hdr->cedt.type; > > > + case ACPI_SUBTABLE_CDAT: > > > + return entry->hdr->cdat.type; > > > } > > > return 0; > > > } > > > @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry) > > > return entry->hdr->prmt.length; > > > case ACPI_SUBTABLE_CEDT: > > > return entry->hdr->cedt.length; > > > + case ACPI_SUBTABLE_CDAT: > > > + return le16_to_cpu((__force __le16)entry->hdr->cdat.length); > > > } > > > return 0; > > > } > > > @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry) > > > return sizeof(entry->hdr->prmt); > > > case ACPI_SUBTABLE_CEDT: > > > return sizeof(entry->hdr->cedt); > > > + case ACPI_SUBTABLE_CDAT: > > > + return sizeof(entry->hdr->cdat); > > > } > > > return 0; > > > } > > > @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id) > > > return ACPI_SUBTABLE_PRMT; > > > if (strncmp(id, ACPI_SIG_CEDT, 4) == 0) > > > return ACPI_SUBTABLE_CEDT; > > > + if (strncmp(id, ACPI_SIG_CDAT, 4) == 0) > > > + return ACPI_SUBTABLE_CDAT; > > > > I'm not super keen on inventing a SIG when the CDAT 'table' > > doesn't actually have one. > > Agree, I think CDAT_SIG makes it clearer that CDAT is not in the > traditional ACPI namespace. Agreed. IF it shouts different I'm fine with this. > > > > > > return ACPI_SUBTABLE_COMMON; > > > } > > > > > > +static unsigned long __init_or_acpilib > > > +acpi_table_get_length(enum acpi_subtable_type type, > > > + struct acpi_table_header *hdr) > > > > I don't like parsing in an acpi_table_header type here when it may not be one. > > I think this length decision needs to be pushed up a level to where we can see > > if we have a CDAT table or not. This comment was (as Dave pointed out) not very helpful. Switch struct acpi_table_header for a union of that and struct acpi_table_cdat (renamed) and it would make me less uncomfortable with this >
On 5/12/23 9:14 AM, Dan Williams wrote: > Jonathan Cameron wrote: >> On Fri, 05 May 2023 10:32:56 -0700 >> Dave Jiang <dave.jiang@intel.com> wrote: >> >>> The CDAT table is very similar to ACPI tables when it comes to sub-table >>> and entry structures. The helper functions can be also used to parse the >>> CDAT table. Add support to the helper functions to deal with an external >>> CDAT table, and also handle the endieness since CDAT can be processed by a >>> BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse >>> a CDAT table. >>> >>> In order to minimize ACPI code changes, __force is being utilized to deal >>> with the case of a big endien (BE) host parsing a CDAT. All CDAT data >>> structure variables are being force casted to __leX as appropriate. >> >> Hi Dave, >> >> This falls into the annoyance that CDAT doesn't have a standard table header. >> Whilst I understand that was done deliberately it means some odd things happen >> in this code. >> >> Just how bad is the duplication if we don't do this at all, but instead roll >> a version for CDAT that doesn't force things through pointers of the wrong types? > > Yes, this was the question before sending this out. The savings is on > the order of ~100 lines which is not amazing, but was enough for me to > say lets keep going with this idea. > > The other observation is that the ACPICA project is doing something > similar for offering disassembly of CDAT buffers within the existing > ACPICA tooling vs building independent infrastructure. So that was > another weight on the scale with proceeding with the code reuse for me. > > The only thing I don't like about the result is still seeing acpi_/ACPI_ > prefixes. I think these entry points and symbol names should be > cdat_/CDAT_ where possible, more below. The name change results in significant ACPICA code changes. We probably want to avoid that. DJ > > ...and as I read to the end of the feedback on this one it seems you > have the same reaction. > >> >> Otherwise, maybe we need some unions so that the type mashups don't happen. >> >>> >>> Cc: Rafael J. Wysocki <rafael@kernel.org> >>> Cc: Len Brown <lenb@kernel.org> >>> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >>> --- >>> drivers/acpi/tables.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- >>> include/acpi/actbl1.h | 3 +++ >>> include/linux/acpi.h | 4 ++++ >>> 3 files changed, 52 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >>> index 7b4680da57d7..08486f6df442 100644 >>> --- a/drivers/acpi/tables.c >>> +++ b/drivers/acpi/tables.c >>> @@ -42,6 +42,7 @@ enum acpi_subtable_type { >>> ACPI_SUBTABLE_HMAT, >>> ACPI_SUBTABLE_PRMT, >>> ACPI_SUBTABLE_CEDT, >>> + ACPI_SUBTABLE_CDAT, > > To your point about ACPI_SIG_CDAT I also think this should be named > differently, like CDAT_SUBTABLE, just to make it clear that this is a > special case and not another ACPI table. > >>> }; >>> >>> struct acpi_subtable_entry { >>> @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry) >>> return 0; >>> case ACPI_SUBTABLE_CEDT: >>> return entry->hdr->cedt.type; >>> + case ACPI_SUBTABLE_CDAT: >>> + return entry->hdr->cdat.type; >>> } >>> return 0; >>> } >>> @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry) >>> return entry->hdr->prmt.length; >>> case ACPI_SUBTABLE_CEDT: >>> return entry->hdr->cedt.length; >>> + case ACPI_SUBTABLE_CDAT: >>> + return le16_to_cpu((__force __le16)entry->hdr->cdat.length); >>> } >>> return 0; >>> } >>> @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry) >>> return sizeof(entry->hdr->prmt); >>> case ACPI_SUBTABLE_CEDT: >>> return sizeof(entry->hdr->cedt); >>> + case ACPI_SUBTABLE_CDAT: >>> + return sizeof(entry->hdr->cdat); >>> } >>> return 0; >>> } >>> @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id) >>> return ACPI_SUBTABLE_PRMT; >>> if (strncmp(id, ACPI_SIG_CEDT, 4) == 0) >>> return ACPI_SUBTABLE_CEDT; >>> + if (strncmp(id, ACPI_SIG_CDAT, 4) == 0) >>> + return ACPI_SUBTABLE_CDAT; >> >> I'm not super keen on inventing a SIG when the CDAT 'table' >> doesn't actually have one. > > Agree, I think CDAT_SIG makes it clearer that CDAT is not in the > traditional ACPI namespace. > >> >>> return ACPI_SUBTABLE_COMMON; >>> } >>> >>> +static unsigned long __init_or_acpilib >>> +acpi_table_get_length(enum acpi_subtable_type type, >>> + struct acpi_table_header *hdr) >> >> I don't like parsing in an acpi_table_header type here when it may not be one. >> I think this length decision needs to be pushed up a level to where we can see >> if we have a CDAT table or not. >> >> >>> +{ >>> + if (type == ACPI_SUBTABLE_CDAT) >>> + return le32_to_cpu( >>> + (__force __le32)((struct acpi_table_cdat *)hdr)->length); >> >> Perhaps a local variable in here somewhere would make it more readable. >> __le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length; >> >> return le32_to_cpu(length)? >> >> >>> + >>> + return hdr->length; >>> +} >>> + >>> static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc) >>> { >>> return proc->handler || proc->handler_arg; >>> @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array( >>> int proc_num, unsigned int max_entries) >>> { >>> struct acpi_subtable_entry entry; >>> + enum acpi_subtable_type type; >>> unsigned long table_end, subtable_len, entry_len; >>> int count = 0; >>> int errs = 0; >>> int i; >>> >>> - table_end = (unsigned long)table_header + table_header->length; >>> + type = acpi_get_subtable_type(id); >>> + table_end = (unsigned long)table_header + >>> + acpi_table_get_length(type, table_header); >> As above, I don't like carrying CDAT which doesn't have an acpi_table_header >> section around as that type of pointer. >> >>> >>> /* Parse all entries looking for a match. */ >>> >>> - entry.type = acpi_get_subtable_type(id); >>> + entry.type = type; >>> entry.hdr = (union acpi_subtable_headers *) >>> ((unsigned long)table_header + table_size); >>> subtable_len = acpi_get_subtable_header_length(&entry); >>> @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, >>> handler, max_entries); >>> } >>> >>> +int acpi_table_parse_cdat(enum acpi_cdat_type type, >>> + acpi_tbl_entry_handler_arg handler_arg, void *arg, >>> + struct acpi_table_cdat *table_header) >>> +{ >>> + struct acpi_subtable_proc proc = { >>> + .id = type, >>> + .handler_arg = handler_arg, >>> + .arg = arg, >>> + }; >>> + >>> + if (!table_header) >>> + return -EINVAL; >>> + >>> + return acpi_parse_entries_array(ACPI_SIG_CDAT, >>> + sizeof(struct acpi_table_cdat), >>> + (struct acpi_table_header *)table_header, >>> + &proc, 1, 0); >>> +} >>> +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL); >>> + >>> /** >>> * acpi_table_parse - find table with @id, run @handler on it >>> * @id: table id to find >>> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h >>> index 81b9e794424d..3119be093cfe 100644 >>> --- a/include/acpi/actbl1.h >>> +++ b/include/acpi/actbl1.h >>> @@ -66,6 +66,9 @@ >>> #define ACPI_SIG_IEIT "IEIT" >>> #endif >>> >>> +/* External to ACPI */ >>> +#define ACPI_SIG_CDAT "CDAT" /* Coherent Device Attribute Table */ >> >> Worse that that, fictional signature :) >> It's the nameof the 'table', but it's not a signature as it's never >> used as they are in ACPI and doesn't appear anywhere in the table. >> >>> + >>> /* >>> * All tables must be byte-packed to match the ACPI specification, since >>> * the tables are provided by the system BIOS. >>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >>> index efff750f326d..4c3dfe7587e9 100644 >>> --- a/include/linux/acpi.h >>> +++ b/include/linux/acpi.h >>> @@ -135,6 +135,7 @@ union acpi_subtable_headers { >>> struct acpi_hmat_structure hmat; >>> struct acpi_prmt_module_header prmt; >>> struct acpi_cedt_header cedt; >>> + struct acpi_cdat_header cdat; >>> }; >>> >>> typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table); >>> @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id, >>> >>> int acpi_parse_mcfg (struct acpi_table_header *header); >>> void acpi_table_print_madt_entry (struct acpi_subtable_header *madt); >>> +int acpi_table_parse_cdat(enum acpi_cdat_type type, >>> + acpi_tbl_entry_handler_arg handler, void *arg, >>> + struct acpi_table_cdat *table_header); >> How did we end up with an 'acpi_' table that isn't in ACPI? >> (I'm not looking as I fear I might be responsible :) >> Should perhaps consider renaming all the CDAT entries so it doesn't looks like they >> are. >> >>> >>> /* the following numa functions are architecture-dependent */ >>> void acpi_numa_slit_init (struct acpi_table_slit *slit); >>> >>> >>> >> > >
On 5/12/23 4:58 AM, Jonathan Cameron wrote: > On Fri, 05 May 2023 10:32:56 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> The CDAT table is very similar to ACPI tables when it comes to sub-table >> and entry structures. The helper functions can be also used to parse the >> CDAT table. Add support to the helper functions to deal with an external >> CDAT table, and also handle the endieness since CDAT can be processed by a >> BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse >> a CDAT table. >> >> In order to minimize ACPI code changes, __force is being utilized to deal >> with the case of a big endien (BE) host parsing a CDAT. All CDAT data >> structure variables are being force casted to __leX as appropriate. > > Hi Dave, > > This falls into the annoyance that CDAT doesn't have a standard table header. > Whilst I understand that was done deliberately it means some odd things happen > in this code. > > Just how bad is the duplication if we don't do this at all, but instead roll > a version for CDAT that doesn't force things through pointers of the wrong types? > > Otherwise, maybe we need some unions so that the type mashups don't happen. > >> >> Cc: Rafael J. Wysocki <rafael@kernel.org> >> Cc: Len Brown <lenb@kernel.org> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> drivers/acpi/tables.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- >> include/acpi/actbl1.h | 3 +++ >> include/linux/acpi.h | 4 ++++ >> 3 files changed, 52 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >> index 7b4680da57d7..08486f6df442 100644 >> --- a/drivers/acpi/tables.c >> +++ b/drivers/acpi/tables.c >> @@ -42,6 +42,7 @@ enum acpi_subtable_type { >> ACPI_SUBTABLE_HMAT, >> ACPI_SUBTABLE_PRMT, >> ACPI_SUBTABLE_CEDT, >> + ACPI_SUBTABLE_CDAT, >> }; >> >> struct acpi_subtable_entry { >> @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry) >> return 0; >> case ACPI_SUBTABLE_CEDT: >> return entry->hdr->cedt.type; >> + case ACPI_SUBTABLE_CDAT: >> + return entry->hdr->cdat.type; >> } >> return 0; >> } >> @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry) >> return entry->hdr->prmt.length; >> case ACPI_SUBTABLE_CEDT: >> return entry->hdr->cedt.length; >> + case ACPI_SUBTABLE_CDAT: >> + return le16_to_cpu((__force __le16)entry->hdr->cdat.length); >> } >> return 0; >> } >> @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry) >> return sizeof(entry->hdr->prmt); >> case ACPI_SUBTABLE_CEDT: >> return sizeof(entry->hdr->cedt); >> + case ACPI_SUBTABLE_CDAT: >> + return sizeof(entry->hdr->cdat); >> } >> return 0; >> } >> @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id) >> return ACPI_SUBTABLE_PRMT; >> if (strncmp(id, ACPI_SIG_CEDT, 4) == 0) >> return ACPI_SUBTABLE_CEDT; >> + if (strncmp(id, ACPI_SIG_CDAT, 4) == 0) >> + return ACPI_SUBTABLE_CDAT; > > I'm not super keen on inventing a SIG when the CDAT 'table' > doesn't actually have one. > >> return ACPI_SUBTABLE_COMMON; >> } >> >> +static unsigned long __init_or_acpilib >> +acpi_table_get_length(enum acpi_subtable_type type, >> + struct acpi_table_header *hdr) > > I don't like parsing in an acpi_table_header type here when it may not be one. > I think this length decision needs to be pushed up a level to where we can see > if we have a CDAT table or not. > > >> +{ >> + if (type == ACPI_SUBTABLE_CDAT) >> + return le32_to_cpu( >> + (__force __le32)((struct acpi_table_cdat *)hdr)->length); > > Perhaps a local variable in here somewhere would make it more readable. > __le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length; > > return le32_to_cpu(length)? > > >> + >> + return hdr->length; >> +} >> + >> static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc) >> { >> return proc->handler || proc->handler_arg; >> @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array( >> int proc_num, unsigned int max_entries) >> { >> struct acpi_subtable_entry entry; >> + enum acpi_subtable_type type; >> unsigned long table_end, subtable_len, entry_len; >> int count = 0; >> int errs = 0; >> int i; >> >> - table_end = (unsigned long)table_header + table_header->length; >> + type = acpi_get_subtable_type(id); >> + table_end = (unsigned long)table_header + >> + acpi_table_get_length(type, table_header); > As above, I don't like carrying CDAT which doesn't have an acpi_table_header > section around as that type of pointer. > >> >> /* Parse all entries looking for a match. */ >> >> - entry.type = acpi_get_subtable_type(id); >> + entry.type = type; >> entry.hdr = (union acpi_subtable_headers *) >> ((unsigned long)table_header + table_size); >> subtable_len = acpi_get_subtable_header_length(&entry); >> @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, >> handler, max_entries); >> } >> >> +int acpi_table_parse_cdat(enum acpi_cdat_type type, >> + acpi_tbl_entry_handler_arg handler_arg, void *arg, >> + struct acpi_table_cdat *table_header) >> +{ >> + struct acpi_subtable_proc proc = { >> + .id = type, >> + .handler_arg = handler_arg, >> + .arg = arg, >> + }; >> + >> + if (!table_header) >> + return -EINVAL; >> + >> + return acpi_parse_entries_array(ACPI_SIG_CDAT, >> + sizeof(struct acpi_table_cdat), >> + (struct acpi_table_header *)table_header, >> + &proc, 1, 0); >> +} >> +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL); >> + >> /** >> * acpi_table_parse - find table with @id, run @handler on it >> * @id: table id to find >> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h >> index 81b9e794424d..3119be093cfe 100644 >> --- a/include/acpi/actbl1.h >> +++ b/include/acpi/actbl1.h >> @@ -66,6 +66,9 @@ >> #define ACPI_SIG_IEIT "IEIT" >> #endif >> >> +/* External to ACPI */ >> +#define ACPI_SIG_CDAT "CDAT" /* Coherent Device Attribute Table */ > > Worse that that, fictional signature :) > It's the nameof the 'table', but it's not a signature as it's never > used as they are in ACPI and doesn't appear anywhere in the table. This is not necessary as include/acpi/actbl2.h already defines it. But looks like we are stuck with using ACPI_SIG_CDAT. I'm really trying to avoid touching lots of ACPICA code if possible. > >> + >> /* >> * All tables must be byte-packed to match the ACPI specification, since >> * the tables are provided by the system BIOS. >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index efff750f326d..4c3dfe7587e9 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -135,6 +135,7 @@ union acpi_subtable_headers { >> struct acpi_hmat_structure hmat; >> struct acpi_prmt_module_header prmt; >> struct acpi_cedt_header cedt; >> + struct acpi_cdat_header cdat; >> }; >> >> typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table); >> @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id, >> >> int acpi_parse_mcfg (struct acpi_table_header *header); >> void acpi_table_print_madt_entry (struct acpi_subtable_header *madt); >> +int acpi_table_parse_cdat(enum acpi_cdat_type type, >> + acpi_tbl_entry_handler_arg handler, void *arg, >> + struct acpi_table_cdat *table_header); > How did we end up with an 'acpi_' table that isn't in ACPI? > (I'm not looking as I fear I might be responsible :) > Should perhaps consider renaming all the CDAT entries so it doesn't looks like they > are. > >> >> /* the following numa functions are architecture-dependent */ >> void acpi_numa_slit_init (struct acpi_table_slit *slit); >> >> >> >
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 7b4680da57d7..08486f6df442 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -42,6 +42,7 @@ enum acpi_subtable_type { ACPI_SUBTABLE_HMAT, ACPI_SUBTABLE_PRMT, ACPI_SUBTABLE_CEDT, + ACPI_SUBTABLE_CDAT, }; struct acpi_subtable_entry { @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry) return 0; case ACPI_SUBTABLE_CEDT: return entry->hdr->cedt.type; + case ACPI_SUBTABLE_CDAT: + return entry->hdr->cdat.type; } return 0; } @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry) return entry->hdr->prmt.length; case ACPI_SUBTABLE_CEDT: return entry->hdr->cedt.length; + case ACPI_SUBTABLE_CDAT: + return le16_to_cpu((__force __le16)entry->hdr->cdat.length); } return 0; } @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry) return sizeof(entry->hdr->prmt); case ACPI_SUBTABLE_CEDT: return sizeof(entry->hdr->cedt); + case ACPI_SUBTABLE_CDAT: + return sizeof(entry->hdr->cdat); } return 0; } @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id) return ACPI_SUBTABLE_PRMT; if (strncmp(id, ACPI_SIG_CEDT, 4) == 0) return ACPI_SUBTABLE_CEDT; + if (strncmp(id, ACPI_SIG_CDAT, 4) == 0) + return ACPI_SUBTABLE_CDAT; return ACPI_SUBTABLE_COMMON; } +static unsigned long __init_or_acpilib +acpi_table_get_length(enum acpi_subtable_type type, + struct acpi_table_header *hdr) +{ + if (type == ACPI_SUBTABLE_CDAT) + return le32_to_cpu( + (__force __le32)((struct acpi_table_cdat *)hdr)->length); + + return hdr->length; +} + static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc) { return proc->handler || proc->handler_arg; @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array( int proc_num, unsigned int max_entries) { struct acpi_subtable_entry entry; + enum acpi_subtable_type type; unsigned long table_end, subtable_len, entry_len; int count = 0; int errs = 0; int i; - table_end = (unsigned long)table_header + table_header->length; + type = acpi_get_subtable_type(id); + table_end = (unsigned long)table_header + + acpi_table_get_length(type, table_header); /* Parse all entries looking for a match. */ - entry.type = acpi_get_subtable_type(id); + entry.type = type; entry.hdr = (union acpi_subtable_headers *) ((unsigned long)table_header + table_size); subtable_len = acpi_get_subtable_header_length(&entry); @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id, handler, max_entries); } +int acpi_table_parse_cdat(enum acpi_cdat_type type, + acpi_tbl_entry_handler_arg handler_arg, void *arg, + struct acpi_table_cdat *table_header) +{ + struct acpi_subtable_proc proc = { + .id = type, + .handler_arg = handler_arg, + .arg = arg, + }; + + if (!table_header) + return -EINVAL; + + return acpi_parse_entries_array(ACPI_SIG_CDAT, + sizeof(struct acpi_table_cdat), + (struct acpi_table_header *)table_header, + &proc, 1, 0); +} +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL); + /** * acpi_table_parse - find table with @id, run @handler on it * @id: table id to find diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h index 81b9e794424d..3119be093cfe 100644 --- a/include/acpi/actbl1.h +++ b/include/acpi/actbl1.h @@ -66,6 +66,9 @@ #define ACPI_SIG_IEIT "IEIT" #endif +/* External to ACPI */ +#define ACPI_SIG_CDAT "CDAT" /* Coherent Device Attribute Table */ + /* * All tables must be byte-packed to match the ACPI specification, since * the tables are provided by the system BIOS. diff --git a/include/linux/acpi.h b/include/linux/acpi.h index efff750f326d..4c3dfe7587e9 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -135,6 +135,7 @@ union acpi_subtable_headers { struct acpi_hmat_structure hmat; struct acpi_prmt_module_header prmt; struct acpi_cedt_header cedt; + struct acpi_cdat_header cdat; }; typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table); @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id, int acpi_parse_mcfg (struct acpi_table_header *header); void acpi_table_print_madt_entry (struct acpi_subtable_header *madt); +int acpi_table_parse_cdat(enum acpi_cdat_type type, + acpi_tbl_entry_handler_arg handler, void *arg, + struct acpi_table_cdat *table_header); /* the following numa functions are architecture-dependent */ void acpi_numa_slit_init (struct acpi_table_slit *slit);
The CDAT table is very similar to ACPI tables when it comes to sub-table and entry structures. The helper functions can be also used to parse the CDAT table. Add support to the helper functions to deal with an external CDAT table, and also handle the endieness since CDAT can be processed by a BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse a CDAT table. In order to minimize ACPI code changes, __force is being utilized to deal with the case of a big endien (BE) host parsing a CDAT. All CDAT data structure variables are being force casted to __leX as appropriate. Cc: Rafael J. Wysocki <rafael@kernel.org> Cc: Len Brown <lenb@kernel.org> Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/acpi/tables.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- include/acpi/actbl1.h | 3 +++ include/linux/acpi.h | 4 ++++ 3 files changed, 52 insertions(+), 2 deletions(-)