diff mbox

[06/10] acpi: acpitables: add SBBR compliance tests

Message ID 1500963057-4225-7-git-send-email-Sakar.Arora@arm.com
State Superseded
Headers show

Commit Message

Sakar Arora July 25, 2017, 6:10 a.m. UTC
From: Rajat Goyal <Rajat.Goyal@arm.com>

Server Base Boot Requirements (SBBR) specification is intended for SBSA-
compliant 64-bit ARMv8 servers.
It defines the base firmware requirements for out-of-box support of any
ARM SBSA-compatible Operating System or hypervisor.
The requirements in this specification are expected to be minimal yet
complete for booting a multi-core ARMv8 server platform, while leaving
plenty of room for OEM or ODM innovations and design details.
For more information, download the SBBR specification here:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b/index.html

This change introduces additional test cases as per sbbr.
Additional tests include
1. Test that processors only exist in the _SB namespace.
2. Test DSDT and SSDT tables are implemented.
3. Check for recommended ACPI tables.

Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Signed-off-by: Rajat Goyal <Rajat.Goyal@arm.com>
---
 src/Makefile.am                  |   1 +
 src/acpi/acpitables/acpitables.c |   2 +-
 src/sbbr/acpitables/acpitables.c | 264 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 266 insertions(+), 1 deletion(-)
 create mode 100644 src/sbbr/acpitables/acpitables.c

Comments

Alex Hung Aug. 4, 2017, 12:17 a.m. UTC | #1
On 2017-07-24 11:10 PM, Sakar Arora wrote:
> From: Rajat Goyal <Rajat.Goyal@arm.com>
> 
> Server Base Boot Requirements (SBBR) specification is intended for SBSA-
> compliant 64-bit ARMv8 servers.
> It defines the base firmware requirements for out-of-box support of any
> ARM SBSA-compatible Operating System or hypervisor.
> The requirements in this specification are expected to be minimal yet
> complete for booting a multi-core ARMv8 server platform, while leaving
> plenty of room for OEM or ODM innovations and design details.
> For more information, download the SBBR specification here:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b/index.html
> 
> This change introduces additional test cases as per sbbr.
> Additional tests include
> 1. Test that processors only exist in the _SB namespace.
> 2. Test DSDT and SSDT tables are implemented.
> 3. Check for recommended ACPI tables.
> 
> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> Signed-off-by: Rajat Goyal <Rajat.Goyal@arm.com>
> ---
>   src/Makefile.am                  |   1 +
>   src/acpi/acpitables/acpitables.c |   2 +-
>   src/sbbr/acpitables/acpitables.c | 264 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 266 insertions(+), 1 deletion(-)
>   create mode 100644 src/sbbr/acpitables/acpitables.c
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index bd38841..4e9d774 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -48,6 +48,7 @@ fwts_SOURCES = main.c 				\
>   	acpi/acpidump/acpidump.c 		\
>   	acpi/acpiinfo/acpiinfo.c 		\
>   	acpi/acpitables/acpitables.c 		\
> +	sbbr/acpitables/acpitables.c 		\
>   	acpi/apicinstance/apicinstance.c 	\
>   	acpi/asf/asf.c				\
>   	acpi/aspt/aspt.c			\
> diff --git a/src/acpi/acpitables/acpitables.c b/src/acpi/acpitables/acpitables.c
> index 1fcf32a..04a283f 100644
> --- a/src/acpi/acpitables/acpitables.c
> +++ b/src/acpi/acpitables/acpitables.c
> @@ -127,6 +127,6 @@ static fwts_framework_ops acpi_table_check_ops = {
>   	.minor_tests = acpi_table_check_tests
>   };
>   
> -FWTS_REGISTER("acpitables", &acpi_table_check_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_TEST_ACPI)
> +FWTS_REGISTER("acpitables", &acpi_table_check_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_TEST_ACPI | FWTS_FLAG_TEST_SBBR)
>   
>   #endif
> diff --git a/src/sbbr/acpitables/acpitables.c b/src/sbbr/acpitables/acpitables.c
> new file mode 100644
> index 0000000..b0b9140
> --- /dev/null
> +++ b/src/sbbr/acpitables/acpitables.c
> @@ -0,0 +1,264 @@
> +/*
> + * Copyright (C) 2010-2017 Canonical
> + * Copyright (C) 2017      ARM Ltd
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + *
> + */
> +#include "fwts.h"
> +
> +#if defined(FWTS_HAS_ACPI)
> +#include "acpi.h"
> +#include "accommon.h"
> +#include "acnamesp.h"
> +#include "actables.h"
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <dirent.h>
> +#include <ctype.h>
> +#include <unistd.h>
> +#include <inttypes.h>
> +
> +#define TABLE_NAME_LEN (16)
> +#define MIN_SIG        ( 4)
> +#define OEM_ID         ( 6)
> +#define OEM_TABLE_ID   ( 8)
> +#define OEM_CREATOR_ID ( 4)
> +
> +static bool acpi_table_check_field(const char *field, const size_t len)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < len; i++)
> +		if (!isascii(field[i]))
> +			return false;
> +
> +	return true;
> +}
> +
> +static bool acpi_table_check_field_test(
> +	fwts_framework *fw,
> +	const char *table_name,
> +	const char *field_name,
> +	const char *field,
> +	const size_t len)
> +{
> +	if (!acpi_table_check_field(field, len)) {
> +		fwts_failed(fw, LOG_LEVEL_LOW, "ACPITableHdrInfo",
> +			"ACPI Table %s has non-ASCII characters in "
> +			"header field %s\n", table_name, field_name);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +/* Callback function used when searching for processor devices in namespace. */
> +static ACPI_STATUS processor_handler(ACPI_HANDLE ObjHandle, uint32_t level, void *context,
> +                              void **returnvalue)
> +{
> +	ACPI_NAMESPACE_NODE *node = (ACPI_NAMESPACE_NODE *)ObjHandle;
> +	ACPI_NAMESPACE_NODE *parent = node->Parent;
> +	int error_count;
> +
> +	/* Unused parameters trigger errors. */
> +	FWTS_UNUSED(level);
> +	FWTS_UNUSED(context);
> +
> +	/* If the processor device is not located under _SB_, increment the error_count. */
> +	if (strncmp(parent->Name.Ascii, "_SB_", sizeof(int32_t)) != 0) {
> +		error_count = *((int *)returnvalue);
> +		error_count++;
> +		*((int *)returnvalue) = error_count;
> +	}
> +
> +	/* Return 0 so namespace search continues. */
> +	return 0;
> +}
> +
> +/* Test function that makes sure processors are under the _SB_ namespace. */
> +static int acpi_table_sbbr_namespace_check_test1(fwts_framework *fw)
> +{
> +	int error_count = 0;
> +
> +	/* Initializing ACPICA library so we can call AcpiWalkNamespace. */
> +	if (fwts_acpica_init(fw) != FWTS_OK)
> +		return FWTS_ERROR;
> +
> +	/* Searching for all processor devices in the namespace. */
> +	AcpiWalkNamespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
> +	                  processor_handler, NULL, NULL, (void **)&error_count);
> +
> +	/* Deinitializing ACPICA, if we don't call this the terminal will break on exit. */
> +	fwts_acpica_deinit();
> +
> +	/* error_count variable counts the number of processors outside of the _SB_ namespace. */
> +	if (error_count > 0)
> +		fwts_failed(fw, LOG_LEVEL_HIGH, "SbbrAcpiCpuWrongNamespace", "%d Processor devices "
> +		            "were found outside of the _SB_ namespace.", error_count);
> +	else
> +		fwts_passed(fw, "All processor devices were located in the _SB_ namespace.");
> +
> +	return FWTS_OK;
> +}
> +
> +static int acpi_table_sbbr_check_test2(fwts_framework *fw)
> +{
> +	int i;
> +	bool checked = false;
> +	bool dsdt_checked = false;
> +	bool ssdt_checked = false;
> +
> +	for (i = 0; ; i++) {
> +		fwts_acpi_table_info *info;
> +		fwts_acpi_table_header *hdr;
> +		char name[TABLE_NAME_LEN];
> +		bool passed = false;
> +
> +		if (fwts_acpi_get_table(fw, i, &info) != FWTS_OK)
> +			break;
> +		if (info == NULL)
> +			continue;
> +
> +		checked = true;
> +		if (!strcmp(info->name, "DSDT") ||
> +			!strcmp(info->name, "SSDT")) {
> +			if (!strcmp(info->name, "DSDT")) {
> +				dsdt_checked = true;
> +			}
> +			if (!strcmp(info->name, "SSDT")) {
> +				ssdt_checked = true;
> +			}
> +			hdr = (fwts_acpi_table_header *)info->data;
> +			if (acpi_table_check_field(hdr->signature, MIN_SIG)) {
> +				snprintf(name, sizeof(name), "%4.4s", hdr->signature);
> +			} else {
> +				/* Table name not printable, so identify it by the address */
> +				snprintf(name, sizeof(name), "at address 0x%" PRIx64, info->addr);
> +			}
> +
> +			/*
> +			 * Tables shouldn't be short, however, they do have at
> +			 * least 4 bytes with their signature else they would not
> +			 * have been loaded by this stage.
> +			 */
> +			if (hdr->length < sizeof(fwts_acpi_table_header)) {
> +				fwts_failed(fw, LOG_LEVEL_HIGH, "ACPITableHdrShort",
> +					"ACPI Table %s is too short, only %d bytes long. Further "
> +					"header checks will be omitted.", name, hdr->length);
> +				continue;
> +			}
> +			/* Warn about empty tables */
> +			if (hdr->length == sizeof(fwts_acpi_table_header)) {
> +				fwts_warning(fw,
> +					"ACPI Table %s is empty and just contains a table header. Further "
> +					"header checks will be omitted.", name);
> +				continue;
> +			}
> +
> +			passed = acpi_table_check_field_test(fw, name, "Signature", hdr->signature, MIN_SIG) &
> +			    acpi_table_check_field_test(fw, name, "OEM ID", hdr->oem_id, OEM_ID) &
> +			    acpi_table_check_field_test(fw, name, "OEM Table ID", hdr->oem_tbl_id, OEM_TABLE_ID) &
> +			    acpi_table_check_field_test(fw, name, "OEM Creator ID", hdr->creator_id, OEM_CREATOR_ID);
> +			if (passed)
> +				fwts_passed(fw, "Table %s has valid signature and ID strings.", name);
> +		}
> +	}
> +	if (!checked)
> +		fwts_aborted(fw, "Cannot find any ACPI tables.");
> +	if (!dsdt_checked) {
> +		fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
> +				"Test DSDT table is NOT implemented.");
> +	}
> +	if (!ssdt_checked) {
> +		fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
> +				"Test SSDT table is NOT implemented.");
> +	}

