Patchwork hpet: hpetcheck: add ACPI HPET table check

login
register
mail settings
Submitter Colin King
Date Dec. 11, 2012, 11:35 p.m.
Message ID <1355268932-8644-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/205340/
State Accepted
Headers show

Comments

Colin King - Dec. 11, 2012, 11:35 p.m.
From: Colin Ian King <colin.king@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@canonical.com>
---
 src/hpet/hpet_check/hpet_check.c | 224 ++++++++++++++++++++++++++++++++-------
 1 file changed, 186 insertions(+), 38 deletions(-)
Ivan Hu - Dec. 20, 2012, 1:21 a.m.
On 12/12/2012 07:35 AM, Colin King wrote:
> From: Colin Ian King <colin.king@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@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: Ivan Hu <ivan.hu@canonical.com>
Alex Hung - Dec. 21, 2012, 6:11 a.m.
On 12/12/2012 07:35 AM, Colin King wrote:
> From: Colin Ian King <colin.king@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@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@canonical.com>

Patch

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 }
 };