Re: [edk2-devel] [Patch v2 0/6] Add "test then write" mechanism.
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 ;
> ler...@redhat.com
> Cc: Ni, Ray ; leif.lindh...@linaro.org; af...@apple.com;
> Cetola, Stephano ; Kinney, Michael D
>
> 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; ler...@redhat.com
> >Cc: Ni, Ray
> >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 ; devel@edk2.groups.io
> >> Cc: Ni, Ray
> >> 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
> >> > Cc: Laszlo Ersek
> >> >
> >> > 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
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; ler...@redhat.com
>Cc: Ni, Ray
>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 ; devel@edk2.groups.io
>> Cc: Ni, Ray
>> 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
>> > Cc: Laszlo Ersek
>> >
>> > 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
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 ; devel@edk2.groups.io
> Cc: Ni, Ray
> 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
> > Cc: Laszlo Ersek
> >
> > 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
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
> Cc: Laszlo Ersek
>
> 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
