Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

On 2/15/2024 2:17 PM, Ard Biesheuvel wrote:
> On Thu, 15 Feb 2024 at 20:55, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>>
>> On 2/14/2024 11:50 PM, Ard Biesheuvel wrote:
>>> I looked at the EFI spec again, and EfiACPIReclaimMemory is not
>>> actually listed as a memory type that has this 64k alignment
>>> requirement. This makes sense, given that this memory type has no
>>> significance to the firmware itself, only to the OS. OTOH, reserved
>>> memory does appear there.
>>>
>>> So I suggest we fix that first, and then drop any mention of
>>> EfiACPIReclaimMemory from this patch. At least we'll have heap guard
>>> coverage for ACPI table allocations on arm64 going forward.
>>>
>>> The logic in question was added in 2007 in commit 28a00297189c, so
>>> this was probably the rule on Itanium, but that support is long gone.
>>>
>>
>> I wanted to understand this a little bit further. I do see that the spec
>> does not call out EfiACPIReclaimMemory as needing 64k alignment.
>> However, your statement that the memory type does not have have
>> significance to the FW, only to the OS, so we don't need the 64k
>> alignment is confusing to me. Isn't the 64k alignment needed only for
>> regions that the OS does care about? In order to read this information
>> at their 64k page granularity, doesn't this need to be aligned to 64k?
>>
> 
> It is not about accessing the information in the pages. Most consumers
> don't care about this at all, and even code that does will rarely be
> materially different between 4k and 64k OSes.
> 
> When running under the firmware, we run with 4k pages and programming
> the memory attributes is entirely under the control of the firmware
> itself. So in this phase, the alignment truly does not matter,
> especially because all of RAM is 1:1 mapped anyway.
> 
> When running under the OS, all memory that is part of the OS's memory
> pool is entirely under its control, and this includes ACPI reclaim
> memory, given that the firmware itself should not be accessing this:
> the purpose of this memory type is specifically to expose information
> to the OS in memory that it can use as it sees fit after it is done
> processing the information.
> 
> The alignment issues we need to avoid are:
> - runtime DXEs where we want to map code RO and data NX, which implies
> that the boundary between the two must be 64k aligned;
> - other EFI_MEMORY_RUNTIME regions that may be described with
> EFI_MEMORY_WC rather than EFI_MEMORY_WB; this could be related to
> implementations of the varstore or other runtime services, and the OS
> has no idea what the region is for and how the runtime code is using
> it;
> - reserved regions that are mapped, e.g., by AML code, as a
> SystemMemory OpRegion; there are examples in RAS code where the memory
> needs to be mapped uncached so that stores are immediately visible to
> the BMC or other agents that sit on the interconnect. The existence of
> a cacheable alias of the same mapping will interfere with this, i.e.,
> in many cases it will simply make both mappings cacheable.
> 
> 
>> Or are you saying the OS can just read the 64k page(s) containing these
>> 4k aligned EfiACPIReclaimMemory pages and then start reading at the
>> calculated offset from within the larger page, since these pages don't
>> have any pointers to outside memory, unlike image sections?
>>
> 
> In all these examples, the alignment *itself* is not the goal. The 64k
> alignment is way to ensure that the OS is never in a situation where a
> 64k page frame is shared between a cacheable and an uncacheable
> mapping, or between a ROX and a W+NX mapping. Note that, in the
> runtime DXE case, the only way to map the 64k page that has both code
> and data is to map it RWX, given that the contents need to appear
> contiguous in virtual memory (we had great fun with that when we
> designed the MAT)
> 
> For the other cases, there may be mappings that are completely
> disjoint in the virtual address space. But if two mappings exist of
> the same physical page, one cacheable via one alias, and one
> non-cacheable via another, the result is unpredictable and things like
> RAS or BMC comms from runtime firmware may be completely broken.
> 
> 

I see, thanks this was very helpful. I was focused on the alignment to
meet access (which as you say isn't really needed) and for image
sections, but the cacheable/uncacheable piece makes sense, too. I'll
send out the v2 for this patch.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115535): https://edk2.groups.io/g/devel/message/115535
Mute This Topic: https://groups.io/mt/104364784/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-