On Sat, 11 Feb 2023 11:18:33 +0100
Lukas Wunner <lukas@xxxxxxxxx> 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@xxxxxxxxx> 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
>
