[edk2,2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

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

Commit Message

Ard Biesheuvel Nov. 23, 2018, 12:14 p.m.
In preparation of permitting the virt code to define a larger PA space
size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the
CPU actually supports, take the CPU's capabilities into account when
setting up the page tables. This is necessary because KVM will shortly
support variable PA space sizes, and to support running the same UEFI
binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be
treated as an upper bound rather than a fixed size.

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

---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
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, 9:42 a.m. | #1
On 11/23/18 13:14, Ard Biesheuvel wrote:
> In preparation of permitting the virt code to define a larger PA space

> size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the

> CPU actually supports, take the CPU's capabilities into account when

> setting up the page tables. This is necessary because KVM will shortly

> support variable PA space sizes, and to support running the same UEFI

> binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be

> treated as an upper bound rather than a fixed size.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

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

> ---

>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-

>  1 file changed, 4 insertions(+), 1 deletion(-)

> 

> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> index 4b62ecb6a476..a4fde9b59383 100644

> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> @@ -593,6 +593,7 @@ ArmConfigureMmu (

>  {

>    VOID*                         TranslationTable;

>    UINT32                        TranslationTableAttribute;

> +  UINTN                         MaxAddressBits;

>    UINT64                        MaxAddress;

>    UINTN                         T0SZ;

>    UINTN                         RootTableEntryCount;

> @@ -605,7 +606,9 @@ ArmConfigureMmu (

>    }

>  

>    // Cover the entire GCD memory space

> -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;

> +  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),

> +                        PcdGet8 (PcdPrePiCpuMemorySize));


(1) Superficial comment: sticking to the letter of MIN() in
"MdePkg/Include/Base.h", the arguments should be of the exact same type.
Currently they aren't. (It's a different question whether that
requirement makes any sense for MIN().)

(2) Why does it make sense to use MIN() here? Why not just disregard
PcdPrePiCpuMemorySize and map what the CPU is capable of? (This ties in
with my lack of understanding of PcdPrePiCpuMemorySize.)

I mean, not going above ArmGetPhysicalAddressBits() makes total sense.
What's the point of staying below it though? And if so, how much exactly
do we want to stay below it? (I.e., how is a platform supposed to set
PcdPrePiCpuMemorySize, in order to clamp down ArmGetPhysicalAddressBits()?)

> +  MaxAddress = (1UL << MaxAddressBits) - 1;


(3) I understand this just reworks the original code, but the original
code isn't stellar. If we are left-shifting a UINT32 or UINTN value,
then the result is the same, and the << operator is OK. However:

- Here we intend to accommodate a UINT64 result, judged from the type of
MaxAddress (UINT64).

- For that, we should likely left-shift 1ULL, not 1 U;, which in turn
requires LShiftU64() from BaseLib.

(If we indeed want to use UINTN, then I think we should change the type
of MaxAddress, plus write "(UINTN)1", not 1UL.)

>  

>    // Lookup the Table Level to get the information

>    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);

> 


Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Nov. 26, 2018, 9:46 a.m. | #2
On 11/26/18 10:42, Laszlo Ersek wrote:
> On 11/23/18 13:14, Ard Biesheuvel wrote:


>> +  MaxAddress = (1UL << MaxAddressBits) - 1;


> not 1 U;, which


Sorry, typo: s/;/L/, clearly. (They are adjacent in my keyboard layout.)

Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Nov. 26, 2018, 10:06 a.m. | #3
On 11/26/18 10:42, Laszlo Ersek wrote:
> On 11/23/18 13:14, Ard Biesheuvel wrote:

>> In preparation of permitting the virt code to define a larger PA space

>> size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the

>> CPU actually supports, take the CPU's capabilities into account when

>> setting up the page tables. This is necessary because KVM will shortly

>> support variable PA space sizes, and to support running the same UEFI

>> binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be

>> treated as an upper bound rather than a fixed size.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.1

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

>> ---

>>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-

>>  1 file changed, 4 insertions(+), 1 deletion(-)

>>

>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

>> index 4b62ecb6a476..a4fde9b59383 100644

>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

>> @@ -593,6 +593,7 @@ ArmConfigureMmu (

>>  {

>>    VOID*                         TranslationTable;

>>    UINT32                        TranslationTableAttribute;

>> +  UINTN                         MaxAddressBits;

>>    UINT64                        MaxAddress;

>>    UINTN                         T0SZ;

>>    UINTN                         RootTableEntryCount;

>> @@ -605,7 +606,9 @@ ArmConfigureMmu (

>>    }

>>  

>>    // Cover the entire GCD memory space

>> -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;

>> +  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),

>> +                        PcdGet8 (PcdPrePiCpuMemorySize));

> 

> (1) Superficial comment: sticking to the letter of MIN() in

> "MdePkg/Include/Base.h", the arguments should be of the exact same type.

> Currently they aren't. (It's a different question whether that

> requirement makes any sense for MIN().)

> 

> (2) Why does it make sense to use MIN() here? Why not just disregard

> PcdPrePiCpuMemorySize and map what the CPU is capable of? (This ties in

> with my lack of understanding of PcdPrePiCpuMemorySize.)

> 

> I mean, not going above ArmGetPhysicalAddressBits() makes total sense.

> What's the point of staying below it though? And if so, how much exactly

> do we want to stay below it? (I.e., how is a platform supposed to set

> PcdPrePiCpuMemorySize, in order to clamp down ArmGetPhysicalAddressBits()?)


Ugh, okay. (Maybe you've responded already, I'm still in write-only
mode, sorry.) I think I'm getting the idea here. This is just being done
to keep the series bisectable / regression-free in the middle too. Right?

Thanks
Laszlo

> 

>> +  MaxAddress = (1UL << MaxAddressBits) - 1;

> 

> (3) I understand this just reworks the original code, but the original

> code isn't stellar. If we are left-shifting a UINT32 or UINTN value,

> then the result is the same, and the << operator is OK. However:

> 

> - Here we intend to accommodate a UINT64 result, judged from the type of

> MaxAddress (UINT64).

> 

> - For that, we should likely left-shift 1ULL, not 1 U;, which in turn

> requires LShiftU64() from BaseLib.

> 

> (If we indeed want to use UINTN, then I think we should change the type

> of MaxAddress, plus write "(UINTN)1", not 1UL.)

> 

>>  

>>    // Lookup the Table Level to get the information

>>    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);

>>

> 

> 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:43 a.m. | #4
On Mon, 26 Nov 2018 at 10:42, Laszlo Ersek <lersek@redhat.com> wrote:
>

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

> > In preparation of permitting the virt code to define a larger PA space

> > size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the

> > CPU actually supports, take the CPU's capabilities into account when

> > setting up the page tables. This is necessary because KVM will shortly

> > support variable PA space sizes, and to support running the same UEFI

> > binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be

> > treated as an upper bound rather than a fixed size.

> >

> > Contributed-under: TianoCore Contribution Agreement 1.1

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

> > ---

> >  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-

> >  1 file changed, 4 insertions(+), 1 deletion(-)

> >

> > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> > index 4b62ecb6a476..a4fde9b59383 100644

> > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> > @@ -593,6 +593,7 @@ ArmConfigureMmu (

> >  {

> >    VOID*                         TranslationTable;

> >    UINT32                        TranslationTableAttribute;

> > +  UINTN                         MaxAddressBits;

> >    UINT64                        MaxAddress;

> >    UINTN                         T0SZ;

> >    UINTN                         RootTableEntryCount;

> > @@ -605,7 +606,9 @@ ArmConfigureMmu (

> >    }

> >

> >    // Cover the entire GCD memory space

> > -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;

> > +  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),

> > +                        PcdGet8 (PcdPrePiCpuMemorySize));

>

> (1) Superficial comment: sticking to the letter of MIN() in

> "MdePkg/Include/Base.h", the arguments should be of the exact same type.

> Currently they aren't. (It's a different question whether that

> requirement makes any sense for MIN().)

>


OK

> (2) Why does it make sense to use MIN() here? Why not just disregard

> PcdPrePiCpuMemorySize and map what the CPU is capable of? (This ties in

> with my lack of understanding of PcdPrePiCpuMemorySize.)

>


PcdPrePiCpuMemorySize defines the size of the GCD memory space.
PcdPrePiCpuIoSize defines the size of the GCD I/O space.

