From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 09 Aug 2019 08:14:07 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5647A63764; Fri, 9 Aug 2019 15:14:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-119.ams2.redhat.com [10.36.117.119]) by smtp.corp.redhat.com (Postfix) with ESMTP id 48B9060C57; Fri, 9 Aug 2019 15:14:05 +0000 (UTC) Subject: Re: [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros. To: Eric Dong , devel@edk2.groups.io Cc: Ray Ni References: <20190809061159.40248-1-eric.dong@intel.com> <20190809061159.40248-2-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 9 Aug 2019 17:14:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190809061159.40248-2-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 09 Aug 2019 15:14:06 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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] Index Index of the register to program > @param[in] ValueMask Mask of bits in register to write > @param[in] Value Value 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] Index Index of the register to program > + @param[in] Value Value 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] Index Index of the register to program > + @param[in] Value Value 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 (ProcessorNumber, RegisterType, Index, MAX_UINT64, Value, TRUE); \ > } while(FALSE); > > /** > @@ -428,7 +466,30 @@ PreSmmCpuRegisterTableWrite ( > UINT64 ValueMask; \ > ValueMask = MAX_UINT64; \ > ((Type *)(&ValueMask))->Field = 0; \ > - CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, ~ValueMask, Value); \ > + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, ~ValueMask, Value, FALSE); \ > + } while(FALSE); > + > +/** > + Adds a bit field write entry in specified register table. > + > + This macro adds an entry in specified register table, with given register type, > + register index, bit field section, 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] Index Index of the register to program. > + @param[in] Type The data type name of a register structure. > + @param[in] Field The bit fiel name in register structure to write. > + @param[in] Value Value to write to the bit field. > + > + @note This service could be called by BSP only. > +**/ > +#define CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD(ProcessorNumber, RegisterType, Index, Type, Field, Value) \ > + do { \ > + UINT64 ValueMask; \ > + ValueMask = MAX_UINT64; \ > + ((Type *)(&ValueMask))->Field = 0; \ > + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, ~ValueMask, Value, TRUE); \ > } while(FALSE); > > /** > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > index 67885bf69b..152ab75988 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > @@ -1025,6 +1025,8 @@ EnlargeRegisterTable ( > @param[in] ValidBitStart Start of the bit section > @param[in] ValidBitLength Length of the bit section > @param[in] Value Value to write > + @param[in] DetectIt Whether need to detect current Value before writing. > + > **/ > VOID > CpuRegisterTableWriteWorker ( > @@ -1034,7 +1036,8 @@ CpuRegisterTableWriteWorker ( > IN UINT64 Index, > IN UINT8 ValidBitStart, > IN UINT8 ValidBitLength, > - IN UINT64 Value > + IN UINT64 Value, > + IN UINT8 DetectIt > ) > { > CPU_FEATURES_DATA *CpuFeaturesData; > @@ -1070,6 +1073,7 @@ CpuRegisterTableWriteWorker ( > RegisterTableEntry[RegisterTable->TableLength].ValidBitStart = ValidBitStart; > RegisterTableEntry[RegisterTable->TableLength].ValidBitLength = ValidBitLength; > RegisterTableEntry[RegisterTable->TableLength].Value = Value; > + RegisterTableEntry[RegisterTable->TableLength].DetectIt = DetectIt; > > RegisterTable->TableLength++; > } > @@ -1085,6 +1089,7 @@ CpuRegisterTableWriteWorker ( > @param[in] Index Index of the register to program > @param[in] ValueMask Mask of bits in register to write > @param[in] Value Value to write > + @param[in] DetectIt Whether need to detect current Value before writing. > > @note This service could be called by BSP only. > **/ > @@ -1095,7 +1100,8 @@ CpuRegisterTableWrite ( > IN REGISTER_TYPE RegisterType, > IN UINT64 Index, > IN UINT64 ValueMask, > - IN UINT64 Value > + IN UINT64 Value, > + IN UINT8 DetectIt > ) > { > UINT8 Start; > @@ -1105,7 +1111,7 @@ CpuRegisterTableWrite ( > Start = (UINT8)LowBitSet64 (ValueMask); > End = (UINT8)HighBitSet64 (ValueMask); > Length = End - Start + 1; > - CpuRegisterTableWriteWorker (FALSE, ProcessorNumber, RegisterType, Index, Start, Length, Value); > + CpuRegisterTableWriteWorker (FALSE, ProcessorNumber, RegisterType, Index, Start, Length, Value, DetectIt); > } > > /** > @@ -1139,7 +1145,7 @@ PreSmmCpuRegisterTableWrite ( > Start = (UINT8)LowBitSet64 (ValueMask); > End = (UINT8)HighBitSet64 (ValueMask); > Length = End - Start + 1; > - CpuRegisterTableWriteWorker (TRUE, ProcessorNumber, RegisterType, Index, Start, Length, Value); > + CpuRegisterTableWriteWorker (TRUE, ProcessorNumber, RegisterType, Index, Start, Length, Value, FALSE); > } > > /** >