[prev in list] [next in list] [prev in thread] [next in thread] 

List:       fwts-devel
Subject:    ACK: [PATCH] hpet: hpetcheck: add ACPI HPET table check
From:       Alex Hung <alex.hung () canonical ! com>
Date:       2012-12-21 6:11:08
Message-ID: 50D3FD7C.3070506 () canonical ! com
[Download RAW message or body]

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>


[prev in list] [next in list] [prev in thread] [next in thread]