* [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()
2020-03-28 10:43 [PATCH 0/5] ArmPkg: cosmetic cleanups for ArmMmuLib Ard Biesheuvel
@ 2020-03-28 10:43 ` Ard Biesheuvel
2020-04-02 10:16 ` Leif Lindholm
2020-03-28 10:43 ` [PATCH 2/5] ArmPkg/CpuDxe: move PageAttributeToGcdAttribute() out of ArmMmuLib Ard Biesheuvel
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2020-03-28 10:43 UTC (permalink / raw)
To: devel; +Cc: leif, Ard Biesheuvel
Before getting rid of GetRootTranslationTableInfo() and the related
LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
version of the former to CpuDxe, which will be its only remaining
user. While at it, simplify it a bit, since in the CpuDxe cases,
both OUT arguments are always provided.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 7 -------
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 3b6c5e733709..24eb1c4221e3 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define TT_ATTR_INDX_INVALID ((UINT32)~0)
+#define MIN_T0SZ 16
+#define BITS_PER_LEVEL 9
+
+STATIC
+VOID
+GetRootTranslationTableInfo (
+ IN UINTN T0SZ,
+ OUT UINTN *RootTableLevel,
+ OUT UINTN *RootTableEntryCount
+ )
+{
+ *RootTableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
+ *RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
+}
+
STATIC
UINT64
GetFirstPageAttribute (
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index b627c3c50ff8..3fe5c24d5e5b 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -134,13 +134,6 @@ GetMemoryRegion (
OUT UINTN *RegionAttributes
);
-VOID
-GetRootTranslationTableInfo (
- IN UINTN T0SZ,
- OUT UINTN *TableLevel,
- OUT UINTN *TableEntryCount
- );
-
EFI_STATUS
SetGcdMemorySpaceAttributes (
IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap,
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()
2020-03-28 10:43 ` [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo() Ard Biesheuvel
@ 2020-04-02 10:16 ` Leif Lindholm
2020-04-02 10:20 ` [edk2-devel] " Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Leif Lindholm @ 2020-04-02 10:16 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel
On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
> Before getting rid of GetRootTranslationTableInfo() and the related
> LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
> version of the former to CpuDxe, which will be its only remaining
> user. While at it, simplify it a bit, since in the CpuDxe cases,
> both OUT arguments are always provided.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 7 -------
> 2 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> index 3b6c5e733709..24eb1c4221e3 100644
> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> @@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #define TT_ATTR_INDX_INVALID ((UINT32)~0)
>
> +#define MIN_T0SZ 16
> +#define BITS_PER_LEVEL 9
> +
> +STATIC
> +VOID
> +GetRootTranslationTableInfo (
> + IN UINTN T0SZ,
> + OUT UINTN *RootTableLevel,
> + OUT UINTN *RootTableEntryCount
> + )
> +{
> + *RootTableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
> + *RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
> +}
> +
> STATIC
> UINT64
> GetFirstPageAttribute (
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> index b627c3c50ff8..3fe5c24d5e5b 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> @@ -134,13 +134,6 @@ GetMemoryRegion (
> OUT UINTN *RegionAttributes
> );
>
> -VOID
> -GetRootTranslationTableInfo (
So, this may be super picky, but:
Deleting the prototype without making the definition also STATIC would
cause build errors with -Wmissing-prototypes (which someone might be
enabling explicitly or implicitly if say doing some code hardening on
the side).
Now, it's a valid point to say that -Wmissing-prototypes isn't in our
current CFLAGS, but I think it would be a good habit to get into.
So you get a:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
regardless, but I'd appreciate if you could sling a STATIC into
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
pushing.
> - IN UINTN T0SZ,
> - OUT UINTN *TableLevel,
> - OUT UINTN *TableEntryCount
> - );
> -
> EFI_STATUS
> SetGcdMemorySpaceAttributes (
> IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap,
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()
2020-04-02 10:16 ` Leif Lindholm
@ 2020-04-02 10:20 ` Ard Biesheuvel
2020-04-02 10:28 ` Leif Lindholm
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2020-04-02 10:20 UTC (permalink / raw)
To: devel, leif, Ard Biesheuvel
On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
> On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
>> Before getting rid of GetRootTranslationTableInfo() and the related
>> LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
>> version of the former to CpuDxe, which will be its only remaining
>> user. While at it, simplify it a bit, since in the CpuDxe cases,
>> both OUT arguments are always provided.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
>> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 7 -------
>> 2 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> index 3b6c5e733709..24eb1c4221e3 100644
>> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> @@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> #define TT_ATTR_INDX_INVALID ((UINT32)~0)
>>
>> +#define MIN_T0SZ 16
>> +#define BITS_PER_LEVEL 9
>> +
>> +STATIC
>> +VOID
>> +GetRootTranslationTableInfo (
>> + IN UINTN T0SZ,
>> + OUT UINTN *RootTableLevel,
>> + OUT UINTN *RootTableEntryCount
>> + )
>> +{
>> + *RootTableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
>> + *RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
>> +}
>> +
>> STATIC
>> UINT64
>> GetFirstPageAttribute (
>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> index b627c3c50ff8..3fe5c24d5e5b 100644
>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> @@ -134,13 +134,6 @@ GetMemoryRegion (
>> OUT UINTN *RegionAttributes
>> );
>>
>> -VOID
>> -GetRootTranslationTableInfo (
>
> So, this may be super picky, but:
> Deleting the prototype without making the definition also STATIC would
> cause build errors with -Wmissing-prototypes (which someone might be
> enabling explicitly or implicitly if say doing some code hardening on
> the side).
>
> Now, it's a valid point to say that -Wmissing-prototypes isn't in our
> current CFLAGS, but I think it would be a good habit to get into.
> So you get a:
>
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>
> regardless, but I'd appreciate if you could sling a STATIC into
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
> pushing.
>
Well, while I see your point, please note that the prototype only exists
in a local header file that only gets included by CpuDxe, and not by any
of the other consumers of ArmMmuLib.
>> - IN UINTN T0SZ,
>> - OUT UINTN *TableLevel,
>> - OUT UINTN *TableEntryCount
>> - );
>> -
>> EFI_STATUS
>> SetGcdMemorySpaceAttributes (
>> IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap,
>> --
>> 2.17.1
>>
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()
2020-04-02 10:20 ` [edk2-devel] " Ard Biesheuvel
@ 2020-04-02 10:28 ` Leif Lindholm
2020-04-02 10:29 ` Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Leif Lindholm @ 2020-04-02 10:28 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Ard Biesheuvel
On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
> On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
> > On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
> > > Before getting rid of GetRootTranslationTableInfo() and the related
> > > LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
> > > version of the former to CpuDxe, which will be its only remaining
> > > user. While at it, simplify it a bit, since in the CpuDxe cases,
> > > both OUT arguments are always provided.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > > ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
> > > ArmPkg/Drivers/CpuDxe/CpuDxe.h | 7 -------
> > > 2 files changed, 15 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> > > index 3b6c5e733709..24eb1c4221e3 100644
> > > --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> > > +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> > > @@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > #define TT_ATTR_INDX_INVALID ((UINT32)~0)
> > > +#define MIN_T0SZ 16
> > > +#define BITS_PER_LEVEL 9
> > > +
> > > +STATIC
> > > +VOID
> > > +GetRootTranslationTableInfo (
> > > + IN UINTN T0SZ,
> > > + OUT UINTN *RootTableLevel,
> > > + OUT UINTN *RootTableEntryCount
> > > + )
> > > +{
> > > + *RootTableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
> > > + *RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
> > > +}
> > > +
> > > STATIC
> > > UINT64
> > > GetFirstPageAttribute (
> > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > index b627c3c50ff8..3fe5c24d5e5b 100644
> > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > @@ -134,13 +134,6 @@ GetMemoryRegion (
> > > OUT UINTN *RegionAttributes
> > > );
> > > -VOID
> > > -GetRootTranslationTableInfo (
> >
> > So, this may be super picky, but:
> > Deleting the prototype without making the definition also STATIC would
> > cause build errors with -Wmissing-prototypes (which someone might be
> > enabling explicitly or implicitly if say doing some code hardening on
> > the side).
> >
> > Now, it's a valid point to say that -Wmissing-prototypes isn't in our
> > current CFLAGS, but I think it would be a good habit to get into.
> > So you get a:
> >
> > Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> >
> > regardless, but I'd appreciate if you could sling a STATIC into
> > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
> > pushing.
> >
>
> Well, while I see your point, please note that the prototype only exists in
> a local header file that only gets included by CpuDxe, and not by any of the
> other consumers of ArmMmuLib.
What I'm saying is that with -Wmissing-prototypes added, building
after applying this patch (but before applying the one that deletes
the function) gives:
"aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c -Wmissing-prototypes
/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: error: no previous prototype for ‘GetRootTranslationTableInfo’ [-Werror=missing-prototypes]
GetRootTranslationTableInfo (
^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
So it breaks bisect if you're experimenting with hardening.
As I said, I don't insist, but I want to be sure you realise that bit
before you decide.
/
Leif
>
>
> > > - IN UINTN T0SZ,
> > > - OUT UINTN *TableLevel,
> > > - OUT UINTN *TableEntryCount
> > > - );
> > > -
> > > EFI_STATUS
> > > SetGcdMemorySpaceAttributes (
> > > IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap,
> > > --
> > > 2.17.1
> > >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()
2020-04-02 10:28 ` Leif Lindholm
@ 2020-04-02 10:29 ` Ard Biesheuvel
2020-04-02 11:03 ` Leif Lindholm
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2020-04-02 10:29 UTC (permalink / raw)
To: Leif Lindholm; +Cc: devel, Ard Biesheuvel
On 4/2/20 12:28 PM, Leif Lindholm wrote:
> On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
>> On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
>>> On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
>>>> Before getting rid of GetRootTranslationTableInfo() and the related
>>>> LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
>>>> version of the former to CpuDxe, which will be its only remaining
>>>> user. While at it, simplify it a bit, since in the CpuDxe cases,
>>>> both OUT arguments are always provided.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
>>>> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 7 -------
>>>> 2 files changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>>>> index 3b6c5e733709..24eb1c4221e3 100644
>>>> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>>>> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>>>> @@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>> #define TT_ATTR_INDX_INVALID ((UINT32)~0)
>>>> +#define MIN_T0SZ 16
>>>> +#define BITS_PER_LEVEL 9
>>>> +
>>>> +STATIC
>>>> +VOID
>>>> +GetRootTranslationTableInfo (
>>>> + IN UINTN T0SZ,
>>>> + OUT UINTN *RootTableLevel,
>>>> + OUT UINTN *RootTableEntryCount
>>>> + )
>>>> +{
>>>> + *RootTableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
>>>> + *RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
>>>> +}
>>>> +
>>>> STATIC
>>>> UINT64
>>>> GetFirstPageAttribute (
>>>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>>>> index b627c3c50ff8..3fe5c24d5e5b 100644
>>>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>>>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>>>> @@ -134,13 +134,6 @@ GetMemoryRegion (
>>>> OUT UINTN *RegionAttributes
>>>> );
>>>> -VOID
>>>> -GetRootTranslationTableInfo (
>>>
>>> So, this may be super picky, but:
>>> Deleting the prototype without making the definition also STATIC would
>>> cause build errors with -Wmissing-prototypes (which someone might be
>>> enabling explicitly or implicitly if say doing some code hardening on
>>> the side).
>>>
>>> Now, it's a valid point to say that -Wmissing-prototypes isn't in our
>>> current CFLAGS, but I think it would be a good habit to get into.
>>> So you get a:
>>>
>>> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>>>
>>> regardless, but I'd appreciate if you could sling a STATIC into
>>> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
>>> pushing.
>>>
>>
>> Well, while I see your point, please note that the prototype only exists in
>> a local header file that only gets included by CpuDxe, and not by any of the
>> other consumers of ArmMmuLib.
>
> What I'm saying is that with -Wmissing-prototypes added, building
> after applying this patch (but before applying the one that deletes
> the function) gives:
>
> "aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c -Wmissing-prototypes
> /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: error: no previous prototype for ‘GetRootTranslationTableInfo’ [-Werror=missing-prototypes]
> GetRootTranslationTableInfo (
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> So it breaks bisect if you're experimenting with hardening.
>
> As I said, I don't insist, but I want to be sure you realise that bit
> before you decide.
>
But none of the other consumers of ArmMmuLib do an #include of
ArmPkg/Drivers/CpuDxe/CpuDxe.h, right? So I am slightly puzzled by this
result.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()
2020-04-02 10:29 ` Ard Biesheuvel
@ 2020-04-02 11:03 ` Leif Lindholm
2020-04-02 12:44 ` Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Leif Lindholm @ 2020-04-02 11:03 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Ard Biesheuvel
On Thu, Apr 02, 2020 at 12:29:30 +0200, Ard Biesheuvel wrote:
> On 4/2/20 12:28 PM, Leif Lindholm wrote:
> > On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
> > > On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
> > > > On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
> > > > > Before getting rid of GetRootTranslationTableInfo() and the related
> > > > > LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
> > > > > version of the former to CpuDxe, which will be its only remaining
> > > > > user. While at it, simplify it a bit, since in the CpuDxe cases,
> > > > > both OUT arguments are always provided.
Summary after discussing off-list:
This patch isn't *just* moving a public function private, but it's
replacing use of a function that was never meant to be public and had
a completely bonkers forward-declaration in a different module.
Ard has agreed to make the commit message more explicit on that point,
and I'm happy with that. So with that change, for the series:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> > > > So, this may be super picky, but:
> > > > Deleting the prototype without making the definition also STATIC would
> > > > cause build errors with -Wmissing-prototypes (which someone might be
> > > > enabling explicitly or implicitly if say doing some code hardening on
> > > > the side).
> > > >
> > > > Now, it's a valid point to say that -Wmissing-prototypes isn't in our
> > > > current CFLAGS, but I think it would be a good habit to get into.
> > > > So you get a:
> > > >
> > > > Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> > > >
> > > > regardless, but I'd appreciate if you could sling a STATIC into
> > > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
> > > > pushing.
> > > >
> > >
> > > Well, while I see your point, please note that the prototype only exists in
> > > a local header file that only gets included by CpuDxe, and not by any of the
> > > other consumers of ArmMmuLib.
> >
> > What I'm saying is that with -Wmissing-prototypes added, building
> > after applying this patch (but before applying the one that deletes
> > the function) gives:
> >
> > "aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c -Wmissing-prototypes
> > /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: error: no previous prototype for ‘GetRootTranslationTableInfo’ [-Werror=missing-prototypes]
> > GetRootTranslationTableInfo (
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> >
> > So it breaks bisect if you're experimenting with hardening.
> >
> > As I said, I don't insist, but I want to be sure you realise that bit
> > before you decide.
> >
>
> But none of the other consumers of ArmMmuLib do an #include of
> ArmPkg/Drivers/CpuDxe/CpuDxe.h, right? So I am slightly puzzled by this
> result.
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()
2020-04-02 11:03 ` Leif Lindholm
@ 2020-04-02 12:44 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2020-04-02 12:44 UTC (permalink / raw)
To: Leif Lindholm; +Cc: devel, Ard Biesheuvel
On 4/2/20 1:03 PM, Leif Lindholm wrote:
> On Thu, Apr 02, 2020 at 12:29:30 +0200, Ard Biesheuvel wrote:
>> On 4/2/20 12:28 PM, Leif Lindholm wrote:
>>> On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
>>>> On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
>>>>> On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
>>>>>> Before getting rid of GetRootTranslationTableInfo() and the related
>>>>>> LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
>>>>>> version of the former to CpuDxe, which will be its only remaining
>>>>>> user. While at it, simplify it a bit, since in the CpuDxe cases,
>>>>>> both OUT arguments are always provided.
>
> Summary after discussing off-list:
> This patch isn't *just* moving a public function private, but it's
> replacing use of a function that was never meant to be public and had
> a completely bonkers forward-declaration in a different module.
>
> Ard has agreed to make the commit message more explicit on that point,
> and I'm happy with that. So with that change, for the series:
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>
Pushed, thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/5] ArmPkg/CpuDxe: move PageAttributeToGcdAttribute() out of ArmMmuLib
2020-03-28 10:43 [PATCH 0/5] ArmPkg: cosmetic cleanups for ArmMmuLib Ard Biesheuvel
2020-03-28 10:43 ` [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo() Ard Biesheuvel
@ 2020-03-28 10:43 ` Ard Biesheuvel
2020-03-28 10:43 ` [PATCH 3/5] ArmPkg/ArmMmuLib: drop pointless LookupAddresstoRootTable() routine Ard Biesheuvel
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2020-03-28 10:43 UTC (permalink / raw)
To: devel; +Cc: leif, Ard Biesheuvel
The routine PageAttributeToGcdAttribute() is exported by ArmMmuLib
but only ever used in the implementation of CpuDxe. So let's move
the function there and make it STATIC.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 46 ++++++++++++++++++++
ArmPkg/Include/Chipset/AArch64.h | 5 ---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 45 -------------------
3 files changed, 46 insertions(+), 50 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 24eb1c4221e3..29fa08f9e07c 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -30,6 +30,52 @@ GetRootTranslationTableInfo (
*RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
}
+STATIC
+UINT64
+PageAttributeToGcdAttribute (
+ IN UINT64 PageAttributes
+ )
+{
+ UINT64 GcdAttributes;
+
+ switch (PageAttributes & TT_ATTR_INDX_MASK) {
+ case TT_ATTR_INDX_DEVICE_MEMORY:
+ GcdAttributes = EFI_MEMORY_UC;
+ break;
+ case TT_ATTR_INDX_MEMORY_NON_CACHEABLE:
+ GcdAttributes = EFI_MEMORY_WC;
+ break;
+ case TT_ATTR_INDX_MEMORY_WRITE_THROUGH:
+ GcdAttributes = EFI_MEMORY_WT;
+ break;
+ case TT_ATTR_INDX_MEMORY_WRITE_BACK:
+ GcdAttributes = EFI_MEMORY_WB;
+ break;
+ default:
+ DEBUG ((DEBUG_ERROR,
+ "PageAttributeToGcdAttribute: PageAttributes:0x%lX not supported.\n",
+ PageAttributes));
+ ASSERT (0);
+ // The Global Coherency Domain (GCD) value is defined as a bit set.
+ // Returning 0 means no attribute has been set.
+ GcdAttributes = 0;
+ }
+
+ // Determine protection attributes
+ if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) ||
+ ((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO)) {
+ // Read only cases map to write-protect
+ GcdAttributes |= EFI_MEMORY_RO;
+ }
+
+ // Process eXecute Never attribute
+ if ((PageAttributes & (TT_PXN_MASK | TT_UXN_MASK)) != 0) {
+ GcdAttributes |= EFI_MEMORY_XP;
+ }
+
+ return GcdAttributes;
+}
+
STATIC
UINT64
GetFirstPageAttribute (
diff --git a/ArmPkg/Include/Chipset/AArch64.h b/ArmPkg/Include/Chipset/AArch64.h
index e3d877207b38..0ade5cce91c3 100644
--- a/ArmPkg/Include/Chipset/AArch64.h
+++ b/ArmPkg/Include/Chipset/AArch64.h
@@ -219,11 +219,6 @@ ArmReadCurrentEL (
VOID
);
-UINT64
-PageAttributeToGcdAttribute (
- IN UINT64 PageAttributes
- );
-
UINTN
ArmWriteCptr (
IN UINT64 Cptr
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 3b10ef58f0a2..d16e847218b7 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -57,51 +57,6 @@ ArmMemoryAttributeToPageAttribute (
}
}
-UINT64
-PageAttributeToGcdAttribute (
- IN UINT64 PageAttributes
- )
-{
- UINT64 GcdAttributes;
-
- switch (PageAttributes & TT_ATTR_INDX_MASK) {
- case TT_ATTR_INDX_DEVICE_MEMORY:
- GcdAttributes = EFI_MEMORY_UC;
- break;
- case TT_ATTR_INDX_MEMORY_NON_CACHEABLE:
- GcdAttributes = EFI_MEMORY_WC;
- break;
- case TT_ATTR_INDX_MEMORY_WRITE_THROUGH:
- GcdAttributes = EFI_MEMORY_WT;
- break;
- case TT_ATTR_INDX_MEMORY_WRITE_BACK:
- GcdAttributes = EFI_MEMORY_WB;
- break;
- default:
- DEBUG ((DEBUG_ERROR,
- "PageAttributeToGcdAttribute: PageAttributes:0x%lX not supported.\n",
- PageAttributes));
- ASSERT (0);
- // The Global Coherency Domain (GCD) value is defined as a bit set.
- // Returning 0 means no attribute has been set.
- GcdAttributes = 0;
- }
-
- // Determine protection attributes
- if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) ||
- ((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO)) {
- // Read only cases map to write-protect
- GcdAttributes |= EFI_MEMORY_RO;
- }
-
- // Process eXecute Never attribute
- if ((PageAttributes & (TT_PXN_MASK | TT_UXN_MASK)) != 0) {
- GcdAttributes |= EFI_MEMORY_XP;
- }
-
- return GcdAttributes;
-}
-
#define MIN_T0SZ 16
#define BITS_PER_LEVEL 9
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 3/5] ArmPkg/ArmMmuLib: drop pointless LookupAddresstoRootTable() routine
2020-03-28 10:43 [PATCH 0/5] ArmPkg: cosmetic cleanups for ArmMmuLib Ard Biesheuvel
2020-03-28 10:43 ` [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo() Ard Biesheuvel
2020-03-28 10:43 ` [PATCH 2/5] ArmPkg/CpuDxe: move PageAttributeToGcdAttribute() out of ArmMmuLib Ard Biesheuvel
@ 2020-03-28 10:43 ` Ard Biesheuvel
2020-03-28 10:43 ` [PATCH 4/5] ArmPkg/ArmMmuLib: get rid of GetRootTranslationTableInfo() Ard Biesheuvel
2020-03-28 10:43 ` [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol Ard Biesheuvel
4 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2020-03-28 10:43 UTC (permalink / raw)
To: devel; +Cc: leif, Ard Biesheuvel
LookupAddresstoRootTable() uses a loop to go over its MaxAddress
argument, essentially to do a log2() and determine how many bits are
needed to represent it. Since the argument is the result of a shift-left
expression, there is some room for improvement here, and we can simply
use the bit count directly to calculate the value of T0SZ. At the same
time, we can omit calling GetRootTranslationTableInfo() to determine the
number of root table entries, and add a new helper that applies the
trivial calculation directly.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 49 ++++++--------------
1 file changed, 15 insertions(+), 34 deletions(-)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index d16e847218b7..b6f3ef54aa26 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -59,6 +59,16 @@ ArmMemoryAttributeToPageAttribute (
#define MIN_T0SZ 16
#define BITS_PER_LEVEL 9
+#define MAX_VA_BITS 48
+
+STATIC
+UINTN
+GetRootTableEntryCount (
+ IN UINTN T0SZ
+ )
+{
+ return TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
+}
VOID
GetRootTranslationTableInfo (
@@ -284,36 +294,6 @@ UpdateRegionMappingRecursive (
return EFI_SUCCESS;
}
-STATIC
-VOID
-LookupAddresstoRootTable (
- IN UINT64 MaxAddress,
- OUT UINTN *T0SZ,
- OUT UINTN *TableEntryCount
- )
-{
- UINTN TopBit;
-
- // Check the parameters are not NULL
- ASSERT ((T0SZ != NULL) && (TableEntryCount != NULL));
-
- // Look for the highest bit set in MaxAddress
- for (TopBit = 63; TopBit != 0; TopBit--) {
- if ((1ULL << TopBit) & MaxAddress) {
- // MaxAddress top bit is found
- TopBit = TopBit + 1;
- break;
- }
- }
- ASSERT (TopBit != 0);
-
- // Calculate T0SZ from the top bit of the MaxAddress
- *T0SZ = 64 - TopBit;
-
- // Get the Table info from T0SZ
- GetRootTranslationTableInfo (*T0SZ, NULL, TableEntryCount);
-}
-
STATIC
EFI_STATUS
UpdateRegionMapping (
@@ -508,6 +488,7 @@ ArmConfigureMmu (
)
{
VOID* TranslationTable;
+ UINTN MaxAddressBits;
UINT64 MaxAddress;
UINTN T0SZ;
UINTN RootTableEntryCount;
@@ -526,11 +507,11 @@ ArmConfigureMmu (
// into account the architectural limitations that result from UEFI's
// use of 4 KB pages.
//
- MaxAddress = MIN (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()) - 1,
- MAX_ALLOC_ADDRESS);
+ MaxAddressBits = MIN (ArmGetPhysicalAddressBits (), MAX_VA_BITS);
+ MaxAddress = LShiftU64 (1ULL, MaxAddressBits) - 1;
- // Lookup the Table Level to get the information
- LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
+ T0SZ = 64 - MaxAddressBits;
+ RootTableEntryCount = GetRootTableEntryCount (T0SZ);
//
// Set TCR that allows us to retrieve T0SZ in the subsequent functions
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 4/5] ArmPkg/ArmMmuLib: get rid of GetRootTranslationTableInfo()
2020-03-28 10:43 [PATCH 0/5] ArmPkg: cosmetic cleanups for ArmMmuLib Ard Biesheuvel
` (2 preceding siblings ...)
2020-03-28 10:43 ` [PATCH 3/5] ArmPkg/ArmMmuLib: drop pointless LookupAddresstoRootTable() routine Ard Biesheuvel
@ 2020-03-28 10:43 ` Ard Biesheuvel
2020-03-28 10:43 ` [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol Ard Biesheuvel
4 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2020-03-28 10:43 UTC (permalink / raw)
To: devel; +Cc: leif, Ard Biesheuvel
Only a single call to GetRootTranslationTableInfo() remains, which
only provides the root table level. So let's create a new static
helper function that returns just this value, and use it instead.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 22 ++++++--------------
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index b6f3ef54aa26..a82596d290f1 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -70,21 +70,13 @@ GetRootTableEntryCount (
return TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
}
-VOID
-GetRootTranslationTableInfo (
- IN UINTN T0SZ,
- OUT UINTN *TableLevel,
- OUT UINTN *TableEntryCount
+STATIC
+UINTN
+GetRootTableLevel (
+ IN UINTN T0SZ
)
{
- // Get the level of the root table
- if (TableLevel) {
- *TableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
- }
-
- if (TableEntryCount) {
- *TableEntryCount = 1UL << (BITS_PER_LEVEL - (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL);
- }
+ return (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
}
STATIC
@@ -303,7 +295,6 @@ UpdateRegionMapping (
IN UINT64 AttributeClearMask
)
{
- UINTN RootTableLevel;
UINTN T0SZ;
if (((RegionStart | RegionLength) & EFI_PAGE_MASK)) {
@@ -311,11 +302,10 @@ UpdateRegionMapping (
}
T0SZ = ArmGetTCR () & TCR_T0SZ_MASK;
- GetRootTranslationTableInfo (T0SZ, &RootTableLevel, NULL);
return UpdateRegionMappingRecursive (RegionStart, RegionStart + RegionLength,
AttributeSetMask, AttributeClearMask, ArmGetTTBR0BaseAddress (),
- RootTableLevel);
+ GetRootTableLevel (T0SZ));
}
STATIC
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol
2020-03-28 10:43 [PATCH 0/5] ArmPkg: cosmetic cleanups for ArmMmuLib Ard Biesheuvel
` (3 preceding siblings ...)
2020-03-28 10:43 ` [PATCH 4/5] ArmPkg/ArmMmuLib: get rid of GetRootTranslationTableInfo() Ard Biesheuvel
@ 2020-03-28 10:43 ` Ard Biesheuvel
2020-04-02 10:23 ` Leif Lindholm
4 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2020-03-28 10:43 UTC (permalink / raw)
To: devel; +Cc: leif, Ard Biesheuvel
TT_ATTR_INDX_INVALID is #define'd but never used so drop it.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index a82596d290f1..222ff817956f 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -19,9 +19,6 @@
#include <Library/BaseLib.h>
#include <Library/DebugLib.h>
-// We use this index definition to define an invalid block entry
-#define TT_ATTR_INDX_INVALID ((UINT32)~0)
-
STATIC
UINT64
ArmMemoryAttributeToPageAttribute (
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol
2020-03-28 10:43 ` [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol Ard Biesheuvel
@ 2020-04-02 10:23 ` Leif Lindholm
2020-04-02 10:29 ` [edk2-devel] " Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Leif Lindholm @ 2020-04-02 10:23 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel
On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
> TT_ATTR_INDX_INVALID is #define'd but never used so drop it.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index a82596d290f1..222ff817956f 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -19,9 +19,6 @@
> #include <Library/BaseLib.h>
> #include <Library/DebugLib.h>
>
> -// We use this index definition to define an invalid block entry
> -#define TT_ATTR_INDX_INVALID ((UINT32)~0)
> -
Since this is separately defined also in
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
could this patch be tweaked to instead "consolidate" the definitions
to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
all of the other TT_ATTR_INDX_ definitions live?
/
Leif
> STATIC
> UINT64
> ArmMemoryAttributeToPageAttribute (
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [edk2-devel] [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol
2020-04-02 10:23 ` Leif Lindholm
@ 2020-04-02 10:29 ` Ard Biesheuvel
2020-04-02 10:57 ` Leif Lindholm
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2020-04-02 10:29 UTC (permalink / raw)
To: devel, leif, Ard Biesheuvel
On 4/2/20 12:23 PM, Leif Lindholm via groups.io wrote:
> On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
>> TT_ATTR_INDX_INVALID is #define'd but never used so drop it.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> index a82596d290f1..222ff817956f 100644
>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> @@ -19,9 +19,6 @@
>> #include <Library/BaseLib.h>
>> #include <Library/DebugLib.h>
>>
>> -// We use this index definition to define an invalid block entry
>> -#define TT_ATTR_INDX_INVALID ((UINT32)~0)
>> -
>
> Since this is separately defined also in
> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
> could this patch be tweaked to instead "consolidate" the definitions
> to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
> all of the other TT_ATTR_INDX_ definitions live?
>
That would imply that this value is somehow architected, which it is not.
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c is the only remaining user of this
constant, and it only has meaning within the context of the routines
therein.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol
2020-04-02 10:29 ` [edk2-devel] " Ard Biesheuvel
@ 2020-04-02 10:57 ` Leif Lindholm
2020-04-02 11:05 ` Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Leif Lindholm @ 2020-04-02 10:57 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Ard Biesheuvel
On Thu, Apr 02, 2020 at 12:29:40 +0200, Ard Biesheuvel wrote:
> On 4/2/20 12:23 PM, Leif Lindholm via groups.io wrote:
> > On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
> > > TT_ATTR_INDX_INVALID is #define'd but never used so drop it.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
> > > 1 file changed, 3 deletions(-)
> > >
> > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > > index a82596d290f1..222ff817956f 100644
> > > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > > @@ -19,9 +19,6 @@
> > > #include <Library/BaseLib.h>
> > > #include <Library/DebugLib.h>
> > > -// We use this index definition to define an invalid block entry
> > > -#define TT_ATTR_INDX_INVALID ((UINT32)~0)
> > > -
> >
> > Since this is separately defined also in
> > ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
> > could this patch be tweaked to instead "consolidate" the definitions
> > to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
> > all of the other TT_ATTR_INDX_ definitions live?
> >
>
> That would imply that this value is somehow architected, which it is not.
>
> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c is the only remaining user of this
> constant, and it only has meaning within the context of the routines
> therein.
Hmm, ok. So all of those definitions should really move *out* of
ArmPkg/Include/Chipset/AArch64Mmu.h?
No, they can't, because some of the others are used in
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
So I'm still feeling that #defines using the same namespace should
live together in order to reduce risk of future screwups.
So, unrelated to this patch
(Reviewed-by: Leif Lindholm <leif@nuviainc.com>)
should the remaining TT_ATTR_INDX_INVALID be renamed, or should it be
moved to ArmPkg/Include/Chipset/AArch64Mmu.h?
/
Leif
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [edk2-devel] [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol
2020-04-02 10:57 ` Leif Lindholm
@ 2020-04-02 11:05 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2020-04-02 11:05 UTC (permalink / raw)
To: Leif Lindholm; +Cc: devel, Ard Biesheuvel
On 4/2/20 12:57 PM, Leif Lindholm wrote:
> On Thu, Apr 02, 2020 at 12:29:40 +0200, Ard Biesheuvel wrote:
>> On 4/2/20 12:23 PM, Leif Lindholm via groups.io wrote:
>>> On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
>>>> TT_ATTR_INDX_INVALID is #define'd but never used so drop it.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
>>>> 1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>>>> index a82596d290f1..222ff817956f 100644
>>>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>>>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>>>> @@ -19,9 +19,6 @@
>>>> #include <Library/BaseLib.h>
>>>> #include <Library/DebugLib.h>
>>>> -// We use this index definition to define an invalid block entry
>>>> -#define TT_ATTR_INDX_INVALID ((UINT32)~0)
>>>> -
>>>
>>> Since this is separately defined also in
>>> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
>>> could this patch be tweaked to instead "consolidate" the definitions
>>> to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
>>> all of the other TT_ATTR_INDX_ definitions live?
>>>
>>
>> That would imply that this value is somehow architected, which it is not.
>>
>> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c is the only remaining user of this
>> constant, and it only has meaning within the context of the routines
>> therein.
>
> Hmm, ok. So all of those definitions should really move *out* of
> ArmPkg/Include/Chipset/AArch64Mmu.h?
>
> No, they can't, because some of the others are used in
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>
> So I'm still feeling that #defines using the same namespace should
> live together in order to reduce risk of future screwups.
> So, unrelated to this patch
> (Reviewed-by: Leif Lindholm <leif@nuviainc.com>)
> should the remaining TT_ATTR_INDX_INVALID be renamed, or should it be
> moved to ArmPkg/Include/Chipset/AArch64Mmu.h?
>
It should simply be renamed. They are not part of the same namespace, it
is simply a local hack in CpuDxe to distinguish between valid indexes
into the MAIR table and an 'unknown' placeholder value.
CpuDxe is on my list of things I'd like to clean up as well.
^ permalink raw reply [flat|nested] 16+ messages in thread