Re: [edk2-devel] [PATCH v2] add ArmCpuInfo EFI application

["Marcin Juszkiewicz" <marcin.juszkiewicz@linaro.org>]

W dniu 7.04.2023 o 12:55, Ard Biesheuvel pisze:
> Hello Marcin,
> 
> Thanks for this - it looks useful.
 > Some comments below.

Thanks.
> On Fri, 7 Apr 2023 at 12:40, Marcin Juszkiewicz
> <marcin.juszkiewicz@linaro.org> wrote:
>>
>> App goes through ID_AA64*_EL1 system registers and decode their values.
>>
>> First version which does not use much of current AArch64 support code
>> present in EDK2. Written to check what data is there and what can be
>> done with it.
>>
>> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
>> ---
>>   MdeModulePkg/MdeModulePkg.dsc                      |    3 +-
>>   MdeModulePkg/Application/ArmCpuInfo/ArmCpuInfo.inf |   38 +
>>   MdeModulePkg/Application/ArmCpuInfo/readargs.h     |   12 +
>>   MdeModulePkg/Application/ArmCpuInfo/ArmCpuInfo.c   | 2274 ++++++++++++++++++++
>>   MdeModulePkg/Application/ArmCpuInfo/readregs.s     |   49 +
>>   5 files changed, 2375 insertions(+), 1 deletion(-)
>>
> 
> Why are you adding this to MdeModulePkg rather than ArmPkg?

This is the first time I wrote something for EDK2 so worked to get it 
built first rather than to get it right. Looked at HelloWorld example 
and followed.

Moved to ArmPkg/Application/ArmCpuInfo/ in current code.

>> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
>> index 1014598f31c3..1ecfb345159f 100644
>> --- a/MdeModulePkg/MdeModulePkg.dsc
>> +++ b/MdeModulePkg/MdeModulePkg.dsc
>> @@ -216,6 +216,7 @@ [PcdsDynamicExDefault]
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdRecoveryFileName|L"FVMAIN.FV"
>>
>>   [Components]
>> +  MdeModulePkg/Application/ArmCpuInfo/ArmCpuInfo.inf
>>     MdeModulePkg/Application/HelloWorld/HelloWorld.inf
>>     MdeModulePkg/Application/DumpDynPcd/DumpDynPcd.inf
>>     MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.inf
>> @@ -520,4 +521,4 @@ [Components.X64]
>>     MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
>>
>>   [BuildOptions]
>> -
>> +GCC:*_*_AARCH64_CC_FLAGS = -march=armv9-a
> 
> This belongs in the app's INF, not in the package DSC where it affects
> code generation for all components, which may need to run on much
> older CPUs

Dropped that line as it was not needed in the end.

>> diff --git a/MdeModulePkg/Application/ArmCpuInfo/ArmCpuInfo.c b/MdeModulePkg/Application/ArmCpuInfo/ArmCpuInfo.c
>> new file mode 100644
>> index 000000000000..ac27902e3533
>> +void print_values(const char* field, const char* bits, const int value, const char* description)
>> +{
>> +        char binaries[17][5] = {"0000", "0001", "0010", "0011", "0100", "0101", "0110", "0111",
>> +                                "1000", "1001", "1010", "1011", "1100", "1101", "1110", "1111"};
>> +
> 
> STATIC CONST CHAR8[] please, and you can use [] for the first
> dimension (and drop the final element)

done

>> +        AsciiPrint(" %-16a | %5a | %5a | %a\n", field, bits, binaries[value], description);
> 
> This should be value & 0xf so you never access outside of the array.

done

>> +void handle_aa64mmfr0_el1(const UINT64 aa64mmfr0_el1)
>> +{
>> +        UINT64 value;
>> +        char* regname = "ID_AA64MMFR0_EL1";
> 
> STATIC CONST CHAR8 RegName[] = ...

done

>> +        char* description;
>> +        char* bits;
> 
> CONST

It is not const.

>> +
>> +
>> +        bits = "3:0 ";
>> +        value = aa64mmfr0_el1 & 0xf;
>> +        switch(value)
> 
> Space after switch

done

>> +        if(value == 0b0110)
> 
> Space after if

done

> etc etc
> 
> There are some other style issues here, which I am sure the CI will
> whine about, so you might want to run uncrustify on it.

done