| 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 | expand |
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>
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, >
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, >> > >
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>
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,
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(+)