* [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use %08x to print CacheControl Index @ 2020-02-03 7:06 Zeng, Star 2020-02-03 8:46 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Zeng, Star @ 2020-02-03 7:06 UTC (permalink / raw) To: devel; +Cc: Star Zeng, Eric Dong, Ray Ni, Laszlo Ersek 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, -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use %08x to print CacheControl Index 2020-02-03 7:06 [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use %08x to print CacheControl Index Zeng, Star @ 2020-02-03 8:46 ` Laszlo Ersek 2020-02-03 9:09 ` Zeng, Star 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2020-02-03 8:46 UTC (permalink / raw) To: Star Zeng, devel; +Cc: Eric Dong, Ray Ni 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. The rest of the format specifications (including the now-fixed CPU_REGISTER_TABLE_ENTRY.Index) are OK. Thanks Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use %08x to print CacheControl Index 2020-02-03 8:46 ` Laszlo Ersek @ 2020-02-03 9:09 ` Zeng, Star 2020-02-03 13:22 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Zeng, Star @ 2020-02-03 9:09 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Dong, Eric, Ni, Ray, Zeng, Star > -----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. 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. I found another real issue is MMIO : %08lx should be MMIO : %016lx as the code is on purpose to get UINT64 MMIO address. I prefer to just handle the real issue in this patch. How do you think? 😊 Thanks, Star > > The rest of the format specifications (including the now-fixed > CPU_REGISTER_TABLE_ENTRY.Index) are OK. > > Thanks > Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use %08x to print CacheControl Index 2020-02-03 9:09 ` Zeng, Star @ 2020-02-03 13:22 ` Laszlo Ersek 2020-02-04 7:04 ` Zeng, Star 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2020-02-03 13:22 UTC (permalink / raw) To: Zeng, Star, devel@edk2.groups.io; +Cc: Dong, Eric, Ni, Ray 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? 😊 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use %08x to print CacheControl Index 2020-02-03 13:22 ` Laszlo Ersek @ 2020-02-04 7:04 ` Zeng, Star 0 siblings, 0 replies; 5+ messages in thread From: Zeng, Star @ 2020-02-04 7:04 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Dong, Eric, Ni, Ray > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Monday, February 3, 2020 9:23 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 > > 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. 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? 😊 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-04 7:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-03 7:06 [PATCH] UefiCpuPkg RegisterCpuFeaturesLib: Use %08x to print CacheControl Index Zeng, Star 2020-02-03 8:46 ` Laszlo Ersek 2020-02-03 9:09 ` Zeng, Star 2020-02-03 13:22 ` Laszlo Ersek 2020-02-04 7:04 ` Zeng, Star
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox