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

["Zeng, Star" <star.zeng@intel.com>]

> -----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? 😊
>