acpi: gtdt: add checking the valid base physical address
diff mbox series

Message ID 1534318694-31503-1-git-send-email-ivan.hu@canonical.com
State Accepted
Headers show
Series
  • acpi: gtdt: add checking the valid base physical address
Related show

Commit Message

Ivan Hu Aug. 15, 2018, 7:38 a.m. UTC
Some buggy firmwares on Arm server implement GTDT table without impementing
reasonable base physical address and causes kernel complains about ail to get
base address. Add checking for the valid base addresses on GTDT table.

Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
---
 src/acpi/gtdt/gtdt.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

Comments

Colin Ian King Aug. 15, 2018, 7:51 a.m. UTC | #1
On 15/08/18 08:38, Ivan Hu wrote:
> Some buggy firmwares on Arm server implement GTDT table without impementing
> reasonable base physical address and causes kernel complains about ail to get
> base address. Add checking for the valid base addresses on GTDT table.
> 
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>  src/acpi/gtdt/gtdt.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/src/acpi/gtdt/gtdt.c b/src/acpi/gtdt/gtdt.c
> index ded1d5d..dc00785 100644
> --- a/src/acpi/gtdt/gtdt.c
> +++ b/src/acpi/gtdt/gtdt.c
> @@ -56,6 +56,24 @@ static int gtdt_test1(fwts_framework *fw)
>  	uint32_t i = 0, n;
>  	const fwts_acpi_table_gtdt *gtdt = (const fwts_acpi_table_gtdt *)table->data;
>  
> +	if (gtdt->cnt_control_base_phys_addr == 0) {
> +		passed = false;
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"GTDTInvalidBaseAddr",
> +			"The 64-bit physical address at which the "
> +			"Counter Control block should not be Zero. "
> +			"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
> +	}
> +
> +	if (gtdt->cnt_read_base_phys_addr == 0) {
> +		passed = false;
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"GTDTInvalidBaseAddr",
> +			"The 64-bit physical address at which the "
> +			"Counter Read block Read block should not be Zero. "
> +			"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
> +	}
> +
>  	fwts_acpi_reserved_bits_check(fw, "GTDT", "Flags", gtdt->virtual_timer_flags, sizeof(gtdt->virtual_timer_flags), 3, 31, &passed);
>  
>  	ptr = (uint8_t *)table->data + gtdt->platform_timer_offset;
> @@ -89,6 +107,13 @@ static int gtdt_test1(fwts_framework *fw)
>  					i, block->length);
>  				goto done;
>  			}
> +			if (block->physical_address == 0) {
> +				passed = false;
> +				fwts_failed(fw, LOG_LEVEL_HIGH,
> +					"GTDTBlockInvalidBaseAddr",
> +					"The 64-bit physical address at which the "
> +					"GT CntCTLBase Block shouldn't be zero.");
> +			}
>  			if (block->reserved) {
>  				passed = false;
>  				fwts_failed(fw, LOG_LEVEL_HIGH,
> @@ -146,6 +171,21 @@ static int gtdt_test1(fwts_framework *fw)
>  						block_timer->reserved[1],
>  						block_timer->reserved[2]);
>  				}
> +				if (block_timer->cntbase == 0) {
> +					passed = false;
> +					fwts_failed(fw, LOG_LEVEL_HIGH,
> +						"GTDTGTxInvalidBaseAddr",
> +						"Physical Address at which the "
> +						"CntBase block for GTx shouldn't be zero.");
> +				}
> +				if (block_timer->cntel0base == 0) {
> +					passed = false;
> +					fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +						"GTDTGTxInvalidBaseAddr",
> +						"Physical Address at which the "
> +						"CntEL0Base block for GTx should not be Zero. "
> +						"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
> +				}
>  
>  				snprintf(field, sizeof(field), "block %" PRIu32 " physical timer flags", i);
>  				fwts_acpi_reserved_bits_check(fw, "GTDT", field, block_timer->phys_timer_flags,
> @@ -189,6 +229,20 @@ static int gtdt_test1(fwts_framework *fw)
>  					" reserved is non-zero, got 0x%" PRIx8,
>  					i, watchdog->reserved);
>  			}
> +			if (watchdog->refresh_frame_addr == 0) {
> +				passed = false;
> +				fwts_failed(fw, LOG_LEVEL_HIGH,
> +					"GTDTInvalidWatchDogBaseAddr",
> +					"Physical Address at which the "
> +					"RefreshFrame block shouldn't be zero.");
> +			}
> +			if (watchdog->watchdog_control_frame_addr == 0) {
> +				passed = false;
> +				fwts_failed(fw, LOG_LEVEL_HIGH,
> +					"GTDTInvalidWatchDogBaseAddr",
> +					"Physical Address at which the "
> +					"Watchdog Control Frame block shouldn't be zero.");
> +			}
>  
>  			snprintf(field, sizeof(field), "SBSA generic watchdog timer %" PRIu32 " flags", i);
>  			fwts_acpi_reserved_bits_check(fw, "GTDT", field, watchdog->watchdog_timer_flags,
> 

Acked-by: Colin Ian King <colin.king@canonical.com>
Jeffrey Hugo Aug. 15, 2018, 3:16 p.m. UTC | #2
On 8/15/2018 1:38 AM, Ivan Hu wrote:
> Some buggy firmwares on Arm server implement GTDT table without impementing
> reasonable base physical address and causes kernel complains about ail to get
> base address. Add checking for the valid base addresses on GTDT table.

I'm interested to know, what is the exact complaint the kernel gives? 
Can you point me to a log or something?

> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>   src/acpi/gtdt/gtdt.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 54 insertions(+)
> 
> diff --git a/src/acpi/gtdt/gtdt.c b/src/acpi/gtdt/gtdt.c
> index ded1d5d..dc00785 100644
> --- a/src/acpi/gtdt/gtdt.c
> +++ b/src/acpi/gtdt/gtdt.c
> @@ -56,6 +56,24 @@ static int gtdt_test1(fwts_framework *fw)
>   	uint32_t i = 0, n;
>   	const fwts_acpi_table_gtdt *gtdt = (const fwts_acpi_table_gtdt *)table->data;
>   
> +	if (gtdt->cnt_control_base_phys_addr == 0) {
> +		passed = false;
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"GTDTInvalidBaseAddr",
> +			"The 64-bit physical address at which the "
> +			"Counter Control block should not be Zero. "
> +			"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
> +	}
> +
> +	if (gtdt->cnt_read_base_phys_addr == 0) {
> +		passed = false;
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"GTDTInvalidBaseAddr",
> +			"The 64-bit physical address at which the "
> +			"Counter Read block Read block should not be Zero. "
> +			"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
> +	}
> +
>   	fwts_acpi_reserved_bits_check(fw, "GTDT", "Flags", gtdt->virtual_timer_flags, sizeof(gtdt->virtual_timer_flags), 3, 31, &passed);
>   
>   	ptr = (uint8_t *)table->data + gtdt->platform_timer_offset;
> @@ -89,6 +107,13 @@ static int gtdt_test1(fwts_framework *fw)
>   					i, block->length);
>   				goto done;
>   			}
> +			if (block->physical_address == 0) {
> +				passed = false;
> +				fwts_failed(fw, LOG_LEVEL_HIGH,
> +					"GTDTBlockInvalidBaseAddr",
> +					"The 64-bit physical address at which the "
> +					"GT CntCTLBase Block shouldn't be zero.");
> +			}
>   			if (block->reserved) {
>   				passed = false;
>   				fwts_failed(fw, LOG_LEVEL_HIGH,
> @@ -146,6 +171,21 @@ static int gtdt_test1(fwts_framework *fw)
>   						block_timer->reserved[1],
>   						block_timer->reserved[2]);
>   				}
> +				if (block_timer->cntbase == 0) {
> +					passed = false;
> +					fwts_failed(fw, LOG_LEVEL_HIGH,
> +						"GTDTGTxInvalidBaseAddr",
> +						"Physical Address at which the "
> +						"CntBase block for GTx shouldn't be zero.");
> +				}
> +				if (block_timer->cntel0base == 0) {
> +					passed = false;
> +					fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +						"GTDTGTxInvalidBaseAddr",
> +						"Physical Address at which the "
> +						"CntEL0Base block for GTx should not be Zero. "
> +						"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
> +				}
>   
>   				snprintf(field, sizeof(field), "block %" PRIu32 " physical timer flags", i);
>   				fwts_acpi_reserved_bits_check(fw, "GTDT", field, block_timer->phys_timer_flags,
> @@ -189,6 +229,20 @@ static int gtdt_test1(fwts_framework *fw)
>   					" reserved is non-zero, got 0x%" PRIx8,
>   					i, watchdog->reserved);
>   			}
> +			if (watchdog->refresh_frame_addr == 0) {
> +				passed = false;
> +				fwts_failed(fw, LOG_LEVEL_HIGH,
> +					"GTDTInvalidWatchDogBaseAddr",
> +					"Physical Address at which the "
> +					"RefreshFrame block shouldn't be zero.");
> +			}
> +			if (watchdog->watchdog_control_frame_addr == 0) {
> +				passed = false;
> +				fwts_failed(fw, LOG_LEVEL_HIGH,
> +					"GTDTInvalidWatchDogBaseAddr",
> +					"Physical Address at which the "
> +					"Watchdog Control Frame block shouldn't be zero.");
> +			}
>   
>   			snprintf(field, sizeof(field), "SBSA generic watchdog timer %" PRIu32 " flags", i);
>   			fwts_acpi_reserved_bits_check(fw, "GTDT", field, watchdog->watchdog_timer_flags,
>
Ivan Hu Aug. 20, 2018, 1:23 a.m. UTC | #3
Hi Hugo,

The dmesg what I got,

[    6.832162] ACPI GTDT: [Firmware Bug]: failed to get the Watchdog
base address.


Cheers,
Ivan

On 08/15/2018 11:16 PM, Jeffrey Hugo wrote:
> On 8/15/2018 1:38 AM, Ivan Hu wrote:
>> Some buggy firmwares on Arm server implement GTDT table without
>> impementing
>> reasonable base physical address and causes kernel complains about
>> ail to get
>> base address. Add checking for the valid base addresses on GTDT table.
>
> I'm interested to know, what is the exact complaint the kernel gives?
> Can you point me to a log or something?
>
>> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
>> ---
>>   src/acpi/gtdt/gtdt.c | 54
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 54 insertions(+)
>>
>> diff --git a/src/acpi/gtdt/gtdt.c b/src/acpi/gtdt/gtdt.c
>> index ded1d5d..dc00785 100644
>> --- a/src/acpi/gtdt/gtdt.c
>> +++ b/src/acpi/gtdt/gtdt.c
>> @@ -56,6 +56,24 @@ static int gtdt_test1(fwts_framework *fw)
>>       uint32_t i = 0, n;
>>       const fwts_acpi_table_gtdt *gtdt = (const fwts_acpi_table_gtdt
>> *)table->data;
>>   +    if (gtdt->cnt_control_base_phys_addr == 0) {
>> +        passed = false;
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> +            "GTDTInvalidBaseAddr",
>> +            "The 64-bit physical address at which the "
>> +            "Counter Control block should not be Zero. "
>> +            "If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
>> +    }
>> +
>> +    if (gtdt->cnt_read_base_phys_addr == 0) {
>> +        passed = false;
>> +        fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> +            "GTDTInvalidBaseAddr",
>> +            "The 64-bit physical address at which the "
>> +            "Counter Read block Read block should not be Zero. "
>> +            "If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
>> +    }
>> +
>>       fwts_acpi_reserved_bits_check(fw, "GTDT", "Flags",
>> gtdt->virtual_timer_flags, sizeof(gtdt->virtual_timer_flags), 3, 31,
>> &passed);
>>         ptr = (uint8_t *)table->data + gtdt->platform_timer_offset;
>> @@ -89,6 +107,13 @@ static int gtdt_test1(fwts_framework *fw)
>>                       i, block->length);
>>                   goto done;
>>               }
>> +            if (block->physical_address == 0) {
>> +                passed = false;
>> +                fwts_failed(fw, LOG_LEVEL_HIGH,
>> +                    "GTDTBlockInvalidBaseAddr",
>> +                    "The 64-bit physical address at which the "
>> +                    "GT CntCTLBase Block shouldn't be zero.");
>> +            }
>>               if (block->reserved) {
>>                   passed = false;
>>                   fwts_failed(fw, LOG_LEVEL_HIGH,
>> @@ -146,6 +171,21 @@ static int gtdt_test1(fwts_framework *fw)
>>                           block_timer->reserved[1],
>>                           block_timer->reserved[2]);
>>                   }
>> +                if (block_timer->cntbase == 0) {
>> +                    passed = false;
>> +                    fwts_failed(fw, LOG_LEVEL_HIGH,
>> +                        "GTDTGTxInvalidBaseAddr",
>> +                        "Physical Address at which the "
>> +                        "CntBase block for GTx shouldn't be zero.");
>> +                }
>> +                if (block_timer->cntel0base == 0) {
>> +                    passed = false;
>> +                    fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> +                        "GTDTGTxInvalidBaseAddr",
>> +                        "Physical Address at which the "
>> +                        "CntEL0Base block for GTx should not be Zero. "
>> +                        "If not provided, this field must be
>> 0xFFFFFFFFFFFFFFFF");
>> +                }
>>                     snprintf(field, sizeof(field), "block %" PRIu32 "
>> physical timer flags", i);
>>                   fwts_acpi_reserved_bits_check(fw, "GTDT", field,
>> block_timer->phys_timer_flags,
>> @@ -189,6 +229,20 @@ static int gtdt_test1(fwts_framework *fw)
>>                       " reserved is non-zero, got 0x%" PRIx8,
>>                       i, watchdog->reserved);
>>               }
>> +            if (watchdog->refresh_frame_addr == 0) {
>> +                passed = false;
>> +                fwts_failed(fw, LOG_LEVEL_HIGH,
>> +                    "GTDTInvalidWatchDogBaseAddr",
>> +                    "Physical Address at which the "
>> +                    "RefreshFrame block shouldn't be zero.");
>> +            }
>> +            if (watchdog->watchdog_control_frame_addr == 0) {
>> +                passed = false;
>> +                fwts_failed(fw, LOG_LEVEL_HIGH,
>> +                    "GTDTInvalidWatchDogBaseAddr",
>> +                    "Physical Address at which the "
>> +                    "Watchdog Control Frame block shouldn't be zero.");
>> +            }
>>                 snprintf(field, sizeof(field), "SBSA generic watchdog
>> timer %" PRIu32 " flags", i);
>>               fwts_acpi_reserved_bits_check(fw, "GTDT", field,
>> watchdog->watchdog_timer_flags,
>>
>
>
Alex Hung Aug. 20, 2018, 5:16 a.m. UTC | #4
On 2018-08-15 12:38 AM, Ivan Hu wrote:
> Some buggy firmwares on Arm server implement GTDT table without impementing
> reasonable base physical address and causes kernel complains about ail to get

A typo "ail" here. It should be "failing" but I will fix when I apply 
this patch.

> base address. Add checking for the valid base addresses on GTDT table.
> 
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>   src/acpi/gtdt/gtdt.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 54 insertions(+)
> 
> diff --git a/src/acpi/gtdt/gtdt.c b/src/acpi/gtdt/gtdt.c
> index ded1d5d..dc00785 100644
> --- a/src/acpi/gtdt/gtdt.c
> +++ b/src/acpi/gtdt/gtdt.c
> @@ -56,6 +56,24 @@ static int gtdt_test1(fwts_framework *fw)
>   	uint32_t i = 0, n;
>   	const fwts_acpi_table_gtdt *gtdt = (const fwts_acpi_table_gtdt *)table->data;
>   
> +	if (gtdt->cnt_control_base_phys_addr == 0) {
> +		passed = false;
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"GTDTInvalidBaseAddr",
> +			"The 64-bit physical address at which the "
> +			"Counter Control block should not be Zero. "
> +			"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
> +	}
> +
> +	if (gtdt->cnt_read_base_phys_addr == 0) {
> +		passed = false;
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"GTDTInvalidBaseAddr",
> +			"The 64-bit physical address at which the "
> +			"Counter Read block Read block should not be Zero. "
> +			"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
> +	}
> +
>   	fwts_acpi_reserved_bits_check(fw, "GTDT", "Flags", gtdt->virtual_timer_flags, sizeof(gtdt->virtual_timer_flags), 3, 31, &passed);
>   
>   	ptr = (uint8_t *)table->data + gtdt->platform_timer_offset;
> @@ -89,6 +107,13 @@ static int gtdt_test1(fwts_framework *fw)
>   					i, block->length);
>   				goto done;
>   			}
> +			if (block->physical_address == 0) {
> +				passed = false;
> +				fwts_failed(fw, LOG_LEVEL_HIGH,
> +					"GTDTBlockInvalidBaseAddr",
> +					"The 64-bit physical address at which the "
> +					"GT CntCTLBase Block shouldn't be zero.");
> +			}
>   			if (block->reserved) {
>   				passed = false;
>   				fwts_failed(fw, LOG_LEVEL_HIGH,
> @@ -146,6 +171,21 @@ static int gtdt_test1(fwts_framework *fw)
>   						block_timer->reserved[1],
>   						block_timer->reserved[2]);
>   				}
> +				if (block_timer->cntbase == 0) {
> +					passed = false;
> +					fwts_failed(fw, LOG_LEVEL_HIGH,
> +						"GTDTGTxInvalidBaseAddr",
> +						"Physical Address at which the "
> +						"CntBase block for GTx shouldn't be zero.");
> +				}
> +				if (block_timer->cntel0base == 0) {
> +					passed = false;
> +					fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +						"GTDTGTxInvalidBaseAddr",
> +						"Physical Address at which the "
> +						"CntEL0Base block for GTx should not be Zero. "
> +						"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
> +				}
>   
>   				snprintf(field, sizeof(field), "block %" PRIu32 " physical timer flags", i);
>   				fwts_acpi_reserved_bits_check(fw, "GTDT", field, block_timer->phys_timer_flags,
> @@ -189,6 +229,20 @@ static int gtdt_test1(fwts_framework *fw)
>   					" reserved is non-zero, got 0x%" PRIx8,
>   					i, watchdog->reserved);
>   			}
> +			if (watchdog->refresh_frame_addr == 0) {
> +				passed = false;
> +				fwts_failed(fw, LOG_LEVEL_HIGH,
> +					"GTDTInvalidWatchDogBaseAddr",
> +					"Physical Address at which the "
> +					"RefreshFrame block shouldn't be zero.");
> +			}
> +			if (watchdog->watchdog_control_frame_addr == 0) {
> +				passed = false;
> +				fwts_failed(fw, LOG_LEVEL_HIGH,
> +					"GTDTInvalidWatchDogBaseAddr",
> +					"Physical Address at which the "
> +					"Watchdog Control Frame block shouldn't be zero.");
> +			}
>   
>   			snprintf(field, sizeof(field), "SBSA generic watchdog timer %" PRIu32 " flags", i);
>   			fwts_acpi_reserved_bits_check(fw, "GTDT", field, watchdog->watchdog_timer_flags,
> 

Acked-by: Alex Hung <alex.hung@canonical.com>

Patch
diff mbox series

diff --git a/src/acpi/gtdt/gtdt.c b/src/acpi/gtdt/gtdt.c
index ded1d5d..dc00785 100644
--- a/src/acpi/gtdt/gtdt.c
+++ b/src/acpi/gtdt/gtdt.c
@@ -56,6 +56,24 @@  static int gtdt_test1(fwts_framework *fw)
 	uint32_t i = 0, n;
 	const fwts_acpi_table_gtdt *gtdt = (const fwts_acpi_table_gtdt *)table->data;
 
+	if (gtdt->cnt_control_base_phys_addr == 0) {
+		passed = false;
+		fwts_failed(fw, LOG_LEVEL_MEDIUM,
+			"GTDTInvalidBaseAddr",
+			"The 64-bit physical address at which the "
+			"Counter Control block should not be Zero. "
+			"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
+	}
+
+	if (gtdt->cnt_read_base_phys_addr == 0) {
+		passed = false;
+		fwts_failed(fw, LOG_LEVEL_MEDIUM,
+			"GTDTInvalidBaseAddr",
+			"The 64-bit physical address at which the "
+			"Counter Read block Read block should not be Zero. "
+			"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
+	}
+
 	fwts_acpi_reserved_bits_check(fw, "GTDT", "Flags", gtdt->virtual_timer_flags, sizeof(gtdt->virtual_timer_flags), 3, 31, &passed);
 
 	ptr = (uint8_t *)table->data + gtdt->platform_timer_offset;
@@ -89,6 +107,13 @@  static int gtdt_test1(fwts_framework *fw)
 					i, block->length);
 				goto done;
 			}
+			if (block->physical_address == 0) {
+				passed = false;
+				fwts_failed(fw, LOG_LEVEL_HIGH,
+					"GTDTBlockInvalidBaseAddr",
+					"The 64-bit physical address at which the "
+					"GT CntCTLBase Block shouldn't be zero.");
+			}
 			if (block->reserved) {
 				passed = false;
 				fwts_failed(fw, LOG_LEVEL_HIGH,
@@ -146,6 +171,21 @@  static int gtdt_test1(fwts_framework *fw)
 						block_timer->reserved[1],
 						block_timer->reserved[2]);
 				}
+				if (block_timer->cntbase == 0) {
+					passed = false;
+					fwts_failed(fw, LOG_LEVEL_HIGH,
+						"GTDTGTxInvalidBaseAddr",
+						"Physical Address at which the "
+						"CntBase block for GTx shouldn't be zero.");
+				}
+				if (block_timer->cntel0base == 0) {
+					passed = false;
+					fwts_failed(fw, LOG_LEVEL_MEDIUM,
+						"GTDTGTxInvalidBaseAddr",
+						"Physical Address at which the "
+						"CntEL0Base block for GTx should not be Zero. "
+						"If not provided, this field must be 0xFFFFFFFFFFFFFFFF");
+				}
 
 				snprintf(field, sizeof(field), "block %" PRIu32 " physical timer flags", i);
 				fwts_acpi_reserved_bits_check(fw, "GTDT", field, block_timer->phys_timer_flags,
@@ -189,6 +229,20 @@  static int gtdt_test1(fwts_framework *fw)
 					" reserved is non-zero, got 0x%" PRIx8,
 					i, watchdog->reserved);
 			}
+			if (watchdog->refresh_frame_addr == 0) {
+				passed = false;
+				fwts_failed(fw, LOG_LEVEL_HIGH,
+					"GTDTInvalidWatchDogBaseAddr",
+					"Physical Address at which the "
+					"RefreshFrame block shouldn't be zero.");
+			}
+			if (watchdog->watchdog_control_frame_addr == 0) {
+				passed = false;
+				fwts_failed(fw, LOG_LEVEL_HIGH,
+					"GTDTInvalidWatchDogBaseAddr",
+					"Physical Address at which the "
+					"Watchdog Control Frame block shouldn't be zero.");
+			}
 
 			snprintf(field, sizeof(field), "SBSA generic watchdog timer %" PRIu32 " flags", i);
 			fwts_acpi_reserved_bits_check(fw, "GTDT", field, watchdog->watchdog_timer_flags,