[1/2] Remove length test for ACPI 1.0 RSDPs, fix checks against revision field

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
Related show

Commit Message

Alex Hung Nov. 14, 2017, 4:47 a.m.
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(-)

Comments

ivanhu Nov. 15, 2017, 9:43 a.m. | #1
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>
Colin King Nov. 16, 2017, 12:02 p.m. | #2
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>

Patch

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 "