As part of the effort to get rid of ArmPlatformLib (which incorporates
far too many duties in a single library), introduce ArmVirtMemInfoLib
which will be invoked by our ArmVirtMemoryInitPeiLib implementation to
get a description of the virtual address space. This will allow us to
remove this functionality from ArmPlatformLib later, or, in the case of
ArmVirtXen and ArmVirtQemuKernel, drop ArmPlatformLib altogether.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirtPkg.dec                      |  3 ++
 ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h | 39 ++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index a8603e1b80e5..8f656fd2739d 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -30,6 +30,9 @@ [Defines]
 [Includes.common]
   Include                        # Root include for the package
 
+[LibraryClasses]
+  ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h
+
 [Guids.common]
   gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
   gEarlyPL011BaseAddressGuid       = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
new file mode 100644
index 000000000000..65be2cbd8082
--- /dev/null
+++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
@@ -0,0 +1,39 @@
+/** @file
+
+  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
+  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
+
+  This program and the accompanying materials are licensed and made available
+  under the terms and conditions of the BSD License which accompanies this
+  distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef _ARM_VIRT_MEMINFO_LIB_H_
+#define _ARM_VIRT_MEMINFO_LIB_H_
+
+#include <Base.h>
+#include <Library/ArmLib.h>
+
+/**
+  Return the Virtual Memory Map of your platform
+
+  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
+  on your platform.
+
+  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR
+                                    describing a Physical-to-Virtual Memory
+                                    mapping. This array must be ended by a
+                                    zero-filled entry
+
+**/
+VOID
+ArmVirtGetMemoryMap (
+  OUT ARM_MEMORY_REGION_DESCRIPTOR    **VirtualMemoryMap
+  );
+
+#endif
-- 
2.11.0

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

On 11/17/17 17:09, Ard Biesheuvel wrote:
> As part of the effort to get rid of ArmPlatformLib (which incorporates
> far too many duties in a single library), introduce ArmVirtMemInfoLib
> which will be invoked by our ArmVirtMemoryInitPeiLib implementation to
> get a description of the virtual address space. This will allow us to
> remove this functionality from ArmPlatformLib later, or, in the case of
> ArmVirtXen and ArmVirtQemuKernel, drop ArmPlatformLib altogether.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtPkg.dec                      |  3 ++
>  ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h | 39 ++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index a8603e1b80e5..8f656fd2739d 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ b/ArmVirtPkg/ArmVirtPkg.dec
> @@ -30,6 +30,9 @@ [Defines]
>  [Includes.common]
>    Include                        # Root include for the package
>  
> +[LibraryClasses]
> +  ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h
> +
>  [Guids.common]
>    gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
>    gEarlyPL011BaseAddressGuid       = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
> diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> new file mode 100644
> index 000000000000..65be2cbd8082
> --- /dev/null
> +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> @@ -0,0 +1,39 @@
> +/** @file
> +
> +  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef _ARM_VIRT_MEMINFO_LIB_H_
> +#define _ARM_VIRT_MEMINFO_LIB_H_
> +
> +#include <Base.h>
> +#include <Library/ArmLib.h>
> +
> +/**
> +  Return the Virtual Memory Map of your platform
> +
> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
> +  on your platform.
> +
> +  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR
> +                                    describing a Physical-to-Virtual Memory
> +                                    mapping. This array must be ended by a
> +                                    zero-filled entry
> +
> +**/
> +VOID
> +ArmVirtGetMemoryMap (
> +  OUT ARM_MEMORY_REGION_DESCRIPTOR    **VirtualMemoryMap
> +  );
> +
> +#endif
> 

(1) Since this is a library API, please add EFIAPI to the declaration.

(This will affect the instance(s) too.)


(2) If it's not overly restrictive, then please mention in the
"VirtualMemoryMap" param comment that the map is supposed to be
allocated dynamically within the function, using the phase-matching
MemoryAllocationLib instance.

(Judged from the AllocatePages() call in
"ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/QemuVirtMem.c".)


With those addressed,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

On 11/21/17 17:23, Laszlo Ersek wrote:
> On 11/17/17 17:09, Ard Biesheuvel wrote:
>> As part of the effort to get rid of ArmPlatformLib (which incorporates
>> far too many duties in a single library), introduce ArmVirtMemInfoLib
>> which will be invoked by our ArmVirtMemoryInitPeiLib implementation to
>> get a description of the virtual address space. This will allow us to
>> remove this functionality from ArmPlatformLib later, or, in the case of
>> ArmVirtXen and ArmVirtQemuKernel, drop ArmPlatformLib altogether.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/ArmVirtPkg.dec                      |  3 ++
>>  ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h | 39 ++++++++++++++++++++
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>> index a8603e1b80e5..8f656fd2739d 100644
>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>> @@ -30,6 +30,9 @@ [Defines]
>>  [Includes.common]
>>    Include                        # Root include for the package
>>  
>> +[LibraryClasses]
>> +  ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h
>> +
>>  [Guids.common]
>>    gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
>>    gEarlyPL011BaseAddressGuid       = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
>> diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>> new file mode 100644
>> index 000000000000..65be2cbd8082
>> --- /dev/null
>> +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>> @@ -0,0 +1,39 @@
>> +/** @file
>> +
>> +  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
>> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef _ARM_VIRT_MEMINFO_LIB_H_
>> +#define _ARM_VIRT_MEMINFO_LIB_H_
>> +
>> +#include <Base.h>
>> +#include <Library/ArmLib.h>
>> +
>> +/**
>> +  Return the Virtual Memory Map of your platform
>> +
>> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
>> +  on your platform.
>> +
>> +  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR
>> +                                    describing a Physical-to-Virtual Memory
>> +                                    mapping. This array must be ended by a
>> +                                    zero-filled entry
>> +
>> +**/
>> +VOID
>> +ArmVirtGetMemoryMap (
>> +  OUT ARM_MEMORY_REGION_DESCRIPTOR    **VirtualMemoryMap
>> +  );
>> +
>> +#endif
>>
> 
> (1) Since this is a library API, please add EFIAPI to the declaration.
> 
> (This will affect the instance(s) too.)
> 
> 
> (2) If it's not overly restrictive, then please mention in the
> "VirtualMemoryMap" param comment that the map is supposed to be
> allocated dynamically within the function, using the phase-matching
> MemoryAllocationLib instance.
> 
> (Judged from the AllocatePages() call in
> "ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/QemuVirtMem.c".)

Looking at the patch right after this one, dynamic memory allocation
appears wrong to spell out in the library interface.

Then I guess the right thing to say would be, "the returned array is
never supposed to be freed; it is released at the latest when the OS
takes control".

> With those addressed,
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

My R-b stands, just please clarify the expected lifetime of the array
returned, one way or another.

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

On 21 November 2017 at 16:27, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/21/17 17:23, Laszlo Ersek wrote:
>> On 11/17/17 17:09, Ard Biesheuvel wrote:
>>> As part of the effort to get rid of ArmPlatformLib (which incorporates
>>> far too many duties in a single library), introduce ArmVirtMemInfoLib
>>> which will be invoked by our ArmVirtMemoryInitPeiLib implementation to
>>> get a description of the virtual address space. This will allow us to
>>> remove this functionality from ArmPlatformLib later, or, in the case of
>>> ArmVirtXen and ArmVirtQemuKernel, drop ArmPlatformLib altogether.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  ArmVirtPkg/ArmVirtPkg.dec                      |  3 ++
>>>  ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h | 39 ++++++++++++++++++++
>>>  2 files changed, 42 insertions(+)
>>>
>>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>>> index a8603e1b80e5..8f656fd2739d 100644
>>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>>> @@ -30,6 +30,9 @@ [Defines]
>>>  [Includes.common]
>>>    Include                        # Root include for the package
>>>
>>> +[LibraryClasses]
>>> +  ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h
>>> +
>>>  [Guids.common]
>>>    gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
>>>    gEarlyPL011BaseAddressGuid       = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
>>> diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>>> new file mode 100644
>>> index 000000000000..65be2cbd8082
>>> --- /dev/null
>>> +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
>>> @@ -0,0 +1,39 @@
>>> +/** @file
>>> +
>>> +  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
>>> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>>> +
>>> +  This program and the accompanying materials are licensed and made available
>>> +  under the terms and conditions of the BSD License which accompanies this
>>> +  distribution.  The full text of the license may be found at
>>> +  http://opensource.org/licenses/bsd-license.php
>>> +
>>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>> +
>>> +**/
>>> +
>>> +#ifndef _ARM_VIRT_MEMINFO_LIB_H_
>>> +#define _ARM_VIRT_MEMINFO_LIB_H_
>>> +
>>> +#include <Base.h>
>>> +#include <Library/ArmLib.h>
>>> +
>>> +/**
>>> +  Return the Virtual Memory Map of your platform
>>> +
>>> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
>>> +  on your platform.
>>> +
>>> +  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR
>>> +                                    describing a Physical-to-Virtual Memory
>>> +                                    mapping. This array must be ended by a
>>> +                                    zero-filled entry
>>> +
>>> +**/
>>> +VOID
>>> +ArmVirtGetMemoryMap (
>>> +  OUT ARM_MEMORY_REGION_DESCRIPTOR    **VirtualMemoryMap
>>> +  );
>>> +
>>> +#endif
>>>
>>
>> (1) Since this is a library API, please add EFIAPI to the declaration.
>>
>> (This will affect the instance(s) too.)
>>
>>
>> (2) If it's not overly restrictive, then please mention in the
>> "VirtualMemoryMap" param comment that the map is supposed to be
>> allocated dynamically within the function, using the phase-matching
>> MemoryAllocationLib instance.
>>
>> (Judged from the AllocatePages() call in
>> "ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/QemuVirtMem.c".)
>
> Looking at the patch right after this one, dynamic memory allocation
> appears wrong to spell out in the library interface.
>
> Then I guess the right thing to say would be, "the returned array is
> never supposed to be freed; it is released at the latest when the OS
> takes control".
>
>> With those addressed,
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> My R-b stands, just please clarify the expected lifetime of the array
> returned, one way or another.
>

Thanks. Simply not freeing it is the current practice everywhere,
given that PrePi and PEI MemoryAllocationLib implementations don't do
FreePages() in the first place. But I agree it should be mentioned
explicitly.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel