V2 changes:
1. Split CR read/write action in to one discrete patch
2. Keep the old logic which continue the process if error found.

Below code is current implementation:
  if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
    CPU_REGISTER_TABLE_WRITE_FIELD (
      ProcessorNumber,
      Msr,
      MSR_IA32_FEATURE_CONTROL,
      MSR_IA32_FEATURE_CONTROL_REGISTER,
      Bits.Lock,
      1
    );
  }

With below steps, the Bits.Lock bit will lose its value:
1. Trig normal boot, the Bits.Lock is 0. 1 will be added
   into the register table and then will set to the MSR.
2. Trig warm reboot, MSR value preserves. After normal boot phase,
   the Bits.Lock is 1, so it will not be added into the register
   table during the warm reboot phase.
3. Trig S3 then resume, the Bits.Lock change to 0 and Bits.Lock is
   not added in register table during normal boot phase. so it's
   still 0 after resume. 
This is not an expect behavior. The expect result is the value should
always 1 after booting or resuming from S3.

The root cause for this issue is
1. driver bases on current value to insert the "set value action" to
   the register table.
2. Some MSRs may reserve their value during warm reboot. So the insert
   action may be skip after warm reboot.

The solution for this issue is:
1. Always add "Test then Set" action for above referred MSRs.
2. Detect current value before set new value. Only set new value when
   current value not same as new value.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>

Eric Dong (6):
  UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test Then Write" Macros.
  UefiCpuPkg/PiSmmCpuDxeSmm: Combine CR read/write action in one
    function.
  UefiCpuPkg/PiSmmCpuDxeSmm: Supports test then write new value logic.
  UefiCpuPkg/RegisterCpuFeaturesLib: Combine CR read/write action in one
    function.
  UefiCpuPkg/RegisterCpuFeaturesLib: Supports test then write new value
    logic.
  UefiCpuPkg/CpuCommonFeaturesLib: Use new macros.

 UefiCpuPkg/Include/AcpiCpuData.h              |   1 +
 .../Include/Library/RegisterCpuFeaturesLib.h  |  77 +++++++++-
 .../CpuCommonFeaturesLib/CpuCommonFeatures.h  |  15 --
 .../CpuCommonFeaturesLib.c                    |   8 +-
 .../CpuCommonFeaturesLib/FeatureControl.c     | 141 ++++++------------
 .../CpuCommonFeaturesLib/MachineCheck.c       |  23 ++-
 .../CpuFeaturesInitialize.c                   | 141 ++++++++++++------
 .../RegisterCpuFeaturesLib.c                  |  14 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 135 +++++++++++------
 9 files changed, 323 insertions(+), 232 deletions(-)

-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45424): https://edk2.groups.io/g/devel/message/45424
Mute This Topic: https://groups.io/mt/32839204/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 08/12/19 12:31, Eric Dong wrote:
> V2 changes:
> 1. Split CR read/write action in to one discrete patch
> 2. Keep the old logic which continue the process if error found.
> 
> Below code is current implementation:
>   if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
>     CPU_REGISTER_TABLE_WRITE_FIELD (
>       ProcessorNumber,
>       Msr,
>       MSR_IA32_FEATURE_CONTROL,
>       MSR_IA32_FEATURE_CONTROL_REGISTER,
>       Bits.Lock,
>       1
>     );
>   }
> 
> With below steps, the Bits.Lock bit will lose its value:
> 1. Trig normal boot, the Bits.Lock is 0. 1 will be added
>    into the register table and then will set to the MSR.
> 2. Trig warm reboot, MSR value preserves. After normal boot phase,
>    the Bits.Lock is 1, so it will not be added into the register
>    table during the warm reboot phase.
> 3. Trig S3 then resume, the Bits.Lock change to 0 and Bits.Lock is
>    not added in register table during normal boot phase. so it's
>    still 0 after resume. 
> This is not an expect behavior. The expect result is the value should
> always 1 after booting or resuming from S3.
> 
> The root cause for this issue is
> 1. driver bases on current value to insert the "set value action" to
>    the register table.
> 2. Some MSRs may reserve their value during warm reboot. So the insert
>    action may be skip after warm reboot.
> 
> The solution for this issue is:
> 1. Always add "Test then Set" action for above referred MSRs.
> 2. Detect current value before set new value. Only set new value when
>    current value not same as new value.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Eric Dong (6):
>   UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test Then Write" Macros.
>   UefiCpuPkg/PiSmmCpuDxeSmm: Combine CR read/write action in one
>     function.
>   UefiCpuPkg/PiSmmCpuDxeSmm: Supports test then write new value logic.
>   UefiCpuPkg/RegisterCpuFeaturesLib: Combine CR read/write action in one
>     function.
>   UefiCpuPkg/RegisterCpuFeaturesLib: Supports test then write new value
>     logic.
>   UefiCpuPkg/CpuCommonFeaturesLib: Use new macros.
> 
>  UefiCpuPkg/Include/AcpiCpuData.h              |   1 +
>  .../Include/Library/RegisterCpuFeaturesLib.h  |  77 +++++++++-
>  .../CpuCommonFeaturesLib/CpuCommonFeatures.h  |  15 --
>  .../CpuCommonFeaturesLib.c                    |   8 +-
>  .../CpuCommonFeaturesLib/FeatureControl.c     | 141 ++++++------------
>  .../CpuCommonFeaturesLib/MachineCheck.c       |  23 ++-
>  .../CpuFeaturesInitialize.c                   | 141 ++++++++++++------
>  .../RegisterCpuFeaturesLib.c                  |  14 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 135 +++++++++++------
>  9 files changed, 323 insertions(+), 232 deletions(-)
> 

Please don't forget to build-test this series for IA32 too.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45442): https://edk2.groups.io/g/devel/message/45442
Mute This Topic: https://groups.io/mt/32839204/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Laszlo,

Yes, I already checked IA32 build.

As Ray is leaving these days, can you help to review this serial?

Thanks,
Eric

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Monday, August 12, 2019 10:15 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [Patch v2 0/6] Add "test then write" mechanism.
> 
> On 08/12/19 12:31, Eric Dong wrote:
> > V2 changes:
> > 1. Split CR read/write action in to one discrete patch 2. Keep the old
> > logic which continue the process if error found.
> >
> > Below code is current implementation:
> >   if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
> >     CPU_REGISTER_TABLE_WRITE_FIELD (
> >       ProcessorNumber,
> >       Msr,
> >       MSR_IA32_FEATURE_CONTROL,
> >       MSR_IA32_FEATURE_CONTROL_REGISTER,
> >       Bits.Lock,
> >       1
> >     );
> >   }
> >
> > With below steps, the Bits.Lock bit will lose its value:
> > 1. Trig normal boot, the Bits.Lock is 0. 1 will be added
> >    into the register table and then will set to the MSR.
> > 2. Trig warm reboot, MSR value preserves. After normal boot phase,
> >    the Bits.Lock is 1, so it will not be added into the register
> >    table during the warm reboot phase.
> > 3. Trig S3 then resume, the Bits.Lock change to 0 and Bits.Lock is
> >    not added in register table during normal boot phase. so it's
> >    still 0 after resume.
> > This is not an expect behavior. The expect result is the value should
> > always 1 after booting or resuming from S3.
> >
> > The root cause for this issue is
> > 1. driver bases on current value to insert the "set value action" to
> >    the register table.
> > 2. Some MSRs may reserve their value during warm reboot. So the insert
> >    action may be skip after warm reboot.
> >
> > The solution for this issue is:
> > 1. Always add "Test then Set" action for above referred MSRs.
> > 2. Detect current value before set new value. Only set new value when
> >    current value not same as new value.
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> >
> > Eric Dong (6):
> >   UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test Then Write" Macros.
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Combine CR read/write action in one
> >     function.
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Supports test then write new value logic.
> >   UefiCpuPkg/RegisterCpuFeaturesLib: Combine CR read/write action in
> one
> >     function.
> >   UefiCpuPkg/RegisterCpuFeaturesLib: Supports test then write new value
> >     logic.
> >   UefiCpuPkg/CpuCommonFeaturesLib: Use new macros.
> >
> >  UefiCpuPkg/Include/AcpiCpuData.h              |   1 +
> >  .../Include/Library/RegisterCpuFeaturesLib.h  |  77 +++++++++-
> > .../CpuCommonFeaturesLib/CpuCommonFeatures.h  |  15 --
> >  .../CpuCommonFeaturesLib.c                    |   8 +-
> >  .../CpuCommonFeaturesLib/FeatureControl.c     | 141 ++++++------------
> >  .../CpuCommonFeaturesLib/MachineCheck.c       |  23 ++-
> >  .../CpuFeaturesInitialize.c                   | 141 ++++++++++++------
> >  .../RegisterCpuFeaturesLib.c                  |  14 +-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 135 +++++++++++------
> >  9 files changed, 323 insertions(+), 232 deletions(-)
> >
> 
> Please don't forget to build-test this series for IA32 too.
> 
> Thanks
> Laszlo
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45489): https://edk2.groups.io/g/devel/message/45489
Mute This Topic: https://groups.io/mt/32839204/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Eric:
  Is this a bug fix or new feature? Dose it catch to this 201908 stable tag? 

Thanks
Liming
>-----Original Message-----
>From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>Dong, Eric
>Sent: Tuesday, August 13, 2019 10:29 AM
>To: devel@edk2.groups.io; lersek@redhat.com
>Cc: Ni, Ray <ray.ni@intel.com>
>Subject: Re: [edk2-devel] [Patch v2 0/6] Add "test then write" mechanism.
>
>Hi Laszlo,
>
>Yes, I already checked IA32 build.
>
>As Ray is leaving these days, can you help to review this serial?
>
>Thanks,
>Eric
>
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Monday, August 12, 2019 10:15 PM
>> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [Patch v2 0/6] Add "test then write" mechanism.
>>
>> On 08/12/19 12:31, Eric Dong wrote:
>> > V2 changes:
>> > 1. Split CR read/write action in to one discrete patch 2. Keep the old
>> > logic which continue the process if error found.
>> >
>> > Below code is current implementation:
>> >   if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
>> >     CPU_REGISTER_TABLE_WRITE_FIELD (
>> >       ProcessorNumber,
>> >       Msr,
>> >       MSR_IA32_FEATURE_CONTROL,
>> >       MSR_IA32_FEATURE_CONTROL_REGISTER,
>> >       Bits.Lock,
>> >       1
>> >     );
>> >   }
>> >
>> > With below steps, the Bits.Lock bit will lose its value:
>> > 1. Trig normal boot, the Bits.Lock is 0. 1 will be added
>> >    into the register table and then will set to the MSR.
>> > 2. Trig warm reboot, MSR value preserves. After normal boot phase,
>> >    the Bits.Lock is 1, so it will not be added into the register
>> >    table during the warm reboot phase.
>> > 3. Trig S3 then resume, the Bits.Lock change to 0 and Bits.Lock is
>> >    not added in register table during normal boot phase. so it's
>> >    still 0 after resume.
>> > This is not an expect behavior. The expect result is the value should
>> > always 1 after booting or resuming from S3.
>> >
>> > The root cause for this issue is
>> > 1. driver bases on current value to insert the "set value action" to
>> >    the register table.
>> > 2. Some MSRs may reserve their value during warm reboot. So the insert
>> >    action may be skip after warm reboot.
>> >
>> > The solution for this issue is:
>> > 1. Always add "Test then Set" action for above referred MSRs.
>> > 2. Detect current value before set new value. Only set new value when
>> >    current value not same as new value.
>> >
>> > Cc: Ray Ni <ray.ni@intel.com>
>> > Cc: Laszlo Ersek <lersek@redhat.com>
>> >
>> > Eric Dong (6):
>> >   UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test Then Write" Macros.
>> >   UefiCpuPkg/PiSmmCpuDxeSmm: Combine CR read/write action in one
>> >     function.
>> >   UefiCpuPkg/PiSmmCpuDxeSmm: Supports test then write new value
>logic.
>> >   UefiCpuPkg/RegisterCpuFeaturesLib: Combine CR read/write action in
>> one
>> >     function.
>> >   UefiCpuPkg/RegisterCpuFeaturesLib: Supports test then write new value
>> >     logic.
>> >   UefiCpuPkg/CpuCommonFeaturesLib: Use new macros.
>> >
>> >  UefiCpuPkg/Include/AcpiCpuData.h              |   1 +
>> >  .../Include/Library/RegisterCpuFeaturesLib.h  |  77 +++++++++-
>> > .../CpuCommonFeaturesLib/CpuCommonFeatures.h  |  15 --
>> >  .../CpuCommonFeaturesLib.c                    |   8 +-
>> >  .../CpuCommonFeaturesLib/FeatureControl.c     | 141 ++++++------------
>> >  .../CpuCommonFeaturesLib/MachineCheck.c       |  23 ++-
>> >  .../CpuFeaturesInitialize.c                   | 141 ++++++++++++------
>> >  .../RegisterCpuFeaturesLib.c                  |  14 +-
>> >  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 135 +++++++++++------
>> >  9 files changed, 323 insertions(+), 232 deletions(-)
>> >
>>
>> Please don't forget to build-test this series for IA32 too.
>>
>> Thanks
>> Laszlo
>>
>>
>
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45589): https://edk2.groups.io/g/devel/message/45589
Mute This Topic: https://groups.io/mt/32839204/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi liming,

This is a bug fix. It is required by 201908 stable tag.

Thanks,
Eric

> -----Original Message-----
> From: Gao, Liming
> Sent: Wednesday, August 14, 2019 3:27 PM
> To: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>;
> lersek@redhat.com
> Cc: Ni, Ray <ray.ni@intel.com>; leif.lindholm@linaro.org; afish@apple.com;
> Cetola, Stephano <stephano.cetola@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [Patch v2 0/6] Add "test then write" mechanism.
> 
> Eric:
>   Is this a bug fix or new feature? Dose it catch to this 201908 stable tag?
> 
> Thanks
> Liming
> >-----Original Message-----
> >From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> >Dong, Eric
> >Sent: Tuesday, August 13, 2019 10:29 AM
> >To: devel@edk2.groups.io; lersek@redhat.com
> >Cc: Ni, Ray <ray.ni@intel.com>
> >Subject: Re: [edk2-devel] [Patch v2 0/6] Add "test then write" mechanism.
> >
> >Hi Laszlo,
> >
> >Yes, I already checked IA32 build.
> >
> >As Ray is leaving these days, can you help to review this serial?
> >
> >Thanks,
> >Eric
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Monday, August 12, 2019 10:15 PM
> >> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> >> Cc: Ni, Ray <ray.ni@intel.com>
> >> Subject: Re: [edk2-devel] [Patch v2 0/6] Add "test then write" mechanism.
> >>
> >> On 08/12/19 12:31, Eric Dong wrote:
> >> > V2 changes:
> >> > 1. Split CR read/write action in to one discrete patch 2. Keep the
> >> > old logic which continue the process if error found.
> >> >
> >> > Below code is current implementation:
> >> >   if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
> >> >     CPU_REGISTER_TABLE_WRITE_FIELD (
> >> >       ProcessorNumber,
> >> >       Msr,
> >> >       MSR_IA32_FEATURE_CONTROL,
> >> >       MSR_IA32_FEATURE_CONTROL_REGISTER,
> >> >       Bits.Lock,
> >> >       1
> >> >     );
> >> >   }
> >> >
> >> > With below steps, the Bits.Lock bit will lose its value:
> >> > 1. Trig normal boot, the Bits.Lock is 0. 1 will be added
> >> >    into the register table and then will set to the MSR.
> >> > 2. Trig warm reboot, MSR value preserves. After normal boot phase,
> >> >    the Bits.Lock is 1, so it will not be added into the register
> >> >    table during the warm reboot phase.
> >> > 3. Trig S3 then resume, the Bits.Lock change to 0 and Bits.Lock is
> >> >    not added in register table during normal boot phase. so it's
> >> >    still 0 after resume.
> >> > This is not an expect behavior. The expect result is the value
> >> > should always 1 after booting or resuming from S3.
> >> >
> >> > The root cause for this issue is
> >> > 1. driver bases on current value to insert the "set value action" to
> >> >    the register table.
> >> > 2. Some MSRs may reserve their value during warm reboot. So the
> insert
> >> >    action may be skip after warm reboot.
> >> >
> >> > The solution for this issue is:
> >> > 1. Always add "Test then Set" action for above referred MSRs.
> >> > 2. Detect current value before set new value. Only set new value when
> >> >    current value not same as new value.
> >> >
> >> > Cc: Ray Ni <ray.ni@intel.com>
> >> > Cc: Laszlo Ersek <lersek@redhat.com>
> >> >
> >> > Eric Dong (6):
> >> >   UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test Then Write" Macros.
> >> >   UefiCpuPkg/PiSmmCpuDxeSmm: Combine CR read/write action in one
> >> >     function.
> >> >   UefiCpuPkg/PiSmmCpuDxeSmm: Supports test then write new value
> >logic.
> >> >   UefiCpuPkg/RegisterCpuFeaturesLib: Combine CR read/write action
> >> > in
> >> one
> >> >     function.
> >> >   UefiCpuPkg/RegisterCpuFeaturesLib: Supports test then write new
> value
> >> >     logic.
> >> >   UefiCpuPkg/CpuCommonFeaturesLib: Use new macros.
> >> >
> >> >  UefiCpuPkg/Include/AcpiCpuData.h              |   1 +
> >> >  .../Include/Library/RegisterCpuFeaturesLib.h  |  77 +++++++++-
> >> > .../CpuCommonFeaturesLib/CpuCommonFeatures.h  |  15 --
> >> >  .../CpuCommonFeaturesLib.c                    |   8 +-
> >> >  .../CpuCommonFeaturesLib/FeatureControl.c     | 141 ++++++------------
> >> >  .../CpuCommonFeaturesLib/MachineCheck.c       |  23 ++-
> >> >  .../CpuFeaturesInitialize.c                   | 141 ++++++++++++------
> >> >  .../RegisterCpuFeaturesLib.c                  |  14 +-
> >> >  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 135 +++++++++++----
> --
> >> >  9 files changed, 323 insertions(+), 232 deletions(-)
> >> >
> >>
> >> Please don't forget to build-test this series for IA32 too.
> >>
> >> Thanks
> >> Laszlo
> >>
> >>
> >
> >
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45590): https://edk2.groups.io/g/devel/message/45590
Mute This Topic: https://groups.io/mt/32839204/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-