From patchwork Tue Feb 9 01:33:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Al Stone X-Patchwork-Id: 61472 Delivered-To: patches@linaro.org Received: by 10.112.43.199 with SMTP id y7csp1766907lbl; Mon, 8 Feb 2016 17:34:24 -0800 (PST) X-Received: by 10.182.225.132 with SMTP id rk4mr27728051obc.68.1454981652981; Mon, 08 Feb 2016 17:34:12 -0800 (PST) Return-Path: Received: from mail-ob0-x22c.google.com (mail-ob0-x22c.google.com. [2607:f8b0:4003:c01::22c]) by mx.google.com with ESMTPS id y125si1081558oiy.93.2016.02.08.17.34.12 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 08 Feb 2016 17:34:12 -0800 (PST) Received-SPF: pass (google.com: domain of al.stone@linaro.org designates 2607:f8b0:4003:c01::22c as permitted sender) client-ip=2607:f8b0:4003:c01::22c; Authentication-Results: mx.google.com; spf=pass (google.com: domain of al.stone@linaro.org designates 2607:f8b0:4003:c01::22c as permitted sender) smtp.mailfrom=al.stone@linaro.org; dkim=pass header.i=@linaro.org Received: by mail-ob0-x22c.google.com with SMTP id wb13so175745539obb.1 for ; Mon, 08 Feb 2016 17:34:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=0i5JCYuaP8kvaKx9KLLWN/w6mAygRS4PqR4PXM8ZaNk=; b=bIIQ0yms6cl2emjc2IsD1gDYdpR1IucKtdtQYsVXtaYm3xrcsSpaHWu+A28OkcPYXf jv1tZpubH6asNyjQ0VRE0lrYP1Gp0yaeQOJhvCXSWDQKkeC1R8k4wpZsCdEEi25LxoLX 8YcaFZNSS7g9huNvq+pwKlY/6j2MvRG75+3XI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=0i5JCYuaP8kvaKx9KLLWN/w6mAygRS4PqR4PXM8ZaNk=; b=fL4tYq4qbzefTq87VgJ6T8Nf6wDvIGKS5YFYWivlFIofc5EXzc5voKvlg0tW1p+AI8 2xBxONWd6E/DPgN+lc3Krg9yc9t28imFnrCItOy1lDvtq90jNCZMPdVl+mxWRX166leN 0LXn6C8uwfAUbJ8z40eUcY3xSlulYc8K55oSjI1zhj2STplmlmJBo60qiHBwjysth80g xyzvGLExcd41k2gch/Yl9LiJY8jgaaqnjpNHfJ7MFVZ46q78Qu6ql3eC2I9KXbEvp6MU 29gyo6ttBArVToPOxoHSRmqU83nV+Dj6m5lCgtWapYEPoFHc5kuoCba6nnST81UQY1kE aR0w== X-Gm-Message-State: AG10YOQCD50iRESM8auPTP4NaCI6N0+SAmq0YjQWCOPU4CMfB42XjjBPuW1b6Oqg5Hfi1k/t4VM= X-Received: by 10.60.233.43 with SMTP id tt11mr28603035oec.36.1454981652688; Mon, 08 Feb 2016 17:34:12 -0800 (PST) Return-Path: Received: from fidelio.ahs3 (c-50-134-239-249.hsd1.co.comcast.net. [50.134.239.249]) by smtp.googlemail.com with ESMTPSA id qp4sm19097135obc.9.2016.02.08.17.34.10 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 08 Feb 2016 17:34:11 -0800 (PST) From: Al Stone To: fwts-devel@lists.ubuntu.com Cc: linaro-acpi@lists.linaro.org, patches@linaro.org, Al Stone Subject: [PATCH 19/21] FADT: add in compliance tests for C2/C3 latency fields Date: Mon, 8 Feb 2016 18:33:01 -0700 Message-Id: <1454981583-31872-20-git-send-email-al.stone@linaro.org> X-Mailer: git-send-email 2.5.0 In-Reply-To: <1454981583-31872-1-git-send-email-al.stone@linaro.org> References: <1454981583-31872-1-git-send-email-al.stone@linaro.org> Add in new compliance checks for the P_LVL2_LAT and P_LVL3_LAT fields. Both of these require knowing the value of an x86 P_BLK; this in turn requires examination of any Processor() declarations in the ACPI name space, which in turn requires the initialization of the ACPI interpreter. It is probable that these fields are seldom used; processor speeds and feeds are typically controlled via very different mechanisms these days. These tests are therefore for completeness sake and it may be difficult to find ACPI tables using these fields. Note that these tests allow us to remove commented out versions of simpler tests of these fields. Signed-off-by: Al Stone --- src/acpi/fadt/fadt.c | 173 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 148 insertions(+), 25 deletions(-) -- 2.5.0 diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index f932dea..a2ed70c 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -19,6 +19,7 @@ * */ #include "fwts.h" +#include "fwts_acpi_object_eval.h" #include #include @@ -1318,6 +1319,139 @@ static void acpi_table_check_fadt_cst_cnt(fwts_framework *fw) return; } +static uint64_t fadt_find_p_blk(fwts_framework *fw) +{ + uint64_t pblk; + fwts_list *objects; + + pblk = 0; + objects = fwts_acpi_object_get_names(); + if (objects) { + fwts_list_link *obj; + + fwts_list_foreach(obj, objects) { + char *name = fwts_list_data(char*, obj); + ACPI_OBJECT pr = { 0 }; + ACPI_BUFFER buf = { sizeof(ACPI_OBJECT), &pr }; + ACPI_HANDLE handle; + ACPI_OBJECT_TYPE type; + ACPI_STATUS status; + + status = AcpiGetHandle(NULL, name, &handle); + if (ACPI_FAILURE(status)) { + fwts_warning(fw, "Failed to get handle for " + "object %s.", name); + continue; + } + status = AcpiGetType(handle, &type); + if (ACPI_FAILURE(status)) { + fwts_warning(fw, "Failed to get type for " + "object %s.", name); + continue; + } + + /* + * If a CPU is not defined as a Processor object, + * we don't care here. Per section 8.1.2 and 8.1.3, + * defining a CPU with a Device object implies that + * a _CST method must be defined, and whatever is + * in the _CST overrides the P_BLK and P_LVL*_LAT + * values. Since we're only trying to validate the + * P_LVL*_LAT values, and they're only used if the + * CPUs are defined as Processor objects, we can + * ignore any CPUs defined as Device objects. + */ + if (type == ACPI_TYPE_PROCESSOR) { + fwts_log_info(fw, "Found processor %s.", name); + status = AcpiEvaluateObject(handle, NULL, NULL, + &buf); + if (ACPI_FAILURE(status)) + fwts_warning(fw, "Could not evaluate " + "Processor %s", name); + else { + if (pr.Processor.PblkAddress) + pblk = pr.Processor.PblkAddress; + } + } + } + } + + fwts_log_info(fw, "Using P_BLK address of 0x%" PRIx64, pblk); + return pblk; +} + +static void acpi_table_check_fadt_p_lvl2_lat(fwts_framework *fw, uint64_t pblk) +{ + if (pblk) { + if (fadt->p_lvl2_lat <= 100) + fwts_passed(fw, + "FADT P_LVL2_LAT is within proper range " + "at %" PRIu16, fadt->p_lvl2_lat); + else + fwts_warning(fw, + "FADT P_LVL2_LAT is > 100 (%" PRIu16 ") " + "but a P_BLK is defined. This implies " + "a C2 state is not supported, but there " + "is a P_BLK register block defined which " + "implies there might be a C2 state that " + "works. There is not enough information " + "to determine if this is expected or not.", + fadt->p_lvl2_lat); + } else { + if (fadt->p_lvl2_lat <= 100) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "PLvl2LatDefinedButNotUsable", + "FADT P_LVL2_LAT is <= 100 (%" PRIu16 ") " + "which implies a C2 state is supported " + "but there is no P_BLK register block " + "defined to enable the C2 transition.", + fadt->p_lvl2_lat); + else + fwts_passed(fw, + "FADT P_LVL2_LAT is > 100 (%" PRIu16 ") " + "and no P_BLK is defined.", + fadt->p_lvl2_lat); + } + + return; +} + +static void acpi_table_check_fadt_p_lvl3_lat(fwts_framework *fw, uint64_t pblk) +{ + if (pblk) { + if (fadt->p_lvl3_lat <= 1000) + fwts_passed(fw, + "FADT P_LVL3_LAT is within proper range " + "at %" PRIu16, fadt->p_lvl3_lat); + else + fwts_warning(fw, + "FADT P_LVL3_LAT is > 1000 (%" PRIu16 ") " + "but a P_BLK is defined. This implies " + "a C3 state is not supported, but there " + "is a P_BLK register block defined which " + "implies there might be a C3 state that " + "works. There is not enough information " + "to determine if this is expected or not.", + fadt->p_lvl3_lat); + } else { + if (fadt->p_lvl3_lat <= 1000) + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "PLvl3LatDefinedButNotUsable", + "FADT P_LVL3_LAT is <= 1000 (%" PRIu16 ") " + "which implies a C3 state is supported " + "but there is no P_BLK register block " + "defined to enable the C3 transition.", + fadt->p_lvl3_lat); + else + fwts_passed(fw, + "FADT P_LVL3_LAT is > 100 (%" PRIu16 ") " + "and no P_BLK is defined.", + fadt->p_lvl3_lat); + } + + return; +} + static int fadt_test1(fwts_framework *fw) { bool passed = true; @@ -1357,6 +1491,20 @@ static int fadt_test1(fwts_framework *fw) fwts_log_info(fw, "FADT GPE1_BASE is %" PRIu8, fadt->gpe1_base); acpi_table_check_fadt_cst_cnt(fw); + if (fwts_acpi_init(fw) == FWTS_OK) { + uint64_t pblk = fadt_find_p_blk(fw); + + acpi_table_check_fadt_p_lvl2_lat(fw, pblk); + acpi_table_check_fadt_p_lvl3_lat(fw, pblk); + fwts_acpi_deinit(fw); + } else { + fwts_log_error(fw, "Cannot initialize ACPI namespace."); + fwts_log_info(fw, + "Without a namespace, cannot test " + "values of P_LVL2_LAT or P_LVL3_LAT " + "for correctness."); + } + fwts_log_info(fw, "FADT FLUSH_SIZE is %" PRIu16, fadt->flush_size); fwts_log_info(fw, "FADT FLUSH_STRIDE is %" PRIu16, @@ -1371,31 +1519,6 @@ static int fadt_test1(fwts_framework *fw) } /* - * Bug LP: #833644 - * - * Remove these tests, really need to put more intelligence into it - * perhaps in the cstates test rather than here. For the moment we - * shall remove this warning as it's giving users false alarms - * See: https://bugs.launchpad.net/ubuntu/+source/fwts/+bug/833644 - */ - /* - if (fadt->p_lvl2_lat > 100) { - fwts_warning(fw, "FADT P_LVL2_LAT is %" PRIi16 ", a value > 100 indicates a " - "system not to support a C2 state.", fadt->p_lvl2_lat); - fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C2 latency in microseconds. The ACPI specification " - "states that a value > 100 indicates that C2 is not supported and hence the " - "ACPI processor idle routine will not use C2 power states."); - } - if (fadt->p_lvl3_lat > 1000) { - fwts_warning(fw, "FADT P_LVL3_LAT is %" PRIu16 ", a value > 1000 indicates a " - "system not to support a C3 state.", fadt->p_lvl3_lat); - fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C3 latency in microseconds. The ACPI specification " - "states that a value > 1000 indicates that C3 is not supported and hence the " - "ACPI processor idle routine will not use C3 power states."); - } - */ - - /* * Cannot really test the Hypervisor Vendor Identity since * the value is provided by the hypervisor to the OS (as a * sign that the ACPI tables have been fabricated), if it