[PATCH 1/2] acpi: pptt: add ACPI PPTT test

[Jeffrey Hugo]

Tested this.  Some minor nits below that I would really like fixed, but 
I think this is in a pretty good state currently.

On 7/20/2017 1:14 PM, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung at canonical.com>
> ---
>   src/Makefile.am             |   1 +
>   src/acpi/pptt/pptt.c        | 196 ++++++++++++++++++++++++++++++++++++++++++++
>   src/lib/include/fwts_acpi.h |  53 ++++++++++++
>   3 files changed, 250 insertions(+)
>   create mode 100644 src/acpi/pptt/pptt.c
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index ca0a449..11862c9 100644

<snip>

> +static void pptt_id_test(fwts_framework *fw, fwts_acpi_table_pptt_id *entry, bool *passed)
> +{
> +
> +	fwts_log_info_verbatim(fw, "    Reserved:                       0x%4.4" PRIx16, entry->reserved);
> +	fwts_log_info_verbatim(fw, "    VENDOR_ID:                      0x%8.8" PRIx32, entry->vendor_id);

Vendor ID is supposed to be an ASCII value.  I think it would make more 
sense to print it that way, otherwise its not as convenient to verify 
the correct value is here.

> +	fwts_log_info_verbatim(fw, "    LEVEL_1_ID:                     0x%16.16" PRIx64, entry->level1_id);
> +	fwts_log_info_verbatim(fw, "    LEVEL_2_ID:                     0x%16.16" PRIx64, entry->level2_id);
> +	fwts_log_info_verbatim(fw, "    MAJOR_REV:                      0x%4.4" PRIx16, entry->major_rev);
> +	fwts_log_info_verbatim(fw, "    MINOR_REV:                      0x%4.4" PRIx16, entry->minor_rev);
> +	fwts_log_info_verbatim(fw, "    SPIN_REV:                       0x%4.4" PRIx16, entry->spin_rev);
> +

<snip>

> +static int pptt_test1(fwts_framework *fw)
> +{
> +	fwts_acpi_table_pptt_header *entry;
> +	uint32_t offset;
> +	bool passed = true;
> +
> +	fwts_log_info_verbatim(fw, "PPTT Processor Properties Topology Table:");
> +
> +	entry = (fwts_acpi_table_pptt_header *) (table->data + sizeof(fwts_acpi_table_pptt));
> +	offset = sizeof(fwts_acpi_table_pptt);
> +	while (offset < table->length) {
> +
> +		fwts_log_info_verbatim(fw, "  PPTT Processor Topology Structure:");
> +		fwts_log_info_verbatim(fw, "    Type:                           0x%2.2" PRIx8, entry->type);
> +		fwts_log_info_verbatim(fw, "    Length:                         0x%2.2" PRIx8, entry->length);
> +

This is misleading.  Here you print "Topology Structure" for all the 
nodes that get dumped, but only type 0 below is a processor node (spec 
calls it a "processor hierarchy node").  Its very confusing to me to see 
type 1 (cache) and type 2 (id) nodes being labeled as processor nodes.

Maybe this print should be deferred into the switch below?

> +		if (entry->length == 0) {
> +			passed = false;
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "PPTTStructLengthZero",
> +				    "PPTT structure (offset 0x%4.4" PRIx32 ") "
> +				    "length cannot be 0", offset);
> +			break;
> +		}
> +
> +		switch(entry->type) {
> +			case FWTS_ACPI_PPTT_PROCESSOR:
> +				pptt_processor_test(fw, (fwts_acpi_table_pptt_processor *) entry, &passed);
> +				break;
> +			case FWTS_ACPI_PPTT_CACHE:
> +				pptt_cache_test(fw, (fwts_acpi_table_pptt_cache *) entry, &passed);
> +				break;
> +			case FWTS_ACPI_PPTT_ID:
> +				pptt_id_test(fw, (fwts_acpi_table_pptt_id *) entry, &passed);
> +				break;
> +			default:
> +				passed = false;
> +				fwts_failed(fw, LOG_LEVEL_HIGH,
> +					"PPTTBadSubtableType",
> +					"PPTT must have subtable with Type 0..2, got "
> +					"0x%4.4" PRIx16 " instead", entry->type);
> +				break;



-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.