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