diff mbox series

[edk2,4/5] ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD space

Message ID 20181123121431.22353-5-ard.biesheuvel@linaro.org
State New
Headers show
Series ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit | expand

Commit Message

Ard Biesheuvel Nov. 23, 2018, 12:14 p.m. UTC 
Move the call to BuildCpuHob () into ArmVirtMemoryInitPeiLib (which is
shared between all the ArmVirtPkg targets), and drop the inclusion of
CpuPei.inf [which calls it on ArmVirtQemu] and the BuildCpuHob () call
from ArmVirtPrePiUniCoreRelocatable [for the other targets].

This makes the size of the GCD address space and page table mappings
depend only on the size of the physical address space as exposed by
the CPU system registers.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmVirtPkg/ArmVirtQemu.dsc                                             | 1 -
 ArmVirtPkg/ArmVirtQemu.fdf                                             | 1 -
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c   | 3 +++
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf | 1 +
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c             | 5 +----
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf           | 1 -
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf        | 1 -
 ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf                    | 3 ---
 ArmVirtPkg/PrePi/PrePi.c                                               | 3 ---
 9 files changed, 5 insertions(+), 14 deletions(-)

-- 
2.17.1

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

Comments

Laszlo Ersek Nov. 26, 2018, 10:47 a.m. UTC | #1 
On 11/23/18 13:14, Ard Biesheuvel wrote:
> Move the call to BuildCpuHob () into ArmVirtMemoryInitPeiLib (which is

> shared between all the ArmVirtPkg targets), and drop the inclusion of

> CpuPei.inf [which calls it on ArmVirtQemu] and the BuildCpuHob () call

> from ArmVirtPrePiUniCoreRelocatable [for the other targets].


(1) Can you please confirm, in the commit message, that we don't need --
or that we don't exercise anyway -- the other things that
"ArmPkg/Drivers/CpuPei/CpuPei.inf" does, namely:

- ArmEnableBranchPrediction()
- building gArmMpCoreInfoGuid

?

>

> This makes the size of the GCD address space and page table mappings

> depend only on the size of the physical address space as exposed by

> the CPU system registers.

>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/ArmVirtQemu.dsc                                             | 1 -

>  ArmVirtPkg/ArmVirtQemu.fdf                                             | 1 -

>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c   | 3 +++

>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf | 1 +

>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c             | 5 +----

>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf           | 1 -

>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf        | 1 -

>  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf                    | 3 ---

>  ArmVirtPkg/PrePi/PrePi.c                                               | 3 ---

>  9 files changed, 5 insertions(+), 14 deletions(-)

>

> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc

> index 885c6b14b844..cb59c790afcc 100644

> --- a/ArmVirtPkg/ArmVirtQemu.dsc

> +++ b/ArmVirtPkg/ArmVirtQemu.dsc

> @@ -226,7 +226,6 @@

>    }

>    ArmPlatformPkg/PlatformPei/PlatformPeim.inf

>    ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf

> -  ArmPkg/Drivers/CpuPei/CpuPei.inf

>

>    MdeModulePkg/Universal/Variable/Pei/VariablePei.inf

>

> diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf

> index c6a22dc018f3..12bc570c4cb3 100644

> --- a/ArmVirtPkg/ArmVirtQemu.fdf

> +++ b/ArmVirtPkg/ArmVirtQemu.fdf

> @@ -106,7 +106,6 @@ READ_LOCK_STATUS   = TRUE

>    INF MdeModulePkg/Core/Pei/PeiMain.inf

>    INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf

>    INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf

> -  INF ArmPkg/Drivers/CpuPei/CpuPei.inf

>    INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf

>    INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf

>    INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf


OK, so this removes the previous CPU HOB building in ArmVirtQemu.

Quoting out of order:

> diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf

> index 1587bd92f206..e04bd1b7c497 100755

> --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf

> +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf

> @@ -85,9 +85,6 @@

>

>    gArmPlatformTokenSpaceGuid.PcdCoreCount

>

> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize

> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize

> -

>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory

>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS

>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType

> diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c

> index f6abe2f2016b..ecaaac1545c4 100755

> --- a/ArmVirtPkg/PrePi/PrePi.c

> +++ b/ArmVirtPkg/PrePi/PrePi.c

> @@ -79,9 +79,6 @@ PrePiMain (

>    StacksSize = PcdGet32 (PcdCPUCorePrimaryStackSize);

>    BuildStackHob (StacksBase, StacksSize);

>

> -  //TODO: Call CpuPei as a library

> -  BuildCpuHob (PcdGet8 (PcdPrePiCpuMemorySize), PcdGet8 (PcdPrePiCpuIoSize));

> -


Best way to solve a TODO is to delete it. :)

>    // Set the Boot Mode

>    SetBootMode (BOOT_WITH_FULL_CONFIGURATION);

>

>


OK, this removes the previous CPU HOB building in ArmVirtQemuKernel and
ArmVirtXen.


> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c

> index 3f0e9b3a0579..3d86d31ab50e 100644

> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c

> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c

> @@ -116,5 +116,8 @@ MemoryPeim (

>      BuildMemoryTypeInformationHob ();

>    }

>

> +  // Publish the CPU memory and io spaces sizes

> +  BuildCpuHob (ArmGetPhysicalAddressBits (), FixedPcdGet8 (PcdPrePiCpuIoSize));

> +

>    return EFI_SUCCESS;

>  }


Argh, it took me a while to see that this will indeed supplement the CPU
HOB on all ArmVirtPlatforms. The build report file(s) helped. I didn't
think of ArmPlatformPkg originally. :)

So, yes, this is fine; it's OK to produce the CPU HOB anywhere in the
PEI phase.

> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf

> index 54879d590a8a..d0e39df84b20 100644

> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf

> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf

> @@ -59,6 +59,7 @@

>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData

>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode

>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData

> +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize

>

>  [Pcd]

>    gArmTokenSpaceGuid.PcdSystemMemoryBase


OK, expected.

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> index 4eca9b895354..ded87d604f4f 100644

> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> @@ -46,7 +46,6 @@ ArmVirtGetMemoryMap (

>    )

>  {

>    ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;

> -  UINT64                        TopOfMemory;

>

>    ASSERT (VirtualMemoryMap != NULL);

>

> @@ -80,11 +79,9 @@ ArmVirtGetMemoryMap (

>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

>

>    // Peripheral space after DRAM

> -  TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),

> -                     TopOfAddressSpace);

>    VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;

>    VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

> -  VirtualMemoryTable[2].Length       = TopOfMemory -

> +  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

>                                         VirtualMemoryTable[2].PhysicalBase;

>    VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

>

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf

> index f2c461e3b55a..5c5b841051ad 100644

> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf

> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf

> @@ -45,4 +45,3 @@

>

>  [FixedPcd]

>    gArmTokenSpaceGuid.PcdFdSize

> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf

> index f54fb51ee1d4..d12089760b22 100644

> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf

> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf

> @@ -49,4 +49,3 @@

>  [FixedPcd]

>    gArmTokenSpaceGuid.PcdFdSize

>    gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress

> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize


This last part, for QemuVirtMemInfoLib, is functionally valid, in my
opinion. We are eliminating the clamping to PcdPrePiCpuMemorySize, which
was one of my earlier points, so what remains is "TopOfMemory =
TopOfAddressSpace"; good.

However: this change does not relate to the GCD memory space. The
ArmVirtGetMemoryMap() function partakes in the MMU configuration / page
table setup. Its leading comment says,

  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
  on your platform.

(2) Therefore, it should be split to a separate patch. Earlier, I
suggested a slight restructuring for the patch series, so let me refine
that:

- new patch #1: current patch #1 (introduce helper)

- new patch #2: current patch #3 (refactor ArmVirtPkg)

- new patch #3: current patch #2 (consider CPU capabilities in generic
                MMU setup code)

- new patch #4: this specific hunk, from current patch #4 (consider CPU
                capabilities in specific MMU setup code)

