| Message ID | 1421161997-19958-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 13 January 2015 at 15:13, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > The function CoreTerminateMemoryMap() performs some final sanity checks > on the runtime regions in the memory map before allowing ExitBootServices() > to complete. Unfortunately, it does so by testing the EFI_MEMORY_RUNTIME bit > in the Attribute field, which is never set anywhere in the code. > > Fix it by setting the bit where appropriate in CoreAddRange(). > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > MdeModulePkg/Core/Dxe/Mem/Page.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c > index fa84e2612526..3abf934933d8 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c > @@ -240,6 +240,10 @@ CoreAddRange ( > } > } > > + if (mMemoryTypeStatistics[Type].Runtime) { > + Attribute |= EFI_MEMORY_RUNTIME; > + } > + Replying to self: also need to clear the bit in an else { } branch here.
On 13 January 2015 at 16:15, Laszlo Ersek <lersek@redhat.com> wrote: > On 01/13/15 16:13, Ard Biesheuvel wrote: >> The function CoreTerminateMemoryMap() performs some final sanity checks >> on the runtime regions in the memory map before allowing ExitBootServices() >> to complete. Unfortunately, it does so by testing the EFI_MEMORY_RUNTIME bit >> in the Attribute field, which is never set anywhere in the code. >> >> Fix it by setting the bit where appropriate in CoreAddRange(). >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> MdeModulePkg/Core/Dxe/Mem/Page.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c >> index fa84e2612526..3abf934933d8 100644 >> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c >> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c >> @@ -240,6 +240,10 @@ CoreAddRange ( >> } >> } >> >> + if (mMemoryTypeStatistics[Type].Runtime) { >> + Attribute |= EFI_MEMORY_RUNTIME; >> + } >> + >> // >> // Add descriptor >> // >> > > I think the check that you imply in CoreTerminateMemoryMap() is indeed dead code. But, I don't see how your patch would make it less dead. :) > > mMemoryTypeStatistics is a static array. Elements in that array never change their Runtime fields. > > The CoreGetMemoryMap() function uses the Runtime field to set the EFI_MEMORY_RUNTIME bit for various types of memory (EfiRuntimeServicesCode, EfiRuntimeServicesData, EfiPalCode), but that bit is set only in the map built for the caller, never in the internal map. > > When CoreTerminateMemoryMap() looks at the internal attribute bits, I agree that EFI_MEMORY_RUNTIME is never set, so that check is dead. However, the first check within that condition looks for at the type directly, EfiACPIReclaimMemory and EfiACPIMemoryNVS. Even if the EFI_MEMORY_RUNTIME bit had fired, this check would remain dead, because for these two memory types the Runtime bit is never set in the mMemoryTypeStatistics. > > ... Aha! You're not aiming at the first check here. You are looking at the start & end alignments. > Correct. I am looking into changing the allocation strategy for runtime regions so they are aligned multiples of 64k (to accommodate OSes running with 64k pages) > In that case though, I don't think we should change the invariant > > the internal attributes never set EFI_MEMORY_RUNTIME > > Instead, I think CoreTerminateMemoryMap() should replicate the check in seen CoreGetMemoryMap(). Something like: > >> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c >> index fa84e26..d9e2075 100644 >> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c >> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c >> @@ -1790,12 +1790,9 @@ CoreTerminateMemoryMap ( >> >> for (Link = gMemoryMap.ForwardLink; Link != &gMemoryMap; Link = Link->ForwardLink) { >> Entry = CR(Link, MEMORY_MAP, Link, MEMORY_MAP_SIGNATURE); >> - if ((Entry->Attribute & EFI_MEMORY_RUNTIME) != 0) { >> - if (Entry->Type == EfiACPIReclaimMemory || Entry->Type == EfiACPIMemoryNVS) { >> - DEBUG((DEBUG_ERROR | DEBUG_PAGE, "ExitBootServices: ACPI memory entry has RUNTIME attribute set.\n")); >> - Status = EFI_INVALID_PARAMETER; >> - goto Done; >> - } >> + if (mMemoryTypeStatistics[Entry->Type].Runtime) { >> + ASSERT (Entry->Type != EfiACPIReclaimMemory); >> + ASSERT (Entry->Type != EfiACPIMemoryNVS); >> if ((Entry->Start & (EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT - 1)) != 0) { >> DEBUG((DEBUG_ERROR | DEBUG_PAGE, "ExitBootServices: A RUNTIME memory entry is not on a proper alignment.\n")); >> Status = EFI_INVALID_PARAMETER; > > Of course I have no jurisdiction in MdeModulePkg/Core/, so I'm just theorizing :) Still, > > the internal attributes never set EFI_MEMORY_RUNTIME > > looks more like a deliberate invariant than an oversight. > OK, I am fine with that as well. In fact, I don't care deeply about whether or not this check is performed at all, but the current situation is a bit awkward.
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c index fa84e2612526..3abf934933d8 100644 --- a/MdeModulePkg/Core/Dxe/Mem/Page.c +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c @@ -240,6 +240,10 @@ CoreAddRange ( } } + if (mMemoryTypeStatistics[Type].Runtime) { + Attribute |= EFI_MEMORY_RUNTIME; + } + // // Add descriptor //
The function CoreTerminateMemoryMap() performs some final sanity checks on the runtime regions in the memory map before allowing ExitBootServices() to complete. Unfortunately, it does so by testing the EFI_MEMORY_RUNTIME bit in the Attribute field, which is never set anywhere in the code. Fix it by setting the bit where appropriate in CoreAddRange(). Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- MdeModulePkg/Core/Dxe/Mem/Page.c | 4 ++++ 1 file changed, 4 insertions(+)