efi/x86: Revert struct layout change to fix kexec boot regression
Subsystems (4)
Ard Biesheuvel
maintainerPATCH 1/1 efi/x86: Revert struct layout change to fix kexec boot regression
Commit
0a67361dcdaa29 ("efi/x86: Remove runtime table address from kexec EFI setup data")
removed the code that retrieves the non-remapped UEFI runtime services pointer from the data structure provided by kexec, as it was never really needed on the kexec boot path: mapping the runtime services table at its non-remapped address is only needed when calling SetVirtualAddressMap(), which never happens during a kexec boot in the first place.
However, dropping the 'runtime' member from struct efi_setup_data was a mistake. That struct is shared ABI between the kernel and the kexec tooling for x86, and so we cannot simply change its layout. So let's put back the removed field, but call it 'unused' to reflect the fact that we never look at its contents. While at it, add a comment to remind our future selves that the layout is external ABI.
---
Ingo, Thomas, Boris: I sent out my efi-urgent pull request just yesterday, so please take this directly into tip:efi/urgent - no need to wait for the next batch.
arch/x86/include/asm/efi.h | 2 ++ 1 file changed, 2 insertions(+)
arch/x86/include/asm/efi.h +2 -0
@@ -178,8 +178,10 @@ extern void efi_free_boot_services(void);extern pgd_t *__init efi_uv1_memmap_phys_prolog (void );extern void __init efi_uv1_memmap_phys_epilog (pgd_t *save_pgd); +/* kexec external ABI */ struct efi_setup_data {u64 fw_vendor; +u64 unused;u64 tables;u64 smbios;u64 reserved[8 ];
Signature · 1 line
2.17.1
Quoted 47 lines · Ard Biesheuvel
Commit
0a67361dcdaa29("efi/x86: Remove runtime table address from kexec EFI setup data")removed the code that retrieves the non-remapped UEFI runtime services pointer from the data structure provided by kexec, as it was never really needed on the kexec boot path: mapping the runtime services table at its non-remapped address is only needed when calling
SetVirtualAddressMap(), which never happens during a kexec boot in the first place.However, dropping the 'runtime' member from struct
efi_setup_datawas a mistake. That struct is shared ABI between the kernel and the kexec tooling for x86, and so we cannot simply change its layout. So let's put back the removed field, but call it 'unused' to reflect the fact that we never look at its contents. While at it, add a comment to remind our future selves that the layout is external ABI.Reported-by Theodore Ts'o <[email protected]>Tested-by Theodore Ts'o <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>---
Ingo, Thomas, Boris: I sent out my efi-urgent pull request just yesterday, so please take this directly into tip:efi/urgent - no need to wait for the next batch.
arch/x86/include/asm/efi.h | 2 ++ 1 file changed, 2 insertions(+)arch/x86/include/asm/efi.h +2
-0@@ -178,8 +178,10 @@ extern void efi_free_boot_services(void);extern pgd_t *__init efi_uv1_memmap_phys_prolog (void );extern void __init efi_uv1_memmap_phys_epilog (pgd_t *save_pgd); +/* kexec external ABI */ struct efi_setup_data {u64 fw_vendor; +u64 unused;u64 tables;u64 smbios;u64 reserved[8 ];Signature · 1 line
2.17.1
Ah, replied too quick in another mail. I just cced kexec list again.
Thanks for the fix:
Thanks
Dave
Quoted 51 lines · Dave Young
Quoted 47 lines · Ard Biesheuvel
Commit
0a67361dcdaa29("efi/x86: Remove runtime table address from kexec EFI setup data")removed the code that retrieves the non-remapped UEFI runtime services pointer from the data structure provided by kexec, as it was never really needed on the kexec boot path: mapping the runtime services table at its non-remapped address is only needed when calling
SetVirtualAddressMap(), which never happens during a kexec boot in the first place.However, dropping the 'runtime' member from struct
efi_setup_datawas a mistake. That struct is shared ABI between the kernel and the kexec tooling for x86, and so we cannot simply change its layout. So let's put back the removed field, but call it 'unused' to reflect the fact that we never look at its contents. While at it, add a comment to remind our future selves that the layout is external ABI.Reported-by Theodore Ts'o <[email protected]>Tested-by Theodore Ts'o <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>---
Ingo, Thomas, Boris: I sent out my efi-urgent pull request just yesterday, so please take this directly into tip:efi/urgent - no need to wait for the next batch.
arch/x86/include/asm/efi.h | 2 ++ 1 file changed, 2 insertions(+)arch/x86/include/asm/efi.h +2
-0@@ -178,8 +178,10 @@ extern void efi_free_boot_services(void);extern pgd_t *__init efi_uv1_memmap_phys_prolog (void );extern void __init efi_uv1_memmap_phys_epilog (pgd_t *save_pgd); +/* kexec external ABI */ struct efi_setup_data {u64 fw_vendor; +u64 unused;u64 tables;u64 smbios;u64 reserved[8 ];Signature · 1 line
2.17.1Ah, replied too quick in another mail. I just cced kexec list again.
Quoted 4 lines
Thanks for the fix:
Reviewed-by Dave Young <[email protected]>
BTW, a fixes tag is good to have..
Ard Biesheuvel
maintainerQuoted 53 lines · Dave Young
Quoted 51 lines · Dave Young
Quoted 45 lines · Ard Biesheuvel
Commit
0a67361dcdaa29("efi/x86: Remove runtime table address from kexec EFI setup data")removed the code that retrieves the non-remapped UEFI runtime services pointer from the data structure provided by kexec, as it was never really needed on the kexec boot path: mapping the runtime services table at its non-remapped address is only needed when calling
SetVirtualAddressMap(), which never happens during a kexec boot in the first place.However, dropping the 'runtime' member from struct
efi_setup_datawas a mistake. That struct is shared ABI between the kernel and the kexec tooling for x86, and so we cannot simply change its layout. So let's put back the removed field, but call it 'unused' to reflect the fact that we never look at its contents. While at it, add a comment to remind our future selves that the layout is external ABI.Reported-by Theodore Ts'o <[email protected]>Tested-by Theodore Ts'o <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>---
Ingo, Thomas, Boris: I sent out my efi-urgent pull request just yesterday, so please take this directly into tip:efi/urgent - no need to wait for the next batch.
arch/x86/include/asm/efi.h | 2 ++ 1 file changed, 2 insertions(+)arch/x86/include/asm/efi.h +2
-1@@ -178,8 +178,10 @@ extern void efi_free_boot_services(void);extern pgd_t *__init efi_uv1_memmap_phys_prolog (void );extern void __init efi_uv1_memmap_phys_epilog (pgd_t *save_pgd); +/* kexec external ABI */ struct efi_setup_data {u64 fw_vendor; +u64 unused;u64 tables;u64 smbios;u64 reserved[8 ]; --Quoted 2 lines
2.17.1
Ah, replied too quick in another mail. I just cced kexec list again.
Quoted 5 lines
Quoted 4 lines
Thanks for the fix:
Reviewed-by Dave Young <[email protected]>
Thanks Dave
Quoted 2 lines
BTW, a fixes tag is good to have..
I usually omit those for patches that fix bugs that were introduced in the current cycle.
Borislav Petkov
maintainerQuoted 4 lines · Ard Biesheuvel
Quoted 1 line
BTW, a fixes tag is good to have..
I usually omit those for patches that fix bugs that were introduced in the current cycle.
A valid use case for having the Fixes: tag anyway are the backporting kernels gangs which might pick up the first patch for whatever reason and would probably be thankful if they find the second one, i.e., the fix for the first one, through grepping or other, automated means.
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteArd Biesheuvel
maintainerQuoted 4 lines · Borislav Petkov
Quoted 2 lines · Ard Biesheuvel
Quoted 1 line
BTW, a fixes tag is good to have..
Quoted 8 lines
Quoted 2 lines
I usually omit those for patches that fix bugs that were introduced in the current cycle.
A valid use case for having the Fixes: tag anyway are the backporting kernels gangs which might pick up the first patch for whatever reason and would probably be thankful if they find the second one, i.e., the fix for the first one, through grepping or other, automated means.
Fair point.
Quoted 8 lines · Ard Biesheuvel
Quoted 7 lines · Borislav Petkov
Quoted 4 lines · Ard Biesheuvel
Quoted 1 line
BTW, a fixes tag is good to have..
I usually omit those for patches that fix bugs that were introduced in the current cycle.
Quoted 7 lines
Quoted 5 lines
A valid use case for having the Fixes: tag anyway are the backporting kernels gangs which might pick up the first patch for whatever reason and would probably be thankful if they find the second one, i.e., the fix for the first one, through grepping or other, automated means.
Fair point.
I've added:
Out of abundance of caution. :-)
Thanks,
Ingo
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec