On Wed, Mar 11, 2020 at 17:22:47 +0100, Laszlo Ersek wrote:
> >> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {
> >> + { EfiACPIMemoryNVS, 0x004 },
> >> + { EfiACPIReclaimMemory, 0x008 },
> >> + { EfiReservedMemoryType, 0x004 },
> >> + { EfiRuntimeServicesData, 0x024 },
> >> + { EfiRuntimeServicesCode, 0x030 },
> >> + { EfiBootServicesCode, 0x180 },
> >> + { EfiBootServicesData, 0xF00 },
> >> + { EfiMaxMemoryType, 0x000 }
> >> +};
> >
> > Could you add a comment as to where these page counts come from?
> > Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c.
> >
> > It *would* be nice if that could be cleaned up at the same time...
>
> Sorry, I don't understand -- what kind of cleanup do you have in mind
> precisely? The table is not copied, but moved from the original place,
> so I'm unsure what's left in "Platform.c" to clean up.
Not left to clean up there, but something to consider addressing when
moving it here. Yes, it's just a move, so we could argue it doesn't
need fixing - but it's a struct with a bunch of live-coded numerical
values completely without explanation.
"I'd rather not" is an acceptable answer, but I figured I should point
it out.
/
Leif
> Regarding the origin of the table, it's ancient:
>
> - 49ba9447c92d ("Add initial version of Open Virtual Machine Firmware
> (OVMF) platform.", 2009-05-27)
> - 991d95636264 ("Update OVMF Platform PEI to declare IO and MMIO
> resources. Update default memory type information to
> reduce EFI Memory Map fragmentation.", 2010-07-16)
> - 55cdb67acb3d ("OVMF: Support greater than 2GB of memory", 2010-10-13)
>
> (BTW I have a patch as a separate work item in the queue for this -- I'm
> going to remove the EfiBootServicesCode / EfiBootServicesData entries
> from this table, per Jiewen's recommendation / explanation in the "WSMT
> bits" thread:
>
> https://edk2.groups.io/g/devel/message/55712
> a5e71131-65dc-8b85-481a-d35011512987@redhat.com">http://mid.mail-archive.com/a5e71131-65dc-8b85-481a-d35011512987@redhat.com
>
> But that's a separate patch, for later.)
>
> Thank you!
> Laszlo
>
> >
> > /
> > Leif
> >
> >> +
> >> +STATIC
> >> +VOID
> >> +BuildMemTypeInfoHob (
> >> + VOID
> >> + )
> >> +{
> >> + BuildGuidDataHob (
> >> + &gEfiMemoryTypeInformationGuid,
> >> + mDefaultMemoryTypeInformation,
> >> + sizeof mDefaultMemoryTypeInformation
> >> + );
> >> + DEBUG ((DEBUG_INFO, "%a: default memory type information HOB built\n",
> >> + __FUNCTION__));
> >> +}
> >> +
> >> +/**
> >> + Notification function called when EFI_PEI_READ_ONLY_VARIABLE2_PPI
> >> becomes
> >> + available.
> >> +
> >> + @param[in] PeiServices Indirect reference to the PEI Services
> >> Table.
> >> + @param[in] NotifyDescriptor Address of the notification descriptor data
> >> + structure.
> >> + @param[in] Ppi Address of the PPI that was installed.
> >> +
> >> + @return Status of the notification. The status code returned from this
> >> + function is ignored.
> >> +**/
> >> +STATIC
> >> +EFI_STATUS
> >> +EFIAPI
> >> +OnReadOnlyVariable2Available (
> >> + IN EFI_PEI_SERVICES **PeiServices,
> >> + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
> >> + IN VOID *Ppi
> >> + )
> >> +{
> >> + EFI_PEI_READ_ONLY_VARIABLE2_PPI *ReadOnlyVariable2;
> >> + UINTN DataSize;
> >> + EFI_STATUS Status;
> >> +
> >> + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__));
> >> +
> >> + //
> >> + // Check if the "MemoryTypeInformation" variable exists, in the
> >> + // gEfiMemoryTypeInformationGuid namespace.
> >> + //
> >> + ReadOnlyVariable2 = Ppi;
> >> + DataSize = 0;
> >> + Status = ReadOnlyVariable2->GetVariable (
> >> + ReadOnlyVariable2,
> >> + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
> >> + &gEfiMemoryTypeInformationGuid,
> >> + NULL,
> >> + &DataSize,
> >> + NULL
> >> + );
> >> + switch (Status) {
> >> + case EFI_BUFFER_TOO_SMALL:
> >> + //
> >> + // The variable exists; the DXE IPL PEIM will build the HOB from it.
> >> + //
> >> + break;
> >> + case EFI_NOT_FOUND:
> >> + //
> >> + // The variable does not exist; install the default memory type
> >> information
> >> + // HOB.
> >> + //
> >> + BuildMemTypeInfoHob ();
> >> + break;
> >> + default:
> >> + DEBUG ((DEBUG_ERROR, "%a: unexpected: GetVariable(): %r\n",
> >> __FUNCTION__,
> >> + Status));
> >> + ASSERT (FALSE);
> >> + CpuDeadLoop ();
> >> + break;
> >> + }
> >> +
> >> + return EFI_SUCCESS;
> >> +}
> >> +
> >> +//
> >> +// Notification object for registering the callback, for when
> >> +// EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes available.
> >> +//
> >> +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mReadOnlyVariable2Notify = {
> >> + (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH |
> >> + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), // Flags
> >> + &gEfiPeiReadOnlyVariable2PpiGuid, // Guid
> >> + OnReadOnlyVariable2Available // Notify
> >> +};
> >> +
> >> +VOID
> >> +MemTypeInfoInitialization (
> >> + VOID
> >> + )
> >> +{
> >> + EFI_STATUS Status;
> >> +
> >> + if (!FeaturePcdGet (PcdSmmSmramRequire)) {
> >> + //
> >> + // EFI_PEI_READ_ONLY_VARIABLE2_PPI will never be available; install
> >> + // the default memory type information HOB right away.
> >> + //
> >> + BuildMemTypeInfoHob ();
> >> + return;
> >> + }
> >> +
> >> + Status = PeiServicesNotifyPpi (&mReadOnlyVariable2Notify);
> >> + if (EFI_ERROR (Status)) {
> >> + DEBUG ((DEBUG_ERROR, "%a: failed to set up R/O Variable 2 callback:
> >> %r\n",
> >> + __FUNCTION__, Status));
> >> + ASSERT (FALSE);
> >> + CpuDeadLoop ();
> >> + }
> >> +}
> >> diff --git a/OvmfPkg/PlatformPei/Platform.c
> >> b/OvmfPkg/PlatformPei/Platform.c
> >> index 473af102783a..587ca68fc210 100644
> >> --- a/OvmfPkg/PlatformPei/Platform.c
> >> +++ b/OvmfPkg/PlatformPei/Platform.c
> >> @@ -28,7 +28,6 @@
> >> #include <Library/QemuFwCfgLib.h>
> >> #include <Library/QemuFwCfgS3Lib.h>
> >> #include <Library/ResourcePublicationLib.h>
> >> -#include <Guid/MemoryTypeInformation.h>
> >> #include <Ppi/MasterBootMode.h>
> >> #include <IndustryStandard/I440FxPiix4.h>
> >> #include <IndustryStandard/Pci22.h>
> >> @@ -39,18 +38,6 @@
> >> #include "Platform.h"
> >> #include "Cmos.h"
> >>
> >> -EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {
> >> - { EfiACPIMemoryNVS, 0x004 },
> >> - { EfiACPIReclaimMemory, 0x008 },
> >> - { EfiReservedMemoryType, 0x004 },
> >> - { EfiRuntimeServicesData, 0x024 },
> >> - { EfiRuntimeServicesCode, 0x030 },
> >> - { EfiBootServicesCode, 0x180 },
> >> - { EfiBootServicesData, 0xF00 },
> >> - { EfiMaxMemoryType, 0x000 }
> >> -};
> >> -
> >> -
> >> EFI_PEI_PPI_DESCRIPTOR mPpiBootMode[] = {
> >> {
> >> EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> >> @@ -162,15 +149,6 @@ MemMapInitialization (
> >> PciIoBase = 0xC000;
> >> PciIoSize = 0x4000;
> >>
> >> - //
> >> - // Create Memory Type Information HOB
> >> - //
> >> - BuildGuidDataHob (
> >> - &gEfiMemoryTypeInformationGuid,
> >> - mDefaultMemoryTypeInformation,
> >> - sizeof(mDefaultMemoryTypeInformation)
> >> - );
> >> -
> >> //
> >> // Video memory + Legacy BIOS region
> >> //
> >> @@ -811,6 +789,7 @@ InitializePlatform (
> >> ReserveEmuVariableNvStore ();
> >> }
> >> PeiFvInitialization ();
> >> + MemTypeInfoInitialization ();
> >> MemMapInitialization ();
> >> NoexecDxeInitialization ();
> >> }
> >> --
> >> 2.19.1.3.g30247aa5d201
> >>
> >>
> >>
> >>
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55771): https://edk2.groups.io/g/devel/message/55771
Mute This Topic: https://groups.io/mt/71867512/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
