Re: [PATCH 1/4] acpi: tables: Add CDAT table parsing support

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

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