Right, I've missed the dereference of the ibft_addr after
acpi_find_ibft_region().
With this change to iscsi_ibft_find.c instead of the previous one it should
be better:
@@ -80,6 +80,27 @@ static int __init find_ibft_in_mem(void)done:returnlen;}++staticvoid__initacpi_find_ibft_region(unsignedlong*sizep)+{+inti;+structacpi_table_header*table=NULL;+acpi_statusstatus;++if(acpi_disabled)+return;++for(i=0;i<ARRAY_SIZE(ibft_signs)&&!ibft_addr;i++){+status=acpi_get_table(ibft_signs[i].sign,0,&table);+if(ACPI_SUCCESS(status)){+ibft_addr=(structacpi_table_ibft*)table;+*sizep=PAGE_ALIGN(ibft_addr->header.length);+acpi_put_table(table);+break;+}+}+}+/**RoutineusedtofindtheiSCSIBootFormatTable.Thelogical*kerneladdressissetintheibft_addrglobalvariable.
@@ -91,14 +112,16 @@ unsigned long __init find_ibft_region(unsigned long *sizep)/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will*onlyuseACPIforthis*/-if(!efi_enabled(EFI_BOOT))+if(!efi_enabled(EFI_BOOT)){find_ibft_in_mem();--if(ibft_addr){*sizep=PAGE_ALIGN(ibft_addr->header.length);-return(u64)virt_to_phys(ibft_addr);+}else{+acpi_find_ibft_region(sizep);}+if(ibft_addr)+return(u64)virt_to_phys(ibft_addr);+*sizep=0;return0;}
Mike,
No luck. Back to the original KASAN ibft_init crash.
I ran with only the above patch from you. Was that what you wanted? Your
previous patch had a section defined out by #if 0. Was that supposed to be
in there as well?
Sorry, I wasn't clear, but I meant to use the first patch and only replace
changes to iscsi_ibft_find.c with the new patch.
Here's the full patch to be sure we're on the same page:
From: Mike Rapoport <hidden> Date: 2021-02-24 10:39:16
On Tue, Feb 23, 2021 at 04:46:28PM -0500, George Kennedy wrote:
Mike,
Still no luck.
[ 30.193723] iscsi: registered transport (iser)
[ 30.195970] iBFT detected.
[ 30.196571] BUG: unable to handle page fault for address: ffffffffff240004
Hmm, we cannot set ibft_addr to early pointer to the ACPI table.
Let's try something more disruptive and move the reservation back to
iscsi_ibft_find.c.
@@ -70,35 +88,44 @@ static int __init find_ibft_in_mem(void)/* if the length of the table extends past 1M,*thetablecannotbevalid.*/if(pos+len<=(IBFT_END-1)){-ibft_addr=(structacpi_table_ibft*)virt;pr_info("iBFT found at 0x%lx.\n",pos);-gotodone;+returnvirt;}}}}-done:-returnlen;++returnNULL;}++staticvoid__init*find_ibft(void)+{+/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will+*onlyuseACPIforthis*/+if(!efi_enabled(EFI_BOOT))+returnfind_ibft_in_mem();+else+returnacpi_find_ibft_region();+}+/**RoutineusedtofindtheiSCSIBootFormatTable.Thelogical*kerneladdressissetintheibft_addrglobalvariable.*/-unsignedlong__initfind_ibft_region(unsignedlong*sizep)+void__initreserve_ibft_region(void){-ibft_addr=NULL;+structacpi_table_ibft*table;+unsignedlongsize;-/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will-*onlyuseACPIforthis*/+table=find_ibft();+if(!table)+return;-if(!efi_enabled(EFI_BOOT))-find_ibft_in_mem();--if(ibft_addr){-*sizep=PAGE_ALIGN(ibft_addr->header.length);-return(u64)virt_to_phys(ibft_addr);-}+size=PAGE_ALIGN(table->header.length);+memblock_reserve(virt_to_phys(table),size);-*sizep=0;-return0;+if(efi_enabled(EFI_BOOT))+acpi_put_table(&table->header);+else+ibft_addr=table;}
From: George Kennedy <hidden> Date: 2021-02-24 15:15:15
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
quoted hunk
On Tue, Feb 23, 2021 at 04:46:28PM -0500, George Kennedy wrote:
quoted
Mike,
Still no luck.
[ 30.193723] iscsi: registered transport (iser)
[ 30.195970] iBFT detected.
[ 30.196571] BUG: unable to handle page fault for address: ffffffffff240004
Hmm, we cannot set ibft_addr to early pointer to the ACPI table.
Let's try something more disruptive and move the reservation back to
iscsi_ibft_find.c.
@@ -70,35 +88,44 @@ static int __init find_ibft_in_mem(void)/* if the length of the table extends past 1M,*thetablecannotbevalid.*/if(pos+len<=(IBFT_END-1)){-ibft_addr=(structacpi_table_ibft*)virt;pr_info("iBFT found at 0x%lx.\n",pos);-gotodone;+returnvirt;}}}}-done:-returnlen;++returnNULL;}++staticvoid__init*find_ibft(void)+{+/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will+*onlyuseACPIforthis*/+if(!efi_enabled(EFI_BOOT))+returnfind_ibft_in_mem();+else+returnacpi_find_ibft_region();+}+/**RoutineusedtofindtheiSCSIBootFormatTable.Thelogical*kerneladdressissetintheibft_addrglobalvariable.*/-unsignedlong__initfind_ibft_region(unsignedlong*sizep)+void__initreserve_ibft_region(void){-ibft_addr=NULL;+structacpi_table_ibft*table;+unsignedlongsize;-/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will-*onlyuseACPIforthis*/+table=find_ibft();+if(!table)+return;-if(!efi_enabled(EFI_BOOT))-find_ibft_in_mem();--if(ibft_addr){-*sizep=PAGE_ALIGN(ibft_addr->header.length);-return(u64)virt_to_phys(ibft_addr);-}+size=PAGE_ALIGN(table->header.length);+memblock_reserve(virt_to_phys(table),size);-*sizep=0;-return0;+if(efi_enabled(EFI_BOOT))+acpi_put_table(&table->header);+else+ibft_addr=table;}
Still no luck Mike,
We're back to the original problem where the only thing that worked was
to run "SetPageReserved(page)" before calling "kmap(page)". The page is
being "freed" before ibft_init() is called as a result of the recent
buddy page freeing changes.
[ 30.385207] iscsi: registered transport (iser)
[ 30.387462] iBFT detected.
[ 30.388042]
==================================================================
[ 30.388119] BUG: KASAN: use-after-free in ibft_init+0x134/0xc33
[ 30.388119] Read of size 4 at addr ffff8880be453004 by task swapper/0/1
[ 30.388119]
[ 30.388119] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.11.0-f9593a0 #11
[ 30.388119] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 0.0.0 02/06/2015
[ 30.388119] Call Trace:
[ 30.388119] dump_stack+0xdb/0x120
[ 30.388119] ? ibft_init+0x134/0xc33
[ 30.388119] print_address_description.constprop.7+0x41/0x60
[ 30.388119] ? ibft_init+0x134/0xc33
[ 30.388119] ? ibft_init+0x134/0xc33
[ 30.388119] kasan_report.cold.10+0x78/0xd1
[ 30.388119] ? ibft_init+0x134/0xc33
[ 30.388119] __asan_report_load_n_noabort+0xf/0x20
[ 30.388119] ibft_init+0x134/0xc33
[ 30.388119] ? write_comp_data+0x2f/0x90
[ 30.388119] ? ibft_check_initiator_for+0x159/0x159
[ 30.388119] ? write_comp_data+0x2f/0x90
[ 30.388119] ? ibft_check_initiator_for+0x159/0x159
[ 30.388119] do_one_initcall+0xc4/0x3e0
[ 30.388119] ? perf_trace_initcall_level+0x3e0/0x3e0
[ 30.388119] ? unpoison_range+0x14/0x40
[ 30.388119] ? ____kasan_kmalloc.constprop.5+0x8f/0xc0
[ 30.388119] ? kernel_init_freeable+0x420/0x652
George
From: Mike Rapoport <hidden> Date: 2021-02-25 08:54:34
Hi George,
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
quoted
On Tue, Feb 23, 2021 at 04:46:28PM -0500, George Kennedy wrote:
quoted
Mike,
Still no luck.
[ 30.193723] iscsi: registered transport (iser)
[ 30.195970] iBFT detected.
[ 30.196571] BUG: unable to handle page fault for address: ffffffffff240004
Hmm, we cannot set ibft_addr to early pointer to the ACPI table.
Let's try something more disruptive and move the reservation back to
iscsi_ibft_find.c.
@@ -70,35 +88,44 @@ static int __init find_ibft_in_mem(void)/* if the length of the table extends past 1M,*thetablecannotbevalid.*/if(pos+len<=(IBFT_END-1)){-ibft_addr=(structacpi_table_ibft*)virt;pr_info("iBFT found at 0x%lx.\n",pos);-gotodone;+returnvirt;}}}}-done:-returnlen;++returnNULL;}++staticvoid__init*find_ibft(void)+{+/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will+*onlyuseACPIforthis*/+if(!efi_enabled(EFI_BOOT))+returnfind_ibft_in_mem();+else+returnacpi_find_ibft_region();+}+/**RoutineusedtofindtheiSCSIBootFormatTable.Thelogical*kerneladdressissetintheibft_addrglobalvariable.*/-unsignedlong__initfind_ibft_region(unsignedlong*sizep)+void__initreserve_ibft_region(void){-ibft_addr=NULL;+structacpi_table_ibft*table;+unsignedlongsize;-/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will-*onlyuseACPIforthis*/+table=find_ibft();+if(!table)+return;-if(!efi_enabled(EFI_BOOT))-find_ibft_in_mem();--if(ibft_addr){-*sizep=PAGE_ALIGN(ibft_addr->header.length);-return(u64)virt_to_phys(ibft_addr);-}+size=PAGE_ALIGN(table->header.length);+memblock_reserve(virt_to_phys(table),size);-*sizep=0;-return0;+if(efi_enabled(EFI_BOOT))+acpi_put_table(&table->header);+else+ibft_addr=table;}
Still no luck Mike,
We're back to the original problem where the only thing that worked was to
run "SetPageReserved(page)" before calling "kmap(page)". The page is being
"freed" before ibft_init() is called as a result of the recent buddy page
freeing changes.
I keep missing some little details each time :(
Ok, let's try from the different angle.
From: George Kennedy <hidden> Date: 2021-02-25 12:40:24
On 2/25/2021 3:53 AM, Mike Rapoport wrote:
Hi George,
quoted
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
quoted
On Tue, Feb 23, 2021 at 04:46:28PM -0500, George Kennedy wrote:
quoted
Mike,
Still no luck.
[ 30.193723] iscsi: registered transport (iser)
[ 30.195970] iBFT detected.
[ 30.196571] BUG: unable to handle page fault for address: ffffffffff240004
Hmm, we cannot set ibft_addr to early pointer to the ACPI table.
Let's try something more disruptive and move the reservation back to
iscsi_ibft_find.c.
@@ -70,35 +88,44 @@ static int __init find_ibft_in_mem(void)/* if the length of the table extends past 1M,*thetablecannotbevalid.*/if(pos+len<=(IBFT_END-1)){-ibft_addr=(structacpi_table_ibft*)virt;pr_info("iBFT found at 0x%lx.\n",pos);-gotodone;+returnvirt;}}}}-done:-returnlen;++returnNULL;}++staticvoid__init*find_ibft(void)+{+/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will+*onlyuseACPIforthis*/+if(!efi_enabled(EFI_BOOT))+returnfind_ibft_in_mem();+else+returnacpi_find_ibft_region();+}+/**RoutineusedtofindtheiSCSIBootFormatTable.Thelogical*kerneladdressissetintheibft_addrglobalvariable.*/-unsignedlong__initfind_ibft_region(unsignedlong*sizep)+void__initreserve_ibft_region(void){-ibft_addr=NULL;+structacpi_table_ibft*table;+unsignedlongsize;-/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will-*onlyuseACPIforthis*/+table=find_ibft();+if(!table)+return;-if(!efi_enabled(EFI_BOOT))-find_ibft_in_mem();--if(ibft_addr){-*sizep=PAGE_ALIGN(ibft_addr->header.length);-return(u64)virt_to_phys(ibft_addr);-}+size=PAGE_ALIGN(table->header.length);+memblock_reserve(virt_to_phys(table),size);-*sizep=0;-return0;+if(efi_enabled(EFI_BOOT))+acpi_put_table(&table->header);+else+ibft_addr=table;}
Still no luck Mike,
We're back to the original problem where the only thing that worked was to
run "SetPageReserved(page)" before calling "kmap(page)". The page is being
"freed" before ibft_init() is called as a result of the recent buddy page
freeing changes.
I keep missing some little details each time :(
No worries. Thanks for all your help. Does this patch go on top of your
previous patch or is it standalone?
George
From: Mike Rapoport <hidden> Date: 2021-02-25 14:58:48
On Thu, Feb 25, 2021 at 07:38:19AM -0500, George Kennedy wrote:
On 2/25/2021 3:53 AM, Mike Rapoport wrote:
quoted
Hi George,
quoted
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
quoted
On Tue, Feb 23, 2021 at 04:46:28PM -0500, George Kennedy wrote:
quoted
Mike,
Still no luck.
[ 30.193723] iscsi: registered transport (iser)
[ 30.195970] iBFT detected.
[ 30.196571] BUG: unable to handle page fault for address: ffffffffff240004
Hmm, we cannot set ibft_addr to early pointer to the ACPI table.
Let's try something more disruptive and move the reservation back to
iscsi_ibft_find.c.
@@ -70,35 +88,44 @@ static int __init find_ibft_in_mem(void)/* if the length of the table extends past 1M,*thetablecannotbevalid.*/if(pos+len<=(IBFT_END-1)){-ibft_addr=(structacpi_table_ibft*)virt;pr_info("iBFT found at 0x%lx.\n",pos);-gotodone;+returnvirt;}}}}-done:-returnlen;++returnNULL;}++staticvoid__init*find_ibft(void)+{+/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will+*onlyuseACPIforthis*/+if(!efi_enabled(EFI_BOOT))+returnfind_ibft_in_mem();+else+returnacpi_find_ibft_region();+}+/**RoutineusedtofindtheiSCSIBootFormatTable.Thelogical*kerneladdressissetintheibft_addrglobalvariable.*/-unsignedlong__initfind_ibft_region(unsignedlong*sizep)+void__initreserve_ibft_region(void){-ibft_addr=NULL;+structacpi_table_ibft*table;+unsignedlongsize;-/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will-*onlyuseACPIforthis*/+table=find_ibft();+if(!table)+return;-if(!efi_enabled(EFI_BOOT))-find_ibft_in_mem();--if(ibft_addr){-*sizep=PAGE_ALIGN(ibft_addr->header.length);-return(u64)virt_to_phys(ibft_addr);-}+size=PAGE_ALIGN(table->header.length);+memblock_reserve(virt_to_phys(table),size);-*sizep=0;-return0;+if(efi_enabled(EFI_BOOT))+acpi_put_table(&table->header);+else+ibft_addr=table;}
Still no luck Mike,
We're back to the original problem where the only thing that worked was to
run "SetPageReserved(page)" before calling "kmap(page)". The page is being
"freed" before ibft_init() is called as a result of the recent buddy page
freeing changes.
I keep missing some little details each time :(
No worries. Thanks for all your help. Does this patch go on top of your
previous patch or is it standalone?
From: George Kennedy <hidden> Date: 2021-02-25 15:27:47
On 2/25/2021 9:57 AM, Mike Rapoport wrote:
On Thu, Feb 25, 2021 at 07:38:19AM -0500, George Kennedy wrote:
quoted
On 2/25/2021 3:53 AM, Mike Rapoport wrote:
quoted
Hi George,
quoted
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
quoted
On Tue, Feb 23, 2021 at 04:46:28PM -0500, George Kennedy wrote:
quoted
Mike,
Still no luck.
[ 30.193723] iscsi: registered transport (iser)
[ 30.195970] iBFT detected.
[ 30.196571] BUG: unable to handle page fault for address: ffffffffff240004
Hmm, we cannot set ibft_addr to early pointer to the ACPI table.
Let's try something more disruptive and move the reservation back to
iscsi_ibft_find.c.
@@ -70,35 +88,44 @@ static int __init find_ibft_in_mem(void)/* if the length of the table extends past 1M,*thetablecannotbevalid.*/if(pos+len<=(IBFT_END-1)){-ibft_addr=(structacpi_table_ibft*)virt;pr_info("iBFT found at 0x%lx.\n",pos);-gotodone;+returnvirt;}}}}-done:-returnlen;++returnNULL;}++staticvoid__init*find_ibft(void)+{+/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will+*onlyuseACPIforthis*/+if(!efi_enabled(EFI_BOOT))+returnfind_ibft_in_mem();+else+returnacpi_find_ibft_region();+}+/**RoutineusedtofindtheiSCSIBootFormatTable.Thelogical*kerneladdressissetintheibft_addrglobalvariable.*/-unsignedlong__initfind_ibft_region(unsignedlong*sizep)+void__initreserve_ibft_region(void){-ibft_addr=NULL;+structacpi_table_ibft*table;+unsignedlongsize;-/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will-*onlyuseACPIforthis*/+table=find_ibft();+if(!table)+return;-if(!efi_enabled(EFI_BOOT))-find_ibft_in_mem();--if(ibft_addr){-*sizep=PAGE_ALIGN(ibft_addr->header.length);-return(u64)virt_to_phys(ibft_addr);-}+size=PAGE_ALIGN(table->header.length);+memblock_reserve(virt_to_phys(table),size);-*sizep=0;-return0;+if(efi_enabled(EFI_BOOT))+acpi_put_table(&table->header);+else+ibft_addr=table;}
Still no luck Mike,
We're back to the original problem where the only thing that worked was to
run "SetPageReserved(page)" before calling "kmap(page)". The page is being
"freed" before ibft_init() is called as a result of the recent buddy page
freeing changes.
I keep missing some little details each time :(
No worries. Thanks for all your help. Does this patch go on top of your
previous patch or is it standalone?
From: George Kennedy <hidden> Date: 2021-02-25 16:08:21
On 2/25/2021 10:22 AM, George Kennedy wrote:
On 2/25/2021 9:57 AM, Mike Rapoport wrote:
quoted
On Thu, Feb 25, 2021 at 07:38:19AM -0500, George Kennedy wrote:
quoted
On 2/25/2021 3:53 AM, Mike Rapoport wrote:
quoted
Hi George,
quoted
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
quoted
On Tue, Feb 23, 2021 at 04:46:28PM -0500, George Kennedy wrote:
quoted
Mike,
Still no luck.
[ 30.193723] iscsi: registered transport (iser)
[ 30.195970] iBFT detected.
[ 30.196571] BUG: unable to handle page fault for address:
ffffffffff240004
Hmm, we cannot set ibft_addr to early pointer to the ACPI table.
Let's try something more disruptive and move the reservation back to
iscsi_ibft_find.c.
diff --git a/arch/x86/kernel/acpi/boot.c
b/arch/x86/kernel/acpi/boot.c
index 7bdc0239a943..c118dd54a747 100644
* mapped address is set in the ibft_addr variable.
*/
#ifdef CONFIG_ISCSI_IBFT_FIND
-unsigned long find_ibft_region(unsigned long *sizep);
+void reserve_ibft_region(void);
#else
-static inline unsigned long find_ibft_region(unsigned long *sizep)
-{
- *sizep = 0;
- return 0;
-}
+static inline void reserve_ibft_region(void) {}
#endif
#endif /* ISCSI_IBFT_H */
Still no luck Mike,
We're back to the original problem where the only thing that
worked was to
run "SetPageReserved(page)" before calling "kmap(page)". The page
is being
"freed" before ibft_init() is called as a result of the recent
buddy page
freeing changes.
I keep missing some little details each time :(
No worries. Thanks for all your help. Does this patch go on top of your
previous patch or is it standalone?
This is standalone.
quoted
George
quoted
Ok, let's try from the different angle.
diff --git a/drivers/acpi/acpica/tbutils.c
b/drivers/acpi/acpica/tbutils.c
index 4b9b329a5a92..ec43e1447336 100644
Applied just your latest patch, but same failure.
I thought there was an earlier comment (which I can't find now) that
stated that memblock_reserve() wouldn't reserve the page, which is
what's needed here.
Mike,
Here was David's explanation of what he thinks is going on (or should be
going on) from a few days ago:
QUOTE...
I assume that acpi_map()/acpi_unmap() map some firmware blob that is
provided via firmware/bios/... to us.
should_use_kmap() tells us whether
a) we have a "struct page" and should kmap() that one
b) we don't have a "struct page" and should ioremap.
As it is a blob, the firmware should always reserve that memory region
via memblock (e.g., memblock_reserve()), such that we either
1) don't create a memmap ("struct page") at all (-> case b) )
2) if we have to create e memmap, we mark the page PG_reserved and
*never* expose it to the buddy (-> case a) )
Are you telling me that in this case we might have a memmap for the HW
blob that is *not* PG_reserved? In that case it most probably got
exposed to the buddy where it can happily get allocated/freed.
The latent BUG would be that that blob gets exposed to the system like
ordinary RAM, and not reserved via memblock early during boot. Assuming
that blob has a low physical address, with my patch it will get
allocated/used a lot earlier - which would mean we trigger this latent
BUG now more easily.
...END_QUOTE
Your most recent patch has added the memblock_reserve(), but it's still
missing the PG_reserved setting.
Thanks,
George
From: Mike Rapoport <hidden> Date: 2021-02-25 16:10:16
On Thu, Feb 25, 2021 at 10:22:44AM -0500, George Kennedy wrote:
quoted
quoted
quoted
quoted
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
Applied just your latest patch, but same failure.
I thought there was an earlier comment (which I can't find now) that stated
that memblock_reserve() wouldn't reserve the page, which is what's needed
here.
Actually, I think that memblock_reserve() should be just fine, but it seems
I'm missing something in address calculation each time.
What would happen if you stuck
memblock_reserve(0xbe453000, PAGE_SIZE);
say, at the beginning of find_ibft_region()?
From: George Kennedy <hidden> Date: 2021-02-25 16:32:39
On 2/25/2021 11:07 AM, Mike Rapoport wrote:
On Thu, Feb 25, 2021 at 10:22:44AM -0500, George Kennedy wrote:
quoted
quoted
quoted
quoted
quoted
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
Applied just your latest patch, but same failure.
I thought there was an earlier comment (which I can't find now) that stated
that memblock_reserve() wouldn't reserve the page, which is what's needed
here.
Actually, I think that memblock_reserve() should be just fine, but it seems
I'm missing something in address calculation each time.
What would happen if you stuck
memblock_reserve(0xbe453000, PAGE_SIZE);
say, at the beginning of find_ibft_region()?
Added debug to your patch and this is all that shows up. Looks like the
patch is in the wrong place as acpi_tb_parse_root_table() is only called
for the RSDP address.
[ 0.064317] ACPI: Early table checksum verification disabled
[ 0.065437] XXX acpi_tb_parse_root_table: rsdp_address=bfbfa014
[ 0.066612] ACPI: RSDP 0x00000000BFBFA014 000024 (v02 BOCHS )
[ 0.067759] ACPI: XSDT 0x00000000BFBF90E8 00004C (v01 BOCHS BXPCFACP
00000001 01000013)
[ 0.069470] ACPI: FACP 0x00000000BFBF5000 000074 (v01 BOCHS BXPCFACP
00000001 BXPC 00000001)
[ 0.071183] ACPI: DSDT 0x00000000BFBF6000 00238D (v01 BOCHS BXPCDSDT
00000001 BXPC 00000001)
[ 0.072876] ACPI: FACS 0x00000000BFBFD000 000040
[ 0.073806] ACPI: APIC 0x00000000BFBF4000 000090 (v01 BOCHS BXPCAPIC
00000001 BXPC 00000001)
[ 0.075501] ACPI: HPET 0x00000000BFBF3000 000038 (v01 BOCHS BXPCHPET
00000001 BXPC 00000001)
[ 0.077194] ACPI: BGRT 0x00000000BE49B000 000038 (v01 INTEL EDK2
00000002 01000013)
[ 0.078880] ACPI: iBFT 0x00000000BE453000 000800 (v01 BOCHS BXPCFACP
00000000 00000000)
[ 0.080588] ACPI: Local APIC address 0xfee00000
table_index)
acpi_status status;
u32 table_index;
+printk(KERN_ERR "XXX acpi_tb_parse_root_table: rsdp_address=%llx\n",
rsdp_address);
+
ACPI_FUNCTION_TRACE(tb_parse_root_table);
/* Map the entire RSDP and extract the address of the RSDT or XSDT */
Can you explore the relevant area using the page-flags tools (located in
Linux src code located in tools/vm/page-flags.c)
./page-types -L -r -a 0xbe490,0xbe4a0
--
Thanks,
David / dhildenb
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: George Kennedy <hidden> Date: 2021-02-25 17:35:43
On 2/25/2021 11:07 AM, Mike Rapoport wrote:
On Thu, Feb 25, 2021 at 10:22:44AM -0500, George Kennedy wrote:
quoted
quoted
quoted
quoted
quoted
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
Applied just your latest patch, but same failure.
I thought there was an earlier comment (which I can't find now) that stated
that memblock_reserve() wouldn't reserve the page, which is what's needed
here.
Actually, I think that memblock_reserve() should be just fine, but it seems
I'm missing something in address calculation each time.
What would happen if you stuck
memblock_reserve(0xbe453000, PAGE_SIZE);
say, at the beginning of find_ibft_region()?
Good news Mike!
The above hack in yesterday's last patch works - 10 successful reboots.
See: "BE453" below for the hack.
I'll modify the patch to use "table_desc->address" instead, which is the
physical address of the table.
@@ -70,35 +88,52 @@ static int __init find_ibft_in_mem(void)/* if the length of the table extends past 1M,*thetablecannotbevalid.*/if(pos+len<=(IBFT_END-1)){-ibft_addr=(structacpi_table_ibft*)virt;pr_info("iBFT found at 0x%lx.\n",pos);-gotodone;+returnvirt;}}}}-done:-returnlen;++returnNULL;}++staticvoid__init*find_ibft(void)+{+/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will+*onlyuseACPIforthis*/+if(!efi_enabled(EFI_BOOT))+returnfind_ibft_in_mem();+else+returnacpi_find_ibft_region();+}+/**RoutineusedtofindtheiSCSIBootFormatTable.Thelogical*kerneladdressissetintheibft_addrglobalvariable.*/-unsignedlong__initfind_ibft_region(unsignedlong*sizep)+void__initreserve_ibft_region(void){-ibft_addr=NULL;+structacpi_table_ibft*table;+unsignedlongsize;-/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will-*onlyuseACPIforthis*/+table=find_ibft();+if(!table)+return;-if(!efi_enabled(EFI_BOOT))-find_ibft_in_mem();--if(ibft_addr){-*sizep=PAGE_ALIGN(ibft_addr->header.length);-return(u64)virt_to_phys(ibft_addr);-}+size=PAGE_ALIGN(table->header.length);+#if 0+printk(KERN_ERR"XXX reserve_ibft_region: table=%llx,
Can you explore the relevant area using the page-flags tools (located in
Linux src code located in tools/vm/page-flags.c)
./page-types -L -r -a 0xbe490,0xbe4a0
These are not iBFT and they are "ACPI data", so we should have them as
PG_Reserved set at init_unavailable_mem().
[ 0.000000] BIOS-e820: [mem 0x0000000000808000-0x000000000080ffff] usable
[ 0.000000] BIOS-e820: [mem 0x0000000000810000-0x00000000008fffff] ACPI NVS
[ 0.000000] BIOS-e820: [mem 0x0000000000900000-0x00000000be49afff] usable
^ iBFT@0xbe453 lives here ^
And it should be a normal page, as it's in "usable" memory and nothing
reserves it at boot, so no reason it won't be freed to buddy.
If iBFT was in the low memory (<1M) it would have been reserved by
reserve_ibft_region(), but with ACPI any block not marked by BIOS as "ACPI
something" is treated like a normal memory and there is nothing that
reserves it.
So we do need to memblock_reserve() iBFT region, but I still couldn't find
the right place to properly get its address without duplicating ACPI tables
parsing :(
[ 0.000000] BIOS-e820: [mem 0x00000000be49b000-0x00000000be49bfff] ACPI data
--
Sincerely yours,
Mike.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Mike Rapoport <hidden> Date: 2021-02-25 17:55:19
On Thu, Feb 25, 2021 at 11:31:04AM -0500, George Kennedy wrote:
On 2/25/2021 11:07 AM, Mike Rapoport wrote:
quoted
On Thu, Feb 25, 2021 at 10:22:44AM -0500, George Kennedy wrote:
quoted
quoted
quoted
quoted
quoted
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
Applied just your latest patch, but same failure.
I thought there was an earlier comment (which I can't find now) that stated
that memblock_reserve() wouldn't reserve the page, which is what's needed
here.
Actually, I think that memblock_reserve() should be just fine, but it seems
I'm missing something in address calculation each time.
What would happen if you stuck
memblock_reserve(0xbe453000, PAGE_SIZE);
say, at the beginning of find_ibft_region()?
Added debug to your patch and this is all that shows up. Looks like the
patch is in the wrong place as acpi_tb_parse_root_table() is only called for
the RSDP address.
Right, but I think it parses table description of the other tables and
populates local tables with them.
I think the problem is with how I compare the signatures, please see below
table_index)
acpi_status status;
u32 table_index;
+printk(KERN_ERR "XXX acpi_tb_parse_root_table: rsdp_address=%llx\n",
rsdp_address);
+
ACPI_FUNCTION_TRACE(tb_parse_root_table);
/* Map the entire RSDP and extract the address of the RSDT or XSDT */
We have:
include/acpi/actbl1.h:#define ACPI_SIG_IBFT "IBFT" /* iSCSI Boot Firmware Table */
and the BIOS uses "iBFT", so we need to loop over possible signature
variants like iscsi_ibft_find does.
Do you mind replacing ACPI_SIG_IBFT with "iBFT" and try again?
From: George Kennedy <hidden> Date: 2021-02-26 01:22:34
On 2/25/2021 12:33 PM, George Kennedy wrote:
quoted hunk
On 2/25/2021 11:07 AM, Mike Rapoport wrote:
quoted
On Thu, Feb 25, 2021 at 10:22:44AM -0500, George Kennedy wrote:
quoted
quoted
quoted
quoted
quoted
On 2/24/2021 5:37 AM, Mike Rapoport wrote:
Applied just your latest patch, but same failure.
I thought there was an earlier comment (which I can't find now) that
stated
that memblock_reserve() wouldn't reserve the page, which is what's
needed
here.
Actually, I think that memblock_reserve() should be just fine, but it
seems
I'm missing something in address calculation each time.
What would happen if you stuck
memblock_reserve(0xbe453000, PAGE_SIZE);
say, at the beginning of find_ibft_region()?
Good news Mike!
The above hack in yesterday's last patch works - 10 successful
reboots. See: "BE453" below for the hack.
I'll modify the patch to use "table_desc->address" instead, which is
the physical address of the table.
* mapped address is set in the ibft_addr variable.
*/
#ifdef CONFIG_ISCSI_IBFT_FIND
-unsigned long find_ibft_region(unsigned long *sizep);
+void reserve_ibft_region(void);
#else
-static inline unsigned long find_ibft_region(unsigned long *sizep)
-{
- *sizep = 0;
- return 0;
-}
+static inline void reserve_ibft_region(void) {}
#endif
#endif /* ISCSI_IBFT_H */
Mike,
To get rid of the 0x00000000BE453000 hardcoding, I added the following
patch to your above patch to get the iBFT table "address" to use with
memblock_reserve():
From: Mike Rapoport <hidden> Date: 2021-02-26 11:18:55
Hi George,
On Thu, Feb 25, 2021 at 08:19:18PM -0500, George Kennedy wrote:
quoted hunk
Mike,
To get rid of the 0x00000000BE453000 hardcoding, I added the following patch
to your above patch to get the iBFT table "address" to use with
memblock_reserve():
{
struct acpi_table_ibft *table;
unsigned long size;
+ acpi_physical_address address;
table = find_ibft();
if (!table)
return;
size = PAGE_ALIGN(table->header.length);
+ address = acpi_tb_find_table_address(table->header.signature);
#if 0
printk(KERN_ERR "XXX reserve_ibft_region: table=%llx,
virt_to_phys(table)=%llx, size=%lx\n",
(u64)table, virt_to_phys(table), size);
memblock_reserve(virt_to_phys(table), size);
#else
-printk(KERN_ERR "XXX reserve_ibft_region: table=%llx, 0x00000000BE453000,
size=%lx\n",
- (u64)table, size);
- memblock_reserve(0x00000000BE453000, size);
+printk(KERN_ERR "XXX reserve_ibft_region: table=%llx, address=%llx,
size=%lx\n",
+ (u64)table, address, size);
+ if (address)
+ memblock_reserve(address, size);
+ else
+ printk(KERN_ERR "%s: Can't find table address\n", __func__);
#endif
- if (efi_enabled(EFI_BOOT))
+ if (efi_enabled(EFI_BOOT)) {
+printk(KERN_ERR "XXX reserve_ibft_region: calling acpi_put_table(%llx)\n",
(u64)&table->header);
acpi_put_table(&table->header);
- else
+ } else {
ibft_addr = table;
+printk(KERN_ERR "XXX reserve_ibft_region: ibft_addr=%llx\n",
(u64)ibft_addr);
+ }
}
Debug from the above:
[ 0.050646] ACPI: Early table checksum verification disabled
[ 0.051778] ACPI: RSDP 0x00000000BFBFA014 000024 (v02 BOCHS )
[ 0.052922] ACPI: XSDT 0x00000000BFBF90E8 00004C (v01 BOCHS BXPCFACP
00000001 01000013)
[ 0.054623] ACPI: FACP 0x00000000BFBF5000 000074 (v01 BOCHS BXPCFACP
00000001 BXPC 00000001)
[ 0.056326] ACPI: DSDT 0x00000000BFBF6000 00238D (v01 BOCHS BXPCDSDT
00000001 BXPC 00000001)
[ 0.058016] ACPI: FACS 0x00000000BFBFD000 000040
[ 0.058940] ACPI: APIC 0x00000000BFBF4000 000090 (v01 BOCHS BXPCAPIC
00000001 BXPC 00000001)
[ 0.060627] ACPI: HPET 0x00000000BFBF3000 000038 (v01 BOCHS BXPCHPET
00000001 BXPC 00000001)
[ 0.062304] ACPI: BGRT 0x00000000BE49B000 000038 (v01 INTEL EDK2
00000002 01000013)
[ 0.063987] ACPI: iBFT 0x00000000BE453000 000800 (v01 BOCHS BXPCFACP
00000000 00000000)
[ 0.065683] XXX acpi_tb_find_table_address: signature=iBFT
[ 0.066754] XXX acpi_tb_find_table_address(EXIT): address=be453000
[ 0.067959] XXX reserve_ibft_region: table=ffffffffff240000,
address=be453000, size=1000
[ 0.069534] XXX reserve_ibft_region: calling
acpi_put_table(ffffffffff240000)
Not sure if it's the right thing to do, but added
"acpi_tb_find_table_address()" to return the physical address of a table to
use with memblock_reserve().
virt_to_phys(table) does not seem to return the physical address for the
iBFT table (it would be nice if struct acpi_table_header also had a
"address" element for the physical address of the table).
virt_to_phys() does not work that early because then it is mapped with
early_memremap() which uses different virtual to physical scheme.
I'd say that acpi_tb_find_table_address() makes sense if we'd like to
reserve ACPI tables outside of drivers/acpi.
But probably we should simply reserve all the tables during
acpi_table_init() so that any table that firmware put in the normal memory
will be surely reserved.
Ran 10 successful boots with the above without failure.
From: George Kennedy <hidden> Date: 2021-02-26 16:18:25
Hi Mike,
On 2/26/2021 6:17 AM, Mike Rapoport wrote:
Hi George,
On Thu, Feb 25, 2021 at 08:19:18PM -0500, George Kennedy wrote:
quoted
Mike,
To get rid of the 0x00000000BE453000 hardcoding, I added the following patch
to your above patch to get the iBFT table "address" to use with
memblock_reserve():
virt_to_phys(table)=%llx, size=%lx\n",
(u64)table, virt_to_phys(table), size);
memblock_reserve(virt_to_phys(table), size);
#else
-printk(KERN_ERR "XXX reserve_ibft_region: table=%llx, 0x00000000BE453000,
size=%lx\n",
- (u64)table, size);
- memblock_reserve(0x00000000BE453000, size);
+printk(KERN_ERR "XXX reserve_ibft_region: table=%llx, address=%llx,
size=%lx\n",
+ (u64)table, address, size);
+ if (address)
+ memblock_reserve(address, size);
+ else
+ printk(KERN_ERR "%s: Can't find table address\n", __func__);
#endif
- if (efi_enabled(EFI_BOOT))
+ if (efi_enabled(EFI_BOOT)) {
+printk(KERN_ERR "XXX reserve_ibft_region: calling acpi_put_table(%llx)\n",
(u64)&table->header);
acpi_put_table(&table->header);
- else
+ } else {
ibft_addr = table;
+printk(KERN_ERR "XXX reserve_ibft_region: ibft_addr=%llx\n",
(u64)ibft_addr);
+ }
}
Debug from the above:
[ 0.050646] ACPI: Early table checksum verification disabled
[ 0.051778] ACPI: RSDP 0x00000000BFBFA014 000024 (v02 BOCHS )
[ 0.052922] ACPI: XSDT 0x00000000BFBF90E8 00004C (v01 BOCHS BXPCFACP
00000001 01000013)
[ 0.054623] ACPI: FACP 0x00000000BFBF5000 000074 (v01 BOCHS BXPCFACP
00000001 BXPC 00000001)
[ 0.056326] ACPI: DSDT 0x00000000BFBF6000 00238D (v01 BOCHS BXPCDSDT
00000001 BXPC 00000001)
[ 0.058016] ACPI: FACS 0x00000000BFBFD000 000040
[ 0.058940] ACPI: APIC 0x00000000BFBF4000 000090 (v01 BOCHS BXPCAPIC
00000001 BXPC 00000001)
[ 0.060627] ACPI: HPET 0x00000000BFBF3000 000038 (v01 BOCHS BXPCHPET
00000001 BXPC 00000001)
[ 0.062304] ACPI: BGRT 0x00000000BE49B000 000038 (v01 INTEL EDK2
00000002 01000013)
[ 0.063987] ACPI: iBFT 0x00000000BE453000 000800 (v01 BOCHS BXPCFACP
00000000 00000000)
[ 0.065683] XXX acpi_tb_find_table_address: signature=iBFT
[ 0.066754] XXX acpi_tb_find_table_address(EXIT): address=be453000
[ 0.067959] XXX reserve_ibft_region: table=ffffffffff240000,
address=be453000, size=1000
[ 0.069534] XXX reserve_ibft_region: calling
acpi_put_table(ffffffffff240000)
Not sure if it's the right thing to do, but added
"acpi_tb_find_table_address()" to return the physical address of a table to
use with memblock_reserve().
virt_to_phys(table) does not seem to return the physical address for the
iBFT table (it would be nice if struct acpi_table_header also had a
"address" element for the physical address of the table).
virt_to_phys() does not work that early because then it is mapped with
early_memremap() which uses different virtual to physical scheme.
I'd say that acpi_tb_find_table_address() makes sense if we'd like to
reserve ACPI tables outside of drivers/acpi.
But probably we should simply reserve all the tables during
acpi_table_init() so that any table that firmware put in the normal memory
will be surely reserved.
quoted
Ran 10 successful boots with the above without failure.
That's good news indeed :)
Wondering if we could do something like this instead (trying to keep
changes minimal). Just do the memblock_reserve() for all the standard
tables.
From: Mike Rapoport <hidden> Date: 2021-02-28 18:10:18
On Fri, Feb 26, 2021 at 11:16:06AM -0500, George Kennedy wrote:
On 2/26/2021 6:17 AM, Mike Rapoport wrote:
quoted
Hi George,
On Thu, Feb 25, 2021 at 08:19:18PM -0500, George Kennedy wrote:
quoted
Not sure if it's the right thing to do, but added
"acpi_tb_find_table_address()" to return the physical address of a table to
use with memblock_reserve().
virt_to_phys(table) does not seem to return the physical address for the
iBFT table (it would be nice if struct acpi_table_header also had a
"address" element for the physical address of the table).
virt_to_phys() does not work that early because then it is mapped with
early_memremap() which uses different virtual to physical scheme.
I'd say that acpi_tb_find_table_address() makes sense if we'd like to
reserve ACPI tables outside of drivers/acpi.
But probably we should simply reserve all the tables during
acpi_table_init() so that any table that firmware put in the normal memory
will be surely reserved.
quoted
Ran 10 successful boots with the above without failure.
That's good news indeed :)
Wondering if we could do something like this instead (trying to keep changes
minimal). Just do the memblock_reserve() for all the standard tables.
I think something like this should work, but I'm not an ACPI expert to say
if this the best way to reserve the tables.
new_table_desc->flags,
new_table_desc->pointer);
+ acpi_tb_reserve_standard_table(new_table_desc->address,
+ new_table_desc->pointer);
+
acpi_tb_print_table_header(new_table_desc->address,
new_table_desc->pointer);
There should be no harm in doing the memblock_reserve() for all the standard
tables, right?
It should be ok to memblock_reserve() all the tables very early as long as
we don't run out of static entries in memblock.reserved.
We just need to make sure the tables are reserved before memblock
allocations are possible, so we'd still need to move acpi_table_init() in
x86::setup_arch() before e820__memblock_setup().
Not sure how early ACPI is initialized on arm64.
Ran 10 boots with the above without failure.
George
From: George Kennedy <hidden> Date: 2021-03-01 14:33:03
On 2/28/2021 1:08 PM, Mike Rapoport wrote:
On Fri, Feb 26, 2021 at 11:16:06AM -0500, George Kennedy wrote:
quoted
On 2/26/2021 6:17 AM, Mike Rapoport wrote:
quoted
Hi George,
On Thu, Feb 25, 2021 at 08:19:18PM -0500, George Kennedy wrote:
quoted
Not sure if it's the right thing to do, but added
"acpi_tb_find_table_address()" to return the physical address of a table to
use with memblock_reserve().
virt_to_phys(table) does not seem to return the physical address for the
iBFT table (it would be nice if struct acpi_table_header also had a
"address" element for the physical address of the table).
virt_to_phys() does not work that early because then it is mapped with
early_memremap() which uses different virtual to physical scheme.
I'd say that acpi_tb_find_table_address() makes sense if we'd like to
reserve ACPI tables outside of drivers/acpi.
But probably we should simply reserve all the tables during
acpi_table_init() so that any table that firmware put in the normal memory
will be surely reserved.
quoted
Ran 10 successful boots with the above without failure.
That's good news indeed :)
Wondering if we could do something like this instead (trying to keep changes
minimal). Just do the memblock_reserve() for all the standard tables.
I think something like this should work, but I'm not an ACPI expert to say
if this the best way to reserve the tables.
There should be no harm in doing the memblock_reserve() for all the standard
tables, right?
It should be ok to memblock_reserve() all the tables very early as long as
we don't run out of static entries in memblock.reserved.
We just need to make sure the tables are reserved before memblock
allocations are possible, so we'd still need to move acpi_table_init() in
x86::setup_arch() before e820__memblock_setup().
Not sure how early ACPI is initialized on arm64.
Thanks Mike. Will try to move the memblock_reserves() before
e820__memblock_setup().
George
quoted
Ran 10 boots with the above without failure.
George
From: George Kennedy <hidden> Date: 2021-03-02 07:47:23
On 3/1/2021 9:29 AM, George Kennedy wrote:
On 2/28/2021 1:08 PM, Mike Rapoport wrote:
quoted
On Fri, Feb 26, 2021 at 11:16:06AM -0500, George Kennedy wrote:
quoted
On 2/26/2021 6:17 AM, Mike Rapoport wrote:
quoted
Hi George,
On Thu, Feb 25, 2021 at 08:19:18PM -0500, George Kennedy wrote:
quoted
Not sure if it's the right thing to do, but added
"acpi_tb_find_table_address()" to return the physical address of a
table to
use with memblock_reserve().
virt_to_phys(table) does not seem to return the physical address
for the
iBFT table (it would be nice if struct acpi_table_header also had a
"address" element for the physical address of the table).
virt_to_phys() does not work that early because then it is mapped with
early_memremap() which uses different virtual to physical scheme.
I'd say that acpi_tb_find_table_address() makes sense if we'd like to
reserve ACPI tables outside of drivers/acpi.
But probably we should simply reserve all the tables during
acpi_table_init() so that any table that firmware put in the normal
memory
will be surely reserved.
quoted
Ran 10 successful boots with the above without failure.
That's good news indeed :)
Wondering if we could do something like this instead (trying to keep
changes
minimal). Just do the memblock_reserve() for all the standard tables.
I think something like this should work, but I'm not an ACPI expert
to say
if this the best way to reserve the tables.
Adding ACPI maintainers to the CC list.
quoted
quoted
diff --git a/drivers/acpi/acpica/tbinstal.c
b/drivers/acpi/acpica/tbinstal.c
index 0bb15ad..830f82c 100644
new_table_desc->flags,
new_table_desc->pointer);
+ acpi_tb_reserve_standard_table(new_table_desc->address,
+ new_table_desc->pointer);
+
acpi_tb_print_table_header(new_table_desc->address,
new_table_desc->pointer);
There should be no harm in doing the memblock_reserve() for all the
standard
tables, right?
It should be ok to memblock_reserve() all the tables very early as
long as
we don't run out of static entries in memblock.reserved.
We just need to make sure the tables are reserved before memblock
allocations are possible, so we'd still need to move
acpi_table_init() in
x86::setup_arch() before e820__memblock_setup().
Not sure how early ACPI is initialized on arm64.
Thanks Mike. Will try to move the memblock_reserves() before
e820__memblock_setup().
Hi Mike,
Moved acpi_table_init() in x86::setup_arch() before
e820__memblock_setup() as you suggested.
Ran 10 boots with the following without error.
From: Mike Rapoport <hidden> Date: 2021-03-02 10:11:15
Hi George,
On Mon, Mar 01, 2021 at 08:20:45PM -0500, George Kennedy wrote:
quoted
quoted
quoted
quoted
There should be no harm in doing the memblock_reserve() for all
the standard
tables, right?
It should be ok to memblock_reserve() all the tables very early as
long as
we don't run out of static entries in memblock.reserved.
We just need to make sure the tables are reserved before memblock
allocations are possible, so we'd still need to move
acpi_table_init() in
x86::setup_arch() before e820__memblock_setup().
Not sure how early ACPI is initialized on arm64.
Thanks Mike. Will try to move the memblock_reserves() before
e820__memblock_setup().
Hi Mike,
Moved acpi_table_init() in x86::setup_arch() before e820__memblock_setup()
as you suggested.
Ran 10 boots with the following without error.
I'd suggest to send it as a formal patch to see what x86 and ACPI folks
have to say about this.