[PATCH v2] iscsi_ibft: Fix finding the iBFT under Xen Dom 0

[PATCH v2] iscsi_ibft: Fix finding the iBFT under Xen Dom 0

[Ross Lagerwall]

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(-)

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);
 }
-- 
2.31.1

Re: [PATCH v2] iscsi_ibft: Fix finding the iBFT under Xen Dom 0

[Dave Hansen]

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.

Re: [PATCH v2] iscsi_ibft: Fix finding the iBFT under Xen Dom 0

[Dave Hansen]

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.

Re: [PATCH v2] iscsi_ibft: Fix finding the iBFT under Xen Dom 0

[Andrew Cooper]

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

Re: [PATCH v2] iscsi_ibft: Fix finding the iBFT under Xen Dom 0

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1917 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2] iscsi_ibft: Fix finding the iBFT under Xen Dom 0

[Ross Lagerwall]

> 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