On 06/29/16 04:29, Jiewen Yao wrote:
> PI specification Vol 4 - SMM does not have any limitation of BASE_4GB for SMM.
> So we should replace BASE_4GB check with MAX_ADDRESS check to make sure
> the SMM memory is accessible by SMM Core.
>
> Cc: Jeff Fan <[email protected]>
> Cc: Michael D Kinney <[email protected]>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <[email protected]>
> ---
> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 2 +-
> MdeModulePkg/Core/PiSmmCore/Pool.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index acfcc83..3788bd5 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -1556,7 +1556,7 @@ SmmIplEntry (
> }
>
> if (gSmmCorePrivate->SmramRanges[Index].CpuStart >= BASE_1MB) {
> - if ((gSmmCorePrivate->SmramRanges[Index].CpuStart +
> gSmmCorePrivate->SmramRanges[Index].PhysicalSize) <= BASE_4GB) {
> + if ((gSmmCorePrivate->SmramRanges[Index].CpuStart +
> gSmmCorePrivate->SmramRanges[Index].PhysicalSize) <= MAX_ADDRESS) {
> if (gSmmCorePrivate->SmramRanges[Index].PhysicalSize >= MaxSize) {
> MaxSize = gSmmCorePrivate->SmramRanges[Index].PhysicalSize;
> mCurrentSmramRange = &gSmmCorePrivate->SmramRanges[Index];
> diff --git a/MdeModulePkg/Core/PiSmmCore/Pool.c
> b/MdeModulePkg/Core/PiSmmCore/Pool.c
> index 761988e..e493fd2 100644
> --- a/MdeModulePkg/Core/PiSmmCore/Pool.c
> +++ b/MdeModulePkg/Core/PiSmmCore/Pool.c
> @@ -67,7 +67,7 @@ SmmInitializeMemoryServices (
> }
>
> if (SmramRanges[Index].CpuStart >= BASE_1MB) {
> - if ((SmramRanges[Index].CpuStart + SmramRanges[Index].PhysicalSize)
> <= BASE_4GB) {
> + if ((SmramRanges[Index].CpuStart + SmramRanges[Index].PhysicalSize)
> <= MAX_ADDRESS) {
> if (SmramRanges[Index].PhysicalSize >= MaxSize) {
> MaxSize = SmramRanges[Index].PhysicalSize;
> CurrentSmramRangesIndex = Index;
>
I have one question and two suggestions:
(1) These checks refer to the SMRAM ranges that are reported by
EFI_SMM_ACCESS2_PROTOCOL.GetCapabilities() member function, correct?
So whatever worked before should continue to work.
(2) The comparison is not appropriate here (after the patch); it is too
strict.
BASE_4GB is an exclusive limit, hence the pre-patch status is good. But
MAX_ADDRESS is an inclusive limit, so the condition should be
CpuStart + PhysicalSize - 1 <= MAX_ADDRESS
(3) The addition in (2) can easily overflow (wrap around) before the
comparison. Therefore I recommend the following steps instead:
* First, reorder the LHS of the formula in (2):
(CpuStart - 1) + PhysicalSize <= MAX_ADDRESS
The subtraction is safe, because we have an outermost condition that
ensures (CpuStart >= BASE_1MB).
* Second, subtract PhysicalSize from both sides:
CpuStart - 1 <= MAX_ADDRESS - PhysicalSize
Considering X64 only, this could be the final form: MAX_ADDRESS is
MAX_UINT64, so the subtraction on the right hand side is safe too.
* However, that is not the case on Ia32. There we must compare
MAX_ADDRESS vs. PhysicalSize explicitly. Which leaves us with:
if (PhysicalSize <= MAX_ADDRESS &&
CpuStart - 1 <= MAX_ADDRESS - PhysicalSize) {
/* ... */
}
This is the final form that I'm proposing for the patch.
The first condition is always true on X64, but it has "teeth" on Ia32.
Then, once the first condition is true, the RHS of the second condition
will not underflow, and we can safely do the comparison. (The LHS of the
second condition will not underflow due to the argument about CpuStart
above.)
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