SSDT is usually optional. Is it mandatory in SBBR?

> +	if ((!dsdt_checked) || (!ssdt_checked))
> +	  return FWTS_ERROR;
> +
> +	return FWTS_OK;
> +}
> +
> +/* List of ACPI tables recommended by SBBR 4.2.2 */
> +char *recommended_acpi_tables[] = {
> +	"MCFG",
> +	"IORT",
> +	"BERT",
> +	"EINJ",
> +	"ERST",
> +	"HEST",
> +	"RASF",
> +	"SPMI",
> +	"SLIT",
> +	"SRAT",
> +	"CSRT",
> +	"ECDT",
> +	"MPST",
> +	"PCCT",
> +	NULL
> +};
> +
> +/* Searches ACPI tables by signature. */
> +fwts_acpi_table_info *sbbr_search_acpi_tables(fwts_framework *fw, char *signature)
> +{
> +	uint32_t i;
> +	fwts_acpi_table_info *info;
> +
> +	i = 0;
> +	while (fwts_acpi_get_table(fw, i, &info) == FWTS_OK) {
> +		if (info != NULL && strncmp(info->name, signature, sizeof(uint32_t)) == 0) {
> +			return info;
> +		}
> +		i++;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int acpi_table_sbbr_check_test3(fwts_framework *fw)
> +{
> +	uint32_t i;
> +	fwts_acpi_table_info *info;
> +
> +	for (i = 0; recommended_acpi_tables[i] != NULL; i++) {
> +		info = sbbr_search_acpi_tables(fw, recommended_acpi_tables[i]);
> +		if (info == NULL) {
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "SbbrAcpiRecommendedTableNotFound",
> +			            "SBBR Recommended ACPI table \"%s\" not found.",
> +			            recommended_acpi_tables[i]);
> +		} else {
> +			fwts_passed(fw, "SBBR Recommended ACPI table \"%s\" found.",
> +			            recommended_acpi_tables[i]);
> +		}
> +	}
> +	return FWTS_OK;
> +}
> +
> +static fwts_framework_minor_test acpi_table_sbbr_check_tests[] = {
> +	{ acpi_table_sbbr_namespace_check_test1, "Test that processors only exist in the _SB namespace." },
> +	{ acpi_table_sbbr_check_test2, "Test DSDT and SSDT tables are implemented." },
> +	{ acpi_table_sbbr_check_test3, "Check for recommended ACPI tables." },
> +	{ NULL, NULL }
> +};
> +
> +static fwts_framework_ops acpi_table_sbbr_check_ops = {
> +	.description = "ACPI table headers sanity tests.",
> +	.minor_tests = acpi_table_sbbr_check_tests
> +};
> +
> +FWTS_REGISTER("acpi_sbbr", &acpi_table_sbbr_check_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_SBBR)
> +
> +#endif
>
Al Stone Aug. 4, 2017, 9:55 p.m. UTC | #2
On 08/03/2017 06:17 PM, Alex Hung wrote:
> On 2017-07-24 11:10 PM, Sakar Arora wrote:
>> From: Rajat Goyal <Rajat.Goyal@arm.com>
>>
>> Server Base Boot Requirements (SBBR) specification is intended for SBSA-
>> compliant 64-bit ARMv8 servers.
>> It defines the base firmware requirements for out-of-box support of any
>> ARM SBSA-compatible Operating System or hypervisor.
>> The requirements in this specification are expected to be minimal yet
>> complete for booting a multi-core ARMv8 server platform, while leaving
>> plenty of room for OEM or ODM innovations and design details.
>> For more information, download the SBBR specification here:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b/index.html
>>
>> This change introduces additional test cases as per sbbr.
>> Additional tests include
>> 1. Test that processors only exist in the _SB namespace.
>> 2. Test DSDT and SSDT tables are implemented.
>> 3. Check for recommended ACPI tables.
>>
>> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
>> Signed-off-by: Rajat Goyal <Rajat.Goyal@arm.com>
>> ---
>>   src/Makefile.am                  |   1 +
>>   src/acpi/acpitables/acpitables.c |   2 +-
>>   src/sbbr/acpitables/acpitables.c | 264 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 266 insertions(+), 1 deletion(-)
>>   create mode 100644 src/sbbr/acpitables/acpitables.c
>>[snip....]
>> +    }
>> +    if (!checked)
>> +        fwts_aborted(fw, "Cannot find any ACPI tables.");
>> +    if (!dsdt_checked) {
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
>> +                "Test DSDT table is NOT implemented.");
>> +    }
>> +    if (!ssdt_checked) {
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
>> +                "Test SSDT table is NOT implemented.");
>> +    }
> 
> SSDT is usually optional. Is it mandatory in SBBR?

AFAIK, no.  If I misread the language somehow, then the SBBR needs to be fixed,
I believe.  The DSDT should be the only required AML table; SSDTs should be
entirely optional.
Sakar Arora Aug. 7, 2017, 12:33 p.m. UTC | #3
Thanks for the comments.
The SBBR spec requires SSDT tables as mandatory along with the DSDT tables.
http://infocenter.arm.com/help/topic/com.arm.doc.den0044b/DEN0044B_Server_Base_Boot_Requirements.pdf (section 4.2.1.4)

Regards,
Sakar
-----Original Message-----
From: fwts-devel-bounces@lists.ubuntu.com [mailto:fwts-devel-bounces@lists.ubuntu.com] On Behalf Of Al Stone
Sent: Saturday, August 5, 2017 3:25 AM
To: Alex Hung <alex.hung@canonical.com>; fwts-devel@lists.ubuntu.com
Subject: Re: [PATCH 06/10] acpi: acpitables: add SBBR compliance tests

On 08/03/2017 06:17 PM, Alex Hung wrote:
> On 2017-07-24 11:10 PM, Sakar Arora wrote:
>> From: Rajat Goyal <Rajat.Goyal@arm.com>
>>
>> Server Base Boot Requirements (SBBR) specification is intended for
>> SBSA- compliant 64-bit ARMv8 servers.
>> It defines the base firmware requirements for out-of-box support of
>> any ARM SBSA-compatible Operating System or hypervisor.
>> The requirements in this specification are expected to be minimal yet
>> complete for booting a multi-core ARMv8 server platform, while
>> leaving plenty of room for OEM or ODM innovations and design details.
>> For more information, download the SBBR specification here:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b/
>> index.html
>>
>> This change introduces additional test cases as per sbbr.
>> Additional tests include
>> 1. Test that processors only exist in the _SB namespace.
>> 2. Test DSDT and SSDT tables are implemented.
>> 3. Check for recommended ACPI tables.
>>
>> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
>> Signed-off-by: Rajat Goyal <Rajat.Goyal@arm.com>
>> ---
>>   src/Makefile.am                  |   1 +
>>   src/acpi/acpitables/acpitables.c |   2 +-
>>   src/sbbr/acpitables/acpitables.c | 264 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 266 insertions(+), 1 deletion(-)
>>   create mode 100644 src/sbbr/acpitables/acpitables.c [snip....]
>> +    }
>> +    if (!checked)
>> +        fwts_aborted(fw, "Cannot find any ACPI tables.");
>> +    if (!dsdt_checked) {
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
>> +                "Test DSDT table is NOT implemented.");
>> +    }
>> +    if (!ssdt_checked) {
>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
>> +                "Test SSDT table is NOT implemented.");
>> +    }
>
> SSDT is usually optional. Is it mandatory in SBBR?

AFAIK, no.  If I misread the language somehow, then the SBBR needs to be fixed, I believe.  The DSDT should be the only required AML table; SSDTs should be entirely optional.

--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------

--
fwts-devel mailing list
fwts-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Al Stone Aug. 7, 2017, 4:10 p.m. UTC | #4
On 08/07/2017 06:33 AM, Sakar Arora wrote:
> Thanks for the comments.
> The SBBR spec requires SSDT tables as mandatory along with the DSDT tables.
> http://infocenter.arm.com/help/topic/com.arm.doc.den0044b/DEN0044B_Server_Base_Boot_Requirements.pdf (section 4.2.1.4)
> 
> Regards,
> Sakar

Yeah, that section is misleading.  SSDT is listed as a mandatory table and it
really should not be; the text does say "can" (vs "must") but that's not really
enough.  The SBBR needs to be fixed, IMHO.  I can see how it can be read as
mandatory, but that's really very impractical and kind of pointless.  Many of
the vendors do not have an SSDT and would have no reason to provide one, making
the test a false negative.

If you could poke Charles Garcia-Tobin at ARM about this, I'd really appreciate
it.  I can work with him on the change, if he wants.

In the meantime, I'd suggest having an SSDT be optional in this test suite; if
necessary, a warning and/or explanation could be printed.

