* [PATCH v1 0/1] Clean up save state boundary checks and comments in functions ReadSaveStateRegisterByIndex and WriteSaveStateRegister @ 2020-10-15 16:32 Mark Wilson 2020-10-15 16:32 ` [PATCH v1 1/1] UefiCpuPkg: Clean up save state boundary checks and comments Mark Wilson 0 siblings, 1 reply; 6+ messages in thread From: Mark Wilson @ 2020-10-15 16:32 UTC (permalink / raw) To: devel; +Cc: Mark Wilson The update does the following: * For functions ReadSaveStateRegisterByIndex and WriteSaveStateRegister, the boundary check for register copy incorrectly includes if the width is DWORD to determine if to write upper 32-bits. This is clean up as in this case number of bytes to copy is 0. * Correct comments for above functions. * Correct coding style spacing for CopyMem and ASSERT in above functions. Mark Wilson (1): UefiCpuPkg: Clean up save state boundary checks and comments. UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 26 ++++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) -- 2.27.0.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v1 1/1] UefiCpuPkg: Clean up save state boundary checks and comments. 2020-10-15 16:32 [PATCH v1 0/1] Clean up save state boundary checks and comments in functions ReadSaveStateRegisterByIndex and WriteSaveStateRegister Mark Wilson @ 2020-10-15 16:32 ` Mark Wilson 2020-10-19 18:03 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Mark Wilson @ 2020-10-15 16:32 UTC (permalink / raw) To: devel; +Cc: Mark Wilson, Eric Dong, Ray Ni REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2956 In functions ReadSaveStateRegisterByIndex and WriteSaveStateRegister: * check width > 4 instead of >= 4 when writing upper 32 bytes. - This improves the code but will not affect functionality. * Update related comments to better describe code. Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Signed-off-by: Mark Wilson <Mark.Wilson@amd.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 26 ++++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c index 661cc51f361a..4a872e400b7a 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c @@ -297,8 +297,8 @@ ReadSaveStateRegisterByIndex ( // // Write return buffer // - ASSERT(CpuSaveState != NULL); - CopyMem(Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Width); + ASSERT (CpuSaveState != NULL); + CopyMem (Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Width); } else { // // If 64-bit mode width is zero, then the specified register can not be accessed @@ -315,14 +315,14 @@ ReadSaveStateRegisterByIndex ( } // - // Write lower 32-bits of return buffer + // Write at most 4 of the lower bytes of the return buffer // - CopyMem(Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, MIN(4, Width)); - if (Width >= 4) { + CopyMem (Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, MIN(4, Width)); + if (Width > 4) { // - // Write upper 32-bits of return buffer + // Write at most 4 of the upper bytes of the return buffer // - CopyMem((UINT8 *)Buffer + 4, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, Width - 4); + CopyMem ((UINT8 *)Buffer + 4, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, Width - 4); } } return EFI_SUCCESS; @@ -522,7 +522,7 @@ WriteSaveStateRegister ( // Write SMM State register // ASSERT (CpuSaveState != NULL); - CopyMem((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Buffer, Width); + CopyMem ((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Buffer, Width); } else { // // If 64-bit mode width is zero, then the specified register can not be accessed @@ -539,14 +539,14 @@ WriteSaveStateRegister ( } // - // Write lower 32-bits of SMM State register + // Write at most 4 of the lower bytes of SMM State register // - CopyMem((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, Buffer, MIN (4, Width)); - if (Width >= 4) { + CopyMem ((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, Buffer, MIN (4, Width)); + if (Width > 4) { // - // Write upper 32-bits of SMM State register + // Write at most 4 of the upper bytes of SMM State register // - CopyMem((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, (UINT8 *)Buffer + 4, Width - 4); + CopyMem ((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, (UINT8 *)Buffer + 4, Width - 4); } } return EFI_SUCCESS; -- 2.27.0.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] UefiCpuPkg: Clean up save state boundary checks and comments. 2020-10-15 16:32 ` [PATCH v1 1/1] UefiCpuPkg: Clean up save state boundary checks and comments Mark Wilson @ 2020-10-19 18:03 ` Laszlo Ersek 2020-10-19 18:06 ` Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2020-10-19 18:03 UTC (permalink / raw) To: devel, Mark.Wilson; +Cc: Eric Dong, Ray Ni Hi Mark, On 10/15/20 18:32, Mark Wilson wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2956 > > In functions ReadSaveStateRegisterByIndex and WriteSaveStateRegister: > * check width > 4 instead of >= 4 when writing upper 32 bytes. > - This improves the code but will not affect functionality. > * Update related comments to better describe code. > > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Signed-off-by: Mark Wilson <Mark.Wilson@amd.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 26 ++++++++++---------- > 1 file changed, 13 insertions(+), 13 deletions(-) I suggest separating the cosmetic changes (such as the whitespace fixes) from the cleanups (comments etc) to different patches. Also, please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone. Thanks, Laszlo > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c > index 661cc51f361a..4a872e400b7a 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c > @@ -297,8 +297,8 @@ ReadSaveStateRegisterByIndex ( > // > > // Write return buffer > > // > > - ASSERT(CpuSaveState != NULL); > > - CopyMem(Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Width); > > + ASSERT (CpuSaveState != NULL); > > + CopyMem (Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Width); > > } else { > > // > > // If 64-bit mode width is zero, then the specified register can not be accessed > > @@ -315,14 +315,14 @@ ReadSaveStateRegisterByIndex ( > } > > > > // > > - // Write lower 32-bits of return buffer > > + // Write at most 4 of the lower bytes of the return buffer > > // > > - CopyMem(Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, MIN(4, Width)); > > - if (Width >= 4) { > > + CopyMem (Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, MIN(4, Width)); > > + if (Width > 4) { > > // > > - // Write upper 32-bits of return buffer > > + // Write at most 4 of the upper bytes of the return buffer > > // > > - CopyMem((UINT8 *)Buffer + 4, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, Width - 4); > > + CopyMem ((UINT8 *)Buffer + 4, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, Width - 4); > > } > > } > > return EFI_SUCCESS; > > @@ -522,7 +522,7 @@ WriteSaveStateRegister ( > // Write SMM State register > > // > > ASSERT (CpuSaveState != NULL); > > - CopyMem((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Buffer, Width); > > + CopyMem ((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Buffer, Width); > > } else { > > // > > // If 64-bit mode width is zero, then the specified register can not be accessed > > @@ -539,14 +539,14 @@ WriteSaveStateRegister ( > } > > > > // > > - // Write lower 32-bits of SMM State register > > + // Write at most 4 of the lower bytes of SMM State register > > // > > - CopyMem((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, Buffer, MIN (4, Width)); > > - if (Width >= 4) { > > + CopyMem ((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, Buffer, MIN (4, Width)); > > + if (Width > 4) { > > // > > - // Write upper 32-bits of SMM State register > > + // Write at most 4 of the upper bytes of SMM State register > > // > > - CopyMem((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, (UINT8 *)Buffer + 4, Width - 4); > > + CopyMem ((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, (UINT8 *)Buffer + 4, Width - 4); > > } > > } > > return EFI_SUCCESS; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] UefiCpuPkg: Clean up save state boundary checks and comments. 2020-10-19 18:03 ` [edk2-devel] " Laszlo Ersek @ 2020-10-19 18:06 ` Laszlo Ersek 2020-11-11 16:20 ` Mark Wilson 0 siblings, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2020-10-19 18:06 UTC (permalink / raw) To: devel, Mark.Wilson; +Cc: Eric Dong, Ray Ni On 10/19/20 20:03, Laszlo Ersek wrote: > Hi Mark, > > On 10/15/20 18:32, Mark Wilson wrote: >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2956 >> >> In functions ReadSaveStateRegisterByIndex and WriteSaveStateRegister: >> * check width > 4 instead of >= 4 when writing upper 32 bytes. >> - This improves the code but will not affect functionality. >> * Update related comments to better describe code. >> >> Cc: Eric Dong <eric.dong@intel.com> >> Cc: Ray Ni <ray.ni@intel.com> >> Signed-off-by: Mark Wilson <Mark.Wilson@amd.com> >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 26 ++++++++++---------- >> 1 file changed, 13 insertions(+), 13 deletions(-) > > I suggest separating the cosmetic changes (such as the whitespace fixes) > from the cleanups (comments etc) to different patches. > > Also, please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone. Furthermore, I suggest a bit more specific subject: UefiCpuPkg/PiSmmCpuDxeSmm: clean up save state boundary checks and comments (I think this subject length, i.e. 75 characters, should still satisfy "PatchCheck.py".) Sorry about the piecemeal observations; trying to deal with a bit of a backlog. Thanks Laszlo >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c >> index 661cc51f361a..4a872e400b7a 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c >> @@ -297,8 +297,8 @@ ReadSaveStateRegisterByIndex ( >> // >> >> // Write return buffer >> >> // >> >> - ASSERT(CpuSaveState != NULL); >> >> - CopyMem(Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Width); >> >> + ASSERT (CpuSaveState != NULL); >> >> + CopyMem (Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Width); >> >> } else { >> >> // >> >> // If 64-bit mode width is zero, then the specified register can not be accessed >> >> @@ -315,14 +315,14 @@ ReadSaveStateRegisterByIndex ( >> } >> >> >> >> // >> >> - // Write lower 32-bits of return buffer >> >> + // Write at most 4 of the lower bytes of the return buffer >> >> // >> >> - CopyMem(Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, MIN(4, Width)); >> >> - if (Width >= 4) { >> >> + CopyMem (Buffer, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, MIN(4, Width)); >> >> + if (Width > 4) { >> >> // >> >> - // Write upper 32-bits of return buffer >> >> + // Write at most 4 of the upper bytes of the return buffer >> >> // >> >> - CopyMem((UINT8 *)Buffer + 4, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, Width - 4); >> >> + CopyMem ((UINT8 *)Buffer + 4, (UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, Width - 4); >> >> } >> >> } >> >> return EFI_SUCCESS; >> >> @@ -522,7 +522,7 @@ WriteSaveStateRegister ( >> // Write SMM State register >> >> // >> >> ASSERT (CpuSaveState != NULL); >> >> - CopyMem((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Buffer, Width); >> >> + CopyMem ((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset32, Buffer, Width); >> >> } else { >> >> // >> >> // If 64-bit mode width is zero, then the specified register can not be accessed >> >> @@ -539,14 +539,14 @@ WriteSaveStateRegister ( >> } >> >> >> >> // >> >> - // Write lower 32-bits of SMM State register >> >> + // Write at most 4 of the lower bytes of SMM State register >> >> // >> >> - CopyMem((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, Buffer, MIN (4, Width)); >> >> - if (Width >= 4) { >> >> + CopyMem ((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Lo, Buffer, MIN (4, Width)); >> >> + if (Width > 4) { >> >> // >> >> - // Write upper 32-bits of SMM State register >> >> + // Write at most 4 of the upper bytes of SMM State register >> >> // >> >> - CopyMem((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, (UINT8 *)Buffer + 4, Width - 4); >> >> + CopyMem ((UINT8 *)CpuSaveState + mSmmCpuWidthOffset[RegisterIndex].Offset64Hi, (UINT8 *)Buffer + 4, Width - 4); >> >> } >> >> } >> >> return EFI_SUCCESS; >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] UefiCpuPkg: Clean up save state boundary checks and comments. 2020-10-19 18:06 ` Laszlo Ersek @ 2020-11-11 16:20 ` Mark Wilson 2020-11-11 22:37 ` Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Mark Wilson @ 2020-11-11 16:20 UTC (permalink / raw) To: Laszlo Ersek, devel [-- Attachment #1: Type: text/plain, Size: 264 bytes --] I've been swamped and finally getting back to this. I'll update the subject line as suggested. As far as unrelated cosmetic changes, I can just drop them from this. I don't like having a separate commit just for some cosmetic changes such as spacing. -Mark [-- Attachment #2: Type: text/html, Size: 422 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] UefiCpuPkg: Clean up save state boundary checks and comments. 2020-11-11 16:20 ` Mark Wilson @ 2020-11-11 22:37 ` Laszlo Ersek 0 siblings, 0 replies; 6+ messages in thread From: Laszlo Ersek @ 2020-11-11 22:37 UTC (permalink / raw) To: devel, mark.wilson On 11/11/20 17:20, Mark Wilson wrote: > I've been swamped and finally getting back to this. I'll update the subject line as suggested. > > As far as unrelated cosmetic changes, I can just drop them from this. I don't like having a separate commit just for some cosmetic changes such as spacing. Dropping the whitespace modifications is OK. Thanks! Laszlo ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-11 22:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-15 16:32 [PATCH v1 0/1] Clean up save state boundary checks and comments in functions ReadSaveStateRegisterByIndex and WriteSaveStateRegister Mark Wilson 2020-10-15 16:32 ` [PATCH v1 1/1] UefiCpuPkg: Clean up save state boundary checks and comments Mark Wilson 2020-10-19 18:03 ` [edk2-devel] " Laszlo Ersek 2020-10-19 18:06 ` Laszlo Ersek 2020-11-11 16:20 ` Mark Wilson 2020-11-11 22:37 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox