Re: [PATCH v7 13/21] ArmPkg: Add Universal/Smbios/ProcessorSubClassDxe

["Rebecca Cran" <rebecca@nuviainc.com>]

On 2/3/21 8:18 AM, Leif Lindholm wrote:
 > On Sun, Jan 31, 2021 at 16:25:03 -0700, Rebecca Cran wrote:
>> +    // At each level of cache, we can have a single type (unified, instruction or data),
>> +    // or two types - separate data and instruction caches. If we have separate
>> +    // instruction and data caches, then on the first iteration (CacheSubLevel = 0)
>> +    // process the instruction cache.
>> +    for (DataCacheType = 0; DataCacheType <= 1; DataCacheType++) {
>> +      // If there's no separate data/instruction cache, skip the second iteration
>> +      if (DataCacheType > 0 && !SeparateCaches) {
>> +        continue;
>> +      }
>> +
>> +      Type7Record = AllocateAndInitCacheInformation (CacheLevel,
>> +                                                     DataCacheType == 0 && SeparateCaches,
>> +                                                     DataCacheType == 1,
>> +                                                     DataCacheType == 0 && !SeparateCaches
>> +                                                     );
> 
> This is a bit too clever for my poor brain.
> Could we pass DataCacheType and SeparateCaches and do the logic
> conversion in the function?

I hope you don't mind, but I kept the DataCache and UnifiedCache 
parameters in the v8 version and just passed !SeparateCaches instead, 
which I think is neater.

> It feels like this is still somewhat encoding ARM architectural
> behaviour in the API? SMBIOS defines
> typedef enum {
>    CacheTypeOther       = 0x01,
>    CacheTypeUnknown     = 0x02,
>    CacheTypeInstruction = 0x03,
>    CacheTypeData        = 0x04,
>    CacheTypeUnified     = 0x05
> } CACHE_TYPE_DATA;
> 
> OK, let's worry about that aspect later, but please, move the logic
> into the functions.

I don't think there should be any left, now: just a generic assumption 
about caches either being split I/D or unified.

-- 
Rebecca Cran