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.623.1664386045201172530 for ; Wed, 28 Sep 2022 10:27:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=QEyxs9tV; 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 A2B3661F59 for ; Wed, 28 Sep 2022 17:27:23 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 133F5C43140 for ; Wed, 28 Sep 2022 17:27:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1664386043; bh=DhUL1eE5MUVyqA4fa93PIj8cuFpat/YNYIE5EBSG0aM=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=QEyxs9tV4j8KNEytLFFwNjYoB4ByYbWBOdjhDE6UblGYHvoSlfytJsJ+H/hv7jaOU 62veGhfBmgjUEJSQcmEOyZoiX7tVY+cD33n0m0xksjmI42KVmx9Oi/Ga1tOfhKi/LM mZVIU5TGhX7tauiTv2V7BW3MfTbvugyAxRO3SZRv5TAobfxuR3gIWutwMZm5TIBOCI qPoHZs4L11j0k2+yM/bWHnI/hk79DjCU7Jcm17LQzFa4jbHDg1fzi28e92Xi/bNAO8 ZrjqJ0W6PE+HmDLaCYO0g+ingCjIQQ/S5tBENyeEh9XigD3m2FFNc3I+ZfoDQLpadQ iL4cAImRMb9ag== Received: by mail-lj1-f178.google.com with SMTP id g20so15095100ljg.7 for ; Wed, 28 Sep 2022 10:27:22 -0700 (PDT) X-Gm-Message-State: ACrzQf1iw3Pc+DvWXT1Cb8Rtnp7r/tdlYjdJI4XWtbhG5Prg4APM9n15 eva8Ei3SgdxkDN3vzM9cAAbqdweJ4SkTELqNErY= X-Google-Smtp-Source: AMsMyM5zpHSZlSr3x7Lez0qHVhSZBY8nuNKMrW7eCw3exbQ2VOvRTdUtaV5qvbnp4k0uJXa8z7uxKMFVwSVX5v5Q1rE= X-Received: by 2002:a05:651c:239c:b0:26d:94b8:781d with SMTP id bk28-20020a05651c239c00b0026d94b8781dmr6523758ljb.189.1664386040983; Wed, 28 Sep 2022 10:27:20 -0700 (PDT) MIME-Version: 1.0 References: <20220928153323.2583389-1-dionnaglaze@google.com> <20220928153323.2583389-6-dionnaglaze@google.com> In-Reply-To: <20220928153323.2583389-6-dionnaglaze@google.com> From: "Ard Biesheuvel" Date: Wed, 28 Sep 2022 19:27:09 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 5/6] MdeModulePkg: add EnableUnacceptedMemoryProtocol To: Dionna Glaze , "Liming Gao (Byosoft address)" , Andrew Fish , Michael Kinney Cc: devel@edk2.groups.io, Gerd Hoffmann , James Bottomley , Jiewen Yao , Tom Lendacky Content-Type: text/plain; charset="UTF-8" (cc some other maintainers) On Wed, 28 Sept 2022 at 17:33, Dionna Glaze wrote: > > Add a simple protocol that enables the use of the unaccepted memory > type. Must be called before ExitBootServices to be effective. Calling protocols is generally not permitted after ExitBootServices() anyway, so this statement is somewhat redundant. I'd clarify here is that it is the expectation that the OS loader not the firmware invokes this protocol (if it exists) to inform the firmware that it should not accept all memory on behalf of the OS. It also means we have to align this very carefully, and perhaps promote this to a formal EFI protocol. > > Cc: Gerd Hoffmann > Cc: James Bottomley > Cc: Jiewen Yao > Cc: Tom Lendacky > Cc: Ard Biesheuvel > > Signed-off-by: Dionna Glaze AIUI, the only thing the protocol does is change the value of the PCD, right? Given that dynamic PCDs are optional but protocols are not, can we just drop the PCD and (in view of some other comments below) do the following: - Define a protocol in MdeModulePkg that ExitBootServices() will invoke after disarming the timer but before finalizing the memory map. It doesn't have to be specific to memory acceptance at all, the only thing that matters is that it is invoked at the right time (and perhaps only on the first call to EBS()). E.g., gEdkiiExitBootServicesCallbackProtocol with a single method called TerminateMemoryMapPreHook(). This protocol is fundamentally internal to EDK2 and does not require inclusion in PI or UEFI - Define a protocol in OvmfPkg that will be called by the OS loader to indicate that it is capable of taking charge of the unaccepted memory, and so it does not need the firmware to do so on its behalf, by accepting all of it during ExitBootServices(). [Basically, this patch but moved out of MdeModulePkg] - Implement a single DXE driver in OvmfPkg (or add the functionality to an existing one) that provides an implementation of the former protocol, and implements the latter protocol by uninstalling the former again. This means the 'accept all memory' routine can be moved out of DXE core as well, and into your driver. Apologies for not bringing this up before arriving at v4 of your series, but i think these change will be a lot more palatable to the MdeModulePkg maintainers if the DXE core changes are minimal, and not specifically tied to memory acceptance. (some more comments below) > --- > MdeModulePkg/Core/Dxe/DxeMain.h | 22 ++++++++++++ > MdeModulePkg/Core/Dxe/DxeMain.inf | 3 +- > MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 5 +++ > MdeModulePkg/Core/Dxe/Mem/Page.c | 35 ++++++++++++++++++++ > MdeModulePkg/MdeModulePkg.dec | 3 ++ > 5 files changed, 67 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h > index ac943c87a3..5f0114b04f 100644 > --- a/MdeModulePkg/Core/Dxe/DxeMain.h > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h > @@ -2708,6 +2708,28 @@ CoreResolveUnacceptedMemory ( > VOID > ); > > + > +typedef struct _ENABLE_UNACCEPTED_MEMORY_PROTOCOL > + ENABLE_UNACCEPTED_MEMORY_PROTOCOL; > + > +typedef EFI_STATUS (EFIAPI *ENABLE_UNACCEPTED_MEMORY)( > + IN ENABLE_UNACCEPTED_MEMORY_PROTOCOL * > + ); > + > +struct _ENABLE_UNACCEPTED_MEMORY_PROTOCOL { > + ENABLE_UNACCEPTED_MEMORY Enable; > +}; > + Protocol definitions should live in a dedicated .h file under Include/Protocol/ > +extern EFI_GUID gEnableUnacceptedMemoryProtocolGuid; > + We tend to use the Edkii prefix for EDK2 protocols that have no basis in the PI or UEFI specifications. So in this case gEdkiiEnableUnacceptedMemoryProtocolGuid unless we decide that this should be in the EFI spec, in which case we'll need to do a code-first ECR. I'd be happy to collaborate on that. OTOH, I have no problems whatsoever with adopting a protocol on the Linux side that is not in the EFI spec. Same comment as before applies, though: could we find a better name for this? > +/** > + Implement the protocol for enabling unaccepted memory. > + **/ > +VOID > +InstallEnableUnacceptedMemoryProtocol ( > + VOID > + ); > + > /** > Install MemoryAttributesTable on memory allocation. > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf > index deb8bb2ba8..39dcac98bb 100644 > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf > @@ -122,6 +122,7 @@ > gEfiMemoryAttributesTableGuid ## SOMETIMES_PRODUCES ## SystemTable > gEfiEndOfDxeEventGroupGuid ## SOMETIMES_CONSUMES ## Event > gEfiHobMemoryAllocStackGuid ## SOMETIMES_CONSUMES ## SystemTable > + gEnableUnacceptedMemoryProtocolGuid ## PRODUCES ## GUID # Install protocol > This should be under [Protocols] not [Guids] > [Ppis] > gEfiVectorHandoffInfoPpiGuid ## UNDEFINED # HOB > @@ -187,7 +188,7 @@ > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth ## CONSUMES > - gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdEnableUnacceptedMemory ## CONSUMES ## SOMETIMES_PRODUCES > > # [Hob] > # RESOURCE_DESCRIPTOR ## CONSUMES > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > index 8d1de32fe7..bc1a8ab6b2 100644 > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c > @@ -354,6 +354,11 @@ DxeMain ( > Status = CoreInstallConfigurationTable (&gEfiMemoryTypeInformationGuid, &gMemoryTypeInformation); > ASSERT_EFI_ERROR (Status); > > + // > + // Install unaccepted memory configuration protocol > + // > + InstallEnableUnacceptedMemoryProtocol(); > + > // > // If Loading modules At fixed address feature is enabled, install Load moduels at fixed address > // Configuration Table so that user could easily to retrieve the top address to load Dxe and PEI > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c > index cbebe62a28..10e152d80d 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c > @@ -96,6 +96,14 @@ EFI_MEMORY_TYPE_INFORMATION gMemoryTypeInformation[EfiMaxMemoryType + 1] = { > // > GLOBAL_REMOVE_IF_UNREFERENCED BOOLEAN gLoadFixedAddressCodeMemoryReady = FALSE; > > +EFI_STATUS EFIAPI CoreEnableUnacceptedMemory(IN ENABLE_UNACCEPTED_MEMORY_PROTOCOL *); > + > +struct { > + ENABLE_UNACCEPTED_MEMORY enable; > +} mEnableUnacceptedMemoryProtocol = { > + CoreEnableUnacceptedMemory, > +}; > + > /** > Enter critical section by gaining lock on gMemoryLock. > > @@ -2205,6 +2213,33 @@ CoreResolveUnacceptedMemory ( > return AcceptAllUnacceptedMemory(AcceptMemory); > } > > +EFI_STATUS > +EFIAPI > +CoreEnableUnacceptedMemory ( > + IN ENABLE_UNACCEPTED_MEMORY_PROTOCOL *This > + ) > +{ > + return PcdSetBoolS(PcdEnableUnacceptedMemory, TRUE); > +} > + > +VOID > +InstallEnableUnacceptedMemoryProtocol ( > + VOID > + ) > +{ > + EFI_HANDLE Handle; > + EFI_STATUS Status; > + > + Handle = NULL; > + Status = CoreInstallMultipleProtocolInterfaces ( > + &Handle, > + &gEnableUnacceptedMemoryProtocolGuid, > + &mEnableUnacceptedMemoryProtocol, > + NULL > + ); > + ASSERT_EFI_ERROR (Status); > +} > + > /** > 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. > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index dd07b3725a..ce72c06a93 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -244,6 +244,9 @@ > gEdkiiPerformanceMeasurementProtocolGuid = { 0xc85d06be, 0x5f75, 0x48ce, { 0xa8, 0x0f, 0x12, 0x36, 0xba, 0x3b, 0x87, 0xb1 } } > gEdkiiSmmPerformanceMeasurementProtocolGuid = { 0xd56b6d73, 0x1a7b, 0x4015, { 0x9b, 0xb4, 0x7b, 0x07, 0x17, 0x29, 0xed, 0x24 } } > > + ## Bootloader protocol Guid for enabling unaccepted memory support. > + gEnableUnacceptedMemoryProtocolGuid = { 0xc5a010fe, 0x38a7, 0x4531, { 0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49 } } > + Should be under [Protocols] as well > ## Guid is defined for CRC32 encapsulation scheme. > # Include/Guid/Crc32GuidedSectionExtraction.h > gEfiCrc32GuidedSectionExtractionGuid = { 0xFC1BCDB0, 0x7D31, 0x49aa, {0x93, 0x6A, 0xA4, 0x60, 0x0D, 0x9D, 0xD0, 0x83 } } > -- > 2.37.3.998.g577e59143f-goog >