On 8/21/19 10:05 AM, Neil MacLeod wrote:
> Hi John
>
> The following bug might be of interest:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=204645
>
> Let me know if I can be of any help.
>
Hi Neil,
First of all, I'm pasting in the bug information so that it's directly available
here:
===================================================================
Description: Neil MacLeod 2019-08-21 16:29:19 UTC
System: Intel i5 Skylake NUC (NUC6i5SYH)
This system boots fine from internal M2 (128GB) drive with 5.3-rc4.
With 5.3-rc5, it does not boot from M2 and is stuck on the Intel splash screen (no other text is displayed, no panic etc.). It will boot 5.3-rc5 from a USB flash memory stick (via the F10 boot menu), but not from the internal M2.
Bisecting between 5.3-rc4 and 5.3-rc5, the bad commit is:
neil@nm-linux:~/projects/pullrequest_repos/torvalds-linux$ git bisect bad
a90118c445cc7f07781de26a9684d4ec58bfcfd1 is the first bad commit
commit a90118c445cc7f07781de26a9684d4ec58bfcfd1
Author: John Hubbard <[email protected]>
Date: Tue Jul 30 22:46:27 2019 -0700
x86/boot: Save fields explicitly, zero out everything else
Recent gcc compilers (gcc 9.1) generate warnings about an out of bounds
memset, if the memset goes accross several fields of a struct. This
generated a couple of warnings on x86_64 builds in sanitize_boot_params().
Fix this by explicitly saving the fields in struct boot_params
that are intended to be preserved, and zeroing all the rest.
[ tglx: Tagged for stable as it breaks the warning free build there as well ]
Suggested-by: Thomas Gleixner <[email protected]>
Suggested-by: H. Peter Anvin <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
:040000 040000 e0963edca990540dd759765a3d765af4698df892 d07e645eb3a500c31bd65526205e286ff6941187 M arch
Comment 1 Neil MacLeod 2019-08-21 16:31:35 UTC
The kernel is built with gcc-9.2.0.
Comment 2 Neil MacLeod 2019-08-21 16:55:48 UTC
5.3-rc5 with "x86/boot: Save fields explicitly, zero out everything else" reverted will build with gcc-9.2.0, and boot from M2.
===================================================================
I'm also CC'ing the lists, so they know that the patch has caused a regression, and
also out of hope that they can help me choose the shortest path forward to debugging
this. My first reaction is that the list of BOOT_PARAM_PRESERVE() fields is
flawed--by which I include cases of "the system improperly relied on a field that the spec said
should be zeroed". (After all, the boot_params->sentinel is set, which already means
the system is not really doing it right.)
So I'm going to cheat and ask right now if anyone notices either ommissions
or wrong entries here:
static void sanitize_boot_params(struct boot_params *boot_params)
{
...
const struct boot_params_to_save to_save[] = {
BOOT_PARAM_PRESERVE(screen_info),
BOOT_PARAM_PRESERVE(apm_bios_info),
BOOT_PARAM_PRESERVE(tboot_addr),
BOOT_PARAM_PRESERVE(ist_info),
BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
BOOT_PARAM_PRESERVE(hd0_info),
BOOT_PARAM_PRESERVE(hd1_info),
BOOT_PARAM_PRESERVE(sys_desc_table),
BOOT_PARAM_PRESERVE(olpc_ofw_header),
BOOT_PARAM_PRESERVE(efi_info),
BOOT_PARAM_PRESERVE(alt_mem_k),
BOOT_PARAM_PRESERVE(scratch),
BOOT_PARAM_PRESERVE(e820_entries),
BOOT_PARAM_PRESERVE(eddbuf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
BOOT_PARAM_PRESERVE(e820_table),
BOOT_PARAM_PRESERVE(eddbuf),
};
If not, then I think we need to bisect by...well, let's start with the
theory that we zeroed out too much, so we could start adding more fields
to preserve. If that doesn't find the problem, then it's more complicated,
and might be better to go the other direction: starting without my commit,
and zeroing out fields until we see the failure.
Are you able to test patches? It would take some time, since there are quite a
few fields. Alternately, if you provide some more system information details
(especially if we have any other notes about other failures and passes), then
I might be able to borrow a Skylake system and attempt a repro.
thanks,
--
John Hubbard
NVIDIA
Hi John
I can test any patches given a link to the diff, and am happy to do so.
If I've understood your suggestion correctly, I will try commenting
out all of the entries, then add them back one-by-one until I get a
non-booting situation. I'll let you know how I get on.
The OS I'm testing is LibreELEC, which is a custom x86_64 build, and
this hasn't shown any problems on this Skylake i5 NUC in all the years
I've been using it as a test system (since at least 4.15.y). So far
5.3-rc has been trouble-free until 5.3-rc5. As I mentioned in the bug,
I've been able to boot 5.3-rc5 from a USB memory stick, but can't boot
5.3-rc5 from the internal M2 drive unless I revert this commit.
Regards
Neil
On Wed, 21 Aug 2019 at 19:20, John Hubbard <[email protected]> wrote:
>
> On 8/21/19 10:05 AM, Neil MacLeod wrote:
> > Hi John
> >
> > The following bug might be of interest:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=204645
> >
> > Let me know if I can be of any help.
> >
>
> Hi Neil,
>
> First of all, I'm pasting in the bug information so that it's directly available
> here:
>
> ===================================================================
> Description: Neil MacLeod 2019-08-21 16:29:19 UTC
> System: Intel i5 Skylake NUC (NUC6i5SYH)
>
> This system boots fine from internal M2 (128GB) drive with 5.3-rc4.
>
> With 5.3-rc5, it does not boot from M2 and is stuck on the Intel splash screen (no other text is displayed, no panic etc.). It will boot 5.3-rc5 from a USB flash memory stick (via the F10 boot menu), but not from the internal M2.
>
> Bisecting between 5.3-rc4 and 5.3-rc5, the bad commit is:
>
> neil@nm-linux:~/projects/pullrequest_repos/torvalds-linux$ git bisect bad
> a90118c445cc7f07781de26a9684d4ec58bfcfd1 is the first bad commit
> commit a90118c445cc7f07781de26a9684d4ec58bfcfd1
> Author: John Hubbard <[email protected]>
> Date: Tue Jul 30 22:46:27 2019 -0700
>
> x86/boot: Save fields explicitly, zero out everything else
>
> Recent gcc compilers (gcc 9.1) generate warnings about an out of bounds
> memset, if the memset goes accross several fields of a struct. This
> generated a couple of warnings on x86_64 builds in sanitize_boot_params().
>
> Fix this by explicitly saving the fields in struct boot_params
> that are intended to be preserved, and zeroing all the rest.
>
> [ tglx: Tagged for stable as it breaks the warning free build there as well ]
>
> Suggested-by: Thomas Gleixner <[email protected]>
> Suggested-by: H. Peter Anvin <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: [email protected]
> Link: https://lkml.kernel.org/r/[email protected]
>
> :040000 040000 e0963edca990540dd759765a3d765af4698df892 d07e645eb3a500c31bd65526205e286ff6941187 M arch
>
> Comment 1 Neil MacLeod 2019-08-21 16:31:35 UTC
> The kernel is built with gcc-9.2.0.
>
> Comment 2 Neil MacLeod 2019-08-21 16:55:48 UTC
>
> 5.3-rc5 with "x86/boot: Save fields explicitly, zero out everything else" reverted will build with gcc-9.2.0, and boot from M2.
> ===================================================================
>
> I'm also CC'ing the lists, so they know that the patch has caused a regression, and
> also out of hope that they can help me choose the shortest path forward to debugging
> this. My first reaction is that the list of BOOT_PARAM_PRESERVE() fields is
> flawed--by which I include cases of "the system improperly relied on a field that the spec said
> should be zeroed". (After all, the boot_params->sentinel is set, which already means
> the system is not really doing it right.)
>
> So I'm going to cheat and ask right now if anyone notices either ommissions
> or wrong entries here:
>
> static void sanitize_boot_params(struct boot_params *boot_params)
> {
> ...
> const struct boot_params_to_save to_save[] = {
> BOOT_PARAM_PRESERVE(screen_info),
> BOOT_PARAM_PRESERVE(apm_bios_info),
> BOOT_PARAM_PRESERVE(tboot_addr),
> BOOT_PARAM_PRESERVE(ist_info),
> BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> BOOT_PARAM_PRESERVE(hd0_info),
> BOOT_PARAM_PRESERVE(hd1_info),
> BOOT_PARAM_PRESERVE(sys_desc_table),
> BOOT_PARAM_PRESERVE(olpc_ofw_header),
> BOOT_PARAM_PRESERVE(efi_info),
> BOOT_PARAM_PRESERVE(alt_mem_k),
> BOOT_PARAM_PRESERVE(scratch),
> BOOT_PARAM_PRESERVE(e820_entries),
> BOOT_PARAM_PRESERVE(eddbuf_entries),
> BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
> BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> BOOT_PARAM_PRESERVE(e820_table),
> BOOT_PARAM_PRESERVE(eddbuf),
> };
>
> If not, then I think we need to bisect by...well, let's start with the
> theory that we zeroed out too much, so we could start adding more fields
> to preserve. If that doesn't find the problem, then it's more complicated,
> and might be better to go the other direction: starting without my commit,
> and zeroing out fields until we see the failure.
>
> Are you able to test patches? It would take some time, since there are quite a
> few fields. Alternately, if you provide some more system information details
> (especially if we have any other notes about other failures and passes), then
> I might be able to borrow a Skylake system and attempt a repro.
>
>
> thanks,
> --
> John Hubbard
> NVIDIA
On 8/21/19 11:32 AM, Neil MacLeod wrote:
> Hi John
>
> I can test any patches given a link to the diff, and am happy to do so.
>
> If I've understood your suggestion correctly, I will try commenting
> out all of the entries, then add them back one-by-one until I get a
> non-booting situation. I'll let you know how I get on.
>
I was actually going the other direction. Note that the BOOT_PARAM_PRESERVE
entries indicate what *not* to zero out. So if you remove them all, then
everything gets zeroed, including lots of critical fields, and you
definitely won't boot up. (The tricky part is we don't yet know whether
fields are missing, need to be added--or both.)
So I was heading toward adding most (but not all--important) of these fields,
as BOOT_PARAM_PRESERVE entries, as a first bisect step. Let me work that up
and post a patch for that.
struct boot_params {
struct screen_info screen_info; /* 0x000 */
struct apm_bios_info apm_bios_info; /* 0x040 */
__u8 _pad2[4]; /* 0x054 */
__u64 tboot_addr; /* 0x058 */
struct ist_info ist_info; /* 0x060 */
__u64 acpi_rsdp_addr; /* 0x070 */
__u8 _pad3[8]; /* 0x078 */
__u8 hd0_info[16]; /* obsolete! */ /* 0x080 */
__u8 hd1_info[16]; /* obsolete! */ /* 0x090 */
struct sys_desc_table sys_desc_table; /* obsolete! */ /* 0x0a0 */
struct olpc_ofw_header olpc_ofw_header; /* 0x0b0 */
__u32 ext_ramdisk_image; /* 0x0c0 */
__u32 ext_ramdisk_size; /* 0x0c4 */
__u32 ext_cmd_line_ptr; /* 0x0c8 */
__u8 _pad4[116]; /* 0x0cc */
struct edid_info edid_info; /* 0x140 */
struct efi_info efi_info; /* 0x1c0 */
__u32 alt_mem_k; /* 0x1e0 */
__u32 scratch; /* Scratch field! */ /* 0x1e4 */
__u8 e820_entries; /* 0x1e8 */
__u8 eddbuf_entries; /* 0x1e9 */
__u8 edd_mbr_sig_buf_entries; /* 0x1ea */
__u8 kbd_status; /* 0x1eb */
__u8 secure_boot; /* 0x1ec */
__u8 _pad5[2]; /* 0x1ed */
/*
* The sentinel is set to a nonzero value (0xff) in header.S.
*
* A bootloader is supposed to only take setup_header and put
* it into a clean boot_params buffer. If it turns out that
* it is clumsy or too generous with the buffer, it most
* probably will pick up the sentinel variable too. The fact
* that this variable then is still 0xff will let kernel
* know that some variables in boot_params are invalid and
* kernel should zero out certain portions of boot_params.
*/
__u8 sentinel; /* 0x1ef */
__u8 _pad6[1]; /* 0x1f0 */
struct setup_header hdr; /* setup header */ /* 0x1f1 */
__u8 _pad7[0x290-0x1f1-sizeof(struct setup_header)];
__u32 edd_mbr_sig_buffer[EDD_MBR_SIG_MAX]; /* 0x290 */
struct boot_e820_entry e820_table[E820_MAX_ENTRIES_ZEROPAGE]; /* 0x2d0 */
__u8 _pad8[48]; /* 0xcd0 */
struct edd_info eddbuf[EDDMAXNR]; /* 0xd00 */
__u8 _pad9[276]; /* 0xeec */
} __attribute__((packed));
thanks,
--
John Hubbard
NVIDIA
> The OS I'm testing is LibreELEC, which is a custom x86_64 build, and
> this hasn't shown any problems on this Skylake i5 NUC in all the years
> I've been using it as a test system (since at least 4.15.y). So far
> 5.3-rc has been trouble-free until 5.3-rc5. As I mentioned in the bug,
> I've been able to boot 5.3-rc5 from a USB memory stick, but can't boot
> 5.3-rc5 from the internal M2 drive unless I revert this commit.
>
On Wed, 21 Aug 2019, John Hubbard wrote:
> On 8/21/19 10:05 AM, Neil MacLeod wrote:
> static void sanitize_boot_params(struct boot_params *boot_params)
> {
> ...
> const struct boot_params_to_save to_save[] = {
> BOOT_PARAM_PRESERVE(screen_info),
> BOOT_PARAM_PRESERVE(apm_bios_info),
> BOOT_PARAM_PRESERVE(tboot_addr),
> BOOT_PARAM_PRESERVE(ist_info),
> BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> BOOT_PARAM_PRESERVE(hd0_info),
> BOOT_PARAM_PRESERVE(hd1_info),
> BOOT_PARAM_PRESERVE(sys_desc_table),
> BOOT_PARAM_PRESERVE(olpc_ofw_header),
> BOOT_PARAM_PRESERVE(efi_info),
> BOOT_PARAM_PRESERVE(alt_mem_k),
> BOOT_PARAM_PRESERVE(scratch),
> BOOT_PARAM_PRESERVE(e820_entries),
> BOOT_PARAM_PRESERVE(eddbuf_entries),
> BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
> BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> BOOT_PARAM_PRESERVE(e820_table),
> BOOT_PARAM_PRESERVE(eddbuf),
> };
I think I spotted it:
- boot_params->acpi_rsdp_addr = 0;
+ BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
And it does not preserve 'hdr'
Grr. I surely was too tired when staring at this last time.
Thanks,
tglx
John & Thomas
Thanks both - if you can ping me a suitable patch I'll test it and let
you all know ASAP!
Neil
On Wed, 21 Aug 2019 at 19:51, Thomas Gleixner <[email protected]> wrote:
>
> On Wed, 21 Aug 2019, John Hubbard wrote:
> > On 8/21/19 10:05 AM, Neil MacLeod wrote:
> > static void sanitize_boot_params(struct boot_params *boot_params)
> > {
> > ...
> > const struct boot_params_to_save to_save[] = {
> > BOOT_PARAM_PRESERVE(screen_info),
> > BOOT_PARAM_PRESERVE(apm_bios_info),
> > BOOT_PARAM_PRESERVE(tboot_addr),
> > BOOT_PARAM_PRESERVE(ist_info),
> > BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> > BOOT_PARAM_PRESERVE(hd0_info),
> > BOOT_PARAM_PRESERVE(hd1_info),
> > BOOT_PARAM_PRESERVE(sys_desc_table),
> > BOOT_PARAM_PRESERVE(olpc_ofw_header),
> > BOOT_PARAM_PRESERVE(efi_info),
> > BOOT_PARAM_PRESERVE(alt_mem_k),
> > BOOT_PARAM_PRESERVE(scratch),
> > BOOT_PARAM_PRESERVE(e820_entries),
> > BOOT_PARAM_PRESERVE(eddbuf_entries),
> > BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
> > BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> > BOOT_PARAM_PRESERVE(e820_table),
> > BOOT_PARAM_PRESERVE(eddbuf),
> > };
>
> I think I spotted it:
>
> - boot_params->acpi_rsdp_addr = 0;
>
> + BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>
> And it does not preserve 'hdr'
>
> Grr. I surely was too tired when staring at this last time.
>
> Thanks,
>
> tglx
On 8/21/19 12:49 PM, Neil MacLeod wrote:
> The fix looks good - many thanks for the quick turnaround!
>
Great news, and thanks for the bug report!
thanks,
--
John Hubbard
NVIDIA
>
> On Wed, 21 Aug 2019 at 19:56, John Hubbard <[email protected]> wrote:
>>
>> On 8/21/19 11:51 AM, Thomas Gleixner wrote:
>>> On Wed, 21 Aug 2019, John Hubbard wrote:
>>>> On 8/21/19 10:05 AM, Neil MacLeod wrote:
>>>> static void sanitize_boot_params(struct boot_params *boot_params)
>>>> {
>>>> ...
>>>> const struct boot_params_to_save to_save[] = {
>>>> BOOT_PARAM_PRESERVE(screen_info),
>>>> BOOT_PARAM_PRESERVE(apm_bios_info),
>>>> BOOT_PARAM_PRESERVE(tboot_addr),
>>>> BOOT_PARAM_PRESERVE(ist_info),
>>>> BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>>>> BOOT_PARAM_PRESERVE(hd0_info),
>>>> BOOT_PARAM_PRESERVE(hd1_info),
>>>> BOOT_PARAM_PRESERVE(sys_desc_table),
>>>> BOOT_PARAM_PRESERVE(olpc_ofw_header),
>>>> BOOT_PARAM_PRESERVE(efi_info),
>>>> BOOT_PARAM_PRESERVE(alt_mem_k),
>>>> BOOT_PARAM_PRESERVE(scratch),
>>>> BOOT_PARAM_PRESERVE(e820_entries),
>>>> BOOT_PARAM_PRESERVE(eddbuf_entries),
>>>> BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
>>>> BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
>>>> BOOT_PARAM_PRESERVE(e820_table),
>>>> BOOT_PARAM_PRESERVE(eddbuf),
>>>> };
>>>
>>> I think I spotted it:
>>>
>>> - boot_params->acpi_rsdp_addr = 0;
>>>
>>> + BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>>>
>>> And it does not preserve 'hdr'
>>>
>>> Grr. I surely was too tired when staring at this last time.
>>>
>>
>> ohhh man, that's embarrassing. Especially hdr, which was the center of
>> the whole thing...sigh. Patch coming shortly.
>>
>>
>> thanks,
>> --
>> John Hubbard
>> NVIDIA
On 8/21/19 11:51 AM, Thomas Gleixner wrote:
> On Wed, 21 Aug 2019, John Hubbard wrote:
>> On 8/21/19 10:05 AM, Neil MacLeod wrote:
>> static void sanitize_boot_params(struct boot_params *boot_params)
>> {
>> ...
>> const struct boot_params_to_save to_save[] = {
>> BOOT_PARAM_PRESERVE(screen_info),
>> BOOT_PARAM_PRESERVE(apm_bios_info),
>> BOOT_PARAM_PRESERVE(tboot_addr),
>> BOOT_PARAM_PRESERVE(ist_info),
>> BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>> BOOT_PARAM_PRESERVE(hd0_info),
>> BOOT_PARAM_PRESERVE(hd1_info),
>> BOOT_PARAM_PRESERVE(sys_desc_table),
>> BOOT_PARAM_PRESERVE(olpc_ofw_header),
>> BOOT_PARAM_PRESERVE(efi_info),
>> BOOT_PARAM_PRESERVE(alt_mem_k),
>> BOOT_PARAM_PRESERVE(scratch),
>> BOOT_PARAM_PRESERVE(e820_entries),
>> BOOT_PARAM_PRESERVE(eddbuf_entries),
>> BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
>> BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
>> BOOT_PARAM_PRESERVE(e820_table),
>> BOOT_PARAM_PRESERVE(eddbuf),
>> };
>
> I think I spotted it:
>
> - boot_params->acpi_rsdp_addr = 0;
>
> + BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>
> And it does not preserve 'hdr'
>
> Grr. I surely was too tired when staring at this last time.
>
ohhh man, that's embarrassing. Especially hdr, which was the center of
the whole thing...sigh. Patch coming shortly.
thanks,
--
John Hubbard
NVIDIA
commit a90118c445cc ("x86/boot: Save fields explicitly, zero out
everything else") had two errors:
* It preserved boot_params.acpi_rsdp_addr, and
* It failed to preserve boot_params.hdr
Therefore, zero out acpi_rsdp_addr, and preserve hdr.
Fixes: a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")
Reported-by: Neil MacLeod <[email protected]>
Suggested-by: Thomas Gleixner <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: [email protected]
Signed-off-by: John Hubbard <[email protected]>
---
arch/x86/include/asm/bootparam_utils.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index f5e90a849bca..9e5f3c722c33 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -59,7 +59,6 @@ static void sanitize_boot_params(struct boot_params *boot_params)
BOOT_PARAM_PRESERVE(apm_bios_info),
BOOT_PARAM_PRESERVE(tboot_addr),
BOOT_PARAM_PRESERVE(ist_info),
- BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
BOOT_PARAM_PRESERVE(hd0_info),
BOOT_PARAM_PRESERVE(hd1_info),
BOOT_PARAM_PRESERVE(sys_desc_table),
@@ -71,6 +70,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
BOOT_PARAM_PRESERVE(eddbuf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+ BOOT_PARAM_PRESERVE(hdr),
BOOT_PARAM_PRESERVE(e820_table),
BOOT_PARAM_PRESERVE(eddbuf),
};
--
2.22.1
The fix looks good - many thanks for the quick turnaround!
Neil
On Wed, 21 Aug 2019 at 19:56, John Hubbard <[email protected]> wrote:
>
> On 8/21/19 11:51 AM, Thomas Gleixner wrote:
> > On Wed, 21 Aug 2019, John Hubbard wrote:
> >> On 8/21/19 10:05 AM, Neil MacLeod wrote:
> >> static void sanitize_boot_params(struct boot_params *boot_params)
> >> {
> >> ...
> >> const struct boot_params_to_save to_save[] = {
> >> BOOT_PARAM_PRESERVE(screen_info),
> >> BOOT_PARAM_PRESERVE(apm_bios_info),
> >> BOOT_PARAM_PRESERVE(tboot_addr),
> >> BOOT_PARAM_PRESERVE(ist_info),
> >> BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> >> BOOT_PARAM_PRESERVE(hd0_info),
> >> BOOT_PARAM_PRESERVE(hd1_info),
> >> BOOT_PARAM_PRESERVE(sys_desc_table),
> >> BOOT_PARAM_PRESERVE(olpc_ofw_header),
> >> BOOT_PARAM_PRESERVE(efi_info),
> >> BOOT_PARAM_PRESERVE(alt_mem_k),
> >> BOOT_PARAM_PRESERVE(scratch),
> >> BOOT_PARAM_PRESERVE(e820_entries),
> >> BOOT_PARAM_PRESERVE(eddbuf_entries),
> >> BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
> >> BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> >> BOOT_PARAM_PRESERVE(e820_table),
> >> BOOT_PARAM_PRESERVE(eddbuf),
> >> };
> >
> > I think I spotted it:
> >
> > - boot_params->acpi_rsdp_addr = 0;
> >
> > + BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> >
> > And it does not preserve 'hdr'
> >
> > Grr. I surely was too tired when staring at this last time.
> >
>
> ohhh man, that's embarrassing. Especially hdr, which was the center of
> the whole thing...sigh. Patch coming shortly.
>
>
> thanks,
> --
> John Hubbard
> NVIDIA
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 7846f58fba964af7cb8cf77d4d13c33254725211
Gitweb: https://git.kernel.org/tip/7846f58fba964af7cb8cf77d4d13c33254725211
Author: John Hubbard <[email protected]>
AuthorDate: Wed, 21 Aug 2019 12:25:13 -07:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 21 Aug 2019 22:37:09 +02:00
x86/boot: Fix boot regression caused by bootparam sanitizing
commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything
else") had two errors:
* It preserved boot_params.acpi_rsdp_addr, and
* It failed to preserve boot_params.hdr
Therefore, zero out acpi_rsdp_addr, and preserve hdr.
Fixes: a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")
Reported-by: Neil MacLeod <[email protected]>
Suggested-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Tested-by: Neil MacLeod <[email protected]>
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/bootparam_utils.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 9e5f3c7..f5e90a8 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -59,6 +59,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
BOOT_PARAM_PRESERVE(apm_bios_info),
BOOT_PARAM_PRESERVE(tboot_addr),
BOOT_PARAM_PRESERVE(ist_info),
+ BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
BOOT_PARAM_PRESERVE(hd0_info),
BOOT_PARAM_PRESERVE(hd1_info),
BOOT_PARAM_PRESERVE(sys_desc_table),
@@ -70,7 +71,6 @@ static void sanitize_boot_params(struct boot_params *boot_params)
BOOT_PARAM_PRESERVE(eddbuf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
- BOOT_PARAM_PRESERVE(hdr),
BOOT_PARAM_PRESERVE(e820_table),
BOOT_PARAM_PRESERVE(eddbuf),
};