> -----Original Message-----
> From: fwts-devel-bounces@lists.ubuntu.com [mailto:fwts-devel-bounces@lists.ubuntu.com] On Behalf Of Al Stone
> Sent: Saturday, August 5, 2017 3:25 AM
> To: Alex Hung <alex.hung@canonical.com>; fwts-devel@lists.ubuntu.com
> Subject: Re: [PATCH 06/10] acpi: acpitables: add SBBR compliance tests
> 
> On 08/03/2017 06:17 PM, Alex Hung wrote:
>> On 2017-07-24 11:10 PM, Sakar Arora wrote:
>>> From: Rajat Goyal <Rajat.Goyal@arm.com>
>>>
>>> Server Base Boot Requirements (SBBR) specification is intended for
>>> SBSA- compliant 64-bit ARMv8 servers.
>>> It defines the base firmware requirements for out-of-box support of
>>> any ARM SBSA-compatible Operating System or hypervisor.
>>> The requirements in this specification are expected to be minimal yet
>>> complete for booting a multi-core ARMv8 server platform, while
>>> leaving plenty of room for OEM or ODM innovations and design details.
>>> For more information, download the SBBR specification here:
>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b/
>>> index.html
>>>
>>> This change introduces additional test cases as per sbbr.
>>> Additional tests include
>>> 1. Test that processors only exist in the _SB namespace.
>>> 2. Test DSDT and SSDT tables are implemented.
>>> 3. Check for recommended ACPI tables.
>>>
>>> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
>>> Signed-off-by: Rajat Goyal <Rajat.Goyal@arm.com>
>>> ---
>>>   src/Makefile.am                  |   1 +
>>>   src/acpi/acpitables/acpitables.c |   2 +-
>>>   src/sbbr/acpitables/acpitables.c | 264 +++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 266 insertions(+), 1 deletion(-)
>>>   create mode 100644 src/sbbr/acpitables/acpitables.c [snip....]
>>> +    }
>>> +    if (!checked)
>>> +        fwts_aborted(fw, "Cannot find any ACPI tables.");
>>> +    if (!dsdt_checked) {
>>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
>>> +                "Test DSDT table is NOT implemented.");
>>> +    }
>>> +    if (!ssdt_checked) {
>>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
>>> +                "Test SSDT table is NOT implemented.");
>>> +    }
>>
>> SSDT is usually optional. Is it mandatory in SBBR?
> 
> AFAIK, no.  If I misread the language somehow, then the SBBR needs to be fixed, I believe.  The DSDT should be the only required AML table; SSDTs should be entirely optional.
> 
> --
> ciao,
> al
> -----------------------------------
> Al Stone
> Software Engineer
> Linaro Enterprise Group
> al.stone@linaro.org
> -----------------------------------
> 
> --
> fwts-devel mailing list
> fwts-devel@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>
Supreeth Venkatesh Aug. 7, 2017, 4:18 p.m. UTC | #5
Al,

Thanks for the input.
We will talk to Charles/Dong.

Supreeth

-----Original Message-----
From: Al Stone [mailto:al.stone@linaro.org]
Sent: Monday, August 7, 2017 11:10 AM
To: Sakar Arora <Sakar.Arora@arm.com>; Alex Hung <alex.hung@canonical.com>; fwts-devel@lists.ubuntu.com
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; Prasanth Pulla <Prasanth.Pulla@arm.com>; Charles Garcia-Tobin <Charles.Garcia-Tobin@arm.com>
Subject: Re: [PATCH 06/10] acpi: acpitables: add SBBR compliance tests

On 08/07/2017 06:33 AM, Sakar Arora wrote:
> Thanks for the comments.
> The SBBR spec requires SSDT tables as mandatory along with the DSDT tables.
> http://infocenter.arm.com/help/topic/com.arm.doc.den0044b/DEN0044B_Ser
> ver_Base_Boot_Requirements.pdf (section 4.2.1.4)
>
> Regards,
> Sakar

Yeah, that section is misleading.  SSDT is listed as a mandatory table and it really should not be; the text does say "can" (vs "must") but that's not really enough.  The SBBR needs to be fixed, IMHO.  I can see how it can be read as mandatory, but that's really very impractical and kind of pointless.  Many of the vendors do not have an SSDT and would have no reason to provide one, making the test a false negative.

If you could poke Charles Garcia-Tobin at ARM about this, I'd really appreciate it.  I can work with him on the change, if he wants.

In the meantime, I'd suggest having an SSDT be optional in this test suite; if necessary, a warning and/or explanation could be printed.

> -----Original Message-----
> From: fwts-devel-bounces@lists.ubuntu.com
> [mailto:fwts-devel-bounces@lists.ubuntu.com] On Behalf Of Al Stone
> Sent: Saturday, August 5, 2017 3:25 AM
> To: Alex Hung <alex.hung@canonical.com>; fwts-devel@lists.ubuntu.com
> Subject: Re: [PATCH 06/10] acpi: acpitables: add SBBR compliance tests
>
> On 08/03/2017 06:17 PM, Alex Hung wrote:
>> On 2017-07-24 11:10 PM, Sakar Arora wrote:
>>> From: Rajat Goyal <Rajat.Goyal@arm.com>
>>>
>>> Server Base Boot Requirements (SBBR) specification is intended for
>>> SBSA- compliant 64-bit ARMv8 servers.
>>> It defines the base firmware requirements for out-of-box support of
>>> any ARM SBSA-compatible Operating System or hypervisor.
>>> The requirements in this specification are expected to be minimal
>>> yet complete for booting a multi-core ARMv8 server platform, while
>>> leaving plenty of room for OEM or ODM innovations and design details.
>>> For more information, download the SBBR specification here:
>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b
>>> /
>>> index.html
>>>
>>> This change introduces additional test cases as per sbbr.
>>> Additional tests include
>>> 1. Test that processors only exist in the _SB namespace.
>>> 2. Test DSDT and SSDT tables are implemented.
>>> 3. Check for recommended ACPI tables.
>>>
>>> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
>>> Signed-off-by: Rajat Goyal <Rajat.Goyal@arm.com>
>>> ---
>>>   src/Makefile.am                  |   1 +
>>>   src/acpi/acpitables/acpitables.c |   2 +-
>>>   src/sbbr/acpitables/acpitables.c | 264 +++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 266 insertions(+), 1 deletion(-)
>>>   create mode 100644 src/sbbr/acpitables/acpitables.c [snip....]
>>> +    }
>>> +    if (!checked)
>>> +        fwts_aborted(fw, "Cannot find any ACPI tables.");
>>> +    if (!dsdt_checked) {
>>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
>>> +                "Test DSDT table is NOT implemented.");
>>> +    }
>>> +    if (!ssdt_checked) {
>>> +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
>>> +                "Test SSDT table is NOT implemented.");
>>> +    }
>>
>> SSDT is usually optional. Is it mandatory in SBBR?
>
> AFAIK, no.  If I misread the language somehow, then the SBBR needs to be fixed, I believe.  The DSDT should be the only required AML table; SSDTs should be entirely optional.
>
> --
> ciao,
> al
> -----------------------------------
> Al Stone
> Software Engineer
> Linaro Enterprise Group
> al.stone@linaro.org
> -----------------------------------
>
> --
> fwts-devel mailing list
> fwts-devel@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/fwts-devel
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>


--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Al Stone Aug. 7, 2017, 5:04 p.m. UTC | #6
On 08/07/2017 10:53 AM, Dong Wei wrote:
> Yes, SSDT is optional.
> 
>  
> 
> How about we change the statement “Additional definition blocks can be provided
> through SSDT tables” to “This table is optional. One or more of this table can
> be used to provide additional definition blocks”?
> 
>  
> 
> Also in 4.4.2, change “DSDT and SSDT” to “DSDT or SSDT”.
> 
>  
> 
> Dong

That works for me.  I might reword the last sentence a little to "This table is
optional.  One or more of these tables can be used to provide additional
definition blocks, but a fully functional and compliant system could have none
at all."

Or something like that to make it clearer, I guess....

Thanks, Dong.

> *From: *Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
> *Date: *Monday, August 7, 2017 at 5:18 PM
> *To: *Al Stone <al.stone@linaro.org>, Sakar Arora <Sakar.Arora@arm.com>, Alex
> Hung <alex.hung@canonical.com>, "fwts-devel@lists.ubuntu.com"
> <fwts-devel@lists.ubuntu.com>
> *Cc: *Prasanth Pulla <Prasanth.Pulla@arm.com>, Charles Garcia-Tobin
> <Charles.Garcia-Tobin@arm.com>, Dong Wei <Dong.Wei@arm.com>
> *Subject: *RE: [PATCH 06/10] acpi: acpitables: add SBBR compliance tests
> 
>  
> 
> Al,
> 
>  
> 
> Thanks for the input.
> 
> We will talk to Charles/Dong.
> 
>  
> 
> Supreeth
> 
>  
> 
> -----Original Message-----
> 
> From: Al Stone [mailto:al.stone@linaro.org]
> 
> Sent: Monday, August 7, 2017 11:10 AM
> 
> To: Sakar Arora <Sakar.Arora@arm.com <mailto:Sakar.Arora@arm.com>>; Alex Hung
> <alex.hung@canonical.com <mailto:alex.hung@canonical.com>>;
> fwts-devel@lists.ubuntu.com <mailto:fwts-devel@lists.ubuntu.com>
> 
> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com
> <mailto:Supreeth.Venkatesh@arm.com>>; Prasanth Pulla <Prasanth.Pulla@arm.com
> <mailto:Prasanth.Pulla@arm.com>>; Charles Garcia-Tobin
> <Charles.Garcia-Tobin@arm.com <mailto:Charles.Garcia-Tobin@arm.com>>
> 
> Subject: Re: [PATCH 06/10] acpi: acpitables: add SBBR compliance tests
> 
>  
> 
> On 08/07/2017 06:33 AM, Sakar Arora wrote:
> 
>     Thanks for the comments.
> 
>     The SBBR spec requires SSDT tables as mandatory along with the DSDT tables.
> 
>     http://infocenter.arm.com/help/topic/com.arm.doc.den0044b/DEN0044B_Ser
> 
>     ver_Base_Boot_Requirements.pdf (section 4.2.1.4)
> 
>     Regards,
> 
>     Sakar
> 
>  
> 
> Yeah, that section is misleading.  SSDT is listed as a mandatory table and it
> really should not be; the text does say "can" (vs "must") but that's not really
> enough.  The SBBR needs to be fixed, IMHO.  I can see how it can be read as
> mandatory, but that's really very impractical and kind of pointless.  Many of
> the vendors do not have an SSDT and would have no reason to provide one, making
> the test a false negative.
> 
>  
> 
> If you could poke Charles Garcia-Tobin at ARM about this, I'd really appreciate
> it.  I can work with him on the change, if he wants.
> 
>  
> 
> In the meantime, I'd suggest having an SSDT be optional in this test suite; if
> necessary, a warning and/or explanation could be printed.
> 
>  
> 
>     -----Original Message-----
> 
>     From: fwts-devel-bounces@lists.ubuntu.com
>     <mailto:fwts-devel-bounces@lists.ubuntu.com>
> 
>     [mailto:fwts-devel-bounces@lists.ubuntu.com] On Behalf Of Al Stone
> 
>     Sent: Saturday, August 5, 2017 3:25 AM
> 
>     To: Alex Hung <alex.hung@canonical.com <mailto:alex.hung@canonical.com>>;
>     fwts-devel@lists.ubuntu.com <mailto:fwts-devel@lists.ubuntu.com>
> 
>     Subject: Re: [PATCH 06/10] acpi: acpitables: add SBBR compliance tests
> 
>     On 08/03/2017 06:17 PM, Alex Hung wrote:
> 
>         On 2017-07-24 11:10 PM, Sakar Arora wrote:
> 
>             From: Rajat Goyal <Rajat.Goyal@arm.com <mailto:Rajat.Goyal@arm.com>>
> 
>              
> 
>             Server Base Boot Requirements (SBBR) specification is intended for
> 
>             SBSA- compliant 64-bit ARMv8 servers.
> 
>             It defines the base firmware requirements for out-of-box support of
> 
>             any ARM SBSA-compatible Operating System or hypervisor.
> 
>             The requirements in this specification are expected to be minimal
> 
>             yet complete for booting a multi-core ARMv8 server platform, while
> 
>             leaving plenty of room for OEM or ODM innovations and design details.
> 
>             For more information, download the SBBR specification here:
> 
>             http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b
> 
>             /
> 
>             index.html
> 
>              
> 
>             This change introduces additional test cases as per sbbr.
> 
>             Additional tests include
> 
>             1. Test that processors only exist in the _SB namespace.
> 
>             2. Test DSDT and SSDT tables are implemented.
> 
>             3. Check for recommended ACPI tables.
> 
>              
> 
>             Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com
>             <mailto:supreeth.venkatesh@arm.com>>
> 
>             Signed-off-by: Rajat Goyal <Rajat.Goyal@arm.com
>             <mailto:Rajat.Goyal@arm.com>>
> 
>             ---
> 
>                src/Makefile.am                  |   1 +
> 
>                src/acpi/acpitables/acpitables.c |   2 +-
> 
>                src/sbbr/acpitables/acpitables.c | 264
>             +++++++++++++++++++++++++++++++++++++++
> 
>                3 files changed, 266 insertions(+), 1 deletion(-)
> 
>                create mode 100644 src/sbbr/acpitables/acpitables.c [snip....]
> 
>             +    }
> 
>             +    if (!checked)
> 
>             +        fwts_aborted(fw, "Cannot find any ACPI tables.");
> 
>             +    if (!dsdt_checked) {
> 
>             +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
> 
>             +                "Test DSDT table is NOT implemented.");
> 
>             +    }
> 
>             +    if (!ssdt_checked) {
> 
>             +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
> 
>             +                "Test SSDT table is NOT implemented.");
> 
>             +    }
> 
>          
> 
>         SSDT is usually optional. Is it mandatory in SBBR?
> 
>     AFAIK, no.  If I misread the language somehow, then the SBBR needs to be
>     fixed, I believe.  The DSDT should be the only required AML table; SSDTs
>     should be entirely optional.
> 
>     --
> 
>     ciao,
> 
>     al
> 
>     -----------------------------------
> 
>     Al Stone
> 
>     Software Engineer
> 
>     Linaro Enterprise Group
> 
>     al.stone@linaro.org <mailto:al.stone@linaro.org>
> 
>     -----------------------------------
> 
>     --
> 
>     fwts-devel mailing list
> 
>     fwts-devel@lists.ubuntu.com <mailto:fwts-devel@lists.ubuntu.com>
> 
>     Modify settings or unsubscribe at:
> 
>     https://lists.ubuntu.com/mailman/listinfo/fwts-devel
> 
>     IMPORTANT NOTICE: The contents of this email and any attachments are
>     confidential and may also be privileged. If you are not the intended
>     recipient, please notify the sender immediately and do not disclose the
>     contents to any other person, use it for any purpose, or store or copy the
>     information in any medium. Thank you.
> 
>  
> 
>  
> 
> --
> 
> ciao,
> 
> al
> 
> -----------------------------------
> 
> Al Stone
> 
> Software Engineer
> 
> Linaro Enterprise Group
> 
> al.stone@linaro.org <mailto:al.stone@linaro.org>
> 
> -----------------------------------
> 
>  
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
Al Stone Aug. 7, 2017, 5:08 p.m. UTC | #7
Go for it :).  That looks good.

