On 19 January 2015 at 05:17, Tian, Feng <[email protected]> wrote:
> Ard,
>
> I am prone to Laszlo's fix and make a little update to avoid array
> out-of-boundary issue.
>
> Please help review it.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> Review-by: Feng Tian <[email protected]>
>
> Thanks
> Feng
>
Hello Feng,
As Laszlo had already mentioned in his original reply, there is really
no point in doing this:
+ if (mMemoryTypeStatistics[Entry->Type].Runtime) {
+ if (Entry->Type == EfiACPIReclaimMemory || Entry->Type ==
EfiACPIMemoryNVS) {
because we know the mMemoryTypeStatistics array has the Runtime field
cleared for these memory types.
Other than that, the patch looks fine.
Acked-by: Ard Biesheuvel <[email protected]>
Regards,
Ard.
> -----Original Message-----
> From: Ard Biesheuvel [mailto:[email protected]]
> Sent: Thursday, January 15, 2015 16:12
> To: Tian, Feng
> Cc: [email protected]
> Subject: Re: [edk2] [PATCH] DxeCore: set EFI_MEMORY_RUNTIME where appropriate
> on internal memory map
>
> On 15 January 2015 at 07:25, Tian, Feng <[email protected]> wrote:
>> Hi, Ard
>>
>> Do you see any real impact of this issue when you tried to change runtime
>> region allocation strategy?
>>
>
> Yes.
>
> On 64-bit ARM (AArch64), the OS can decide to use 64 KB pages instead of 4 KB
> pages. As an optimization, I tried to change the allocation strategy for
> runtime regions in Tianocore so that they are always 64 KB aligned multiple
> of 64 KB, even if UEFI's native page size is 4 KB.
> The default allocation can remain at 4 KB, as UEFI itself runs with 4 KB
> pages regardless.
>
> With the following patch applied
>
> ---->8----
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Imem.h
> b/MdeModulePkg/Core/Dxe/Mem/Imem.h
> index d09ff3c5220f..41204c8cf412 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Imem.h
> +++ b/MdeModulePkg/Core/Dxe/Mem/Imem.h
> @@ -22,6 +22,14 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> #define EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT (EFI_PAGE_SIZE * 2)
> #define DEFAULT_PAGE_ALLOCATION (EFI_PAGE_SIZE * 2)
>
> +#elif defined (MDE_CPU_AARCH64)
> +///
> +/// OSes on 64-bit ARM may run with 64k pages, so align the runtime ///
> +regions to 64k as well /// #define
> +EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT (EFI_PAGE_SIZE * 16)
> +#define DEFAULT_PAGE_ALLOCATION (EFI_PAGE_SIZE)
> +
> #else
> ///
> /// For genric EFI machines make the default allocations 4K aligned
> ---->8----
>
> I only get a partial result: AllocatePages () will adhere to the larger
> alignment, but AllocatePool () ignores it. This is a separate issues that
> needs to be addressed in Pool.c
>
> But more importantly, the checks in CoreTerminateMemoryMap() do not detect
> this at all, hence my patch.
>
>> I just did a quick search for possible updates on Attribute field, please
>> see the calling flow:
>> gBS->SetMemorySpaceCapabilities() -> CoreUpdateMemoryAttributes() ->
>> gBS->CoreConvertPagesEx() -> CoreAddRange () -> InsertTailList ()
>>
>> it means Attribute field may be set to EFI_MEMORY_RUNTIME by caller, am I
>> right?
>>
>
> Yes, it may be set by the caller, but it is clearly not the intention of the
> sanity check in CoreTerminateMemoryMap() to only detect regions whose
> attributes where modified by SetMemorySpaceCapabilities() [which only has one
> caller in the entire code base]
>
> The intention is obviously to make ExitBootServices () fail if *any* of the
> runtime regions do not adhere to EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT,
> and that is quite clearly broken.
>
> Regards,
> Ard.
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:[email protected]]
>> Sent: Wednesday, January 14, 2015 00:19
>> To: [email protected]
>> Subject: Re: [edk2] [PATCH] DxeCore: set EFI_MEMORY_RUNTIME where
>> appropriate on internal memory map
>>
>> On 13 January 2015 at 16:15, Laszlo Ersek <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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.
>>
>> --
>> Ard.
>>
>> ----------------------------------------------------------------------
>> -------- New Year. New Location. New Benefits. New Data Center in
>> Ashburn, VA.
>> GigeNET is offering a free month of service with a new server in Ashburn.
>> Choose from 2 high performing configs, both with 100TB of bandwidth.
>> Higher redundancy.Lower latency.Increased capacity.Completely compliant.
>> http://p.sf.net/sfu/gigenet
>> _______________________________________________
>> edk2-devel mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
