| Message ID | 20171117160913.17292-11-ard.biesheuvel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ArmVirtPkg: get rid of ArmPlatformLib | expand |
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
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
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(+) -- 2.11.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel