Re: 5.3 boot regression caused by 5.3 TPM changes
On Sun, 4 Aug 2019 at 19:12, Hans de Goede wrote:
>
> Hi,
>
> On 04-08-19 17:33, Ard Biesheuvel wrote:
> > Hi Hans,
> >
> > On Sun, 4 Aug 2019 at 13:00, Hans de Goede wrote:
> >>
> >> Hi All,
> >>
> >> While testing 5.3-rc2 on an Irbis TW90 Intel Cherry Trail based
> >> tablet I noticed that it does not boot on this device.
> >>
> >> A git bisect points to commit 166a2809d65b ("tpm: Don't duplicate
> >> events from the final event log in the TCG2 log")
> >>
> >> And I can confirm that reverting just that single commit makes
> >> the TW90 boot again.
> >>
> >> This machine uses AptIO firmware with base component versions
> >> of: UEFI 2.4 PI 1.3. I've tried to reproduce the problem on
> >> a Teclast X80 Pro which is also CHT based and also uses AptIO
> >> firmware with the same base components. But it does not reproduce
> >> there. Neither does the problem reproduce on a CHT tablet using
> >> InsideH20 based firmware.
> >>
> >> Note that these devices have a software/firmware TPM-2.0
> >> implementation, they do not have an actual TPM chip.
> >>
> >> Comparing TPM firmware setting between the 2 AptIO based
> >> tablets the settings are identical, but the troublesome
> >> TW90 does have some more setting then the X80, it has
> >> the following settings which are not shown on the X80:
> >>
> >> Active PCR banks: SHA-1 (read only)
> >> Available PCR banks:SHA-1,SHA256 (read only)
> >> TPM2.0 UEFI SPEC version: TCG_2 (other possible setting: TCG_1_2
> >> Physical Presence SPEC ver: 1.2 (other possible setting: 1.3)
> >>
> >> I have the feeling that at least the first 2 indicate that
> >> the previous win10 installation has actually used the
> >> TPM, where as on the X80 the TPM is uninitialized.
> >> Note this is just a hunch I could be completely wrong.
> >>
> >> I would be happy to run any commands to try and debug this
> >> or to build a kernel with some patches to gather more info.
> >>
> >> Note any kernel patches to printk some debug stuff need
> >> to be based on 5.3 with 166a2809d65b reverted, without that
> >> reverted the device will not boot, and thus I cannot collect
> >> logs without it reverted.
> >>
> >
> > Are you booting a 64-bit kernel on 32-bit firmware?
>
> Yes you are right, I must say that this is somewhat surprising
> most Cherry Trail devices do use 64 bit firmware (where as Bay Trail
> typically uses 32 bit). But I just checked efibootmgr output and it
> says it is booting: \EFI\FEDORA\SHIMIA32.EFI so yeah 32 bit firmware.
>
> Recent Fedora releases take care of this so seamlessly I did not
> even realize...
>
OK, so we'll have to find out how this patch affects 64-bit code
running on 32-bit firmware. The only EFI call in that patch is
get_config_table(), which is not actually a EFI boot service call but
a EFI stub helper that parses the config table array in the EFI system
table.
Re: [PATCH] arm64/efi: fix variable 'si' set but not used
On Tue, Jul 30, 2019 at 05:23:48PM -0400, Qian Cai wrote:
> GCC throws out this warning on arm64.
>
> drivers/firmware/efi/libstub/arm-stub.c: In function 'efi_entry':
> drivers/firmware/efi/libstub/arm-stub.c:132:22: warning: variable 'si'
> set but not used [-Wunused-but-set-variable]
>
> Fix it by making free_screen_info() a static inline function.
>
> Signed-off-by: Qian Cai
> ---
> arch/arm64/include/asm/efi.h | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8e79ce9c3f5c..76a144702586 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -105,7 +105,11 @@ static inline unsigned long
> efi_get_max_initrd_addr(unsigned long dram_base,
> ((protocol##_t *)instance)->f(instance, ##__VA_ARGS__)
>
> #define alloc_screen_info(x...) _info
> -#define free_screen_info(x...)
> +
> +static inline void free_screen_info(efi_system_table_t *sys_table_arg,
> + struct screen_info *si)
> +{
> +}
Is this issue caused by the EFI stub being built with non-default
CFLAGS?
The toplevel Makefile specifies -Wno-unused-but-set-variable, which
would silence this warning.
It's debatable whether calling an empty inline function "uses" the
arguments, so I think your patch only silences the warning by accident:
different GCC versions, or clang, might still warn.
I wonder if we're missing any other options that would make sense for
the EFI stub.
[...]
Cheers
---Dave
[PATCH 2/2] ia64: Replace strncmp with str_has_prefix
strncmp(str, const, len) is error-prone since the len is
easy to be wrong because of counting error or sizeof(const)
without - 1.
Use the newly introduced str_has_prefix() to substitute
it to make code better.
Signed-off-by: Chuhong Yuan
---
arch/ia64/kernel/acpi.c | 9 -
arch/ia64/kernel/efi.c | 2 +-
arch/ia64/kernel/esi.c | 2 +-
arch/ia64/kernel/sal.c | 2 +-
4 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index c597ab5275b8..f01e6fb2f962 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c
@@ -77,7 +77,7 @@ acpi_get_sysname(void)
}
rsdp = (struct acpi_table_rsdp *)__va(rsdp_phys);
- if (strncmp(rsdp->signature, ACPI_SIG_RSDP, sizeof(ACPI_SIG_RSDP) - 1))
{
+ if (!str_has_prefix(rsdp->signature, ACPI_SIG_RSDP)) {
printk(KERN_ERR
"ACPI 2.0 RSDP signature incorrect, default to
\"dig\"\n");
return "dig";
@@ -85,7 +85,7 @@ acpi_get_sysname(void)
xsdt = (struct acpi_table_xsdt *)__va(rsdp->xsdt_physical_address);
hdr = >header;
- if (strncmp(hdr->signature, ACPI_SIG_XSDT, sizeof(ACPI_SIG_XSDT) - 1)) {
+ if (!str_has_prefix(hdr->signature, ACPI_SIG_XSDT)) {
printk(KERN_ERR
"ACPI 2.0 XSDT signature incorrect, default to
\"dig\"\n");
return "dig";
@@ -106,8 +106,7 @@ acpi_get_sysname(void)
sizeof(xsdt->table_offset_entry[0]);
for (i = 0; i < nentries; i++) {
hdr = __va(xsdt->table_offset_entry[i]);
- if (strncmp(hdr->signature, ACPI_SIG_DMAR,
- sizeof(ACPI_SIG_DMAR) - 1) == 0)
+ if (str_has_prefix(hdr->signature, ACPI_SIG_DMAR))
return "dig_vtd";
}
#endif
@@ -348,7 +347,7 @@ acpi_parse_nmi_src(union acpi_subtable_headers * header,
const unsigned long end
static void __init acpi_madt_oem_check(char *oem_id, char *oem_table_id)
{
- if (!strncmp(oem_id, "IBM", 3) && (!strncmp(oem_table_id, "SERMOW",
6))) {
+ if (str_has_prefix(oem_id, "IBM") && (str_has_prefix(oem_table_id,
"SERMOW"))) {
/*
* Unfortunately ITC_DRIFT is not yet part of the
diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 3795d18276c4..615e73bd88fd 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -434,7 +434,7 @@ static void __init handle_palo(unsigned long phys_addr)
struct palo_table *palo = __va(phys_addr);
u8 checksum;
- if (strncmp(palo->signature, PALO_SIG, sizeof(PALO_SIG) - 1)) {
+ if (!str_has_prefix(palo->signature, PALO_SIG)) {
printk(KERN_INFO "PALO signature incorrect.\n");
return;
}
diff --git a/arch/ia64/kernel/esi.c b/arch/ia64/kernel/esi.c
index cb514126ef7f..d929689a99e4 100644
--- a/arch/ia64/kernel/esi.c
+++ b/arch/ia64/kernel/esi.c
@@ -70,7 +70,7 @@ static int __init esi_init (void)
systab = __va(esi);
- if (strncmp(systab->signature, "ESIT", 4) != 0) {
+ if (!str_has_prefix(systab->signature, "ESIT")) {
printk(KERN_ERR "bad signature in ESI system table!");
return -ENODEV;
}
diff --git a/arch/ia64/kernel/sal.c b/arch/ia64/kernel/sal.c
index 9b2331ac10ce..9f02e3ec28e5 100644
--- a/arch/ia64/kernel/sal.c
+++ b/arch/ia64/kernel/sal.c
@@ -315,7 +315,7 @@ ia64_sal_init (struct ia64_sal_systab *systab)
return;
}
- if (strncmp(systab->signature, "SST_", 4) != 0)
+ if (!str_has_prefix(systab->signature, "SST_"))
printk(KERN_ERR "bad signature in system table!");
check_versions(systab);
--
2.20.1
