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


Zeng, Star
 

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Monday, February 3, 2020 9:23 PM
To: Zeng, Star <star.zeng@...>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>
Subject: Re: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use %08x to print
CacheControl Index

On 02/03/20 10:09, Zeng, Star wrote:
-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Monday, February 3, 2020 4:47 PM
To: Zeng, Star <star.zeng@...>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>
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@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Star Zeng <star.zeng@...>
---
.../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.
Got your point, sent an updated patch at https://edk2.groups.io/g/devel/message/53703.

Thanks,
Star


Thanks
Laszlo

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

Join devel@edk2.groups.io to automatically receive all group messages.