| Message ID | 20230530150106.2703849-1-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] iscsi_ibft: Fix finding the iBFT under Xen Dom 0 | expand |
On 5/30/23 08:01, Ross Lagerwall wrote: > Since firmware doesn't indicate the iBFT in the E820, add a reserved > region so that it gets identity mapped when running as Dom 0 so that it > is possible to search for it. Move the call to reserve_ibft_region() > later so that it is called after the Xen identity mapping adjustments > are applied. > > Finally, instead of using isa_bus_to_virt() which doesn't do the right > thing under Xen, use early_memremap() like the dmi_scan code does. This is connecting Xen, iSCSI and x86. Some background here would be *really* nice for dummies like me that deal heavily in only one of those three. One or two sentences like this: Firmware can provide an iSCSI-specific table called the iBFT which helps the OS boot from iSCSI devices. can go a long way for dummies like me. As could some background about why this: ... add a reserved region so that it gets identity mapped when running as Dom 0 so that it is possible to search for it. These are all English words, but off the top of my head, I have no idea why reserved regions get identity mapped when running as Dom 0 or why that makes it possible to search. The addresses and size here: > +#ifdef CONFIG_ISCSI_IBFT_FIND > + /* Reserve 0.5 MiB to 1 MiB region so iBFT can be found */ > + xen_e820_table.entries[xen_e820_table.nr_entries].addr = 0x80000; > + xen_e820_table.entries[xen_e820_table.nr_entries].size = 0x80000; > + xen_e820_table.entries[xen_e820_table.nr_entries].type = E820_TYPE_RESERVED; > + xen_e820_table.nr_entries++; > +#endif also appear to be conjured out of thin air. As does the move of: > + reserve_ibft_region(); I'm sure I can go figure this all out with some research. But, I'd really appreciate some extra effort from you in this changelog to save me the time. I bet you can explain it a lot more efficiently than I can go figure it out.
On 6/1/23 09:57, Dave Hansen wrote: > On 5/30/23 08:01, Ross Lagerwall wrote: >> Since firmware doesn't indicate the iBFT in the E820, add a reserved >> region so that it gets identity mapped when running as Dom 0 so that it >> is possible to search for it. Move the call to reserve_ibft_region() >> later so that it is called after the Xen identity mapping adjustments >> are applied. Oh, and one more thing: What is the end user visible effect of this problem and of your solution? Do Xen Dom 0 systems fail to find their boot iSCSI volume and refuse to boot? I take it after this patch that they can boot again.
On 01/06/2023 6:08 pm, Dave Hansen wrote: > On 6/1/23 09:57, Dave Hansen wrote: >> On 5/30/23 08:01, Ross Lagerwall wrote: >>> Since firmware doesn't indicate the iBFT in the E820, add a reserved >>> region so that it gets identity mapped when running as Dom 0 so that it >>> is possible to search for it. Move the call to reserve_ibft_region() >>> later so that it is called after the Xen identity mapping adjustments >>> are applied. > Oh, and one more thing: > > What is the end user visible effect of this problem and of your solution? > > Do Xen Dom 0 systems fail to find their boot iSCSI volume and refuse to > boot? I take it after this patch that they can boot again. > Yeah, this isn't as clear as it could be. In short... The iBFT suffers from the same problem as legacy ACPI RDSP. You've got to search lowmem for a magic marker to find it. Xen dom0 is just a VM with root-like perms. Anything it wants an identity map of, it has to ask for. And because dom0 is commonly sharing ownership of hardware, it requests identity mappings for everything reserved in the E820 table. The consequence of not having this patch is that if you try iSCSI boot under Xen, dom0 can't find it's filesystem, because it can't get at the iSCSI initiator. ~Andrew
On 01.06.23 18:57, Dave Hansen wrote: > On 5/30/23 08:01, Ross Lagerwall wrote: >> Since firmware doesn't indicate the iBFT in the E820, add a reserved >> region so that it gets identity mapped when running as Dom 0 so that it >> is possible to search for it. Move the call to reserve_ibft_region() >> later so that it is called after the Xen identity mapping adjustments >> are applied. >> >> Finally, instead of using isa_bus_to_virt() which doesn't do the right >> thing under Xen, use early_memremap() like the dmi_scan code does. > > This is connecting Xen, iSCSI and x86. Some background here would be > *really* nice for dummies like me that deal heavily in only one of those > three. > > One or two sentences like this: > > Firmware can provide an iSCSI-specific table called the iBFT > which helps the OS boot from iSCSI devices. > > can go a long way for dummies like me. As could some background about > why this: > > ... add a reserved region so that it gets identity mapped when > running as Dom 0 so that it is possible to search for it. > > These are all English words, but off the top of my head, I have no idea > why reserved regions get identity mapped when running as Dom 0 or why > that makes it possible to search. > > The addresses and size here: > >> +#ifdef CONFIG_ISCSI_IBFT_FIND >> + /* Reserve 0.5 MiB to 1 MiB region so iBFT can be found */ >> + xen_e820_table.entries[xen_e820_table.nr_entries].addr = 0x80000; >> + xen_e820_table.entries[xen_e820_table.nr_entries].size = 0x80000; >> + xen_e820_table.entries[xen_e820_table.nr_entries].type = E820_TYPE_RESERVED; >> + xen_e820_table.nr_entries++; >> +#endif > > also appear to be conjured out of thin air. I'd suggest to move the definitions of IBFT_START and IBFT_END from drivers/firmware/iscsi_ibft_find.c to include/linux/iscsi_ibft.h and use them here. Juergen
> From: Dave Hansen <dave.hansen@intel.com> > Sent: Thursday, June 1, 2023 5:57 PM > To: Ross Lagerwall <ross.lagerwall@citrix.com>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org> > Cc: Jan Beulich <jbeulich@suse.com>; Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; Borislav Petkov <bp@alien8.de>; Dave Hansen <dave.hansen@linux.intel.com>; x86@kernel.org <x86@kernel.org>; Juergen Gross <jgross@suse.com>; Boris Ostrovsky <boris.ostrovsky@oracle.com>; Peter Jones <pjones@redhat.com>; Konrad Rzeszutek Wilk <konrad@kernel.org> > Subject: Re: [PATCH v2] iscsi_ibft: Fix finding the iBFT under Xen Dom 0 > > On 5/30/23 08:01, Ross Lagerwall wrote: > > Since firmware doesn't indicate the iBFT in the E820, add a reserved > > region so that it gets identity mapped when running as Dom 0 so that it > > is possible to search for it. Move the call to reserve_ibft_region() > > later so that it is called after the Xen identity mapping adjustments > > are applied. > > > > Finally, instead of using isa_bus_to_virt() which doesn't do the right > > thing under Xen, use early_memremap() like the dmi_scan code does. > > This is connecting Xen, iSCSI and x86. Some background here would be > *really* nice for dummies like me that deal heavily in only one of those > three. > > One or two sentences like this: > > Firmware can provide an iSCSI-specific table called the iBFT > which helps the OS boot from iSCSI devices. > > can go a long way for dummies like me. As could some background about > why this: > > ... add a reserved region so that it gets identity mapped when > running as Dom 0 so that it is possible to search for it. > > These are all English words, but off the top of my head, I have no idea > why reserved regions get identity mapped when running as Dom 0 or why > that makes it possible to search. > > The addresses and size here: > > > +#ifdef CONFIG_ISCSI_IBFT_FIND > > + /* Reserve 0.5 MiB to 1 MiB region so iBFT can be found */ > > + xen_e820_table.entries[xen_e820_table.nr_entries].addr = 0x80000; > > + xen_e820_table.entries[xen_e820_table.nr_entries].size = 0x80000; > > + xen_e820_table.entries[xen_e820_table.nr_entries].type = E820_TYPE_RESERVED; > > + xen_e820_table.nr_entries++; > > +#endif > > also appear to be conjured out of thin air. > > As does the move of: > > > + reserve_ibft_region(); > > I'm sure I can go figure this all out with some research. But, I'd > really appreciate some extra effort from you in this changelog to save > me the time. I bet you can explain it a lot more efficiently than I can > go figure it out. Sure, I will resend with an expanded commit message. Ross
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 16babff771bd..616b80507abd 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -796,7 +796,6 @@ static void __init early_reserve_memory(void) memblock_x86_reserve_range_setup_data(); - reserve_ibft_region(); reserve_bios_regions(); trim_snb_memory(); } @@ -1032,6 +1031,7 @@ void __init setup_arch(char **cmdline_p) if (efi_enabled(EFI_BOOT)) efi_init(); + reserve_ibft_region(); dmi_setup(); /* diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c index c2be3efb2ba0..07c7039bdd66 100644 --- a/arch/x86/xen/setup.c +++ b/arch/x86/xen/setup.c @@ -764,17 +764,26 @@ char * __init xen_memory_setup(void) BUG_ON(memmap.nr_entries == 0); xen_e820_table.nr_entries = memmap.nr_entries; - /* - * Xen won't allow a 1:1 mapping to be created to UNUSABLE - * regions, so if we're using the machine memory map leave the - * region as RAM as it is in the pseudo-physical map. - * - * UNUSABLE regions in domUs are not handled and will need - * a patch in the future. - */ - if (xen_initial_domain()) + if (xen_initial_domain()) { + /* + * Xen won't allow a 1:1 mapping to be created to UNUSABLE + * regions, so if we're using the machine memory map leave the + * region as RAM as it is in the pseudo-physical map. + * + * UNUSABLE regions in domUs are not handled and will need + * a patch in the future. + */ xen_ignore_unusable(); +#ifdef CONFIG_ISCSI_IBFT_FIND + /* Reserve 0.5 MiB to 1 MiB region so iBFT can be found */ + xen_e820_table.entries[xen_e820_table.nr_entries].addr = 0x80000; + xen_e820_table.entries[xen_e820_table.nr_entries].size = 0x80000; + xen_e820_table.entries[xen_e820_table.nr_entries].type = E820_TYPE_RESERVED; + xen_e820_table.nr_entries++; +#endif + } + /* Make sure the Xen-supplied memory map is well-ordered. */ e820__update_table(&xen_e820_table); diff --git a/drivers/firmware/iscsi_ibft_find.c b/drivers/firmware/iscsi_ibft_find.c index 94b49ccd23ac..e3c1449987dd 100644 --- a/drivers/firmware/iscsi_ibft_find.c +++ b/drivers/firmware/iscsi_ibft_find.c @@ -52,9 +52,9 @@ static const struct { */ void __init reserve_ibft_region(void) { - unsigned long pos; + unsigned long pos, virt_pos = 0; unsigned int len = 0; - void *virt; + void *virt = NULL; int i; ibft_phys_addr = 0; @@ -70,13 +70,20 @@ void __init reserve_ibft_region(void) * so skip that area */ if (pos == VGA_MEM) pos += VGA_SIZE; - virt = isa_bus_to_virt(pos); + + /* Map page by page */ + if (offset_in_page(pos) == 0) { + if (virt) + early_memunmap(virt, PAGE_SIZE); + virt = early_memremap_ro(pos, PAGE_SIZE); + virt_pos = pos; + } for (i = 0; i < ARRAY_SIZE(ibft_signs); i++) { - if (memcmp(virt, ibft_signs[i].sign, IBFT_SIGN_LEN) == - 0) { + if (memcmp(virt + (pos - virt_pos), ibft_signs[i].sign, + IBFT_SIGN_LEN) == 0) { unsigned long *addr = - (unsigned long *)isa_bus_to_virt(pos + 4); + (unsigned long *)(virt + pos - virt_pos + 4); len = *addr; /* if the length of the table extends past 1M, * the table cannot be valid. */ @@ -84,9 +91,12 @@ void __init reserve_ibft_region(void) ibft_phys_addr = pos; memblock_reserve(ibft_phys_addr, PAGE_ALIGN(len)); pr_info("iBFT found at %pa.\n", &ibft_phys_addr); - return; + goto out; } } } } + +out: + early_memunmap(virt, PAGE_SIZE); }
Since firmware doesn't indicate the iBFT in the E820, add a reserved region so that it gets identity mapped when running as Dom 0 so that it is possible to search for it. Move the call to reserve_ibft_region() later so that it is called after the Xen identity mapping adjustments are applied. Finally, instead of using isa_bus_to_virt() which doesn't do the right thing under Xen, use early_memremap() like the dmi_scan code does. Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> --- In v2: * Fix style issue * Make added e820 entry conditional on ISCSI_IBFT_FIND arch/x86/kernel/setup.c | 2 +- arch/x86/xen/setup.c | 27 ++++++++++++++++++--------- drivers/firmware/iscsi_ibft_find.c | 24 +++++++++++++++++------- 3 files changed, 36 insertions(+), 17 deletions(-)