From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web12.133.1664383869441318483 for ; Wed, 28 Sep 2022 09:51:09 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=NrmRK/X4; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id D1BDA61F2E for ; Wed, 28 Sep 2022 16:51:08 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3E1BEC433D7 for ; Wed, 28 Sep 2022 16:51:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1664383868; bh=GPlg3EIWJh44dNlDuJF0AoVArmWCteafP4erBcu+QKU=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=NrmRK/X4cWskLqwxiyeOAOmyT7jxTen8JydoDvA+2g9D9ybtNYf3ktBRpKT/+gaFj x/pJTbnHLt9bqiXHe0m30KiozUAFHWyBZ8+On7mShFkRrCv5xYL4k5lAZtPLb8MozP 2rxa8UW1x6MQK0yx54sXrgbmWyDl0Pntj7fdDc6dc4ZcimaR38EyH0an6RO2ZxLULQ gEaCJXs5kAJTummVhUzEQNCJTyvX+5ZuZTn1gPYIR3V+r4hwmSjDhucrYqFduyYcg0 LevS/mVaEpXhoNvSG8mVxRwemdNv5mRFAJ0P6DbKlXJEw7morPNgmp77n+q2pBAGIq 8xQEbjFWUtSpg== Received: by mail-lf1-f53.google.com with SMTP id a3so21214226lfk.9 for ; Wed, 28 Sep 2022 09:51:08 -0700 (PDT) X-Gm-Message-State: ACrzQf3JBO1IB1zu6foebwo5YCwLeKYEAZFDCdhTvbFB21mF8lvvWh8s 7SmrnVfmoRYMFu53E6BsovRbR/jEf+3XPRbL6TE= X-Google-Smtp-Source: AMsMyM6aiGRm2HEamqKh/tzO59gYvWC7mNKfZp+wcD+5OP9DB3MFLJIBQmkNiPpZyJTSVCu6tI6hVJmdMHtZmjxr9bs= X-Received: by 2002:a05:6512:3d09:b0:497:ab35:fd12 with SMTP id d9-20020a0565123d0900b00497ab35fd12mr14221362lfv.539.1664383866233; Wed, 28 Sep 2022 09:51:06 -0700 (PDT) MIME-Version: 1.0 References: <20220928153323.2583389-1-dionnaglaze@google.com> <20220928153323.2583389-5-dionnaglaze@google.com> In-Reply-To: <20220928153323.2583389-5-dionnaglaze@google.com> From: "Ard Biesheuvel" Date: Wed, 28 Sep 2022 18:50:54 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 4/6] MdeModulePkg: DxeMain accepts all memory at EBS if needed To: Dionna Glaze Cc: devel@edk2.groups.io, Gerd Hoffmann , James Bottomley , Jiewen Yao , Tom Lendacky Content-Type: text/plain; charset="UTF-8" On Wed, 28 Sept 2022 at 17:33, Dionna Glaze wrote: > > With the addition of the EfiUnacceptedMemory memory type, it is possible > the EFI-enlightened guests do not themselves support the new memory > type. This commit uses the new PcdEnableUnacceptedMemory to enable > unaccepted memory support before ExitBootServices is called by not > accepting all unaccepted memory at EBS. > > The expected usage is to set the new Pcd with a protocol that is usable > by bootloaders and directly-booted OSes when they can determine that the > OS does indeed support unaccepted memory. > > Cc: Gerd Hoffmann > Cc: James Bottomley > Cc: Jiewen Yao > Cc: Tom Lendacky > Cc: Ard Biesheuvel > > Signed-off-by: Dionna Glaze > --- > MdeModulePkg/Core/Dxe/DxeMain.h | 10 +++ > MdeModulePkg/Core/Dxe/DxeMain.inf | 2 + > MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 14 +++- > MdeModulePkg/Core/Dxe/Mem/Page.c | 87 ++++++++++++++++++++ > 4 files changed, 112 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h > index 815a6b4bd8..ac943c87a3 100644 > --- a/MdeModulePkg/Core/Dxe/DxeMain.h > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h > @@ -2698,6 +2698,16 @@ CoreInitializeMemoryProtection ( > VOID > ); > > +/** > + Accept and convert unaccepted memory to conventional memory if unaccepted > + memory is not enabled and there is an implementation of MemoryAcceptProtocol > + installed. > + **/ > +EFI_STATUS > +CoreResolveUnacceptedMemory ( > + VOID > + ); > + > /** > Install MemoryAttributesTable on memory allocation. > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf > index e4bca89577..deb8bb2ba8 100644 > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf > @@ -153,6 +153,7 @@ > gEfiHiiPackageListProtocolGuid ## SOMETIMES_PRODUCES > gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUMES > gEdkiiPeCoffImageEmulatorProtocolGuid ## SOMETIMES_CONSUMES > + gEfiMemoryAcceptProtocolGuid ## SOMETIMES_CONSUMES > > # Arch Protocols > gEfiBdsArchProtocolGuid ## CONSUMES > @@ -186,6 +187,7 @@ > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ## CONSUMES > > # [Hob] > # RESOURCE_DESCRIPTOR ## CONSUMES > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > index 5733f0c8ec..8d1de32fe7 100644 > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > @@ -768,13 +768,25 @@ CoreExitBootServices ( > // > gTimer->SetTimerPeriod (gTimer, 0); > > + // > + // Accept all memory if unaccepted memory isn't enabled. > + // > + Status = CoreResolveUnacceptedMemory(); > + if (EFI_ERROR (Status)) { > + // > + // Notify other drivers that ExitBootServices failed > + // > + CoreNotifySignalList (&gEventExitBootServicesFailedGuid); > + return Status; > + } > + > // > // Terminate memory services if the MapKey matches > // > Status = CoreTerminateMemoryMap (MapKey); > if (EFI_ERROR (Status)) { > // > - // Notify other drivers that ExitBootServices fail > + // Notify other drivers that ExitBootServices failed > // > CoreNotifySignalList (&gEventExitBootServicesFailedGuid); > return Status; > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c > index ffe79dcca9..cbebe62a28 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c > @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include "DxeMain.h" > #include "Imem.h" > #include "HeapGuard.h" > +#include > +#include > > // > // Entry for tracking the memory regions for each memory type to coalesce similar memory types > @@ -2118,6 +2120,91 @@ CoreFreePoolPages ( > CoreConvertPages (Memory, NumberOfPages, EfiConventionalMemory); > } > > +EFI_EVENT gExitBootServiceEvent = NULL; > + > +STATIC > +EFI_STATUS > +AcceptAllUnacceptedMemory ( > + IN EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory > + ) > +{ > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap; > + UINTN NumEntries; > + UINTN Index; > + EFI_STATUS Status; > + > + /* > + * Get a copy of the memory space map to iterate over while > + * changing the map. > + */ > + Status = CoreGetMemorySpaceMap (&NumEntries, &AllDescMap); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + for (Index = 0; Index < NumEntries; Index++) { > + CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Desc; > + > + Desc = &AllDescMap[Index]; > + if (Desc->GcdMemoryType != EfiGcdMemoryTypeUnaccepted) { > + continue; > + } > + > + Status = AcceptMemory->AcceptMemory ( > + AcceptMemory, > + Desc->BaseAddress, > + Desc->Length > + ); > + if (EFI_ERROR(Status)) { > + goto done; > + } > + > + Status = CoreRemoveMemorySpace(Desc->BaseAddress, Desc->Length); > + if (EFI_ERROR(Status)) { > + goto done; > + } > + > + Status = CoreAddMemorySpace ( > + EfiGcdMemoryTypeSystemMemory, > + Desc->BaseAddress, > + Desc->Length, > + EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP > + ); > + if (EFI_ERROR(Status)) { > + goto done; > + } > + } > + > +done: > + FreePool (AllDescMap); > + return Status; > +} > + I am not following the logic here 100%. As far as I can tell, if accepting all memory succeeded without errors, ExitBootServices() returns with EFI_SUCCESS, even though it has modified the memory map. This means the actual memory map is out of sync with the last GetMemoryMap() call performed by the OS loader before it called ExitBootServices(), and so it will still contain unaccepted memory, right? The approach I suggested before was to accept all memory and then forcible fail the ExitBootServices() call [which is documented in the spec as an expected occurrence, as events dispatched off the timer interrupt may race and allocate or free pages between GetMemoryMap and ExitBootServices). Doing so would force the caller to call GetMemoryMap() again, which now no longer contains any unaccepted memory, and call ExitBootServices() a second time. This means that, afaict, the call to CoreResolveUnacceptedMemory () is in the right spot, i.e., after the point where the timer interrupt is disabled (so we don't risk failing in ExitBootServices() twice). I also wonder whether we need to deal specifically with the fact that, if CoreResolveUnacceptedMemory() accepts any memory, it will be called again the second time around as well, but perhaps we can just rely on the fact that no unaccepted regions should remain in the GCD memory map. But a comment to that effect would be helpful. > +EFI_STATUS > +CoreResolveUnacceptedMemory ( > + VOID > + ) > +{ > + EFI_MEMORY_ACCEPT_PROTOCOL *AcceptMemory; > + EFI_STATUS Status; > + > + // No need to accept anything. Unaccepted memory is enabled. > + if (PcdGetBool(PcdEnableUnacceptedMemory)) { > + return EFI_SUCCESS; > + } > + > + Status = gBS->LocateProtocol (&gEfiMemoryAcceptProtocolGuid, NULL, > + (VOID **)&AcceptMemory); > + if (Status == EFI_NOT_FOUND) { > + return EFI_SUCCESS; > + } > + if (Status != EFI_SUCCESS) { > + DEBUG ((DEBUG_ERROR, "Error locating MemoryAcceptProtocol: %d\n", Status)); > + return Status; > + } > + > + return AcceptAllUnacceptedMemory(AcceptMemory); > +} > + > /** > Make sure the memory map is following all the construction rules, > it is the last time to check memory map error before exit boot services. > -- > 2.37.3.998.g577e59143f-goog >