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
