Re: [edk2-devel] [PATCH v5 2/2] add ArmCpuInfo EFI application

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

W dniu 20.04.2023 o 14:43, Leif Lindholm pisze:
> On Fri, Apr 07, 2023 at 17:29:57 +0200, Marcin Juszkiewicz wrote:

>> +#
>> +#  This flag specifies whether HII resource section is generated into PE image.
>> +#
>> +  UEFI_HII_RESOURCE_SECTION      = TRUE
> 
> The above stanza, and its comment, can be dropped.
> This relates to native language support, which there isn't any in this app.

done


>> +VOID
>> +PrintText (
>> +  CONST CHAR8  *field,
> 
> For coding style, name should be "*Field".

done, in all places

>> +  CONST CHAR8  *Bits,
>> +  CONST UINT8  Value,
>> +  CONST CHAR8  *Description
>> +  )
>> +{
>> +  STATIC CONST CHAR8  binaries[][5] = {
> 
> Could I propose renaming "binaries" to "Nibbles", since this is an
> array holding string values for all possible nibbles?

done

>> +    "0000", "0001", "0010", "0011", "0100", "0101", "0110", "0111",
>> +    "1000", "1001", "1010", "1011", "1100", "1101", "1110", "1111"
>> +  };

>> +  switch (Value) {
>> +    case 0b0000:
> 
> I agree with Pedro's concern w.r.t. binary literals being a GCC
> extension. But I also think this is the most appropriate
> representation since this is how it's documented in the ARM ARM.
> 
> A bit hacky, but could we do
> 
> enum {
>    b0000,
>    b0001,
>    ...
>    b1111
> };
> 
> and then use those instead?
> 
> (you can build with -pedantic to verify you catch them all)

done, passed with -pedantic

>> +  // only valid for BigEnd != 0b0000
> 
> Could we possibly reword as
>    // If mixed-endian support is present, check whether supported at EL0

done


>> +    case 0b0001:             // add FEAT_LPA2 check
> 
> That's sounds like a TODO. And we don't want TODOs in code.
> Can we either drop the comment, add the check, or skip the test?
> (That is my order of preference - we shouldn't need to be verifying
> architectural compliance.)

dropped

>> +    case 0b0010:             // add FEAT_LPA2 check
> 
> Same comment as for 4k.

dropped as well

>> +  Bits  = "59:56";
>> +  Value = (Aa64Pfr0 >> 56) & 0xf;
>> +  switch (Value) {
>> +    case 0b0000:
>> +      Description = "no info is FEAT_CSV2 implemented.";
> 
> Consider rewording.
> Is the text from the ARM ARM too long?
> "The implementation does not disclose whether FEAT_CSV2 is
> implemented."
> if so, maybe
> "Not disclosed whether FEAT_CSV2 is implemented."?

thx, done

>> +  PrintValues (RegName, Bits, Value, Description);
>> +
>> +  // 35:32 is CSV2_frac
> 
> That's a TODO.

Is it more info why those bits are not handled here. Same with 15:12 for 
RAS_frac and 19:16 for MPAM_frac. They are not reserved like 63:40 are.

And CSV2_frac bits are used earlier, in PRF0 handling. If we have CSV2 
implemented then CSV2_frac says do we have CSV2_1p1 or CSV2_1p2 implemented.

MPAM_frac and RAS_frac are used in PRF0 handling as well.

>> +EFI_STATUS
>> +EFIAPI
>> +UefiMain  (
> 
> Stray space before '(' here too. Is it an editor setting?

fixed