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

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

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