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

[Alex Hung]

On Thu, Jul 20, 2017 at 5:07 PM, Jeffrey Hugo <jhugo at codeaurora.org> wrote:
> Tested this.  Some minor nits below that I would really like fixed, but I
> think this is in a pretty good state currently.

Thanks for testing and your feedback.

>
> 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.

Thanks for pointing out. I will change it in V2

>
>> +       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?

Processor Topology Structure was actually quoted from Table 5-149
Processor Properties Topology Table - "Processor topology
structure[N]", but this is very confusing indeed. I think I will move
it into each function where each sub-structure is tested.

>
>
>> +               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.



-- 
Cheers,
Alex Hung