ACK: [PATCH] hpet: hpetcheck: add ACPI HPET table check
Alex Hung
alex.hung at canonical.com
Fri Dec 21 06:11:08 UTC 2012
On 12/12/2012 07:35 AM, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
>
> The original test has some notion of checking the HPET table
> but this was #ifdef'd out from the original Linux Firmware
> Developer Kit code and never implemented in fwts. Remove the
> old legacy code and fully implement a HPET table validation.
>
> Since we want to sanity check where the kernel's view of where
> the HPET is located and what the HPET table states, I've re-ordered
> the sub-tests to ensure the new test runs 2nd.
>
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
> src/hpet/hpet_check/hpet_check.c | 224 ++++++++++++++++++++++++++++++++-------
> 1 file changed, 186 insertions(+), 38 deletions(-)
>
> diff --git a/src/hpet/hpet_check/hpet_check.c b/src/hpet/hpet_check/hpet_check.c
> index 399bde2..9b8eac6 100644
> --- a/src/hpet/hpet_check/hpet_check.c
> +++ b/src/hpet/hpet_check/hpet_check.c
> @@ -34,28 +34,6 @@ static fwts_list *klog;
> static uint64_t hpet_base_p = 0;
> static void *hpet_base_v = 0;
>
> -#if 0
> -/* check_hpet_base_hpet() -- used to parse the HPET Table for HPET base info */
> -static void check_hpet_base_hpet(void)
> -{
> - unsigned long address = 0;
> - unsigned long size = 0;
> - struct hpet_table *table;
> - char *table_ptr;
> -
> - if (locate_acpi_table("HPET", &address, &size))
> - return;
> -
> - if (address == 0 || address == -1)
> - return;
> -
> - table = (struct hpet_table *) address;
> -
> - hpet_base_p = table->base_address.address;
> - free((void *) address);
> -}
> -#endif
> -
> static void hpet_parse_check_base(fwts_framework *fw,
> const char *table, fwts_list_link *item)
> {
> @@ -74,14 +52,14 @@ static void hpet_parse_check_base(fwts_framework *fw,
> fwts_failed(fw, LOG_LEVEL_MEDIUM,
> "HPETBaseMismatch",
> "Mismatched HPET base between %s (%" PRIx64 ") "
> - "and the kernel (%" PRIx64 ").",
> + "and the kernel (0x%" PRIx64 ").",
> table,
> hpet_base_p,
> address_base);
> else
> fwts_passed(fw,
> "HPET base matches that between %s and "
> - "the kernel (%" PRIx64 ").",
> + "the kernel (0x%" PRIx64 ").",
> table,
> hpet_base_p);
> }
> @@ -185,7 +163,7 @@ static int hpet_check_test1(fwts_framework *fw)
> if (str) {
> hpet_base_p = strtoul(str+6, NULL, 0x10);
> fwts_passed(fw,
> - "Found HPET base %" PRIx64 " in kernel log.",
> + "Found HPET base 0x%" PRIx64 " in kernel log.",
> hpet_base_p);
> break;
> }
> @@ -198,7 +176,7 @@ static int hpet_check_test1(fwts_framework *fw)
> if (str) {
> hpet_base_p = strtoul(str+8, NULL, 0x10);
> fwts_passed(fw,
> - "Found HPET base %" PRIx64 " in kernel log.",
> + "Found HPET base 0x%" PRIx64 " in kernel log.",
> hpet_base_p);
> break;
> }
> @@ -213,8 +191,187 @@ static int hpet_check_test1(fwts_framework *fw)
> return FWTS_OK;
> }
>
> +/*
> + * Sanity check HPET table, see:
> + * http://www.intel.co.uk/content/www/uk/en/software-developers/software-developers-hpet-spec-1-0a.html
> + */
> static int hpet_check_test2(fwts_framework *fw)
> {
> + fwts_acpi_table_info *table;
> + fwts_acpi_table_hpet *hpet;
> + bool passed = true;
> + char *page_prot;
> +
> + if (fwts_acpi_find_table(fw, "HPET", 0, &table) != FWTS_OK) {
> + fwts_log_error(fw, "Cannot read ACPI table HPET.");
> + return FWTS_ERROR;
> + }
> +
> + if (table == NULL) {
> + fwts_log_error(fw, "ACPI table HPET does not exist!");
> + return FWTS_ERROR;
> + }
> +
> + if (table->length < sizeof(fwts_acpi_table_hpet)) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "HPETAcpiTableTooSmall",
> + "HPET ACPI table is %zd bytes long which is smaller "
> + "than the expected size of %zd bytes.",
> + table->length, sizeof(fwts_acpi_table_hpet));
> + return FWTS_ERROR;
> + }
> +
> + hpet = (fwts_acpi_table_hpet *)table->data;
> +
> + if (hpet->base_address.address == 0) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "HPETAcpiBaseAddressNull",
> + "HPET base address in ACPI HPET table is null.");
> + passed = false;
> + }
> +
> + /*
> + * If we've already figured out the HPET base address then
> + * sanity check it against HPET. This should never happen.
> + */
> + if ((hpet_base_p != 0) &&
> + (hpet_base_p != hpet->base_address.address)) {
> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
> + "HPETAcpiBaseAddressDifferFromKernel",
> + "HPET base address in ACPI HPET table "
> + "is 0x%" PRIx64 " which is different from "
> + "the kernel HPET base address of "
> + "0x%" PRIx64 ".",
> + hpet->base_address.address, hpet_base_p);
> + passed = false;
> + }
> +
> +#if 0
> + /*
> + * The Intel spec states that the address space ID
> + * must be memory or I/O space.
> + */
> + if ((hpet->base_address.address_space_id != 0) &&
> + (hpet->base_address.address_space_id != 1)) {
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "HPETAcpiBaseAddressSpaceId",
> + "The HPET base address space ID was 0x%" PRIx8
> + ", was expecting 0 (System Memory) or "
> + "1 (System I/O).",
> + hpet->base_address.address_space_id);
> + passed = false;
> + }
> +#endif
> + /*
> + * The kernel expects the HPET address space ID
> + * to be memory.
> + */
> + if (hpet->base_address.address_space_id != 0) {
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "HPETAcpiBaseAddressSpaceIdNotMemory",
> + "The HPET base address space ID was 0x%" PRIx8
> + ", however, the Kernel expects the HPET address "
> + "space ID to be 0 (System Memory). The kernel "
> + "will not parse this and the HPET configuration "
> + "will be ignored.",
> + hpet->base_address.address_space_id);
> + }
> +
> + /*
> + * Some broken firmware advertises the HPET at
> + * 0xfed0000000000000 instead of 0xfed00000. The kernel
> + * detects this and fixes it, but even so, it is wrong
> + * so lets check for this.
> + */
> + if ((hpet->base_address.address >> 32) != 0) {
> + fwts_failed(fw, LOG_LEVEL_CRITICAL,
> + "HPETAcpiBaseAddressBogus",
> + "The HPET base address is bogus: 0x%" PRIx64 ".",
> + hpet->base_address.address);
> + fwts_advice(fw,
> + "Bogus HPET base address can be worked around "
> + "by using the kernel parameter 'hpet=force' if "
> + "the base addess is 0xfed0000000000000. "
> + "This will make the kernel shift the address "
> + "down 32 bits to 0xfed00000.");
> + passed = false;
> + }
> +
> + /*
> + * We don't need to check for GAS address space widths etc
> + * since the kernel does not care and the spec doesn't
> + * stipulate these need to be sane
> + */
> +
> + /*
> + * Dump out HPET
> + */
> + fwts_log_info_verbatum(fw, "Hardware ID of Event Block:");
> + fwts_log_info_verbatum(fw, " PCI Vendor ID : 0x%" PRIx32,
> + (hpet->event_timer_block_id >> 16) & 0xffff);
> + fwts_log_info_verbatum(fw, " Legacy IRQ Routing Capable : %" PRIu32,
> + (hpet->event_timer_block_id >> 15) & 1);
> + fwts_log_info_verbatum(fw, " COUNT_SIZE_CAP counter size: %" PRIu32,
> + (hpet->event_timer_block_id >> 13) & 1);
> + fwts_log_info_verbatum(fw, " Number of comparitors : %" PRIu32,
> + (hpet->event_timer_block_id >> 8) & 0x1f);
> + fwts_log_info_verbatum(fw, " Hardwre Revision ID : 0x%" PRIx8,
> + hpet->event_timer_block_id & 0xff);
> +
> + fwts_log_info_verbatum(fw, "Lower 32 bit base Address : 0x%" PRIx64,
> + hpet->base_address.address);
> + fwts_log_info_verbatum(fw, " Address Space ID : 0x%" PRIx8,
> + hpet->base_address.address_space_id);
> + fwts_log_info_verbatum(fw, " Register Bit Width : 0x%" PRIx8,
> + hpet->base_address.register_bit_width);
> + fwts_log_info_verbatum(fw, " Register Bit Offset : 0x%" PRIx8,
> + hpet->base_address.register_bit_offset);
> + fwts_log_info_verbatum(fw, " Address Width : 0x%" PRIx8,
> + hpet->base_address.access_width);
> + fwts_log_info_verbatum(fw, "HPET sequence number : 0x%" PRIx8,
> + hpet->hpet_number);
> + fwts_log_info_verbatum(fw, "Minimum clock tick : 0x%" PRIx16,
> + hpet->main_counter_minimum);
> +
> + switch (hpet->page_prot_and_oem_attribute & 0xf) {
> + case 0:
> + page_prot = "No guaranteed protection";
> + break;
> + case 1:
> + page_prot = "4K page protected";
> + break;
> + case 2:
> + page_prot = "64K page protected";
> + break;
> + default:
> + page_prot = "Reserved";
> + break;
> + }
> + fwts_log_info_verbatum(fw, "Page Protection : 0x%" PRIx8 " (%s)",
> + hpet->page_prot_and_oem_attribute & 0xf,
> + page_prot);
> + fwts_log_info_verbatum(fw, "OEM attributes : 0x%" PRIx8,
> + (hpet->page_prot_and_oem_attribute >> 4) & 0xf);
> +
> + if (passed)
> + fwts_passed(fw, "HPET looks sane.");
> +
> + return FWTS_OK;
> +}
> +
> +static int hpet_check_test3(fwts_framework *fw)
> +{
> + int i;
> +
> + hpet_check_base_acpi_table(fw, "DSDT", 0);
> +
> + for (i=0;i<11;i++) {
> + hpet_check_base_acpi_table(fw, "SSDT", i);
> + }
> + return FWTS_OK;
> +}
> +
> +
> +static int hpet_check_test4(fwts_framework *fw)
> +{
> uint64_t hpet_id;
> uint32_t vendor_id;
> uint32_t clk_period;
> @@ -238,7 +395,7 @@ static int hpet_check_test2(fwts_framework *fw)
> "Invalid Vendor ID: %04" PRIx32 " - this should be configured.",
> vendor_id);
> else
> - fwts_passed(fw, "Vendor ID looks sane: %04" PRIx32 ".", vendor_id);
> + fwts_passed(fw, "Vendor ID looks sane: 0x%04" PRIx32 ".", vendor_id);
>
> clk_period = hpet_id >> 32;
> if ((clk_period > MAX_CLK_PERIOD) || (clk_period == 0))
> @@ -253,21 +410,12 @@ static int hpet_check_test2(fwts_framework *fw)
> return FWTS_OK;
> }
>
> -static int hpet_check_test3(fwts_framework *fw)
> -{
> - int i;
> -
> - hpet_check_base_acpi_table(fw, "DSDT", 0);
> - for (i=0;i<11;i++) {
> - hpet_check_base_acpi_table(fw, "SSDT", i);
> - }
> - return FWTS_OK;
> -}
>
> static fwts_framework_minor_test hpet_check_tests[] = {
> { hpet_check_test1, "Check HPET base in kernel log." },
> - { hpet_check_test2, "Sanity check HPET configuration." },
> + { hpet_check_test2, "Check HPET base in HPET table. "},
> { hpet_check_test3, "Check HPET base in DSDT and/or SSDT. "},
> + { hpet_check_test4, "Sanity check HPET configuration." },
> { NULL, NULL }
> };
>
>
Acked-by: Alex Hung <alex.hung at canonical.com>
More information about the fwts-devel
mailing list