[Patch V3] EmulatorPkg: don't display the cpu current speed

[Patch V3] EmulatorPkg: don't display the cpu current speed

[Zhiguang Liu]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1686

V3: I hope that changing the status of the mCpuSmbiosType4
    wouldn't affect other features except showing CPU speed.
	The value is zero in NT32Pkg.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 EmulatorPkg/CpuRuntimeDxe/Cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/EmulatorPkg/CpuRuntimeDxe/Cpu.c b/EmulatorPkg/CpuRuntimeDxe/Cpu.c
index 00e93016af..a5e19b4181 100644
--- a/EmulatorPkg/CpuRuntimeDxe/Cpu.c
+++ b/EmulatorPkg/CpuRuntimeDxe/Cpu.c
@@ -104,7 +104,7 @@ SMBIOS_TABLE_TYPE4 mCpuSmbiosType4 = {
   0,                      // ExternalClock;
   0,                      // MaxSpeed;
   0,                      // CurrentSpeed;
-  0x41,                   // Status;
+  0,                      // Status;
   ProcessorUpgradeOther,  // ProcessorUpgrade;      ///< The enumeration value from PROCESSOR_UPGRADE.
   0,                      // L1CacheHandle;
   0,                      // L2CacheHandle;
-- 
2.21.0.windows.1

Re: [Patch V3] EmulatorPkg: don't display the cpu current speed

[Jordan Justen]

On 2019-06-11 00:32:27, Zhiguang Liu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1686
> 
> V3: I hope that changing the status of the mCpuSmbiosType4
>     wouldn't affect other features except showing CPU speed.
>         The value is zero in NT32Pkg.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  EmulatorPkg/CpuRuntimeDxe/Cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/EmulatorPkg/CpuRuntimeDxe/Cpu.c b/EmulatorPkg/CpuRuntimeDxe/Cpu.c
> index 00e93016af..a5e19b4181 100644
> --- a/EmulatorPkg/CpuRuntimeDxe/Cpu.c
> +++ b/EmulatorPkg/CpuRuntimeDxe/Cpu.c
> @@ -104,7 +104,7 @@ SMBIOS_TABLE_TYPE4 mCpuSmbiosType4 = {
>    0,                      // ExternalClock;
>    0,                      // MaxSpeed;
>    0,                      // CurrentSpeed;
> -  0x41,                   // Status;
> +  0,                      // Status;

It looks like bit 6 means the process is populated, and bits[2:0]==1
means the CPU is enabled.

So, it looks like this change will make SMBIOS indicate the the
processor socket is not populated, and bit2[2:0]==0 means that the CPU
status is unknown.

I think the commit message for this patch should have been:

===

EmulatorPkg: Change SMBIOS processor to unpopulated

This change updates the SMBIOS processor information to indicate that
the processor is not populated, and that it's status is unknown.

With this change the processor speed will not be shown in setup.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1686

===

But, I'm not sure I agree we should make this change to fix this bug.
I'm not particularly concerned with this bug, but I wonder if perhaps
the MdeModulePkg should just suppress the item if the speed is 0.

Or, alternately, perhaps we can investigate some methods to attempt to
determine the processor speed. I guess for all OS's, it might be
difficult, but we probably could support finding the processor speed
under the most common environments.

-Jordan

Re: [Patch V3] EmulatorPkg: don't display the cpu current speed

[Zhiguang Liu]

Thanks for the comments.
I will change the commit message in the next version.
But I don't think this is a issue worth making a major change.
Given that the change is consistent with NT32, will you agree with this change?

Best Regards,
Zhiguang Liu

>-----Original Message-----
>From: Justen, Jordan L
>Sent: Tuesday, June 11, 2019 3:56 PM
>To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io
>Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>
>Subject: Re: [Patch V3] EmulatorPkg: don't display the cpu current speed
>
>On 2019-06-11 00:32:27, Zhiguang Liu wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1686
>>
>> V3: I hope that changing the status of the mCpuSmbiosType4
>>     wouldn't affect other features except showing CPU speed.
>>         The value is zero in NT32Pkg.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
>> ---
>>  EmulatorPkg/CpuRuntimeDxe/Cpu.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/EmulatorPkg/CpuRuntimeDxe/Cpu.c
>> b/EmulatorPkg/CpuRuntimeDxe/Cpu.c index 00e93016af..a5e19b4181
>100644
>> --- a/EmulatorPkg/CpuRuntimeDxe/Cpu.c
>> +++ b/EmulatorPkg/CpuRuntimeDxe/Cpu.c
>> @@ -104,7 +104,7 @@ SMBIOS_TABLE_TYPE4 mCpuSmbiosType4 = {
>>    0,                      // ExternalClock;
>>    0,                      // MaxSpeed;
>>    0,                      // CurrentSpeed;
>> -  0x41,                   // Status;
>> +  0,                      // Status;
>
>It looks like bit 6 means the process is populated, and bits[2:0]==1 means the
>CPU is enabled.
>
>So, it looks like this change will make SMBIOS indicate the the processor
>socket is not populated, and bit2[2:0]==0 means that the CPU status is
>unknown.
>
>I think the commit message for this patch should have been:
>
>===
>
>EmulatorPkg: Change SMBIOS processor to unpopulated
>
>This change updates the SMBIOS processor information to indicate that the
>processor is not populated, and that it's status is unknown.
>
>With this change the processor speed will not be shown in setup.
>
>Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1686
>
>===
>
>But, I'm not sure I agree we should make this change to fix this bug.
>I'm not particularly concerned with this bug, but I wonder if perhaps the
>MdeModulePkg should just suppress the item if the speed is 0.
>
>Or, alternately, perhaps we can investigate some methods to attempt to
>determine the processor speed. I guess for all OS's, it might be difficult, but
>we probably could support finding the processor speed under the most
>common environments.
>
>-Jordan

Re: [Patch V3] EmulatorPkg: don't display the cpu current speed

[Jordan Justen]

On 2019-06-11 22:42:13, Liu, Zhiguang wrote:
> Thanks for the comments.
> I will change the commit message in the next version.
> But I don't think this is a issue worth making a major change.
> Given that the change is consistent with NT32, will you agree with this change?

Hmm. You are right that NT32 also does this. I don't agree enough to
give you a Reviewed-by, but if Andrew or Ray give a Reviewed-by, then
I'm ok for us to take the patch.

-Jordan

Re: [Patch V3] EmulatorPkg: don't display the cpu current speed

[Zhiguang Liu]

Thanks for the reply.
I understand now that this is not a very proper change.
I will talk to the bug  reporter in Bugzilla to see if  we can keep the current status.

Best Regards,
Zhiguang Liu

>-----Original Message-----
>From: Justen, Jordan L
>Sent: Thursday, June 13, 2019 3:47 PM
>To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io
>Cc: Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>
>Subject: RE: [Patch V3] EmulatorPkg: don't display the cpu current speed
>
>On 2019-06-11 22:42:13, Liu, Zhiguang wrote:
>> Thanks for the comments.
>> I will change the commit message in the next version.
>> But I don't think this is a issue worth making a major change.
>> Given that the change is consistent with NT32, will you agree with this
>change?
>
>Hmm. You are right that NT32 also does this. I don't agree enough to give you
>a Reviewed-by, but if Andrew or Ray give a Reviewed-by, then I'm ok for us to
>take the patch.
>
>-Jordan