Date:	Mon, 16 Sep 2013 13:50:46 +0200
From:	Laszlo Ersek <lersek@...hat.com>
To:	Matt Fleming <matt@...sole-pimps.org>
CC:	jerry.hoemann@...com, Andrew Fish <afish@...le.com>,
	edk2-devel@...ts.sourceforge.net, linux-efi@...r.kernel.org,
	Gleb Natapov <gleb@...hat.com>,
	lkml <linux-kernel@...r.kernel.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Matthew Garrett <mjg59@...f.ucam.org>,
	Brian Richardson <brian.richardson@...el.com>,
	Colin Ian King <colin.king@...onical.com>,
	Randy Wright <rwright@...com>,
	Linn Crosetto <linn.crosetto@...com>, terry.lee@...com,
	samer.el-haj-mahmoud@...com, randy.pawell@...com, chrisp@...com,
	linda.knippers@...com, dong.wei@...com,
	"H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>,
	Josh Triplett <josh@...htriplett.org>,
	Chao Zhang <chao.b.zhang@...el.com>,
	Yao Jiewen <jiewen.yao@...el.com>
Subject: Re: [edk2] Corrupted EFI region

On 09/16/13 12:59, Matt Fleming wrote:
> On Fri, 13 Sep, at 02:38:12PM, jerry.hoemann@...com wrote:
>> Matt,
>>
>> We have hit an issue on our new platform in development related to the
>> call of efi_reserve_boot_services() from setup_arch().
>>
>> The reservation can interfere with allocation of the crash kernel.
>  
> Jerry, thanks for bringing this up.
> 
>> In pre 3.9(?) kernels,  the crash kernel is required to be allocated from
>> physically contiguous memory below 896 MB.
>>
>> Our new platforms are large in both the amount of memory and the amount
>> of IO. This requires large crash kernels for kdump to work.  This is even
>> after the work done for makedumpfile v 1.5 to allow it to work with a
>> smaller foot print.
>>
>>
>> One of the problems is that drivers will allocate memory as boot code and/or
>> data in the region < 896 that effectively fragments this memory.
>> With the reservation, we can't reuse the memory when needed for the
>> crash kernels.   If we remove the reservation and allow the kernel
>> to reuse the memory,  we the reservation of the crash kernel succeeds.
>>
>> This is definitely a problem for distros that are pre 3.9.  Probably less
>> so for top of tree, but i haven't been focused there.
>>
>> So we are definitely interested in finding a mechanism to not
>> do this reservation on platforms that don't have the issues described
>> earlier in this thread.
> 
> OK, in an ideal world we'd move the crash kernel reservation after
> efi_free_boot_services(), because at that point the boot regions are
> available again. But it seems that we reserve the boot regions really
> early during startup and release them relatively late. The reason is
> that the Boot Graphics Resource Table (BGRT) data, if present, is
> located in the Boot Services Data regions but we can't extract the
> address of the region from the ACPI tables until we've setup the ACPI
> subsystem, which happens quite late.

Why is BGRT allocated as Boot Services Data?

In file
"MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c":

InstallBootGraphicsResourceTable()
  BgrtAllocateBsDataMemoryBelow4G()
    gBS->AllocatePages(... EfiBootServicesData ...)

>From Table 25. Memory Type Usage before ExitBootServices():

  EfiBootServicesData  -- The data portions of a loaded Boot Services
                          Driver, and the default data allocation type
                          used by a Boot Services Driver to allocate
                          pool memory.

  EfiACPIReclaimMemory -- Memory that holds the ACPI tables.

>From Table 26. Memory Type Usage after ExitBootServices():

  EfiBootServicesData -- Memory available for general use.

  EfiACPIReclaimMemory -- This memory is to be preserved by the loader
                          and OS until ACPI is enabled. Once ACPI is
                          enabled, the memory in this range is available
                          for general use.

I thought that anything referenced by a pointer in any ACPI table was
EfiACPIReclaimMemory or stricter. Specifically, the RSDT or XSDT points
to BGRT, so BGRT is EfiACPIReclaimMemory.  BGRT points to the image data
(with its Image Address field), hence the image data should be
EfiACPIReclaimMemory too.

Otherwise, the pointer (BGRT.ImageAddress) can outlive the pointed-to
storage (the image data).

The image data sounds to me like textbook example for
EfiACPIReclaimMemory. This way the kernel could free Boot Services Data
early, perform the crash kernel reservation right after, and safely
access BGRT whenever the ACPI subsystem is brought up later.


The edk2 commit that flipped the memory type underneath the image data
from EfiReservedMemoryType to EfiBootServicesData is:

    https://github.com/tianocore/edk2/commit/4c58575e

I think this commit is wrong. It's fine for OSPM to release the image
data at some point, but not right after ExitBootServices(), because
referencing pointers in ACPI tables survive strictly longer.

... Actually, the commit does follow the ACPI spec 5.0:

5.2.22.4 Image Address

    The Image Address contains the location in memory where an
    in-memory copy of the boot image can be found. The image should be
    stored in EfiBootServicesData, allowing the system to reclaim
    the memory when the image is no longer needed.

The ACPI spec 5.0 should recommend EfiACPIReclaimMemory here IMO. (I
take the current wording ("should be stored") as a recommendation only.)

If that's in fact a recommendation (and not a hard requirement), then it
should be easy to change BgrtAllocateBsDataMemoryBelow4G() again.

Thanks,
Laszlo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists