On Thu, Apr 18, 2013 at 12:00:26PM +0100, Matt Fleming wrote: > On 17/04/13 23:00, Bryan O'Donoghue wrote: > > In my mind the only memory that is relevant to efi_enter_virtual_mode is > > memory that the BIOS has marked as EFI_RUNTIME_SERVICE > > > > md->attribute & 0x80000000_00000000 > > > > I couldn't quite understand why the code in > > > > arch/x86/platform/efi/efi.c:enter_virtual_mode() looks like this > > > > for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) { > > md = p; > > if (!(md->attribute & EFI_MEMORY_RUNTIME) && > > md->type != EFI_BOOT_SERVICES_CODE && > > md->type != EFI_BOOT_SERVICES_DATA) > > continue; > > > > While the code in > > > > arch/ia64/kernel/efi.c:enter_virtual_mode > > > > for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) { > > md = p; > > if (md->attribute & EFI_MEMORY_RUNTIME) { > > > > The ia64 version is consistent with the standard - but obviously isn't > > accounting for the work-around implemented to retrieve the BGRT on ia32. > > > > Looking at the commit message associated with > > arch/x86/platform/efi/efi-bgrt.c > > > > It's pretty obvious the mapping of boot code/data was done to facilitate > > BGRT. > > No, that's incorrect. The patch that introduced the mapping of > EFI_BOOT_SERVICES_{CODE,DATA} was committed before support for bgrt > existed. git blame is a good tool to use when doing one of these > historical digs, and in this case it shows that the above lines from > efi_enter_virtual_mode() were introduced in the following commit, > > commit 916f676f8dc016103f983c7ec54c18ecdbb6e349 > Author: Matthew Garrett <mjg@xxxxxxxxxx> > Date: Wed May 25 09:53:13 2011 -0400 > > x86, efi: Retain boot service code until after switching to virtual mode > > UEFI stands for "Unified Extensible Firmware Interface", where "Firmware" > is an ancient African word meaning "Why do something right when you can > do it so wrong that children will weep and brave adults will cower before > you", and "UEI" is Celtic for "We missed DOS so we burned it into your > ROMs". The UEFI specification provides for runtime services (ie, another > way for the operating system to be forced to depend on the firmware) and > we rely on these for certain trivial tasks such as setting up the > bootloader. But some hardware fails to work if we attempt to use these > runtime services from physical mode, and so we have to switch into virtual > mode. So far so dreadful. > > The specification makes it clear that the operating system is free to do > whatever it wants with boot services code after ExitBootServices() has been > called. SetVirtualAddressMap() can't be called until ExitBootServices() has > been. So, obviously, a whole bunch of EFI implementations call into boot > services code when we do that. Since we've been charmingly naive and > trusted that the specification may be somehow relevant to the real world, > we've already stuffed a picture of a penguin or something in that address > space. And just to make things more entertaining, we've also marked it > non-executable. > > This patch allocates the boot services regions during EFI init and makes > sure that they're executable. Then, after SetVirtualAddressMap(), it > discards them and everyone lives happily ever after. Except for the ones > who have to work on EFI, who live sad lives haunted by the knowledge that > someone's eventually going to write yet another firmware specification. > > [ hpa: adding this to urgent with a stable tag since it fixes currently-broken > hardware. However, I do not know what the dependencies are and so I do > not know which -stable versions this may be a candidate for. ] > > Signed-off-by: Matthew Garrett <mjg@xxxxxxxxxx> > Link: http://lkml.kernel.org/r/1306331593-28715-1-git-send-email-mjg@xxxxxxxxxx > Signed-off-by: H. Peter Anvin <hpa@xxxxxxxxxxxxxxx> > Cc: Tony Luck <tony.luck@xxxxxxxxx> > Cc: <stable@xxxxxxxxxx> > > Yes the bgrt code accesses the Boot Service mappings, but that isn't the > only reason we want to map those regions. > > > That's one solution - you'd need to make the i386::efi_ioremap() aware > > of the BGRT work-around. > > > > Presumably this work-around is also required on ia64 too. > > No, we've never seen an ia64 firmware implementation with the "access > EFI Boot Services Code/Data after ExitBootServices() bug", and it > doesn't suffer from the same virtual address space limitations that i386 > does. > > > No, no - we *don't* have a BGRT object at all. > > > > We have a completely clean memory map - but the BGRT code is causing the > > is_ram() failure. > > You assume that mapping of the Boot Services regions is done purely for > the benefit of pulling out the bgrt image - it's not, see the above > commit log - and I assumed that you had an ACPI bgrt pointer in your > memory map, but you don't. > > Darren, Josh, have you ever seen an i386 machine with a bgrt pointer? If > not, and given that we've never seen an i386 firmware that requires the > above workaround from Matthew, combined with the fact that there are so > few i386 implementations out there, I'm inclined to apply the patch > below, because anything else is a lot more work. We can address this > properly if we ever start seeing i386 machines with bgrt pointers that > reference highmem. Hm. I'm probably the least clueful person to ask on this one. Fedora has a number of 32-bit bug reports, but we explicitly don't support 32-bit UEFI. BGRT is a new addition in ACPI 5.0, right? Hopefully with it being relatively recent, and new 32-bit firmware being somewhat rare, it won't be a problem. josh -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html