- new patch #5: the rest of current patch #4 (consider CPU caps in GCD /
                CPU HOB)

- new patch #6: kill PcdPrePiCpuMemorySize in ArmVirtPkg DSC files

Does this sound acceptable?

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 26, 2018, 11:59 a.m. UTC | #2 
On Mon, 26 Nov 2018 at 11:47, Laszlo Ersek <lersek@redhat.com> wrote:
>

> On 11/23/18 13:14, Ard Biesheuvel wrote:

> > Move the call to BuildCpuHob () into ArmVirtMemoryInitPeiLib (which is

> > shared between all the ArmVirtPkg targets), and drop the inclusion of

> > CpuPei.inf [which calls it on ArmVirtQemu] and the BuildCpuHob () call

> > from ArmVirtPrePiUniCoreRelocatable [for the other targets].

>

> (1) Can you please confirm, in the commit message, that we don't need --

> or that we don't exercise anyway -- the other things that

> "ArmPkg/Drivers/CpuPei/CpuPei.inf" does, namely:

>

> - ArmEnableBranchPrediction()


This does nothing on AArch64, and even on ARM, this is something that
is enabled by the bare metal boot code, not per-guest.

> - building gArmMpCoreInfoGuid

>


That is for the MPCore versions of PrePi and PrePeiCore, and we only
use UniCore versions in ArmVirtPkg

I will add some clarification to the commit log.

> >

> > This makes the size of the GCD address space and page table mappings

> > depend only on the size of the physical address space as exposed by

> > the CPU system registers.

> >

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> >  ArmVirtPkg/ArmVirtQemu.dsc                                             | 1 -

> >  ArmVirtPkg/ArmVirtQemu.fdf                                             | 1 -

> >  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c   | 3 +++

> >  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf | 1 +

> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c             | 5 +----

> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf           | 1 -

> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf        | 1 -

> >  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf                    | 3 ---

> >  ArmVirtPkg/PrePi/PrePi.c                                               | 3 ---

> >  9 files changed, 5 insertions(+), 14 deletions(-)

> >

> > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc

> > index 885c6b14b844..cb59c790afcc 100644

> > --- a/ArmVirtPkg/ArmVirtQemu.dsc

> > +++ b/ArmVirtPkg/ArmVirtQemu.dsc

> > @@ -226,7 +226,6 @@

> >    }

> >    ArmPlatformPkg/PlatformPei/PlatformPeim.inf

> >    ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf

> > -  ArmPkg/Drivers/CpuPei/CpuPei.inf

> >

> >    MdeModulePkg/Universal/Variable/Pei/VariablePei.inf

> >

> > diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf

> > index c6a22dc018f3..12bc570c4cb3 100644

> > --- a/ArmVirtPkg/ArmVirtQemu.fdf

> > +++ b/ArmVirtPkg/ArmVirtQemu.fdf

> > @@ -106,7 +106,6 @@ READ_LOCK_STATUS   = TRUE

> >    INF MdeModulePkg/Core/Pei/PeiMain.inf

> >    INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf

> >    INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf

> > -  INF ArmPkg/Drivers/CpuPei/CpuPei.inf

> >    INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf

> >    INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf

> >    INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf

>

> OK, so this removes the previous CPU HOB building in ArmVirtQemu.

>

> Quoting out of order:

>

> > diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf

> > index 1587bd92f206..e04bd1b7c497 100755

> > --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf

> > +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf

> > @@ -85,9 +85,6 @@

> >

> >    gArmPlatformTokenSpaceGuid.PcdCoreCount

> >

> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize

> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize

> > -

> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory

> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS

> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType

> > diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c

> > index f6abe2f2016b..ecaaac1545c4 100755

> > --- a/ArmVirtPkg/PrePi/PrePi.c

> > +++ b/ArmVirtPkg/PrePi/PrePi.c

