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