From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4D9EE21A6F105 for ; Tue, 6 Jun 2017 11:15:35 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DF090C04B923; Tue, 6 Jun 2017 18:16:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com DF090C04B923 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com DF090C04B923 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-130.phx2.redhat.com [10.3.116.130]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4999F17F2E; Tue, 6 Jun 2017 18:16:40 +0000 (UTC) To: Jordan Justen , edk2-devel-01 Cc: Ard Biesheuvel , Gerd Hoffmann , Igor Mammedov References: <20170603154203.29907-1-lersek@redhat.com> <20170603154203.29907-2-lersek@redhat.com> <149668486395.8581.9154897144168590783@jljusten-skl> From: Laszlo Ersek Message-ID: <5e0277a2-0bf4-e8a1-f61f-77cdb896da06@redhat.com> Date: Tue, 6 Jun 2017 20:16:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <149668486395.8581.9154897144168590783@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 06 Jun 2017 18:16:42 +0000 (UTC) Subject: Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Jun 2017 18:15:35 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/05/17 19:47, Jordan Justen wrote: > On 2017-06-03 08:42:03, Laszlo Ersek wrote: >> ... by narrower than 8-byte ADD_POINTER references. >> >> Introduce the CollectRestrictedAllocations() function, which iterates over > > How about Collect32BitRestrictedAllocations and similar treatment for > other names that just say 'restricted'? Something like this crossed my mind, but I didn't know how to prefix the simple variable / parameter names "RestrictedAllocations" with "32Bit", as the identifiers cannot start with a digit. I even thought of spelling it out, as in "ThirtyTwoBitRestrictedAllocations", but that seemed ridiculous. Prefixing "32Bit" with an underscore, _32Bit, looks ugly, plus the C standard actually reserves it: All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces. While I'd only use this variable name as function parameter / local variable, and thereby I'd shadow any such impl. defined global variable ("identifiers with file scope"), the shadowing would trigger a compiler warning for sure, and break the build. What do you suggest? Thanks, Laszlo > > With that Reviewed-by: Jordan Justen > >> the linker/loader script, and collects the names of the fw_cfg blobs that >> are referenced by QEMU_LOADER_ADD_POINTER.PointeeFile fields, such that >> QEMU_LOADER_ADD_POINTER.PointerSize is less than 8. This means that the >> pointee blob's address will have to be patched into a narrower-than-8 byte >> pointer field, hence the pointee blob must not be allocated from 64-bit >> address space. >> >> In ProcessCmdAllocate(), consult these restrictions when setting the >> maximum address for gBS->AllocatePages(). The default is now MAX_UINT64, >> unless restricted like described above to the pre-patch MAX_UINT32 limit. >> >> In combination with Ard's QEMU commit cb51ac2ffe36 ("hw/arm/virt: generate >> 64-bit addressable ACPI objects", 2017-04-10), this patch enables >> OvmfPkg/AcpiPlatformDxe to work entirely above the 4GB mark. >> >> (An upcoming / planned aarch64 QEMU machine type will have no RAM under >> 4GB at all. Plus, moving the allocations higher is beneficial to the >> current "virt" machine type as well; in Ard's words: "having all firmware >> allocations inside the same 1 GB (or 512 MB for 64k pages) frame reduces >> the TLB footprint".) >> >> Cc: Ard Biesheuvel >> Cc: Gerd Hoffmann >> Cc: Igor Mammedov >> Cc: Jordan Justen >> Suggested-by: Igor Mammedov >> Suggested-by: Gerd Hoffmann >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek >> --- >> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 190 +++++++++++++++++++- >> 1 file changed, 183 insertions(+), 7 deletions(-) >> >> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c >> index 1bc5fe297a96..7aeb6472188b 100644 >> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c >> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c >> @@ -132,14 +132,169 @@ PointerCompare ( >> } >> >> >> /** >> + Comparator function for two ASCII strings. Can be used as both Key and >> + UserStruct comparator. >> + >> + This function exists solely so we can avoid casting &AsciiStrCmp to >> + ORDERED_COLLECTION_USER_COMPARE and ORDERED_COLLECTION_KEY_COMPARE. >> + >> + @param[in] AsciiString1 Pointer to the first ASCII string. >> + >> + @param[in] AsciiString2 Pointer to the second ASCII string. >> + >> + @return The return value of AsciiStrCmp (AsciiString1, AsciiString2). >> +**/ >> +STATIC >> +INTN >> +EFIAPI >> +AsciiStringCompare ( >> + IN CONST VOID *AsciiString1, >> + IN CONST VOID *AsciiString2 >> + ) >> +{ >> + return AsciiStrCmp (AsciiString1, AsciiString2); >> +} >> + >> + >> +/** >> + Release the ORDERED_COLLECTION structure populated by >> + CollectRestrictedAllocations() (below). >> + >> + This function may be called by CollectRestrictedAllocations() itself, on >> + the error path. >> + >> + @param[in] RestrictedAllocations The ORDERED_COLLECTION structure to >> + release. >> +**/ >> +STATIC >> +VOID >> +ReleaseRestrictedAllocations ( >> + IN ORDERED_COLLECTION *RestrictedAllocations >> +) >> +{ >> + ORDERED_COLLECTION_ENTRY *Entry, *Entry2; >> + >> + for (Entry = OrderedCollectionMin (RestrictedAllocations); >> + Entry != NULL; >> + Entry = Entry2) { >> + Entry2 = OrderedCollectionNext (Entry); >> + OrderedCollectionDelete (RestrictedAllocations, Entry, NULL); >> + } >> + OrderedCollectionUninit (RestrictedAllocations); >> +} >> + >> + >> +/** >> + Iterate over the linker/loader script, and collect the names of the fw_cfg >> + blobs that are referenced by QEMU_LOADER_ADD_POINTER.PointeeFile fields, such >> + that QEMU_LOADER_ADD_POINTER.PointerSize is less than 8. This means that the >> + pointee blob's address will have to be patched into a narrower-than-8 byte >> + pointer field, hence the pointee blob must not be allocated from 64-bit >> + address space. >> + >> + @param[out] RestrictedAllocations The ORDERED_COLLECTION structure linking >> + (not copying / owning) such >> + QEMU_LOADER_ADD_POINTER.PointeeFile fields >> + that name the blobs restricted from 64-bit >> + allocation. >> + >> + @param[in] LoaderStart Points to the first entry in the >> + linker/loader script. >> + >> + @param[in] LoaderEnd Points one past the last entry in the >> + linker/loader script. >> + >> + @retval EFI_SUCCESS RestrictedAllocations has been populated. >> + >> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. >> + >> + @retval EFI_PROTOCOL_ERROR Invalid linker/loader script contents. >> +**/ >> +STATIC >> +EFI_STATUS >> +CollectRestrictedAllocations ( >> + OUT ORDERED_COLLECTION **RestrictedAllocations, >> + IN CONST QEMU_LOADER_ENTRY *LoaderStart, >> + IN CONST QEMU_LOADER_ENTRY *LoaderEnd >> +) >> +{ >> + ORDERED_COLLECTION *Collection; >> + CONST QEMU_LOADER_ENTRY *LoaderEntry; >> + EFI_STATUS Status; >> + >> + Collection = OrderedCollectionInit (AsciiStringCompare, AsciiStringCompare); >> + if (Collection == NULL) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + >> + for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) { >> + CONST QEMU_LOADER_ADD_POINTER *AddPointer; >> + >> + if (LoaderEntry->Type != QemuLoaderCmdAddPointer) { >> + continue; >> + } >> + AddPointer = &LoaderEntry->Command.AddPointer; >> + >> + if (AddPointer->PointerSize >= 8) { >> + continue; >> + } >> + >> + if (AddPointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0') { >> + DEBUG ((DEBUG_ERROR, "%a: malformed file name\n", __FUNCTION__)); >> + Status = EFI_PROTOCOL_ERROR; >> + goto RollBack; >> + } >> + >> + Status = OrderedCollectionInsert ( >> + Collection, >> + NULL, // Entry >> + (VOID *)AddPointer->PointeeFile >> + ); >> + switch (Status) { >> + case EFI_SUCCESS: >> + DEBUG (( >> + DEBUG_VERBOSE, >> + "%a: restricting blob \"%a\" from 64-bit allocation\n", >> + __FUNCTION__, >> + AddPointer->PointeeFile >> + )); >> + break; >> + case EFI_ALREADY_STARTED: >> + // >> + // The restriction has been recorded already. >> + // >> + break; >> + case EFI_OUT_OF_RESOURCES: >> + goto RollBack; >> + default: >> + ASSERT (FALSE); >> + } >> + } >> + >> + *RestrictedAllocations = Collection; >> + return EFI_SUCCESS; >> + >> +RollBack: >> + ReleaseRestrictedAllocations (Collection); >> + return Status; >> +} >> + >> + >> +/** >> Process a QEMU_LOADER_ALLOCATE command. >> >> - @param[in] Allocate The QEMU_LOADER_ALLOCATE command to process. >> + @param[in] Allocate The QEMU_LOADER_ALLOCATE command to >> + process. >> >> - @param[in,out] Tracker The ORDERED_COLLECTION tracking the BLOB user >> - structures created thus far. >> + @param[in,out] Tracker The ORDERED_COLLECTION tracking the BLOB >> + user structures created thus far. >> + >> + @param[in] RestrictedAllocations The ORDERED_COLLECTION populated by >> + CollectRestrictedAllocations(), naming the >> + fw_cfg blobs that must not be allocated >> + from 64-bit address space. >> >> @retval EFI_SUCCESS An area of whole AcpiNVS pages has been >> allocated for the blob contents, and the >> contents have been saved. A BLOB object (user >> @@ -163,9 +318,10 @@ STATIC >> EFI_STATUS >> EFIAPI >> ProcessCmdAllocate ( >> IN CONST QEMU_LOADER_ALLOCATE *Allocate, >> - IN OUT ORDERED_COLLECTION *Tracker >> + IN OUT ORDERED_COLLECTION *Tracker, >> + IN ORDERED_COLLECTION *RestrictedAllocations >> ) >> { >> FIRMWARE_CONFIG_ITEM FwCfgItem; >> UINTN FwCfgSize; >> @@ -192,9 +348,12 @@ ProcessCmdAllocate ( >> return Status; >> } >> >> NumPages = EFI_SIZE_TO_PAGES (FwCfgSize); >> - Address = 0xFFFFFFFF; >> + Address = MAX_UINT64; >> + if (OrderedCollectionFind (RestrictedAllocations, Allocate->File) != NULL) { >> + Address = MAX_UINT32; >> + } >> Status = gBS->AllocatePages (AllocateMaxAddress, EfiACPIMemoryNVS, NumPages, >> &Address); >> if (EFI_ERROR (Status)) { >> return Status; >> @@ -805,8 +964,9 @@ InstallQemuFwCfgTables ( >> CONST QEMU_LOADER_ENTRY *LoaderEntry, *LoaderEnd; >> CONST QEMU_LOADER_ENTRY *WritePointerSubsetEnd; >> ORIGINAL_ATTRIBUTES *OriginalPciAttributes; >> UINTN OriginalPciAttributesCount; >> + ORDERED_COLLECTION *RestrictedAllocations; >> S3_CONTEXT *S3Context; >> ORDERED_COLLECTION *Tracker; >> UINTN *InstalledKey; >> INT32 Installed; >> @@ -833,17 +993,26 @@ InstallQemuFwCfgTables ( >> QemuFwCfgReadBytes (FwCfgSize, LoaderStart); >> RestorePciDecoding (OriginalPciAttributes, OriginalPciAttributesCount); >> LoaderEnd = LoaderStart + FwCfgSize / sizeof *LoaderEntry; >> >> + Status = CollectRestrictedAllocations ( >> + &RestrictedAllocations, >> + LoaderStart, >> + LoaderEnd >> + ); >> + if (EFI_ERROR (Status)) { >> + goto FreeLoader; >> + } >> + >> S3Context = NULL; >> if (QemuFwCfgS3Enabled ()) { >> // >> // Size the allocation pessimistically, assuming that all commands in the >> // script are QEMU_LOADER_WRITE_POINTER commands. >> // >> Status = AllocateS3Context (&S3Context, LoaderEnd - LoaderStart); >> if (EFI_ERROR (Status)) { >> - goto FreeLoader; >> + goto FreeRestrictedAllocations; >> } >> } >> >> Tracker = OrderedCollectionInit (BlobCompare, BlobKeyCompare); >> @@ -862,9 +1031,13 @@ InstallQemuFwCfgTables ( >> WritePointerSubsetEnd = LoaderStart; >> for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) { >> switch (LoaderEntry->Type) { >> case QemuLoaderCmdAllocate: >> - Status = ProcessCmdAllocate (&LoaderEntry->Command.Allocate, Tracker); >> + Status = ProcessCmdAllocate ( >> + &LoaderEntry->Command.Allocate, >> + Tracker, >> + RestrictedAllocations >> + ); >> break; >> >> case QemuLoaderCmdAddPointer: >> Status = ProcessCmdAddPointer (&LoaderEntry->Command.AddPointer, >> @@ -1009,8 +1182,11 @@ FreeS3Context: >> if (S3Context != NULL) { >> ReleaseS3Context (S3Context); >> } >> >> +FreeRestrictedAllocations: >> + ReleaseRestrictedAllocations (RestrictedAllocations); >> + >> FreeLoader: >> FreePool (LoaderStart); >> >> return Status; >> -- >> 2.9.3 >>