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