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

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

[Zeng, Star]

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

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

[Laszlo Ersek]

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

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

[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

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

[Laszlo Ersek]

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

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

[Zeng, Star]

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