> -----Original Message----- > From: Laszlo Ersek <[email protected]> > Sent: Monday, February 3, 2020 9:23 PM > To: Zeng, Star <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]>; Ni, Ray <[email protected]> > 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 <[email protected]> > >> Sent: Monday, February 3, 2020 4:47 PM > >> To: Zeng, Star <[email protected]>; [email protected] > >> Cc: Dong, Eric <[email protected]>; Ni, Ray <[email protected]> > >> 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 <[email protected]> > >>> Cc: Ray Ni <[email protected]> > >>> Cc: Laszlo Ersek <[email protected]> > >>> Signed-off-by: Star Zeng <[email protected]> > >>> --- > >>> .../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? 😊 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53706): https://edk2.groups.io/g/devel/message/53706 Mute This Topic: https://groups.io/mt/70940928/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