On 08/07/2017 11:06 AM, Dong Wei wrote:
> Or
> 
> "This table is optional.  One or more of these tables can be used to provide additional definition blocks if necessary."?
> 
> - DW
> -
> -----Original Message-----
> From: Al Stone [mailto:al.stone@linaro.org]
> Sent: Monday, August 7, 2017 10:04 AM
> To: Dong Wei <Dong.Wei@arm.com>; Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; Sakar Arora <Sakar.Arora@arm.com>; Alex Hung <alex.hung@canonical.com>; fwts-devel@lists.ubuntu.com
> Cc: Prasanth Pulla <Prasanth.Pulla@arm.com>; Charles Garcia-Tobin <Charles.Garcia-Tobin@arm.com>
> Subject: Re: [PATCH 06/10] acpi: acpitables: add SBBR compliance tests
> 
> On 08/07/2017 10:53 AM, Dong Wei wrote:
>> Yes, SSDT is optional.
>>
>>
>>
>> How about we change the statement “Additional definition blocks can be
>> provided through SSDT tables” to “This table is optional. One or more
>> of this table can be used to provide additional definition blocks”?
>>
>>
>>
>> Also in 4.4.2, change “DSDT and SSDT” to “DSDT or SSDT”.
>>
>>
>>
>> Dong
> 
> That works for me.  I might reword the last sentence a little to "This table is optional.  One or more of these tables can be used to provide additional definition blocks, but a fully functional and compliant system could have none at all."
> 
> Or something like that to make it clearer, I guess....
> 
> Thanks, Dong.
> 
>> *From: *Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
>> *Date: *Monday, August 7, 2017 at 5:18 PM
>> *To: *Al Stone <al.stone@linaro.org>, Sakar Arora
>> <Sakar.Arora@arm.com>, Alex Hung <alex.hung@canonical.com>, "fwts-devel@lists.ubuntu.com"
>> <fwts-devel@lists.ubuntu.com>
>> *Cc: *Prasanth Pulla <Prasanth.Pulla@arm.com>, Charles Garcia-Tobin
>> <Charles.Garcia-Tobin@arm.com>, Dong Wei <Dong.Wei@arm.com>
>> *Subject: *RE: [PATCH 06/10] acpi: acpitables: add SBBR compliance
>> tests
>>
>>
>>
>> Al,
>>
>>
>>
>> Thanks for the input.
>>
>> We will talk to Charles/Dong.
>>
>>
>>
>> Supreeth
>>
>>
>>
>> -----Original Message-----
>>
>> From: Al Stone [mailto:al.stone@linaro.org]
>>
>> Sent: Monday, August 7, 2017 11:10 AM
>>
>> To: Sakar Arora <Sakar.Arora@arm.com <mailto:Sakar.Arora@arm.com>>;
>> Alex Hung <alex.hung@canonical.com <mailto:alex.hung@canonical.com>>;
>> fwts-devel@lists.ubuntu.com <mailto:fwts-devel@lists.ubuntu.com>
>>
>> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com
>> <mailto:Supreeth.Venkatesh@arm.com>>; Prasanth Pulla
>> <Prasanth.Pulla@arm.com <mailto:Prasanth.Pulla@arm.com>>; Charles
>> Garcia-Tobin <Charles.Garcia-Tobin@arm.com
>> <mailto:Charles.Garcia-Tobin@arm.com>>
>>
>> Subject: Re: [PATCH 06/10] acpi: acpitables: add SBBR compliance tests
>>
>>
>>
>> On 08/07/2017 06:33 AM, Sakar Arora wrote:
>>
>>     Thanks for the comments.
>>
>>     The SBBR spec requires SSDT tables as mandatory along with the DSDT tables.
>>
>>
>> http://infocenter.arm.com/help/topic/com.arm.doc.den0044b/DEN0044B_Ser
>>
>>     ver_Base_Boot_Requirements.pdf (section 4.2.1.4)
>>
>>     Regards,
>>
>>     Sakar
>>
>>
>>
>> Yeah, that section is misleading.  SSDT is listed as a mandatory table
>> and it really should not be; the text does say "can" (vs "must") but
>> that's not really enough.  The SBBR needs to be fixed, IMHO.  I can
>> see how it can be read as mandatory, but that's really very
>> impractical and kind of pointless.  Many of the vendors do not have an
>> SSDT and would have no reason to provide one, making the test a false negative.
>>
>>
>>
>> If you could poke Charles Garcia-Tobin at ARM about this, I'd really
>> appreciate it.  I can work with him on the change, if he wants.
>>
>>
>>
>> In the meantime, I'd suggest having an SSDT be optional in this test
>> suite; if necessary, a warning and/or explanation could be printed.
>>
>>
>>
>>     -----Original Message-----
>>
>>     From: fwts-devel-bounces@lists.ubuntu.com
>>     <mailto:fwts-devel-bounces@lists.ubuntu.com>
>>
>>     [mailto:fwts-devel-bounces@lists.ubuntu.com] On Behalf Of Al Stone
>>
>>     Sent: Saturday, August 5, 2017 3:25 AM
>>
>>     To: Alex Hung <alex.hung@canonical.com <mailto:alex.hung@canonical.com>>;
>>     fwts-devel@lists.ubuntu.com <mailto:fwts-devel@lists.ubuntu.com>
>>
>>     Subject: Re: [PATCH 06/10] acpi: acpitables: add SBBR compliance
>> tests
>>
>>     On 08/03/2017 06:17 PM, Alex Hung wrote:
>>
>>         On 2017-07-24 11:10 PM, Sakar Arora wrote:
>>
>>             From: Rajat Goyal <Rajat.Goyal@arm.com
>> <mailto:Rajat.Goyal@arm.com>>
>>
>>
>>
>>             Server Base Boot Requirements (SBBR) specification is
>> intended for
>>
>>             SBSA- compliant 64-bit ARMv8 servers.
>>
>>             It defines the base firmware requirements for out-of-box
>> support of
>>
>>             any ARM SBSA-compatible Operating System or hypervisor.
>>
>>             The requirements in this specification are expected to be
>> minimal
>>
>>             yet complete for booting a multi-core ARMv8 server
>> platform, while
>>
>>             leaving plenty of room for OEM or ODM innovations and design details.
>>
>>             For more information, download the SBBR specification here:
>>
>>
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b
>>
>>             /
>>
>>             index.html
>>
>>
>>
>>             This change introduces additional test cases as per sbbr.
>>
>>             Additional tests include
>>
>>             1. Test that processors only exist in the _SB namespace.
>>
>>             2. Test DSDT and SSDT tables are implemented.
>>
>>             3. Check for recommended ACPI tables.
>>
>>
>>
>>             Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com
>>             <mailto:supreeth.venkatesh@arm.com>>
>>
>>             Signed-off-by: Rajat Goyal <Rajat.Goyal@arm.com
>>             <mailto:Rajat.Goyal@arm.com>>
>>
>>             ---
>>
>>                src/Makefile.am                  |   1 +
>>
>>                src/acpi/acpitables/acpitables.c |   2 +-
>>
>>                src/sbbr/acpitables/acpitables.c | 264
>>             +++++++++++++++++++++++++++++++++++++++
>>
>>                3 files changed, 266 insertions(+), 1 deletion(-)
>>
>>                create mode 100644 src/sbbr/acpitables/acpitables.c
>> [snip....]
>>
>>             +    }
>>
>>             +    if (!checked)
>>
>>             +        fwts_aborted(fw, "Cannot find any ACPI tables.");
>>
>>             +    if (!dsdt_checked) {
>>
>>             +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
>>
>>             +                "Test DSDT table is NOT implemented.");
>>
>>             +    }
>>
>>             +    if (!ssdt_checked) {
>>
>>             +        fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
>>
>>             +                "Test SSDT table is NOT implemented.");
>>
>>             +    }
>>
>>
>>
>>         SSDT is usually optional. Is it mandatory in SBBR?
>>
>>     AFAIK, no.  If I misread the language somehow, then the SBBR needs to be
>>     fixed, I believe.  The DSDT should be the only required AML table; SSDTs
>>     should be entirely optional.
>>
>>     --
>>
>>     ciao,
>>
>>     al
>>
>>     -----------------------------------
>>
>>     Al Stone
>>
>>     Software Engineer
>>
>>     Linaro Enterprise Group
>>
>>     al.stone@linaro.org <mailto:al.stone@linaro.org>
>>
>>     -----------------------------------
>>
>>     --
>>
>>     fwts-devel mailing list
>>
>>     fwts-devel@lists.ubuntu.com <mailto:fwts-devel@lists.ubuntu.com>
>>
>>     Modify settings or unsubscribe at:
>>
>>     https://lists.ubuntu.com/mailman/listinfo/fwts-devel
>>
>>     IMPORTANT NOTICE: The contents of this email and any attachments are
>>     confidential and may also be privileged. If you are not the intended
>>     recipient, please notify the sender immediately and do not disclose the
>>     contents to any other person, use it for any purpose, or store or copy the
>>     information in any medium. Thank you.
>>
>>
>>
>>
>>
>> --
>>
>> ciao,
>>
>> al
>>
>> -----------------------------------
>>
>> Al Stone
>>
>> Software Engineer
>>
>> Linaro Enterprise Group
>>
>> al.stone@linaro.org <mailto:al.stone@linaro.org>
>>
>> -----------------------------------
>>
>>
>>
>> IMPORTANT NOTICE: The contents of this email and any attachments are
>> confidential and may also be privileged. If you are not the intended
>> recipient, please notify the sender immediately and do not disclose
>> the contents to any other person, use it for any purpose, or store or
>> copy the information in any medium. Thank you.
> 
> 
> --
> ciao,
> al
> -----------------------------------
> Al Stone
> Software Engineer
> Linaro Enterprise Group
> al.stone@linaro.org
> -----------------------------------
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>
diff mbox

