| Message ID | 1510634864-678-1-git-send-email-alex.hung@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] Remove length test for ACPI 1.0 RSDPs, fix checks against revision field | expand |
On 11/14/2017 12:47 PM, Alex Hung wrote: > From: Chris Goldsworthy <goldswo@amazon.com> > > Remove RSDP length test for ACPI 1.0 RSDPs. Fix RSDP revision field checks to > check against revision values 0 and 2 only. > > The RSDP length field test for ACPI 1.0 RSDPs is invalid, since a ACPI 1.0 RSDP > has no length field. FWTS also checks the RSDP revision number incorrectly - > the revision field of a ACPI 1.0 RSDP has a value of 0, not 1 (as per ACPI > specification version 6.2, page 122). > > Add descriptions for specific RSDP tests. > > Fix a typo in a RSDT error message. > > Signed-off-by: Christopher Goldsworthy <goldswo@amazon.de> > --- > src/acpi/rsdp/rsdp.c | 27 ++++++++++++--------------- > src/acpi/rsdt/rsdt.c | 2 +- > 2 files changed, 13 insertions(+), 16 deletions(-) > > diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c > index becdbb2..2f537be 100644 > --- a/src/acpi/rsdp/rsdp.c > +++ b/src/acpi/rsdp/rsdp.c > @@ -80,6 +80,7 @@ static int rsdp_test1(fwts_framework *fw) > "RSDPBadFirstChecksum", > "RSDP first checksum is incorrect: 0x%x", checksum); > > + /* ensure oem_id only contains printable characters */ > for (i = 0; i < 6; i++) { > if (!isprint(rsdp->oem_id[i])) { > passed = false; > @@ -102,27 +103,23 @@ static int rsdp_test1(fwts_framework *fw) > "firmware ACPI complaint."); > } > > - /* ACPI 6.1 errata clarifies revision 1 must have length 20 */ > - if (rsdp->revision == 1) { > - if (rsdp->length == 20) > - fwts_passed(fw, "RSDP: the table is the correct length."); > - else > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "RSDPBadLength", > - "RSDP: length of Revision 1 is %d bytes but should be 20.", > - rsdp->length); > - > - return FWTS_OK; > - } > - > - if (rsdp->revision <= 2) > + /* check if revision number is valid. note: revision field for > + * ACPI 1.0 RSDP is 0, not 1, as per ACPI specification version > + * 6.2, p.122. */ > + if (rsdp->revision == 0 || rsdp->revision == 2) > fwts_passed(fw, > "RSDP: revision is %" PRIu8 ".", rsdp->revision); > else > fwts_failed(fw, LOG_LEVEL_MEDIUM, > "RSDPBadRevisionId", > "RSDP: revision is %" PRIu8 ", expected " > - "value less than 2.", rsdp->revision); > + "value to be 0 or 2.", rsdp->revision); > + > + /* all proceeding tests involve fields which are only > + * defined in ACPI specifications 2.0 and greater, skip > + * if ACPI version is 1.0 */ > + if (rsdp->revision == 0) > + return FWTS_OK; > > /* whether RSDT or XSDT depends arch */ > if (rsdp->rsdt_address == 0 && rsdp->xsdt_address == 0) > diff --git a/src/acpi/rsdt/rsdt.c b/src/acpi/rsdt/rsdt.c > index bb28e20..3374b8b 100644 > --- a/src/acpi/rsdt/rsdt.c > +++ b/src/acpi/rsdt/rsdt.c > @@ -60,7 +60,7 @@ static int rsdt_test1(fwts_framework *fw) > "RSDTEntryNull", > "RSDT Entry %zd is null, should not be non-zero.", i); > fwts_advice(fw, > - "A XSDT pointer is null and therefore erroneously " > + "A RSDT pointer is null and therefore erroneously " > "points to an invalid 32 bit ACPI table header. " > "At worse this will cause the kernel to oops, at " > "best the kernel may ignore this. However, it " > Acked-by: Ivan Hu <ivan.hu@canonical.com>
On 14/11/17 04:47, Alex Hung wrote: > From: Chris Goldsworthy <goldswo@amazon.com> > > Remove RSDP length test for ACPI 1.0 RSDPs. Fix RSDP revision field checks to > check against revision values 0 and 2 only. > > The RSDP length field test for ACPI 1.0 RSDPs is invalid, since a ACPI 1.0 RSDP > has no length field. FWTS also checks the RSDP revision number incorrectly - > the revision field of a ACPI 1.0 RSDP has a value of 0, not 1 (as per ACPI > specification version 6.2, page 122). > > Add descriptions for specific RSDP tests. > > Fix a typo in a RSDT error message. > > Signed-off-by: Christopher Goldsworthy <goldswo@amazon.de> > --- > src/acpi/rsdp/rsdp.c | 27 ++++++++++++--------------- > src/acpi/rsdt/rsdt.c | 2 +- > 2 files changed, 13 insertions(+), 16 deletions(-) > > diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c > index becdbb2..2f537be 100644 > --- a/src/acpi/rsdp/rsdp.c > +++ b/src/acpi/rsdp/rsdp.c > @@ -80,6 +80,7 @@ static int rsdp_test1(fwts_framework *fw) > "RSDPBadFirstChecksum", > "RSDP first checksum is incorrect: 0x%x", checksum); > > + /* ensure oem_id only contains printable characters */ > for (i = 0; i < 6; i++) { > if (!isprint(rsdp->oem_id[i])) { > passed = false; > @@ -102,27 +103,23 @@ static int rsdp_test1(fwts_framework *fw) > "firmware ACPI complaint."); > } > > - /* ACPI 6.1 errata clarifies revision 1 must have length 20 */ > - if (rsdp->revision == 1) { > - if (rsdp->length == 20) > - fwts_passed(fw, "RSDP: the table is the correct length."); > - else > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "RSDPBadLength", > - "RSDP: length of Revision 1 is %d bytes but should be 20.", > - rsdp->length); > - > - return FWTS_OK; > - } > - > - if (rsdp->revision <= 2) > + /* check if revision number is valid. note: revision field for > + * ACPI 1.0 RSDP is 0, not 1, as per ACPI specification version > + * 6.2, p.122. */ > + if (rsdp->revision == 0 || rsdp->revision == 2) > fwts_passed(fw, > "RSDP: revision is %" PRIu8 ".", rsdp->revision); > else > fwts_failed(fw, LOG_LEVEL_MEDIUM, > "RSDPBadRevisionId", > "RSDP: revision is %" PRIu8 ", expected " > - "value less than 2.", rsdp->revision); > + "value to be 0 or 2.", rsdp->revision); > + > + /* all proceeding tests involve fields which are only > + * defined in ACPI specifications 2.0 and greater, skip > + * if ACPI version is 1.0 */ > + if (rsdp->revision == 0) > + return FWTS_OK; > > /* whether RSDT or XSDT depends arch */ > if (rsdp->rsdt_address == 0 && rsdp->xsdt_address == 0) > diff --git a/src/acpi/rsdt/rsdt.c b/src/acpi/rsdt/rsdt.c > index bb28e20..3374b8b 100644 > --- a/src/acpi/rsdt/rsdt.c > +++ b/src/acpi/rsdt/rsdt.c > @@ -60,7 +60,7 @@ static int rsdt_test1(fwts_framework *fw) > "RSDTEntryNull", > "RSDT Entry %zd is null, should not be non-zero.", i); > fwts_advice(fw, > - "A XSDT pointer is null and therefore erroneously " > + "A RSDT pointer is null and therefore erroneously " > "points to an invalid 32 bit ACPI table header. " > "At worse this will cause the kernel to oops, at " > "best the kernel may ignore this. However, it " > Acked-by: Colin Ian King <colin.king@canonical.com>
diff --git a/src/acpi/rsdp/rsdp.c b/src/acpi/rsdp/rsdp.c index becdbb2..2f537be 100644 --- a/src/acpi/rsdp/rsdp.c +++ b/src/acpi/rsdp/rsdp.c @@ -80,6 +80,7 @@ static int rsdp_test1(fwts_framework *fw) "RSDPBadFirstChecksum", "RSDP first checksum is incorrect: 0x%x", checksum); + /* ensure oem_id only contains printable characters */ for (i = 0; i < 6; i++) { if (!isprint(rsdp->oem_id[i])) { passed = false; @@ -102,27 +103,23 @@ static int rsdp_test1(fwts_framework *fw) "firmware ACPI complaint."); } - /* ACPI 6.1 errata clarifies revision 1 must have length 20 */ - if (rsdp->revision == 1) { - if (rsdp->length == 20) - fwts_passed(fw, "RSDP: the table is the correct length."); - else - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "RSDPBadLength", - "RSDP: length of Revision 1 is %d bytes but should be 20.", - rsdp->length); - - return FWTS_OK; - } - - if (rsdp->revision <= 2) + /* check if revision number is valid. note: revision field for + * ACPI 1.0 RSDP is 0, not 1, as per ACPI specification version + * 6.2, p.122. */ + if (rsdp->revision == 0 || rsdp->revision == 2) fwts_passed(fw, "RSDP: revision is %" PRIu8 ".", rsdp->revision); else fwts_failed(fw, LOG_LEVEL_MEDIUM, "RSDPBadRevisionId", "RSDP: revision is %" PRIu8 ", expected " - "value less than 2.", rsdp->revision); + "value to be 0 or 2.", rsdp->revision); + + /* all proceeding tests involve fields which are only + * defined in ACPI specifications 2.0 and greater, skip + * if ACPI version is 1.0 */ + if (rsdp->revision == 0) + return FWTS_OK; /* whether RSDT or XSDT depends arch */ if (rsdp->rsdt_address == 0 && rsdp->xsdt_address == 0) diff --git a/src/acpi/rsdt/rsdt.c b/src/acpi/rsdt/rsdt.c index bb28e20..3374b8b 100644 --- a/src/acpi/rsdt/rsdt.c +++ b/src/acpi/rsdt/rsdt.c @@ -60,7 +60,7 @@ static int rsdt_test1(fwts_framework *fw) "RSDTEntryNull", "RSDT Entry %zd is null, should not be non-zero.", i); fwts_advice(fw, - "A XSDT pointer is null and therefore erroneously " + "A RSDT pointer is null and therefore erroneously " "points to an invalid 32 bit ACPI table header. " "At worse this will cause the kernel to oops, at " "best the kernel may ignore this. However, it "