> > @@ -79,9 +79,6 @@ PrePiMain (

> >    StacksSize = PcdGet32 (PcdCPUCorePrimaryStackSize);

> >    BuildStackHob (StacksBase, StacksSize);

> >

> > -  //TODO: Call CpuPei as a library

> > -  BuildCpuHob (PcdGet8 (PcdPrePiCpuMemorySize), PcdGet8 (PcdPrePiCpuIoSize));

> > -

>

> Best way to solve a TODO is to delete it. :)

>

> >    // Set the Boot Mode

> >    SetBootMode (BOOT_WITH_FULL_CONFIGURATION);

> >

> >

>

> OK, this removes the previous CPU HOB building in ArmVirtQemuKernel and

> ArmVirtXen.

>

>

> > diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c

> > index 3f0e9b3a0579..3d86d31ab50e 100644

> > --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c

> > +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c

> > @@ -116,5 +116,8 @@ MemoryPeim (

> >      BuildMemoryTypeInformationHob ();

> >    }

> >

> > +  // Publish the CPU memory and io spaces sizes

> > +  BuildCpuHob (ArmGetPhysicalAddressBits (), FixedPcdGet8 (PcdPrePiCpuIoSize));

> > +

> >    return EFI_SUCCESS;

> >  }

>

> Argh, it took me a while to see that this will indeed supplement the CPU

> HOB on all ArmVirtPlatforms. The build report file(s) helped. I didn't

> think of ArmPlatformPkg originally. :)

>

> So, yes, this is fine; it's OK to produce the CPU HOB anywhere in the

> PEI phase.

>

> > diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf

> > index 54879d590a8a..d0e39df84b20 100644

> > --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf

> > +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf

> > @@ -59,6 +59,7 @@

> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData

> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode

> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData

> > +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize

> >

> >  [Pcd]

> >    gArmTokenSpaceGuid.PcdSystemMemoryBase

>

> OK, expected.

>

> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> > index 4eca9b895354..ded87d604f4f 100644

> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> > @@ -46,7 +46,6 @@ ArmVirtGetMemoryMap (

> >    )

> >  {

> >    ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;

> > -  UINT64                        TopOfMemory;

> >

> >    ASSERT (VirtualMemoryMap != NULL);

> >

> > @@ -80,11 +79,9 @@ ArmVirtGetMemoryMap (

> >    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

> >

> >    // Peripheral space after DRAM

> > -  TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),

> > -                     TopOfAddressSpace);

> >    VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;

> >    VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

> > -  VirtualMemoryTable[2].Length       = TopOfMemory -

> > +  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

> >                                         VirtualMemoryTable[2].PhysicalBase;

> >    VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

> >

> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf

> > index f2c461e3b55a..5c5b841051ad 100644

> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf

> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf

> > @@ -45,4 +45,3 @@

> >

> >  [FixedPcd]

> >    gArmTokenSpaceGuid.PcdFdSize

> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize

> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf

> > index f54fb51ee1d4..d12089760b22 100644

> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf

> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf

> > @@ -49,4 +49,3 @@

> >  [FixedPcd]

> >    gArmTokenSpaceGuid.PcdFdSize

> >    gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress

> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize

>

> This last part, for QemuVirtMemInfoLib, is functionally valid, in my

> opinion. We are eliminating the clamping to PcdPrePiCpuMemorySize, which

> was one of my earlier points, so what remains is "TopOfMemory =

> TopOfAddressSpace"; good.

>


Now, I'm not so sure anymore whether this hunk is correct.

This describes the extent of DRAM to the generic MMU code, and we only
want to map what
a) the CPU can support and
b) what we actually describe as being populated by anything

So if your guest supports 52 bits of PA space in hardware, we want the
GCD space to accommodate that (so we can report memory as residing
there). However, we cannot map all of that memory in UEFI anyway, and
so capping the mapping (this hunk) to the PcdPrePiCpuMemorySize size
actually makes sense.


> However: this change does not relate to the GCD memory space. The

> ArmVirtGetMemoryMap() function partakes in the MMU configuration / page

> table setup. Its leading comment says,

>

>   This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU

>   on your platform.

>

> (2) Therefore, it should be split to a separate patch. Earlier, I

> suggested a slight restructuring for the patch series, so let me refine