Patch

diff --git a/src/Makefile.am b/src/Makefile.am
index bd38841..4e9d774 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -48,6 +48,7 @@  fwts_SOURCES = main.c 				\
 	acpi/acpidump/acpidump.c 		\
 	acpi/acpiinfo/acpiinfo.c 		\
 	acpi/acpitables/acpitables.c 		\
+	sbbr/acpitables/acpitables.c 		\
 	acpi/apicinstance/apicinstance.c 	\
 	acpi/asf/asf.c				\
 	acpi/aspt/aspt.c			\
diff --git a/src/acpi/acpitables/acpitables.c b/src/acpi/acpitables/acpitables.c
index 1fcf32a..04a283f 100644
--- a/src/acpi/acpitables/acpitables.c
+++ b/src/acpi/acpitables/acpitables.c
@@ -127,6 +127,6 @@  static fwts_framework_ops acpi_table_check_ops = {
 	.minor_tests = acpi_table_check_tests
 };
 
-FWTS_REGISTER("acpitables", &acpi_table_check_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_TEST_ACPI)
+FWTS_REGISTER("acpitables", &acpi_table_check_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_TEST_ACPI | FWTS_FLAG_TEST_SBBR)
 
 #endif
diff --git a/src/sbbr/acpitables/acpitables.c b/src/sbbr/acpitables/acpitables.c
new file mode 100644
index 0000000..b0b9140
--- /dev/null
+++ b/src/sbbr/acpitables/acpitables.c
@@ -0,0 +1,264 @@ 
+/*
+ * Copyright (C) 2010-2017 Canonical
+ * Copyright (C) 2017      ARM Ltd
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ *
+ */
+#include "fwts.h"
+
+#if defined(FWTS_HAS_ACPI)
+#include "acpi.h"
+#include "accommon.h"
+#include "acnamesp.h"
+#include "actables.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <dirent.h>
+#include <ctype.h>
+#include <unistd.h>
+#include <inttypes.h>
+
+#define TABLE_NAME_LEN (16)
+#define MIN_SIG        ( 4)
+#define OEM_ID         ( 6)
+#define OEM_TABLE_ID   ( 8)
+#define OEM_CREATOR_ID ( 4)
+
+static bool acpi_table_check_field(const char *field, const size_t len)
+{
+	size_t i;
+
+	for (i = 0; i < len; i++)
+		if (!isascii(field[i]))
+			return false;
+
+	return true;
+}
+
+static bool acpi_table_check_field_test(
+	fwts_framework *fw,
+	const char *table_name,
+	const char *field_name,
+	const char *field,
+	const size_t len)
+{
+	if (!acpi_table_check_field(field, len)) {
+		fwts_failed(fw, LOG_LEVEL_LOW, "ACPITableHdrInfo",
+			"ACPI Table %s has non-ASCII characters in "
+			"header field %s\n", table_name, field_name);
+		return false;
+	}
+	return true;
+}
+
+/* Callback function used when searching for processor devices in namespace. */
+static ACPI_STATUS processor_handler(ACPI_HANDLE ObjHandle, uint32_t level, void *context,
+                              void **returnvalue)
+{
+	ACPI_NAMESPACE_NODE *node = (ACPI_NAMESPACE_NODE *)ObjHandle;
+	ACPI_NAMESPACE_NODE *parent = node->Parent;
+	int error_count;
+
+	/* Unused parameters trigger errors. */
+	FWTS_UNUSED(level);
+	FWTS_UNUSED(context);
+
+	/* If the processor device is not located under _SB_, increment the error_count. */
+	if (strncmp(parent->Name.Ascii, "_SB_", sizeof(int32_t)) != 0) {
+		error_count = *((int *)returnvalue);
+		error_count++;
+		*((int *)returnvalue) = error_count;
+	}
+
+	/* Return 0 so namespace search continues. */
+	return 0;
+}
+
+/* Test function that makes sure processors are under the _SB_ namespace. */
+static int acpi_table_sbbr_namespace_check_test1(fwts_framework *fw)
+{
+	int error_count = 0;
+
+	/* Initializing ACPICA library so we can call AcpiWalkNamespace. */
+	if (fwts_acpica_init(fw) != FWTS_OK)
+		return FWTS_ERROR;
+
+	/* Searching for all processor devices in the namespace. */
+	AcpiWalkNamespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
+	                  processor_handler, NULL, NULL, (void **)&error_count);
+
+	/* Deinitializing ACPICA, if we don't call this the terminal will break on exit. */
+	fwts_acpica_deinit();
+
+	/* error_count variable counts the number of processors outside of the _SB_ namespace. */
+	if (error_count > 0)
+		fwts_failed(fw, LOG_LEVEL_HIGH, "SbbrAcpiCpuWrongNamespace", "%d Processor devices "
+		            "were found outside of the _SB_ namespace.", error_count);
+	else
+		fwts_passed(fw, "All processor devices were located in the _SB_ namespace.");
+
+	return FWTS_OK;
+}
+
+static int acpi_table_sbbr_check_test2(fwts_framework *fw)
+{
+	int i;
+	bool checked = false;
+	bool dsdt_checked = false;
+	bool ssdt_checked = false;
+
+	for (i = 0; ; i++) {
+		fwts_acpi_table_info *info;
+		fwts_acpi_table_header *hdr;
+		char name[TABLE_NAME_LEN];
+		bool passed = false;
+
+		if (fwts_acpi_get_table(fw, i, &info) != FWTS_OK)
+			break;
+		if (info == NULL)
+			continue;
+
+		checked = true;
+		if (!strcmp(info->name, "DSDT") ||
+			!strcmp(info->name, "SSDT")) {
+			if (!strcmp(info->name, "DSDT")) {
+				dsdt_checked = true;
+			}
+			if (!strcmp(info->name, "SSDT")) {
+				ssdt_checked = true;
+			}
+			hdr = (fwts_acpi_table_header *)info->data;
+			if (acpi_table_check_field(hdr->signature, MIN_SIG)) {
+				snprintf(name, sizeof(name), "%4.4s", hdr->signature);
+			} else {
+				/* Table name not printable, so identify it by the address */
+				snprintf(name, sizeof(name), "at address 0x%" PRIx64, info->addr);
+			}
+
+			/*
+			 * Tables shouldn't be short, however, they do have at
+			 * least 4 bytes with their signature else they would not
+			 * have been loaded by this stage.
+			 */
+			if (hdr->length < sizeof(fwts_acpi_table_header)) {
+				fwts_failed(fw, LOG_LEVEL_HIGH, "ACPITableHdrShort",
+					"ACPI Table %s is too short, only %d bytes long. Further "
+					"header checks will be omitted.", name, hdr->length);
+				continue;
+			}
+			/* Warn about empty tables */
+			if (hdr->length == sizeof(fwts_acpi_table_header)) {
+				fwts_warning(fw,
+					"ACPI Table %s is empty and just contains a table header. Further "
+					"header checks will be omitted.", name);
+				continue;
+			}
+
+			passed = acpi_table_check_field_test(fw, name, "Signature", hdr->signature, MIN_SIG) &
+			    acpi_table_check_field_test(fw, name, "OEM ID", hdr->oem_id, OEM_ID) &
+			    acpi_table_check_field_test(fw, name, "OEM Table ID", hdr->oem_tbl_id, OEM_TABLE_ID) &
+			    acpi_table_check_field_test(fw, name, "OEM Creator ID", hdr->creator_id, OEM_CREATOR_ID);
+			if (passed)
+				fwts_passed(fw, "Table %s has valid signature and ID strings.", name);
+		}
+	}
+	if (!checked)
+		fwts_aborted(fw, "Cannot find any ACPI tables.");
+	if (!dsdt_checked) {
+		fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
+				"Test DSDT table is NOT implemented.");
+	}
+	if (!ssdt_checked) {
+		fwts_failed(fw, LOG_LEVEL_HIGH, "acpi_table_check_test4",
+				"Test SSDT table is NOT implemented.");
+	}
+	if ((!dsdt_checked) || (!ssdt_checked))
+	  return FWTS_ERROR;
+
+	return FWTS_OK;
+}
+
+/* List of ACPI tables recommended by SBBR 4.2.2 */
+char *recommended_acpi_tables[] = {
+	"MCFG",
+	"IORT",
+	"BERT",
+	"EINJ",
+	"ERST",
+	"HEST",
+	"RASF",
+	"SPMI",
+	"SLIT",
+	"SRAT",
+	"CSRT",
+	"ECDT",
+	"MPST",
+	"PCCT",
+	NULL
+};
+
+/* Searches ACPI tables by signature. */
+fwts_acpi_table_info *sbbr_search_acpi_tables(fwts_framework *fw, char *signature)
+{
+	uint32_t i;
+	fwts_acpi_table_info *info;
+
+	i = 0;
+	while (fwts_acpi_get_table(fw, i, &info) == FWTS_OK) {
+		if (info != NULL && strncmp(info->name, signature, sizeof(uint32_t)) == 0) {
+			return info;
+		}
+		i++;
+	}
+
+	return NULL;
+}
+
+static int acpi_table_sbbr_check_test3(fwts_framework *fw)
+{
+	uint32_t i;
+	fwts_acpi_table_info *info;
+
+	for (i = 0; recommended_acpi_tables[i] != NULL; i++) {
+		info = sbbr_search_acpi_tables(fw, recommended_acpi_tables[i]);
+		if (info == NULL) {
+			fwts_failed(fw, LOG_LEVEL_HIGH, "SbbrAcpiRecommendedTableNotFound",
+			            "SBBR Recommended ACPI table \"%s\" not found.",
+			            recommended_acpi_tables[i]);
+		} else {
+			fwts_passed(fw, "SBBR Recommended ACPI table \"%s\" found.",
+			            recommended_acpi_tables[i]);
+		}
+	}
+	return FWTS_OK;
+}
+
+static fwts_framework_minor_test acpi_table_sbbr_check_tests[] = {
+	{ acpi_table_sbbr_namespace_check_test1, "Test that processors only exist in the _SB namespace." },
+	{ acpi_table_sbbr_check_test2, "Test DSDT and SSDT tables are implemented." },
+	{ acpi_table_sbbr_check_test3, "Check for recommended ACPI tables." },
+	{ NULL, NULL }
+};
+
+static fwts_framework_ops acpi_table_sbbr_check_ops = {
+	.description = "ACPI table headers sanity tests.",
+	.minor_tests = acpi_table_sbbr_check_tests
+};
+
+FWTS_REGISTER("acpi_sbbr", &acpi_table_sbbr_check_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_SBBR)
+
+#endif