Re: [edk2-devel] [Patch v2 0/6] Add "test then write" mechanism.

["Dong, Eric" <eric.dong@intel.com>]

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