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

[Lukas Wunner <lukas@wunner.de>]

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