Re: [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Laszlo Ersek <lersek@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4746 bytes --]

Hi Ray,

On 11/10/17 01:52, Ni, Ruiyu wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, November 9, 2017 9:16 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Jeff Fan <vanjeff_919@hotmail.com>
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org;
>> Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ard
>> Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to
>> calculate optimal settings

>> (1b) is an option we may or may not want to exercise in OVMF. I have the
>> patches ready for enlarging the temp SEC/PEI RAM (and as part of that, the
>> temp stack), which is one alternative. However, because OVMF's PEI phase runs
>> from RAM (and not flash), the other alternative is just to add a sufficiently large
>> static UINT8 array to PlatformPei, and pass that as scratch space to MtrrLib.

>> Is my understanding correct that MtrrSetMemoryAttribute() is the only function
>> that is affected?
> 
> 1. yes. Only MtrrSetMemoryAttribute() call in OVMF is affected.

>> (3) Is my understanding correct that
>> MtrrSetMemoryAttributesInMtrrSettings() should be used like this:
>>
>> (3a) start with MtrrGetAllMtrrs()
>>
>> (3b) collect all *foreseeable* MtrrSetMemoryAttribute() calls into an
>>      array of MTRR_MEMORY_RANGE elements
>>
>> (3c) Perform a batch update on the output of (3a) by calling
>>      MtrrSetMemoryAttributesInMtrrSettings(). For this, the array from
>>      (3b), plus a caller-allocated scratch space, have to be passed in,.
>>
>> (3d) Finally, call MtrrSetAllMtrrs().
>>
>> Is this correct?
> 
> 2. yes. In summary, there are three ways to call this new API. The first way is what
>     you described. The second way is a bit change to (3a), ZeroMem() is called 
>     instead of MtrrGetAllMtrrs() to initialize the MTRR. The third way is to call
>     this new API using NULL MtrrSetting, which cause the API itself to retrieve
>     the current MTRR settings from CPU, apply the new setting, write to CPU.
>     But the third way doesn't support batch setting.
> 
>>
>> I think we could use this. Jordan, which alternative do you prefer; larger stack
>> and unchanged code in PlatformPei, or same stack and updated code in
>> PlatformPei?
>>
>>
>> (4) Ray: would it be possible to expose SCRATCH_BUFFER_SIZE (with a new
>> name MTRR_SCRATCH_BUFFER_SIZE) in the library class header? I see the new
>> RETURN_BUFFER_TOO_SMALL status codes, and I don't really want to deal with
>> them. The library class header should provide clients with a size macro that
>> *guarantees* that RETURN_BUFFER_TOO_SMALL will not occur.
>>
>> Practically speaking, I would use MTRR_SCRATCH_BUFFER_SIZE in the definition
>> of the static UINT8 array in PlatformPei. (If Jordan prefers this alternative to the
>> larger temp stack.)
> 
> 3. Not quite correct. Because even when pass in the scratch buffer whose size equal
>     to MTRR_SCRATCH_BUFFER_SIZE, the BUFFER_TOO_SMALL could be returned.
>     That's why the BUFFER_TOO_SMALL status is invented. It requires caller to re-supply
>     the enough scratch buffer for calculation.
>     As such, I do not think exposing SCRATCH_BUFFER_SIZE helps.
>     When implementing the code, I tried to find out the maximum scratch buffer size but
>     found that the maximum could be up to 256KB. I cannot use such large stack because
>     as Jordan said, MSVC will inject some code results in unresolved symbol in EDKII code.
>     And DxeIpl only allocates 128KB stack for whole DXE phase.

Thank you very much for the explanation!

I have an untested prototype for (1b), using a 16KB static array as
MtrrLib scratch space, in OvmfPkg/PlatformPei. In the compressed
FVMAIN_COMPACT volume, its size impact is 320 bytes only. (For
illustration, I'm attaching this "proof of concept".)

However, after some more thinking, I dislike this approach.

First, I'd like to keep this added complexity out of PlatformPei, if
possible.

Second, a 16KB growth in PEIFV (current total size: 896 KB) just to
preserve the "status quo" is not really nice; we could use that space
for including executable code and related static data.

Third, this scratch space cannot be used for any other purpose. A larger
temp stack is generally available to other functions in PlatformPei, and
to all other PEIMs as well. In particular, if OVMF included a PEIM in
the future that used the traditional MtrrSetMemoryAttribute() API, then
PlatformPei's dedicated scratch space could not be shared by that PEIM.

So, I'll go ahead and post the variant that grows the temp SEC/PEI RAM
for OVMF.

Thanks!
Laszlo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-OvmfPkg-PlatformPei-manage-MtrrLib-scratch-space-manually.patch --]
[-- Type: text/x-patch; name="0001-OvmfPkg-PlatformPei-manage-MtrrLib-scratch-space-manually.patch", Size: 6133 bytes --]

From 796092bd81390a7f398ca923c509838d36d4b97d Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Fri, 10 Nov 2017 14:56:09 +0100
Subject: [PATCH] OvmfPkg/PlatformPei: manage MtrrLib scratch space manually

This is an untested prototype, as an alternative to growing the temp
SEC/PEI stack. Refer to:

- https://bugzilla.tianocore.org/show_bug.cgi?id=747
- http://mid.mail-archive.com/9474d983-b1c1-b83b-34ef-10bb84586ef6@redhat.com

This patch increases the PEIFV usage by 16KB:

> -PEIFV [39%Full] 917504 total, 365576 used, 551928 free
> +PEIFV [41%Full] 917504 total, 380552 used, 536952 free

while the difference for FVMAIN_COMPACT is just 320 bytes:

> -FVMAIN_COMPACT [43%Full] 3440640 total, 1508752 used, 1931888 free
> +FVMAIN_COMPACT [43%Full] 3440640 total, 1508432 used, 1932208 free

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformPei/Platform.h  |  4 ++++
 OvmfPkg/PlatformPei/MemDetect.c | 42 ++++++++++++++++++++++++++++++++++-------
 OvmfPkg/PlatformPei/Platform.c  |  2 ++
 OvmfPkg/PlatformPei/Xen.c       | 26 ++++++++++++++++++++++++-
 4 files changed, 66 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
index f942e61bb4f9..513a2e2373e6 100644
--- a/OvmfPkg/PlatformPei/Platform.h
+++ b/OvmfPkg/PlatformPei/Platform.h
@@ -17,6 +17,8 @@
 
 #include <IndustryStandard/E820.h>
 
+#define MTRR_LIB_SCRATCH_BUFFER_SIZE SIZE_16KB
+
 VOID
 AddIoMemoryBaseSizeHob (
   EFI_PHYSICAL_ADDRESS        MemoryBase,
@@ -115,4 +117,6 @@ extern UINT32 mMaxCpuCount;
 
 extern UINT16 mHostBridgeDevId;
 
+extern UINT8 mMtrrLibScratchBuffer[MTRR_LIB_SCRATCH_BUFFER_SIZE];
+
 #endif // _PLATFORM_PEI_H_INCLUDED_
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 2b2f3e4bec55..1d09dc44b69b 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -595,6 +595,8 @@ QemuInitializeRam (
   UINT64                      LowerMemorySize;
   UINT64                      UpperMemorySize;
   MTRR_SETTINGS               MtrrSettings;
+  MTRR_MEMORY_RANGE           Ranges[2];
+  UINTN                       MtrrLibScratchBufferSize;
   EFI_STATUS                  Status;
 
   DEBUG ((EFI_D_INFO, "%a called\n", __FUNCTION__));
@@ -682,22 +684,48 @@ QemuInitializeRam (
     SetMem (&MtrrSettings.Fixed, sizeof MtrrSettings.Fixed, 0x06);
     ZeroMem (&MtrrSettings.Variables, sizeof MtrrSettings.Variables);
     MtrrSettings.MtrrDefType |= BIT11 | BIT10 | 6;
-    MtrrSetAllMtrrs (&MtrrSettings);
 
     //
     // Set memory range from 640KB to 1MB to uncacheable
     //
-    Status = MtrrSetMemoryAttribute (BASE_512KB + BASE_128KB,
-               BASE_1MB - (BASE_512KB + BASE_128KB), CacheUncacheable);
-    ASSERT_EFI_ERROR (Status);
+    Ranges[0].BaseAddress = BASE_512KB + BASE_128KB;
+    Ranges[0].Length      = BASE_1MB - Ranges[0].BaseAddress;
+    Ranges[0].Type        = CacheUncacheable;
 
     //
     // Set memory range from the "top of lower RAM" (RAM below 4GB) to 4GB as
     // uncacheable
     //
-    Status = MtrrSetMemoryAttribute (LowerMemorySize,
-               SIZE_4GB - LowerMemorySize, CacheUncacheable);
-    ASSERT_EFI_ERROR (Status);
+    Ranges[1].BaseAddress = LowerMemorySize;
+    Ranges[1].Length      = SIZE_4GB - LowerMemorySize;
+    Ranges[1].Type        = CacheUncacheable;
+
+    //
+    // Apply Ranges to MtrrSettings.
+    //
+    MtrrLibScratchBufferSize = sizeof mMtrrLibScratchBuffer;
+    Status = MtrrSetMemoryAttributesInMtrrSettings (
+               &MtrrSettings,
+               mMtrrLibScratchBuffer,
+               &MtrrLibScratchBufferSize,
+               Ranges,
+               ARRAY_SIZE (Ranges)
+               );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: MtrrSetMemoryAttributesInMtrrSettings(): %r (ScratchSize=%Lu)\n",
+        __FUNCTION__,
+        Status,
+        (UINT64)MtrrLibScratchBufferSize
+        ));
+      ASSERT (FALSE);
+    }
+
+    //
+    // Program the hardware with MtrrSettings.
+    //
+    MtrrSetAllMtrrs (&MtrrSettings);
   }
 }
 
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 544ac547bf5f..db16e6756fda 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -71,6 +71,8 @@ BOOLEAN mS3Supported = FALSE;
 
 UINT32 mMaxCpuCount;
 
+UINT8 mMtrrLibScratchBuffer[MTRR_LIB_SCRATCH_BUFFER_SIZE];
+
 VOID
 AddIoMemoryBaseSizeHob (
   EFI_PHYSICAL_ADDRESS        MemoryBase,
diff --git a/OvmfPkg/PlatformPei/Xen.c b/OvmfPkg/PlatformPei/Xen.c
index ab38f97a67aa..f06c258d55c0 100644
--- a/OvmfPkg/PlatformPei/Xen.c
+++ b/OvmfPkg/PlatformPei/Xen.c
@@ -181,6 +181,9 @@ XenPublishRamRegions (
     UINT32 Loop;
 
     for (Loop = 0; Loop < E820EntriesCount; Loop++) {
+      MTRR_MEMORY_RANGE Range;
+      UINTN             MtrrLibScratchBufferSize;
+
       Entry = E820Map + Loop;
 
       //
@@ -192,7 +195,28 @@ XenPublishRamRegions (
 
       AddMemoryBaseSizeHob (Entry->BaseAddr, Entry->Length);
 
-      MtrrSetMemoryAttribute (Entry->BaseAddr, Entry->Length, CacheWriteBack);
+      Range.BaseAddress        = Entry->BaseAddr;
+      Range.Length             = Entry->Length;
+      Range.Type               = CacheWriteBack;
+      MtrrLibScratchBufferSize = sizeof mMtrrLibScratchBuffer;
+
+      Status = MtrrSetMemoryAttributesInMtrrSettings (
+                 NULL,                      // MtrrSetting
+                 mMtrrLibScratchBuffer,
+                 &MtrrLibScratchBufferSize,
+                 &Range,
+                 1
+                 );
+      if (EFI_ERROR (Status)) {
+        DEBUG ((
+          DEBUG_WARN,
+          ("%a: MtrrSetMemoryAttributesInMtrrSettings(): %r "
+           "(ScratchSize=%Lu)\n"),
+          __FUNCTION__,
+          Status,
+          (UINT64)MtrrLibScratchBufferSize
+          ));
+      }
     }
   }
 }
-- 
2.14.1.3.gb7cf6e02401b