From: "Zeng, Star" <star.zeng@intel.com>
To: "Dong, Eric" <eric.dong@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Ni, Ray" <ray.ni@intel.com>, Laszlo Ersek <lersek@redhat.com>,
"Yao, Jiewen" <jiewen.yao@intel.com>,
"Gao, Liming" <liming.gao@intel.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"Zeng, Star" <star.zeng@intel.com>
Subject: Re: [Patch v3 1/6] UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test Then Write" Macros.
Date: Fri, 16 Aug 2019 02:08:03 +0000 [thread overview]
Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483104039C7D8@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <ED077930C258884BBCB450DB737E662259EC3435@shsmsx102.ccr.corp.intel.com>
> -----Original Message-----
> From: Dong, Eric
> Sent: Friday, August 16, 2019 9:27 AM
> To: Zeng, Star <star.zeng@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [Patch v3 1/6] UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test
> Then Write" Macros.
>
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Friday, August 16, 2019 9:15 AM
> > To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > Zeng, Star <star.zeng@intel.com>
> > Subject: RE: [Patch v3 1/6] UefiCpuPkg/RegisterCpuFeaturesLib: Add
> > "Test Then Write" Macros.
> >
> >
> >
> > > -----Original Message-----
> > > From: Dong, Eric
> > > Sent: Thursday, August 15, 2019 10:51 AM
> > > To: devel@edk2.groups.io
> > > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > > Zeng, Star <star.zeng@intel.com>
> > > Subject: [Patch v3 1/6] UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test
> > > Then Write" Macros.
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
> > >
> > > Add below new micros which test the current value before write the
> > > new value. Only write 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
> > >
> > > Also add below API:
> > > CpuRegisterTableTestThenWrite
> > >
> > > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Star Zeng <star.zeng@intel.com>
> > > ---
> > > UefiCpuPkg/Include/AcpiCpuData.h | 1 +
> > > .../Include/Library/RegisterCpuFeaturesLib.h | 91
> +++++++++++++++++++
> > > .../RegisterCpuFeaturesLib.c | 45 ++++++++-
> > > 3 files changed, 134 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/Include/AcpiCpuData.h
> > > b/UefiCpuPkg/Include/AcpiCpuData.h
> > > index b963a2f592..472a1a8070 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 TestThenWrite; // 0ffset 24
> >
> > Could we use one byte of the Reserved field, but not add new field?
> > And use BOOLEAN type for it?
>
> I'm not sure whether use the Reserved field is an correct approach, do you
> have samples which use the reserved fields?
> But I think add new field is a more safe one.
What "more safe" means here? Adding new field extends the structure, from the structure layout view of point, it is an incompatible change, the structure size is not just from 24 to 25 bytes, but to be nature aligned, the structure size will be (24 + 8) 32. Since there is Reserved field can be reused, that size impact can be removed.
FspGlobalData.h:
SHA-1: a2e61f341d26a78751b2f19b5004c6bbfc8b4fa9
* IntelFsp2Pkg: Support FSP Dispatch mode
MemoryProfile.h
SHA-1: 072a3ca1d36a42aec97f871c808776ee7038ca06 (related to 94092aa60341a3e4b1e1ea7c362781b8404ac538)
* MdeModulePkg MemoryProfile.h:two bytes of Reserved[4] as ActionStringOffset
If needed, more comments from Ray, Laszlo, Jiewen, Liming and Mike will be better.
Thanks,
Star
>
> Thanks,
> Eric
> >
> > Thanks,
> > Star
> >
> > > } CPU_REGISTER_TABLE_ENTRY;
> > >
> > > //
> > > diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > index e420e7f075..5bd464b32e 100644
> > > --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > @@ -348,6 +348,32 @@ CpuRegisterTableWrite (
> > > IN UINT64 Value
> > > );
> > >
> > > +/**
> > > + Adds an entry in specified register table.
> > > +
> > > + This function adds an entry in specified register table, with
> > > + given register type, register index, bit section and value.
> > > +
> > > + Driver will test the current value before setting new 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] ValueMask Mask of bits in register to write
> > > + @param[in] Value Value to write
> > > +
> > > + @note This service could be called by BSP only.
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +CpuRegisterTableTestThenWrite (
> > > + IN UINTN ProcessorNumber,
> > > + IN REGISTER_TYPE RegisterType,
> > > + IN UINT64 Index,
> > > + IN UINT64 ValueMask,
> > > + IN UINT64 Value
> > > + );
> > > +
> > > /**
> > > Adds an entry in specified Pre-SMM register table.
> > >
> > > @@ -390,6 +416,26 @@ PreSmmCpuRegisterTableWrite (
> > > CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> > > MAX_UINT32, Value); \
> > > } 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.
> > > +
> > > + Driver will test the current value before setting new 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 { \
> > > + CpuRegisterTableTestThenWrite (ProcessorNumber, RegisterType,
> > > +Index, MAX_UINT32, Value); \
> > > + } while(FALSE);
> > > +
> > > /**
> > > Adds a 64-bit register write entry in specified register table.
> > >
> > > @@ -408,6 +454,26 @@ PreSmmCpuRegisterTableWrite (
> > > CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> > > MAX_UINT64, Value); \
> > > } 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.
> > > +
> > > + Driver will test the current value before setting new 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_WRITE64(ProcessorNumber,
> > > RegisterType, Index, Value) \
> > > + do { \
> > > + CpuRegisterTableTestThenWrite (ProcessorNumber, RegisterType,
> > > +Index, MAX_UINT64, Value); \
> > > + } while(FALSE);
> > > +
> > > /**
> > > Adds a bit field write entry in specified register table.
> > >
> > > @@ -431,6 +497,31 @@ PreSmmCpuRegisterTableWrite (
> > > CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> > > ~ValueMask, Value); \
> > > } 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.
> > > +
> > > + Driver will test the current value before setting new 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;
> \
> > > + CpuRegisterTableTestThenWrite (ProcessorNumber, RegisterType,
> > > + Index,
> > > ~ValueMask, Value); \
> > > + } while(FALSE);
> > > +
> > > /**
> > > Adds a 32-bit register write entry in specified register table.
> > >
> > > diff --git
> > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > > index 67885bf69b..e9769882b9 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] TestThenWrite Whether need to test 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 TestThenWrite
> > > )
> > > {
> > > 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].TestThenWrite =
> > > + TestThenWrite;
> > >
> > > RegisterTable->TableLength++;
> > > }
> > > @@ -1105,7 +1109,42 @@ 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, FALSE); }
> > > +
> > > +/**
> > > + Adds an entry in specified register table.
> > > +
> > > + This function adds an entry in specified register table, with
> > > + given register type, register index, bit 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] ValueMask Mask of bits in register to write
> > > + @param[in] Value Value to write
> > > + @param[in] TestThenWrite Whether need to test current Value
> before
> > > writing.
> > > +
> > > + @note This service could be called by BSP only.
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +CpuRegisterTableTestThenWrite (
> > > + IN UINTN ProcessorNumber,
> > > + IN REGISTER_TYPE RegisterType,
> > > + IN UINT64 Index,
> > > + IN UINT64 ValueMask,
> > > + IN UINT64 Value
> > > + )
> > > +{
> > > + UINT8 Start;
> > > + UINT8 End;
> > > + UINT8 Length;
> > > +
> > > + Start = (UINT8)LowBitSet64 (ValueMask);
> > > + End = (UINT8)HighBitSet64 (ValueMask);
> > > + Length = End - Start + 1;
> > > + CpuRegisterTableWriteWorker (FALSE, ProcessorNumber,
> > > + RegisterType, Index, Start, Length, Value, TRUE);
> > > }
> > >
> > > /**
> > > @@ -1139,7 +1178,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);
> > > }
> > >
> > > /**
> > > --
> > > 2.21.0.windows.1
next prev parent reply other threads:[~2019-08-16 2:08 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-15 2:50 [Patch v3 0/6] Add "test then write" mechanism Dong, Eric
2019-08-15 2:50 ` [Patch v3 1/6] UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test Then Write" Macros Dong, Eric
2019-08-16 1:14 ` Zeng, Star
2019-08-16 1:27 ` Dong, Eric
2019-08-16 2:08 ` Zeng, Star [this message]
2019-08-16 2:20 ` Dong, Eric
2019-08-16 3:44 ` Liming Gao
2019-08-16 3:58 ` Dong, Eric
2019-08-15 2:50 ` [Patch v3 2/6] UefiCpuPkg/PiSmmCpuDxeSmm: Combine CR read/write action Dong, Eric
2019-08-15 2:50 ` [Patch v3 3/6] UefiCpuPkg/PiSmmCpuDxeSmm: Supports test then write new value logic Dong, Eric
2019-08-15 2:50 ` [Patch v3 4/6] UefiCpuPkg/RegisterCpuFeaturesLib: Combine CR read/write action Dong, Eric
2019-08-15 2:50 ` [Patch v3 5/6] UefiCpuPkg/RegisterCpuFeaturesLib: Supports test then write new value logic Dong, Eric
2019-08-15 2:50 ` [Patch v3 6/6] UefiCpuPkg/CpuCommonFeaturesLib: Use new macros Dong, Eric
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0C09AFA07DD0434D9E2A0C6AEB0483104039C7D8@shsmsx102.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox