Page MenuHomeFreeBSD

Add NFIT tables to `acpidump -t` output
ClosedPublic

Authored by ygy on Jul 4 2017, 7:29 PM.
Tags
None
Referenced Files
F102718219: D11479.id30497.diff
Sat, Nov 16, 7:32 AM
Unknown Object (File)
Thu, Nov 7, 2:11 AM
Unknown Object (File)
Tue, Nov 5, 5:40 PM
Unknown Object (File)
Sun, Nov 3, 1:10 AM
Unknown Object (File)
Fri, Nov 1, 9:44 PM
Unknown Object (File)
Sun, Oct 20, 6:57 PM
Unknown Object (File)
Sun, Oct 20, 8:34 AM
Unknown Object (File)
Sat, Oct 19, 12:18 PM
Subscribers

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

usr.sbin/acpi/acpidump/acpi.c
277–278 ↗(On Diff #30406)

We can address this one separately I think (a separate commit prior to the NFIT support).

Also it should likely be if (subtable->Length < sizeof(ACPI_SUBTABLE_HEADER)) -- i.e., subtable length < 2 is invalid.

And finally, we can probably just do (instead of errx)

warn("invalid subtable length %d", subtable->Length);
return;

i.e., still print out other tables even if one is corrupt.

296 ↗(On Diff #30406)

Should acpi_walk_nfit get a length validation as well?

1174 ↗(On Diff #30406)

This one needs a different implementation -- I will discuss.

ygy marked 3 inline comments as done.
usr.sbin/acpi/acpidump/acpi.c
1178 ↗(On Diff #30462)

Hmm, despite the rest of acpidump using %d, this should really be %u to correspond with the unsigned nfit->Type (and similarly for the rest of the printfs below)

1187 ↗(On Diff #30462)

These ones are actually UINT64 (or uint64_t) so they need to be cast to (uintmax_t) and printed with 0x%016jx (Address, Length, and MemoryMapping)

I'd say the length should be in hex too.

1208–1209 ↗(On Diff #30462)

%016jx for these two (size, offset)

1233 ↗(On Diff #30462)

This one's not right: it's defined in the struct as LineOffset[1] but it's a placeholder for a variable sized array. Will discuss.

1237 ↗(On Diff #30462)

variable length array here too

1242–1243 ↗(On Diff #30462)

Vendor ID and Device ID in general are presented in hex -- probably 0x%04x

1245–1246 ↗(On Diff #30462)

also hex

1253–1257 ↗(On Diff #30462)

these are all 64-bit

1273–1276 ↗(On Diff #30462)

four 64-bit values

1282 ↗(On Diff #30462)

variable length

ygy marked 6 inline comments as done.Jul 6 2017, 5:39 PM

Update printf format (again)

ygy marked 4 inline comments as done.Jul 6 2017, 5:56 PM
usr.sbin/acpi/acpidump/acpi.c
300 ↗(On Diff #30497)

I realized after I shared the previous snippet with you that this should be warnx -- i.e., don't print a string corresponding to errno. Also by convention it's lower case i -- invalid subtable length

1208–1209 ↗(On Diff #30497)

RegionSize and RegionOffset are 64-bit so need %016jx

1282 ↗(On Diff #30497)

These ones still need to be fixed - I will explain.

ygy marked 3 inline comments as done.
usr.sbin/acpi/acpidump/acpi.c
300 ↗(On Diff #30497)

oh, also should be %u here for unsigned argument

1184 ↗(On Diff #30497)

We should probably check the return value in status as well. An error is practically not going to occur, so exiting on error ought to be sufficient. Could just be

if (status != uuid_s_ok)
        errx("uuid_to_string: status=%u", status)
1233 ↗(On Diff #30497)

Can leave this one out at first as well - we need to address all three of these variable length entries in a similar way.

ygy marked 3 inline comments as done.
usr.sbin/acpi/acpidump/acpi.c
1156 ↗(On Diff #30796)

You may use array-designated-initializers there to explicitely specify array indexes for string names.

[ACPI_NFIT_TYPE_SYSTEM_ADDRESS] = "System Address",
1235 ↗(On Diff #30796)

Use C-style comments /* */, instead of C++.

usr.sbin/acpi/acpidump/acpi.c
1156 ↗(On Diff #30796)

This one definitely makes more sense. Do you think I should change the style for other occurrences in this file?

usr.sbin/acpi/acpidump/acpi.c
1156 ↗(On Diff #30796)

I think that's reasonable, but submit a separate diff for that change, and we'll commit it first.

ygy marked 4 inline comments as done.
usr.sbin/acpi/acpidump/acpi.c
1156 ↗(On Diff #30870)

static const char * perhaps ?

1157 ↗(On Diff #30870)

Indent is too large, use one tab.

ygy marked 2 inline comments as done.
This revision is now accepted and ready to land.Jul 20 2017, 5:28 PM
This revision was automatically updated to reflect the committed changes.