Re: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use %08x to print CacheControl Index

["Laszlo Ersek" <lersek@redhat.com>]

On 02/03/20 10:09, Zeng, Star wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Monday, February 3, 2020 4:47 PM
>> To: Zeng, Star <star.zeng@intel.com>; devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use %08x to print
>> CacheControl Index
>>
>> Hello Star,
>>
>> On 02/03/20 08:06, Star Zeng wrote:
>>> Instead of %08lx, use %08x to print CacheControl Index as it is UINT32
>>> type.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>>> ---
>>>  .../Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c      | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git
>>> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
>>> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
>>> index 0a4fcff033a3..1a02809b0e7c 100644
>>> ---
>>> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
>>> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.
>>> +++ c
>>> @@ -465,7 +465,7 @@ DumpRegisterTableOnProcessor (
>>>      case CacheControl:
>>>        DEBUG ((
>>>          DebugPrintErrorLevel,
>>> -        "Processor: %04d: Index %04d, CACHE: %08lx, Bit Start: %02d,
>> Bit Length: %02d, Value: %016lx\r\n",
>>> +        "Processor: %04d: Index %04d, CACHE: %08x, Bit Start: %02d,
>>> + Bit Length: %02d, Value: %016lx\r\n",
>>>          ProcessorNumber,
>>>          FeatureIndex,
>>>          RegisterTableEntry->Index,
>>>
>>
>> if you are already touching this DEBUG invocation, can you please fix the
>> rest of the issues with the format string?
>>
>> - ProcessorNumber is UINTN. If we know for sure it can be represented in a
>> UINT32, then it should be cast to UINT32 explicitly, and logged with "%04u".
>> (Otherwise, UINTN needs to be cast to UINT64, and logged with %lu or %lx.)
>>
>> - Ditto for FeatureIndex.
> 
> %04u or %04d is not enough for UINT32 which needs %08x.
> I thought the code is just taking assumption about their value should be not > 9999u. It is not a real issue.

I disagree. It's not about the field width / padding (4 vs. 8
characters), but the width of the data type. The parameter that's being
passed is a UINTN, which is UINT64 on X64. But the format specifier (%d,
%u, %x all alike) only expect a UINT32.

If we pass a UINT64 (in the form of a UINTN), then we should print it
with a UINT64 specifier (such as %lu or %lx).

Alternatively, if we know for sure that the value of the UINT64 in
question will fit in a UINT32, then we can use %u or %x for printing,
but then we need to truncate (cast) the data that's being passed in, to
UINT32.

My point is that the data size should be a match between what's passed
in and what is described with a format specifier. There is no format
specifier that directly matches UINTN, so you either need to cast UINTN
to UINT64 and use %lx, or cast UINTN to UINT32 and use %x.

> 
> This patch is to fix a real issue, without it, the print for ValidBitStart, ValidBitLength and Value will be wrong because the parameter for them are shifted for Index to fetch UINT64 value.

The patch is not wrong, it's just incomplete (given that we're modifying
a format string that mismatches the argument list in other places too).

ProcessorNumber and FeatureIndex are UINTNs, and they are being printed
with %d. Those are real issues too.

> I found another real issue is MMIO : %08lx should be MMIO : %016lx as the code is on purpose to get UINT64 MMIO address.

Field width / padding are useful to get right, but getting the data
types to match is even more important.

Thanks
Laszlo

> I prefer to just handle the real issue in this patch. How do you think? 😊