Re: [edk2-devel] [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
On 08/12/19 09:46, Dong, Eric wrote: > Hi Laszlo, > >> -Original Message- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >> Laszlo Ersek >> Sent: Friday, August 9, 2019 11:14 PM >> To: Dong, Eric ; devel@edk2.groups.io >> Cc: Ni, Ray >> Subject: Re: [edk2-devel] [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: >> Add "detect before set" Micros. >> >> On 08/09/19 08:11, Eric Dong wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040 >>> >>> Add below new micros which test the current value before set the new >>> value. Only set new value when current value not same as new value. >>> CPU_REGISTER_TABLE_TEST_THEN_WRITE32 >>> CPU_REGISTER_TABLE_TEST_THEN_WRITE64 >>> CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD >>> >>> Signed-off-by: Eric Dong >>> Cc: Ray Ni >>> Cc: Laszlo Ersek >>> --- >>> UefiCpuPkg/Include/AcpiCpuData.h | 1 + >>> .../Include/Library/RegisterCpuFeaturesLib.h | 77 +-- >>> .../RegisterCpuFeaturesLib.c | 14 +++- >>> 3 files changed, 80 insertions(+), 12 deletions(-) >> >> (1) When you format your patch sets, can you please use the following two >> options: >> >> --stat=1000 --stat-graph-width=20 >> >> Otherwise the diffstats are truncated (on the left) and hard to understand. >> > > 1. I use TortoiseGit to create patch, do you know how to enable these setting > in TortoiseGit? Sorry, no clue :( > >>> >>> diff --git a/UefiCpuPkg/Include/AcpiCpuData.h >>> b/UefiCpuPkg/Include/AcpiCpuData.h >>> index b963a2f592..c764e209cf 100644 >>> --- a/UefiCpuPkg/Include/AcpiCpuData.h >>> +++ b/UefiCpuPkg/Include/AcpiCpuData.h >>> @@ -81,6 +81,7 @@ typedef struct { >>>UINT16 Reserved; // offset 10 - 11 >>>UINT32 HighIndex; // offset 12-15, only valid for >> MemoryMapped >>>UINT64 Value; // offset 16-23 >>> + UINT8 DetectIt; // 0ffset 24 >>> } CPU_REGISTER_TABLE_ENTRY; >> >> (2) Another quite generic comment -- "DetectIt" does not look helpful. >> Somehow the verb "detect" does not communicate the right action to me. >> >> How about using a more established name, such as: >> >> - CompareAndSwap >> - CompareAndSet >> - TestAndSet >> >> ? >> >> If you agree, then I suggest updating the parameter names, and their >> comments too, below. > > 2. Yes, I change from "TestIt" to "DetectIt" because I think it's better. > Seems like this is not a correct change. > I will use "TestThenWrite" as the new name which align our new macro " > CPU_REGISTER_TABLE_TEST_THEN_WRITE". What do you think? Sounds good, thanks. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45432): https://edk2.groups.io/g/devel/message/45432 Mute This Topic: https://groups.io/mt/32807819/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
Hi Laszlo, Will fix it in my next version change. Thanks, Eric > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, August 9, 2019 11:31 PM > To: Dong, Eric ; devel@edk2.groups.io > Cc: Ni, Ray > Subject: Re: [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before > set" Micros. > > On 08/09/19 08:11, Eric Dong wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040 > > > > Add below new micros which test the current value before set > > Both the subject line and the commit message should say "macros", not > "micros". > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45411): https://edk2.groups.io/g/devel/message/45411 Mute This Topic: https://groups.io/mt/32807819/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
On 08/09/19 08:11, Eric Dong wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040 > > Add below new micros which test the current value before set > the new value. Only set new value when current value not > same as new value. > CPU_REGISTER_TABLE_TEST_THEN_WRITE32 > CPU_REGISTER_TABLE_TEST_THEN_WRITE64 > CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD > > Signed-off-by: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > --- > UefiCpuPkg/Include/AcpiCpuData.h | 1 + > .../Include/Library/RegisterCpuFeaturesLib.h | 77 +-- > .../RegisterCpuFeaturesLib.c | 14 +++- > 3 files changed, 80 insertions(+), 12 deletions(-) (1) When you format your patch sets, can you please use the following two options: --stat=1000 --stat-graph-width=20 Otherwise the diffstats are truncated (on the left) and hard to understand. > > diff --git a/UefiCpuPkg/Include/AcpiCpuData.h > b/UefiCpuPkg/Include/AcpiCpuData.h > index b963a2f592..c764e209cf 100644 > --- a/UefiCpuPkg/Include/AcpiCpuData.h > +++ b/UefiCpuPkg/Include/AcpiCpuData.h > @@ -81,6 +81,7 @@ typedef struct { >UINT16 Reserved; // offset 10 - 11 >UINT32 HighIndex; // offset 12-15, only valid for > MemoryMapped >UINT64 Value; // offset 16-23 > + UINT8 DetectIt; // 0ffset 24 > } CPU_REGISTER_TABLE_ENTRY; (2) Another quite generic comment -- "DetectIt" does not look helpful. Somehow the verb "detect" does not communicate the right action to me. How about using a more established name, such as: - CompareAndSwap - CompareAndSet - TestAndSet ? If you agree, then I suggest updating the parameter names, and their comments too, below. Thanks Laszlo > > // > diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h > b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h > index e420e7f075..87fe87acb7 100644 > --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h > +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h > @@ -335,6 +335,7 @@ SwitchBspAfterFeaturesInitialize ( >@param[in] IndexIndex of the register to program >@param[in] ValueMaskMask of bits in register to write >@param[in] ValueValue to write > + @param[in] DetectIt Whether need to detect current Value before > writing. > >@note This service could be called by BSP only. > **/ > @@ -345,7 +346,8 @@ CpuRegisterTableWrite ( >IN REGISTER_TYPE RegisterType, >IN UINT64 Index, >IN UINT64 ValueMask, > - IN UINT64 Value > + IN UINT64 Value, > + IN UINT8 DetectIt >); > > /** > @@ -385,9 +387,45 @@ PreSmmCpuRegisterTableWrite ( > >@note This service could be called by BSP only. > **/ > -#define CPU_REGISTER_TABLE_WRITE32(ProcessorNumber, RegisterType, Index, > Value) \ > - do { > \ > -CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, > Value); \ > +#define CPU_REGISTER_TABLE_WRITE32(ProcessorNumber, RegisterType, Index, > Value) \ > + do { > \ > +CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, > Value, FALSE); \ > + } while(FALSE); > + > +/** > + Adds a 32-bit register write entry in specified register table. > + > + This macro adds an entry in specified register table, with given register > type, > + register index, and value. > + > + @param[in] ProcessorNumber The index of the CPU to add a register table > entry. > + @param[in] RegisterType Type of the register to program > + @param[in] IndexIndex of the register to program > + @param[in] ValueValue to write > + > + @note This service could be called by BSP only. > +**/ > +#define CPU_REGISTER_TABLE_TEST_THEN_WRITE32(ProcessorNumber, RegisterType, > Index, Value) \ > + do { >\ > +CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, > Value, TRUE); \ > + } while(FALSE); > + > +/** > + Adds a 64-bit register write entry in specified register table. > + > + This macro adds an entry in specified register table, with given register > type, > + register index, and value. > + > + @param[in] ProcessorNumber The index of the CPU to add a register table > entry. > + @param[in] RegisterType Type of the register to program > + @param[in] IndexIndex of the register to program > + @param[in] ValueValue to write > + > + @note This service could be called by BSP only. > +**/ > +#define CPU_REGISTER_TABLE_WRITE64(ProcessorNumber, RegisterType, Index, > Value) \ > + do {
[edk2-devel] [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040 Add below new micros which test the current value before set the new value. Only set new value when current value not same as new value. CPU_REGISTER_TABLE_TEST_THEN_WRITE32 CPU_REGISTER_TABLE_TEST_THEN_WRITE64 CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD Signed-off-by: Eric Dong Cc: Ray Ni Cc: Laszlo Ersek --- UefiCpuPkg/Include/AcpiCpuData.h | 1 + .../Include/Library/RegisterCpuFeaturesLib.h | 77 +-- .../RegisterCpuFeaturesLib.c | 14 +++- 3 files changed, 80 insertions(+), 12 deletions(-) diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h index b963a2f592..c764e209cf 100644 --- a/UefiCpuPkg/Include/AcpiCpuData.h +++ b/UefiCpuPkg/Include/AcpiCpuData.h @@ -81,6 +81,7 @@ typedef struct { UINT16 Reserved; // offset 10 - 11 UINT32 HighIndex; // offset 12-15, only valid for MemoryMapped UINT64 Value; // offset 16-23 + UINT8 DetectIt; // 0ffset 24 } CPU_REGISTER_TABLE_ENTRY; // diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h index e420e7f075..87fe87acb7 100644 --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h @@ -335,6 +335,7 @@ SwitchBspAfterFeaturesInitialize ( @param[in] IndexIndex of the register to program @param[in] ValueMaskMask of bits in register to write @param[in] ValueValue to write + @param[in] DetectIt Whether need to detect current Value before writing. @note This service could be called by BSP only. **/ @@ -345,7 +346,8 @@ CpuRegisterTableWrite ( IN REGISTER_TYPE RegisterType, IN UINT64 Index, IN UINT64 ValueMask, - IN UINT64 Value + IN UINT64 Value, + IN UINT8 DetectIt ); /** @@ -385,9 +387,45 @@ PreSmmCpuRegisterTableWrite ( @note This service could be called by BSP only. **/ -#define CPU_REGISTER_TABLE_WRITE32(ProcessorNumber, RegisterType, Index, Value) \ - do { \ -CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, Value); \ +#define CPU_REGISTER_TABLE_WRITE32(ProcessorNumber, RegisterType, Index, Value) \ + do { \ +CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, Value, FALSE); \ + } while(FALSE); + +/** + Adds a 32-bit register write entry in specified register table. + + This macro adds an entry in specified register table, with given register type, + register index, and value. + + @param[in] ProcessorNumber The index of the CPU to add a register table entry. + @param[in] RegisterType Type of the register to program + @param[in] IndexIndex of the register to program + @param[in] ValueValue to write + + @note This service could be called by BSP only. +**/ +#define CPU_REGISTER_TABLE_TEST_THEN_WRITE32(ProcessorNumber, RegisterType, Index, Value) \ + do { \ +CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, Value, TRUE); \ + } while(FALSE); + +/** + Adds a 64-bit register write entry in specified register table. + + This macro adds an entry in specified register table, with given register type, + register index, and value. + + @param[in] ProcessorNumber The index of the CPU to add a register table entry. + @param[in] RegisterType Type of the register to program + @param[in] IndexIndex of the register to program + @param[in] ValueValue to write + + @note This service could be called by BSP only. +**/ +#define CPU_REGISTER_TABLE_WRITE64(ProcessorNumber, RegisterType, Index, Value) \ + do { \ +CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT64, Value, FALSE); \ } while(FALSE); /** @@ -403,9 +441,9 @@ PreSmmCpuRegisterTableWrite ( @note This service could be called by BSP only. **/ -#define CPU_REGISTER_TABLE_WRITE64(ProcessorNumber, RegisterType, Index, Value) \ - do { \ -CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT64, Value); \ +#define CPU_REGISTER_TABLE_TEST_THEN_WRITE64(ProcessorNumber, RegisterType, Index, Value) \ + do { \ +CpuRegisterTableWrite