On 13 November 2015 at 17:55, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
> On 13 November 2015 at 17:40, Laszlo Ersek <ler...@redhat.com> wrote:
>> On 11/13/15 15:55, Ard Biesheuvel wrote:
>>> The ARM architecture version 7 and later mandates that device mappings
>>> have the XN (non-executable) bit set, to prevent speculative instruction
>>> fetches from read-sensitive regions. This implies that we should not map
>>> regions as device if we want to execute from them, so the NOR region that
>>> contains our FD image should be mapped as normal memory instead.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>> ---
>>> ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c | 58 ++++++++++++++++----
>>> 1 file changed, 47 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>>> b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>>> index d5d288fb1b48..f5198fff9402 100644
>>> --- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>>> +++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>>> @@ -22,7 +22,7 @@
>>> #include <ArmPlatform.h>
>>>
>>> // Number of Virtual Memory Map Descriptors
>>> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 4
>>> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 6
>>>
>>> // DDR attributes
>>> #define DDR_ATTRIBUTES_CACHED ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
>>> @@ -52,6 +52,7 @@ ArmPlatformGetVirtualMemoryMap (
>>> {
>>> ARM_MEMORY_REGION_ATTRIBUTES CacheAttributes;
>>> ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
>>> + UINTN Index;
>>>
>>> ASSERT (VirtualMemoryMap != NULL);
>>>
>>> @@ -88,20 +89,55 @@ ArmPlatformGetVirtualMemoryMap (
>>> VirtualMemoryTable[0].VirtualBase,
>>> VirtualMemoryTable[0].Length));
>>>
>>> - // Peripheral space before DRAM
>>> - VirtualMemoryTable[1].PhysicalBase = 0x0;
>>> - VirtualMemoryTable[1].VirtualBase = 0x0;
>>> - VirtualMemoryTable[1].Length = VirtualMemoryTable[0].PhysicalBase;
>>> - VirtualMemoryTable[1].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>> + //
>>> + // We cannot map the NOR flash that contains the FD image as a device,
>>> since
>>> + // that may imply XN on v7 and later, and we may still be executing from
>>> it at
>>> + // this point. So explicitly map the FD as normal memory if it does not
>>> + // intersect with system memory.
>>> + //
>>> +
>>> + // Disregard the case where the FD sits above DRAM, to keep the logic
>>> simple.
>>> + ASSERT (FixedPcdGet64(PcdFdBaseAddress) + FixedPcdGet32(PcdFdSize) <=
>>> + PcdGet64 (PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize));
>>> +
>>> + Index = 0;
>>> + if (FixedPcdGet64(PcdFdBaseAddress) >= PcdGet64 (PcdSystemMemoryBase)) {
>>> + // FD is loaded in DRAM, map everything below DRAM as device
>>> + VirtualMemoryTable[++Index].PhysicalBase = 0x0;
>>> + VirtualMemoryTable[Index].VirtualBase = 0x0;
>>> + VirtualMemoryTable[Index].Length = PcdGet64
>>> (PcdSystemMemoryBase);
>>> + VirtualMemoryTable[Index].Attributes =
>>> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>> + } else {
>>> + // FD is loaded below DRAM, map everything except the FD as device
>>> + if (FixedPcdGet64(PcdFdBaseAddress) > 0) {
>>> + // Peripheral space before flash device
>>> + VirtualMemoryTable[++Index].PhysicalBase = 0x0;
>>> + VirtualMemoryTable[Index].VirtualBase = 0x0;
>>> + VirtualMemoryTable[Index].Length =
>>> FixedPcdGet64(PcdFdBaseAddress);
>>> + VirtualMemoryTable[Index].Attributes =
>>> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>> + }
>>> +
>>> + // Map the FD as normal memory
>>> + VirtualMemoryTable[++Index].PhysicalBase =
>>> FixedPcdGet64(PcdFdBaseAddress);
>>> + VirtualMemoryTable[Index].VirtualBase =
>>> VirtualMemoryTable[Index].PhysicalBase;
>>> + VirtualMemoryTable[Index].Length = FixedPcdGet32(PcdFdSize);
>>> + VirtualMemoryTable[Index].Attributes = CacheAttributes;
>>> +
>>> + // Peripheral space between flash device and DRAM
>>> + VirtualMemoryTable[++Index].PhysicalBase =
>>> FixedPcdGet64(PcdFdBaseAddress) + FixedPcdGet32(PcdFdSize);
>>> + VirtualMemoryTable[Index].VirtualBase =
>>> VirtualMemoryTable[Index].PhysicalBase;
>>> + VirtualMemoryTable[Index].Length = PcdGet64
>>> (PcdSystemMemoryBase) - VirtualMemoryTable[Index].PhysicalBase;
>>> + VirtualMemoryTable[Index].Attributes =
>>> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>> + }
>>>
>>> // Peripheral space after DRAM
>>> - VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length +
>>> VirtualMemoryTable[1].Length;
>>> - VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase;
>>> - VirtualMemoryTable[2].Length = ArmGetPhysAddrTop () -
>>> VirtualMemoryTable[2].PhysicalBase;
>>> - VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>> + VirtualMemoryTable[++Index].PhysicalBase = PcdGet64
>>> (PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize);
>>> + VirtualMemoryTable[Index].VirtualBase =
>>> VirtualMemoryTable[Index].PhysicalBase;
>>> + VirtualMemoryTable[Index].Length = ArmGetPhysAddrTop () -
>>> VirtualMemoryTable[Index].PhysicalBase;
>>> + VirtualMemoryTable[Index].Attributes =
>>> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>>
>>> // End of Table
>>> - ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>>> + ZeroMem (&VirtualMemoryTable[++Index], sizeof
>>> (ARM_MEMORY_REGION_DESCRIPTOR));
>>>
>>> *VirtualMemoryMap = VirtualMemoryTable;
>>> }
>>>
>>
>> (1) I don't think that the flash chip we're executing from is
>> read-sensitive.
>
>
> No, it is not. The only potential problem that *could* happen is that
> an instruction fetch occurs when the NOR is in command mode, reading
> incorrect data. However, the only time the NOR enters command mode is
> during the DXE phase, when we are not executing from NOR anymore
> anyway.
>
>> So I don't think this patch is strictly necessary. Do I
>> understand right that you are doing this only because the last patch
>> will set XN indiscriminately?
>>
>
> Yes. I am reluctant to add a 'executable device' type, because it does
> not make sense. The NOR should not be mapped executable while it is in
> command mode (because of the issue mentioned above), so we have to
> choose between memory and device. Note that in the next patch, I just
> map the whole NOR as memory, but that is on a static platform.
>
>> (2) Anyway, I don't "resist" this patch, but: have you tested guests
>> built with this patch on phys hw / KVM?
>>
>
> I am doing so at the moment, and I can confirm that KVM 32-bit host is
> still broken :-)
> But seriously, I will make sure it is tested thoroughly before I commit it.
>
>> (3) New code added by the patch is inconsistent in its use of "space
>> before opening paren". Please stick with one space in function calls and
>> macro applications.
>>
>
> Sure, I will fix that.
>
>> (4) The comment style isn't perfect either, but I guess it conforms to
>> the prior code.
>>
>> (5) In the branch that starts with "FD is loaded below DRAM, map
>> everything except the FD as device", you don't assert that the end of
>> the FD range does not hang over into system RAM:
>>
>> ASSERT (FdBase + FdSize <= SystemMemoryBase);
>>
>> I think this should be asserted; otherwise the entry under "Peripheral
>> space between flash device and DRAM" will have negative (I guess:
>> wrapped around) length.
>>
>> (If, on the other hand, we can already be sure that partial overlapping
>> is not possible, then the ASSERT() that *is* being added could be
>> simplified -- you can then compare the end-of-FD against base-of-DRAM,
>> rather than the currently visible end-of-DRAM.)
>>
>
> Partial overlapping would imply that the image is *built* that way,
> and that part of it is loaded at the base of DRAM by an earlier stage,
> and the other part is executed in place. That seems highly unlikely to
> me. However, the FD could be entirely in DRAM, in which case we don't
> have to do anything special.
>
> However, it probably makes sense to identify those cases more clearly.
>
>> (6) Right after "FD is loaded below DRAM, map everything except the FD
>> as device", you make sure that "Peripheral space before flash device"
>> doesn't get added as an entry if its size would be zero. That's fine.
>>
>> However, there are other possibilities for ending up with zero-length
>> entries. For example (actually, I think this is the only one example),
>> if the end of the FD falls exactly at the beginning of DRAM, then
>> "Peripheral space between flash device and DRAM" will have zero length.
>>
>> If that's a problem, then it should be protected against; whereas if
>> it's fine, then the currently proposed check against a zero-sized
>> "Peripheral space before flash device" is superfluous.
>>
>
> Actually, the MMU code handles zero size sections fine, but it is
> better to be explicit. The symmetry probably makes the flow easier to
> understand as well, so I will add the zero size test for the region
> between FD and DRAM.
>
After having another look. I think I can simplify this code
substantially by simply doing the following
--- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
+++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
@@ -22,7 +22,7 @@
#include <ArmPlatform.h>
// Number of Virtual Memory Map Descriptors
-#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 4
+#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 5
// DDR attributes
#define DDR_ATTRIBUTES_CACHED ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
@@ -100,8 +100,13 @@ ArmPlatformGetVirtualMemoryMap (
VirtualMemoryTable[2].Length = ArmGetPhysAddrTop () -
VirtualMemoryTable[2].PhysicalBase;
VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+ VirtualMemoryTable[3].PhysicalBase = FixedPcdGet64 (PcdFdBaseAddress);
+ VirtualMemoryTable[3].VirtualBase = VirtualMemoryTable[3].PhysicalBase;
+ VirtualMemoryTable[3].Length = FixedPcdGet32(PcdFdSize);
+ VirtualMemoryTable[3].Attributes = CacheAttributes;
+
// End of Table
- ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
+ ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
*VirtualMemoryMap = VirtualMemoryTable;
}
We can do this since the MMU code can deal with overlapping entries in
the array. So we can simply override the memory attributes for the FD
region, regardless of where it resides
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