However, we infer other quantities from it as well:
- Up until now, it never made sense to make the GCD space larger than
what the CPU is able to address, so we used it as an upper bound for
the page table code;
- On a given platform, the physical address space may not be entirely
populated, and fewer PA bits means fewer levels of page tables, and so
less memory used *. This means it might make sense to reduce
PcdPrePiCpuMemorySize to a lower value than what the h/w supports.

* this was briefly a concern when some (all?) levels of page tables
were allocated from temporary RAM but that broke some platforms IIRC
so that was reverted.

> I mean, not going above ArmGetPhysicalAddressBits() makes total sense.

> What's the point of staying below it though? And if so, how much exactly

> do we want to stay below it? (I.e., how is a platform supposed to set

> PcdPrePiCpuMemorySize, in order to clamp down ArmGetPhysicalAddressBits()?)

>


As explained above, staying below it gives you less overhead in the
GCD memory map and the page tables.

> > +  MaxAddress = (1UL << MaxAddressBits) - 1;

>

> (3) I understand this just reworks the original code, but the original

> code isn't stellar. If we are left-shifting a UINT32 or UINTN value,

> then the result is the same, and the << operator is OK. However:

>

> - Here we intend to accommodate a UINT64 result, judged from the type of

> MaxAddress (UINT64).

>

> - For that, we should likely left-shift 1ULL, not 1 U;, which in turn

> requires LShiftU64() from BaseLib.

>


This code currently only executes on AArch64, but for correctness (and
in case we ever use this code for SMMUs on 32-bit ARM), it should
indeed be fixed.

> (If we indeed want to use UINTN, then I think we should change the type

> of MaxAddress, plus write "(UINTN)1", not 1UL.)

>

> >

> >    // Lookup the Table Level to get the information

> >    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);

> >

>

> Thanks,

> Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Nov. 26, 2018, 5:45 p.m. | #5
On Fri, Nov 23, 2018 at 01:14:28PM +0100, Ard Biesheuvel wrote:
> In preparation of permitting the virt code to define a larger PA space

> size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the

> CPU actually supports, take the CPU's capabilities into account when

> setting up the page tables. This is necessary because KVM will shortly

> support variable PA space sizes, and to support running the same UEFI

> binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be

> treated as an upper bound rather than a fixed size.


Why do we keep PcdPrePiCpuMemorySize at all? (I.e., rather than just
using the probed value?
Mainly for the purpose of being able to restrict ourselves to 32/48
bits?

If we keep it, should we rename
PcdPrePiCpuMemorySize -> PcdPrePiCpuMemoryBits
and
PcdPrePiCpuIoSize -> PcdPrePiCpuIoBits
?

Argument against this would be that later consumers still refer to
the value extracted from the HOB as SizeOf*Space, and happily shifts
1s around by it :|

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.1

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

> ---

>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-

>  1 file changed, 4 insertions(+), 1 deletion(-)

> 

> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> index 4b62ecb6a476..a4fde9b59383 100644

> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> @@ -593,6 +593,7 @@ ArmConfigureMmu (

>  {

>    VOID*                         TranslationTable;

>    UINT32                        TranslationTableAttribute;

> +  UINTN                         MaxAddressBits;

>    UINT64                        MaxAddress;

>    UINTN                         T0SZ;

>    UINTN                         RootTableEntryCount;

> @@ -605,7 +606,9 @@ ArmConfigureMmu (

>    }

>  

>    // Cover the entire GCD memory space

> -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;

> +  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),

> +                        PcdGet8 (PcdPrePiCpuMemorySize));

> +  MaxAddress = (1UL << MaxAddressBits) - 1;

>  

>    // Lookup the Table Level to get the information

>    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);

> -- 

> 2.17.1

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 26, 2018, 5:50 p.m. | #6
On Mon, 26 Nov 2018 at 18:46, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>

> On Fri, Nov 23, 2018 at 01:14:28PM +0100, Ard Biesheuvel wrote:

> > In preparation of permitting the virt code to define a larger PA space

> > size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the

> > CPU actually supports, take the CPU's capabilities into account when

> > setting up the page tables. This is necessary because KVM will shortly

> > support variable PA space sizes, and to support running the same UEFI

> > binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be

> > treated as an upper bound rather than a fixed size.

>

> Why do we keep PcdPrePiCpuMemorySize at all? (I.e., rather than just

> using the probed value?

> Mainly for the purpose of being able to restrict ourselves to 32/48

> bits?

>

> If we keep it, should we rename

> PcdPrePiCpuMemorySize -> PcdPrePiCpuMemoryBits

> and

> PcdPrePiCpuIoSize -> PcdPrePiCpuIoBits

> ?

>

> Argument against this would be that later consumers still refer to

> the value extracted from the HOB as SizeOf*Space, and happily shifts

> 1s around by it :|

>


I am reworking this so PcdPrePiCpuMemorySize retains its meaning.
Instead, I will add something like this to ArmLib.h

//
// ARM_MMU_IDMAP_LIMIT defines the maximum size of the identity mapping
// that covers the entire address space when running in UEFI. This is limited
// to what can architecturally be mapped using a 4 KB granule, even if the
// hardware is capable of mapping more using larger pages.
//
#ifdef MDE_CPU_ARM
#define ARM_MMU_IDMAP_LIMIT     (MAX_UINT32)
#else
#define ARM_MMU_IDMAP_LIMIT     (1ULL << 48)
#endif

and use that in the MMU code as an upper bound in case the CPU supports 52 bits.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Nov. 26, 2018, 5:57 p.m. | #7
On Mon, Nov 26, 2018 at 06:50:09PM +0100, Ard Biesheuvel wrote:
> On Mon, 26 Nov 2018 at 18:46, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> >

> > On Fri, Nov 23, 2018 at 01:14:28PM +0100, Ard Biesheuvel wrote:

> > > In preparation of permitting the virt code to define a larger PA space

> > > size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the

> > > CPU actually supports, take the CPU's capabilities into account when

> > > setting up the page tables. This is necessary because KVM will shortly

> > > support variable PA space sizes, and to support running the same UEFI

> > > binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be

> > > treated as an upper bound rather than a fixed size.

> >

> > Why do we keep PcdPrePiCpuMemorySize at all? (I.e., rather than just

> > using the probed value?

> > Mainly for the purpose of being able to restrict ourselves to 32/48

> > bits?

> >

> > If we keep it, should we rename

> > PcdPrePiCpuMemorySize -> PcdPrePiCpuMemoryBits

> > and

> > PcdPrePiCpuIoSize -> PcdPrePiCpuIoBits

> > ?

> >

> > Argument against this would be that later consumers still refer to

> > the value extracted from the HOB as SizeOf*Space, and happily shifts

> > 1s around by it :|

> >

> 

> I am reworking this so PcdPrePiCpuMemorySize retains its meaning.

> Instead, I will add something like this to ArmLib.h

> 

> //

> // ARM_MMU_IDMAP_LIMIT defines the maximum size of the identity mapping

> // that covers the entire address space when running in UEFI. This is limited

> // to what can architecturally be mapped using a 4 KB granule, even if the

> // hardware is capable of mapping more using larger pages.

> //

> #ifdef MDE_CPU_ARM

> #define ARM_MMU_IDMAP_LIMIT     (MAX_UINT32)

> #else

> #define ARM_MMU_IDMAP_LIMIT     (1ULL << 48)

> #endif

> 

> and use that in the MMU code as an upper bound in case the CPU supports 52 bits.


OK, that works for me.

And gets you off the hook for Pcd name bikesheding :)

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

Patch

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 4b62ecb6a476..a4fde9b59383 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -593,6 +593,7 @@  ArmConfigureMmu (
 {
   VOID*                         TranslationTable;
   UINT32                        TranslationTableAttribute;
+  UINTN                         MaxAddressBits;
   UINT64                        MaxAddress;
   UINTN                         T0SZ;
   UINTN                         RootTableEntryCount;
@@ -605,7 +606,9 @@  ArmConfigureMmu (
   }
 
   // Cover the entire GCD memory space
-  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;
+  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),
+                        PcdGet8 (PcdPrePiCpuMemorySize));
+  MaxAddress = (1UL << MaxAddressBits) - 1;
 
   // Lookup the Table Level to get the information
   LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);