> that:

>

> - new patch #1: current patch #1 (introduce helper)

>

> - new patch #2: current patch #3 (refactor ArmVirtPkg)

>

> - new patch #3: current patch #2 (consider CPU capabilities in generic

>                 MMU setup code)

>

> - new patch #4: this specific hunk, from current patch #4 (consider CPU

>                 capabilities in specific MMU setup code)

>

> - new patch #5: the rest of current patch #4 (consider CPU caps in GCD /

>                 CPU HOB)

>

> - new patch #6: kill PcdPrePiCpuMemorySize in ArmVirtPkg DSC files

>

> Does this sound acceptable?

>


Works for me.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 885c6b14b844..cb59c790afcc 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -226,7 +226,6 @@ 
   }
   ArmPlatformPkg/PlatformPei/PlatformPeim.inf
   ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
-  ArmPkg/Drivers/CpuPei/CpuPei.inf
 
   MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
 
diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
index c6a22dc018f3..12bc570c4cb3 100644
--- a/ArmVirtPkg/ArmVirtQemu.fdf
+++ b/ArmVirtPkg/ArmVirtQemu.fdf
@@ -106,7 +106,6 @@  READ_LOCK_STATUS   = TRUE
   INF MdeModulePkg/Core/Pei/PeiMain.inf
   INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
   INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
-  INF ArmPkg/Drivers/CpuPei/CpuPei.inf
   INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
   INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
   INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
index 3f0e9b3a0579..3d86d31ab50e 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
@@ -116,5 +116,8 @@  MemoryPeim (
     BuildMemoryTypeInformationHob ();
   }
 
+  // Publish the CPU memory and io spaces sizes
+  BuildCpuHob (ArmGetPhysicalAddressBits (), FixedPcdGet8 (PcdPrePiCpuIoSize));
+
   return EFI_SUCCESS;
 }
diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
index 54879d590a8a..d0e39df84b20 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
@@ -59,6 +59,7 @@ 
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData
+  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
 
 [Pcd]
   gArmTokenSpaceGuid.PcdSystemMemoryBase
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index 4eca9b895354..ded87d604f4f 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -46,7 +46,6 @@  ArmVirtGetMemoryMap (
   )
 {
   ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
-  UINT64                        TopOfMemory;
 
   ASSERT (VirtualMemoryMap != NULL);
 
@@ -80,11 +79,9 @@  ArmVirtGetMemoryMap (
   VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
   // Peripheral space after DRAM
-  TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),
-                     TopOfAddressSpace);
   VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
   VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
-  VirtualMemoryTable[2].Length       = TopOfMemory -
+  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
                                        VirtualMemoryTable[2].PhysicalBase;
   VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
index f2c461e3b55a..5c5b841051ad 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
@@ -45,4 +45,3 @@ 
 
 [FixedPcd]
   gArmTokenSpaceGuid.PcdFdSize
-  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
index f54fb51ee1d4..d12089760b22 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
@@ -49,4 +49,3 @@ 
 [FixedPcd]
   gArmTokenSpaceGuid.PcdFdSize
   gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
-  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
index 1587bd92f206..e04bd1b7c497 100755
--- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
+++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
@@ -85,9 +85,6 @@ 
 
   gArmPlatformTokenSpaceGuid.PcdCoreCount
 
-  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
-  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
-
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
index f6abe2f2016b..ecaaac1545c4 100755
--- a/ArmVirtPkg/PrePi/PrePi.c
+++ b/ArmVirtPkg/PrePi/PrePi.c
@@ -79,9 +79,6 @@  PrePiMain (
   StacksSize = PcdGet32 (PcdCPUCorePrimaryStackSize);
   BuildStackHob (StacksBase, StacksSize);
 
-  //TODO: Call CpuPei as a library
-  BuildCpuHob (PcdGet8 (PcdPrePiCpuMemorySize), PcdGet8 (PcdPrePiCpuIoSize));
-
   // Set the Boot Mode
   SetBootMode (BOOT_WITH_FULL_CONFIGURATION);