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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sat, 11 Feb 2023 11:18:33 +0100
Lukas Wunner <lukas@wunner.de> 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;
> }

This is indeed better given we always know the type before accessing.

The above trick might be useful for any code that treats it as
generic entries though a straight forwards check might be easier
and is already present in Lukas' latest code.

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