Re: [edk2] [PATCH v3 2/4] ArmVirtPkg/ArmVirtPlatformLib: do not map executable NOR region as device

2015-11-19 Thread Heyi Guo 



On 11/19/2015 01:54 PM, Ard Biesheuvel wrote:

On 19 November 2015 at 00:55, Heyi Guo  wrote:


On 11/18/2015 04:25 PM, 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.

The MMU code deals correctly with overlapping ARM_MEMORY_REGION_DESCRIPTOR
entries, and later entries in the array take precedence over earlier ones.
So simply add an entry to the end of the array that overrides the mapping
attributes of the FD image, wherever it resides.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
   ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c | 10 --
   1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
index d5d288fb1b48..530f7d608e0b 100644
--- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
+++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
@@ -22,7 +22,7 @@
   #include 
 // 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_CACHEDARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
@@ -100,8 +100,14 @@ ArmPlatformGetVirtualMemoryMap (
 VirtualMemoryTable[2].Length   = ArmGetPhysAddrTop () -
VirtualMemoryTable[2].PhysicalBase;
 VirtualMemoryTable[2].Attributes   =
ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
   +  // Remap the FD region as normal executable memory
+  VirtualMemoryTable[3].PhysicalBase = FixedPcdGet64 (PcdFdBaseAddress);
+  VirtualMemoryTable[3].VirtualBase  =
VirtualMemoryTable[3].PhysicalBase;
+  VirtualMemoryTable[3].Length   = FixedPcdGet32 (PcdFdSize);
+  VirtualMemoryTable[3].Attributes   = CacheAttributes;
+
 // End of Table
-  ZeroMem ([3], sizeof
(ARM_MEMORY_REGION_DESCRIPTOR));
+  ZeroMem ([4], sizeof
(ARM_MEMORY_REGION_DESCRIPTOR));


Just a minor suggestion: is it better to use
MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS - 1 instead of 4 to avoid changing two
pieces of code when changing descriptor number?


The end of table entry should follow directly after the last valid
one, while the maximum number of entries could be higher (e.g., I
could have changed it to 8, since we will end up using a single page
anyway).

Got it. Thanks

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v3 2/4] ArmVirtPkg/ArmVirtPlatformLib: do not map executable NOR region as device

2015-11-18 Thread Ard Biesheuvel 
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.

The MMU code deals correctly with overlapping ARM_MEMORY_REGION_DESCRIPTOR
entries, and later entries in the array take precedence over earlier ones.
So simply add an entry to the end of the array that overrides the mapping
attributes of the FD image, wherever it resides.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c 
b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
index d5d288fb1b48..530f7d608e0b 100644
--- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
+++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
@@ -22,7 +22,7 @@
 #include 
 
 // 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_CACHEDARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
@@ -100,8 +100,14 @@ ArmPlatformGetVirtualMemoryMap (
   VirtualMemoryTable[2].Length   = ArmGetPhysAddrTop () - 
VirtualMemoryTable[2].PhysicalBase;
   VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
+  // Remap the FD region as normal executable memory
+  VirtualMemoryTable[3].PhysicalBase = FixedPcdGet64 (PcdFdBaseAddress);
+  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
+  VirtualMemoryTable[3].Length   = FixedPcdGet32 (PcdFdSize);
+  VirtualMemoryTable[3].Attributes   = CacheAttributes;
+
   // End of Table
-  ZeroMem ([3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
+  ZeroMem ([4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
 
   *VirtualMemoryMap = VirtualMemoryTable;
 }
-- 
1.9.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 2/4] ArmVirtPkg/ArmVirtPlatformLib: do not map executable NOR region as device

2015-11-18 Thread Laszlo Ersek 
On 11/18/15 09:25, 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.
> 
> The MMU code deals correctly with overlapping ARM_MEMORY_REGION_DESCRIPTOR
> entries, and later entries in the array take precedence over earlier ones.
> So simply add an entry to the end of the array that overrides the mapping
> attributes of the FD image, wherever it resides.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c 
> b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
> index d5d288fb1b48..530f7d608e0b 100644
> --- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
> +++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
> @@ -22,7 +22,7 @@
>  #include 
>  
>  // 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_CACHEDARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
> @@ -100,8 +100,14 @@ ArmPlatformGetVirtualMemoryMap (
>VirtualMemoryTable[2].Length   = ArmGetPhysAddrTop () - 
> VirtualMemoryTable[2].PhysicalBase;
>VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>  
> +  // Remap the FD region as normal executable memory
> +  VirtualMemoryTable[3].PhysicalBase = FixedPcdGet64 (PcdFdBaseAddress);
> +  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
> +  VirtualMemoryTable[3].Length   = FixedPcdGet32 (PcdFdSize);
> +  VirtualMemoryTable[3].Attributes   = CacheAttributes;
> +
>// End of Table
> -  ZeroMem ([3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
> +  ZeroMem ([4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>  
>*VirtualMemoryMap = VirtualMemoryTable;
>  }
> 

Thanks, this update is very welcome.

Please don't forget to test it on phys hw / KVM. (If you haven't done so
yet.)

Reviewed-by: Laszlo Ersek 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 2/4] ArmVirtPkg/ArmVirtPlatformLib: do not map executable NOR region as device

2015-11-18 Thread Ard Biesheuvel 
On 18 November 2015 at 10:02, Laszlo Ersek  wrote:
> On 11/18/15 09:25, 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.
>>
>> The MMU code deals correctly with overlapping ARM_MEMORY_REGION_DESCRIPTOR
>> entries, and later entries in the array take precedence over earlier ones.
>> So simply add an entry to the end of the array that overrides the mapping
>> attributes of the FD image, wherever it resides.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c | 10 --
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c 
>> b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> index d5d288fb1b48..530f7d608e0b 100644
>> --- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> +++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> @@ -22,7 +22,7 @@
>>  #include 
>>
>>  // 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_CACHEDARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
>> @@ -100,8 +100,14 @@ ArmPlatformGetVirtualMemoryMap (
>>VirtualMemoryTable[2].Length   = ArmGetPhysAddrTop () - 
>> VirtualMemoryTable[2].PhysicalBase;
>>VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>
>> +  // Remap the FD region as normal executable memory
>> +  VirtualMemoryTable[3].PhysicalBase = FixedPcdGet64 (PcdFdBaseAddress);
>> +  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
>> +  VirtualMemoryTable[3].Length   = FixedPcdGet32 (PcdFdSize);
>> +  VirtualMemoryTable[3].Attributes   = CacheAttributes;
>> +
>>// End of Table
>> -  ZeroMem ([3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>> +  ZeroMem ([4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>>
>>*VirtualMemoryMap = VirtualMemoryTable;
>>  }
>>
>
> Thanks, this update is very welcome.
>
> Please don't forget to test it on phys hw / KVM. (If you haven't done so
> yet.)
>

Yes, both AARCH64 and ARM still work fine under KVM on my Seattle Overdrive

> Reviewed-by: Laszlo Ersek 

Thanks
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 2/4] ArmVirtPkg/ArmVirtPlatformLib: do not map executable NOR region as device

2015-11-18 Thread Heyi Guo 



On 11/18/2015 04:25 PM, 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.

The MMU code deals correctly with overlapping ARM_MEMORY_REGION_DESCRIPTOR
entries, and later entries in the array take precedence over earlier ones.
So simply add an entry to the end of the array that overrides the mapping
attributes of the FD image, wherever it resides.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
  ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c 
b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
index d5d288fb1b48..530f7d608e0b 100644
--- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
+++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
@@ -22,7 +22,7 @@
  #include 
  
  // 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_CACHEDARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
@@ -100,8 +100,14 @@ ArmPlatformGetVirtualMemoryMap (
VirtualMemoryTable[2].Length   = ArmGetPhysAddrTop () - 
VirtualMemoryTable[2].PhysicalBase;
VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
  
+  // Remap the FD region as normal executable memory

+  VirtualMemoryTable[3].PhysicalBase = FixedPcdGet64 (PcdFdBaseAddress);
+  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
+  VirtualMemoryTable[3].Length   = FixedPcdGet32 (PcdFdSize);
+  VirtualMemoryTable[3].Attributes   = CacheAttributes;
+
// End of Table
-  ZeroMem ([3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
+  ZeroMem ([4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));


Just a minor suggestion: is it better to use 
MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS - 1 instead of 4 to avoid changing 
two pieces of code when changing descriptor number?


Heyi

  
*VirtualMemoryMap = VirtualMemoryTable;

  }


___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 2/4] ArmVirtPkg/ArmVirtPlatformLib: do not map executable NOR region as device

2015-11-18 Thread Ard Biesheuvel 
On 19 November 2015 at 00:55, Heyi Guo  wrote:
>
>
> On 11/18/2015 04:25 PM, 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.
>>
>> The MMU code deals correctly with overlapping ARM_MEMORY_REGION_DESCRIPTOR
>> entries, and later entries in the array take precedence over earlier ones.
>> So simply add an entry to the end of the array that overrides the mapping
>> attributes of the FD image, wherever it resides.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>   ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c | 10 --
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> index d5d288fb1b48..530f7d608e0b 100644
>> --- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> +++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> @@ -22,7 +22,7 @@
>>   #include 
>> // 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_CACHEDARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
>> @@ -100,8 +100,14 @@ ArmPlatformGetVirtualMemoryMap (
>> VirtualMemoryTable[2].Length   = ArmGetPhysAddrTop () -
>> VirtualMemoryTable[2].PhysicalBase;
>> VirtualMemoryTable[2].Attributes   =
>> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>   +  // Remap the FD region as normal executable memory
>> +  VirtualMemoryTable[3].PhysicalBase = FixedPcdGet64 (PcdFdBaseAddress);
>> +  VirtualMemoryTable[3].VirtualBase  =
>> VirtualMemoryTable[3].PhysicalBase;
>> +  VirtualMemoryTable[3].Length   = FixedPcdGet32 (PcdFdSize);
>> +  VirtualMemoryTable[3].Attributes   = CacheAttributes;
>> +
>> // End of Table
>> -  ZeroMem ([3], sizeof
>> (ARM_MEMORY_REGION_DESCRIPTOR));
>> +  ZeroMem ([4], sizeof
>> (ARM_MEMORY_REGION_DESCRIPTOR));
>
>
> Just a minor suggestion: is it better to use
> MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS - 1 instead of 4 to avoid changing two
> pieces of code when changing descriptor number?
>

The end of table entry should follow directly after the last valid
one, while the maximum number of entries could be higher (e.g., I
could have changed it to 8, since we will end up using a single page
anyway).
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel