[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86: slightly simplify MB2/EFI "magic" check
Hi, On Mon Aug 12, 2024 at 3:43 PM BST, Jan Beulich wrote: > On 12.08.2024 16:34, Alejandro Vallejo wrote: > > On Thu Aug 8, 2024 at 9:49 AM BST, Jan Beulich wrote: > >> A few dozen lines down from here we repeatedly use a pattern involving > >> just a single (conditional) branch. Do so also when checking for the > >> boot loader magic value. > >> > >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> > >> --- > >> I further question the placement of the clearing of vga_text_buffer, > >> just out of context: Shouldn't that be placed with the increments of > >> efi_platform and skip_realmode? Or else is the terminology in comments > >> ("on EFI platforms") wrong in one of the two places? In the end, if we > >> are entered at __efi64_mb2_start but the magic doesn't match, we simply > >> don't know what environment we're in. There may or may not be a VGA > >> console at the default address, so we may as well (try to) write to it > >> (just like we do when entered at start). > > > > It's fair to assume we're in 64bits, and in that situation it's also fair to > > assume the text console is long gone (pun intended). > > How is being in 64-bit mode correlated with there being a VGA console? > (I question "fair to assume" here anyway. We're on a path where we know > something's wonky.) The only way in which you could plausibly have a text-mode console in 64bits is if you booted from BIOS and didn't set a VESA mode, so it boils down to what failure modes you want to consider. For "anything goes" you're right that we can't even be sure of being in 64bit (or 32bit) mode, but that's too draconian an assumption to try to uphold, imo. I think that while details in the boot protocol might be incorrect (like the magic tag), broad strokes (like being in long mode and having a UEFI runtime) must still hold. Trying to use the serial is fine (worst-case scenario it doesn't work), but trying to use a framebuffer you're not sure about is not unlikely to triple fault your machine prematurely and then debugging it is a pain even with an emulator. > > >> --- a/xen/arch/x86/boot/head.S > >> +++ b/xen/arch/x86/boot/head.S > >> @@ -233,13 +233,11 @@ __efi64_mb2_start: > >> > >> /* Check for Multiboot2 bootloader. */ > >> cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax > >> - je .Lefi_multiboot2_proto > >> > >> /* Jump to .Lnot_multiboot after switching CPU to x86_32 mode. */ > >> lea .Lnot_multiboot(%rip), %r15 > > > > I don't think there's much benefit to this, but it would read more > > naturally if > > lea was before cmp. Then cmp would be next to its (new) associated jne. > > You did look at the pattern though that I'm referring to in the description? > Knowing that generally paring the CMP/TEST with the Jcc, I would have > switched things around. Yet I wanted to make things as similar as possible, > in the hope that be(com)ing consistent would make it easiest to get such a > minor change in. > > Jan I'm not sure about the pattern you mention. Seems like a standard set of doX+check+cond_jump finished in an unconditional jump. All of them pretty normal. Regardless, I'm not arguing against this. While I happen to find it easier to mentally parse it in its current form we do save a jump instruction after your change. It's just that it'd be easier to follow with the mentioned reversal of lea and cmp. Cheers, Alejandro
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |