Re: memory type information HOB / UEFI memmap defrag

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-02-21 18:46:01, Kinney, Michael D wrote:
> Laszlo,
> 
> The only side effect of not producing the HOB when the variable does
> not exist is that the first boot of a platform has a fragmented
> memory map and you may get an extra reboot when the variable is set.
> A fragmented memory map will also be produced if the variable store 
> contents are corrupt or zeroed.
> 

Would it be possible to inhibit the reboot until we fully support S4?

I think it'd be fine to have a fragmented map for one boot if it was
corrected on future boots of the machine.

I think we should consider continuing to produce the HOB in
PlatformPei if we can fairly easily reduce the fragmentation on the
first boot via the HOB.

-Jordan

> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Tuesday, February 21, 2017 6:31 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel-01 <edk2-
> > devel@ml01.01.org>
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Andrew Fish <afish@apple.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>
> > Subject: Re: [edk2] memory type information HOB / UEFI memmap defrag
> > 
> > On 02/22/17 02:48, Kinney, Michael D wrote:
> > > Jordan,
> > >
> > > The usage of EfiLoaderCode/ EfiBootServicesCode/ EfiBootServicesData
> > > may vary from boot to boot, especially if the shell or other applications
> > > are run or different OSes are booted.  A change in the bin size causes
> > > extra variable writes and potentially extra reboots.
> > 
> > As I wrote elsewere, in a few days (or, well, weeks) I would like to
> > research the simpler-looking avenue of (a) simply not producing this HOB
> > in OVMF's PlatformPei at all, and (b) pulling in VariablePei. As far as
> > I understand the code in the DXE IPL PEIM and BDS DXE, this should
> > enable those two modules to communicate with each other through the
> > variable highlighted by Jordan, and to create the HOB automatically. The
> > code seems to track / maintain the maximum memory usage seen during
> > previous boots, which I believe is appropriate for OVMF.
> > 
> > If this worked without any more platform cooperation than (a) and (b),
> > that would be awesome & my preference.
> > 
> > Thanks!
> > Laszlo
> > 
> > >
> > > Mike
> > >
> > >> -----Original Message-----
> > >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jordan
> > Justen
> > >> Sent: Tuesday, February 21, 2017 5:32 PM
> > >> To: Yao, Jiewen <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-
> > devel-
> > >> 01 <edk2-devel@lists.01.org>
> > >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Andrew Fish <afish@apple.com>; Ard Biesheuvel
> > >> <ard.biesheuvel@linaro.org>
> > >> Subject: Re: [edk2] memory type information HOB / UEFI memmap defrag
> > >>
> > >> On 2017-02-21 16:46:40, Yao, Jiewen wrote:
> > >>> HI Laszlo
> > >>>
> > >>> The purpose of this table to put OS consumed memory together to avoid S4 resume
> > >>> issue. EfiLoaderCode/ EfiBootServicesCode/ EfiBootServicesData are not used by
> > >>> OS. There is no need to put them here.
> > >>>
> > >>> I suggest we remove EfiLoaderCode/ EfiBootServicesCode/ EfiBootServicesData to
> > >>> avoid confusing.
> > >>>
> > >>
> > >> Is there any other advantage to removing them?
> > >>
> > >> I guess it would be easy enough to re-add them, but I don't think we
> > >> need to move away from supporting S4. While I agree that S4 should not
> > >> be a big priority, I'd prefer that we try to support it at some point.
> > >>
> > >> -Jordan
> > >>
> > >>>
> > >>>> +  { EfiReservedMemoryType,  EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *   202) },
> > >>>> +  { EfiLoaderCode,          EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *  1439) },
> > >>>> +  { EfiBootServicesCode,    EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *  5980) },
> > >>>> +  { EfiBootServicesData,    EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB * 41643) },
> > >>>> +  { EfiRuntimeServicesCode, EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *  1025) },
> > >>>> +  { EfiRuntimeServicesData, EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *  3629) },
> > >>>> +  { EfiACPIReclaimMemory,   EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *    36) },
> > >>>> +  { EfiACPIMemoryNVS,       EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *  1301) },
> > >>>> +  { EfiMaxMemoryType,       0                                           }
> > >>>
> > >>>
> > >>>
> > >>> Thank you
> > >>>
> > >>> Yao Jiewen
> > >>>
> > >>>
> > >>>
> > >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> > >>> Ersek
> > >>> Sent: Tuesday, February 21, 2017 11:24 PM
> > >>> To: edk2-devel-01 <edk2-devel@ml01.01.org>
> > >>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Justen, Jordan L
> > >>> <jordan.l.justen@intel.com>; Andrew Fish <afish@apple.com>; Ard Biesheuvel
> > >>> <ard.biesheuvel@linaro.org>
> > >>> Subject: [edk2] memory type information HOB / UEFI memmap defrag
> > >>>
> > >>>
> > >>>
> > >>> Hi,
> > >>>
> > >>> the UEFI memmap under OVMF is getting very fragmented, I'm now counting
> > >>> ~80 entries in it, under various circumstances.
> > >>>
> > >>> I recall that a platform's PlatformPei can "prime" the DXE/UEFI memory
> > >>> allocation system (not the GCD services) for various memory types, by
> > >>> producing a memory type information HOB.
> > >>>
> > >>> My vague understanding is that BDS will in turn check if the actual
> > >>> allocations fit in the allotments from the HOB, and if not, it will try
> > >>> to feed back the increased amount to PEI, for the next boot.
> > >>>
> > >>> As far as I understand, this requires the VariablePei (read only driver)
> > >>> for a platform (so that its PlatformPei can read the info from BDS, and
> > >>> produce the HOB accordingly). Some questions:
> > >>>
> > >>> - how big is VariablePei in binary form?
> > >>> - does it depend on permanent RAM being installed / discovered?
> > >>> - If so, is that dependency implemented with a static DEPEX, or with a
> > >>>   callback?
> > >>>
> > >>> Further questions:
> > >>> - what is the variable (GUID and Name) that BDS uses for this
> > >>>   information?
> > >>> - What is the format of the variable?
> > >>> - Does the logic depend on particular boot modes? OVMF only supports two
> > >>>   boot modes, BOOT_WITH_FULL_CONFIGURATION and BOOT_ON_S3_RESUME.
> > >>>
> > >>> In OVMF we currently use a static array for populating the HOB (see
> > >>> "mDefaultMemoryTypeInformation" in "PlatformPei/Platform.c"). If making
> > >>> it all dynamic is easy, I think I'd like to do it (sometime later).
> > >>>
> > >>> If, however, it would require us to up-end OVMF's PlatformPei, then I
> > >>> think it's not worth it; we can just bump the values in
> > >>> "mDefaultMemoryTypeInformation" suitably.
> > >>>
> > >>> Some examples I consider as up-ending OVMF's PlatformPei:
> > >>>
> > >>> (1) If VariablePei needs permanent RAM with a hard DEPEX. In OVMF,
> > >>>     permanent RAM is installed by PlatformPei (thereby potentially
> > >>>     unblocking VariablePei's dispatch); however, it is also PlatformPei
> > >>>     that would require the r/o variable service to work, because
> > >>>     PlatformPei produces the memory type information HOB. So, such a
> > >>>     DEPEX in VariablePei would require splitting up PlatformPei, which
> > >>>     makes the dynamism totally not worth it.
> > >>>
> > >>>     *Maybe* we could add a callback for when the variable service PPI is
> > >>>     installed. Dunno.
> > >>>
> > >>> (2) Supporting a third boot mode beyond BOOT_WITH_FULL_CONFIGURATION and
> > >>>     BOOT_ON_S3_RESUME. Not even worth the audit of current boot mode
> > >>>     checks.
> > >>>
> > >>> Further remarks:
> > >>>
> > >>> - OVMF doesn't care about supporting S4 at the moment, and I personally
> > >>>   have no plans to work on that. (I'm saying this because I vaguely
> > >>>   recall that the memory type info HOB is related to S4 resume, so an
> > >>>   argument could perhaps be made, "this could enable S4 for OVMF".
> > >>>   Personally, I'm not interested. Still carrying the scars of S3.)
> > >>>
> > >>> - I actually tried to bump the values in "mDefaultMemoryTypeInformation"
> > >>>   quite a few months back, but the benefits I saw were negligible. I was
> > >>>   left confused about the memory type info HOB, and that was the reason
> > >>>   I didn't ultimately post any patch (and why I stopped pursuing this
> > >>>   question). For reference, this was the patch:
> > >>>
> > >>>> commit b357e8d88c0304ea2b31aefafe53d06c9769fb78
> > >>>> Author: Laszlo Ersek <lersek@redhat.com>
> > >>>> Date:   Thu Sep 17 16:18:46 2015 +0200
> > >>>>
> > >>>>     OvmfPkg: PlatformPei: decrease memmap fragmentation
> > >>>>
> > >>>>     Inspired by ArmVirtPkg commit c199315 ("ArmVirtPkg: increase memory
> > >>>>     preallocations to reduce region count"), I checked the number of entries
> > >>>>     in the UEFI memory map, as dumped by the UEFI shell's MEMMAP command, and
> > >>>>     by the Linux kernel. The number of entries is quite high, about 50-55.
> > >>>>
> > >>>>     I calculated the new preallocations as follows:
> > >>>>     - added 15% to each byte count usage reported by the MEMMAP command, for
> > >>>>       some future-proofing,
> > >>>>     - expressed the result in kilobytes (both pages and byte counts are hard
> > >>>>       to read),
> > >>>>     - just for our information, I calculated the ratio between the new
> > >>>>       preallocation and the old one.
> > >>>>
> > >>>>     For example, the UEFI shell reported 44 pages (180224 bytes) of reserved
> > >>>>     memory usage. The new preallocation, expressed in kilobytes, is
> > >>>>     trunc(180224 * 1.15 / 1024) = 202. This preallocation is approx. 12.62
> > >>>>     times the previous preallocation (which was 4 pages, ie. 16384 bytes).
> > >>>>
> > >>>>     Here's the full table:
> > >>>>
> > >>>>       memory type  pages from  bytes from  new KB    factor of former
> > >>>>                    MEMMAP cmd  MEMMAP cmd  prealloc  prealloc
> > >>>>       -----------  ----------  ----------  --------  ----------------
> > >>>>       Reserved             44      180224       202             12.62
> > >>>>       LoaderCode          313     1282048      1439               n/a
> > >>>>       BS_Code            1300     5324800      5980              3.89
> > >>>>       BS_Data            9053    37081088     41643              2.71
> > >>>>       RT_Code             223      913408      1025              5.33
> > >>>>       RT_Data             789     3231744      3629             25.20
> > >>>>       ACPI_Recl             8       32768        36              1.12
> > >>>>       ACPI_NVS            283     1159168      1301             81.31
> > >>>>
> > >>>>     ... Unfortunately, when the patch is applied, the memory map remains
> > >>>>     fragmented;
> > >>>  mostly due to small unused Conventional Memory entries between
> > >>>>     other types of allocations. The entry count doesn't go below 40.
> > >>>>
> > >>>>     Contributed-under: TianoCore Contribution Agreement 1.0
> > >>>>     Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > >>>>
> > >>>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> > >>>> index a6d961673d3a..38abf3811600 100644
> > >>>> --- a/OvmfPkg/PlatformPei/Platform.c
> > >>>> +++ b/OvmfPkg/PlatformPei/Platform.c
> > >>>> @@ -41,14 +41,15 @@
> > >>>>  #include "Cmos.h"
> > >>>>
> > >>>>  EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {
> > >>>> -  { EfiACPIMemoryNVS,       0x004 },
> > >>>> -  { EfiACPIReclaimMemory,   0x008 },
> > >>>> -  { EfiReservedMemoryType,  0x004 },
> > >>>> -  { EfiRuntimeServicesData, 0x024 },
> > >>>> -  { EfiRuntimeServicesCode, 0x030 },
> > >>>> -  { EfiBootServicesCode,    0x180 },
> > >>>> -  { EfiBootServicesData,    0xF00 },
> > >>>> -  { EfiMaxMemoryType,       0x000 }
> > >>>> +  { EfiReservedMemoryType,  EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *   202) },
> > >>>> +  { EfiLoaderCode,          EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *  1439) },
> > >>>> +  { EfiBootServicesCode,    EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *  5980) },
> > >>>> +  { EfiBootServicesData,    EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB * 41643) },
> > >>>> +  { EfiRuntimeServicesCode, EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *  1025) },
> > >>>> +  { EfiRuntimeServicesData, EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *  3629) },
> > >>>> +  { EfiACPIReclaimMemory,   EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *    36) },
> > >>>> +  { EfiACPIMemoryNVS,       EFI_SIZE_TO_PAGES ((UINTN)SIZE_1KB *  1301) },
> > >>>> +  { EfiMaxMemoryType,       0                                           }
> > >>>>  };
> > >>>>
> > >>>>
> > >>>
> > >>> As you can see in the commit message, at that time the patch only
> > >>> managed to decrease the number of memmap entries from ~55 to ~40, which
> > >>> I found "meh". I figured I'd ask again, because now I'm seeing about 80
> > >>> entries in the memmap. (I wonder if that is related to OVMF's recently
> > >>> increased ACPI S3 boot script usage!)
> > >>>
> > >>> Thanks,
> > >>> Laszlo
> > >>>
> > >>> _______________________________________________
> > >>> edk2-devel mailing list
> > >>> edk2-devel@lists.01.org
> > >>> https://lists.01.org/mailman/listinfo/edk2-devel
> > >>>
> > >> _______________________________________________
> > >> edk2-devel mailing list
> > >> edk2-devel@lists.01.org
> > >> https://lists.01.org/mailman/listinfo/edk2-devel
>