From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 7A7BD21A16EFF for ; Mon, 5 Jun 2017 10:46:39 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jun 2017 10:47:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,302,1493708400"; d="scan'208";a="95808323" Received: from sjaworsk-mobl1.amr.corp.intel.com (HELO localhost) ([10.255.73.108]) by orsmga002.jf.intel.com with ESMTP; 05 Jun 2017 10:47:44 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <149668486395.8581.9154897144168590783@jljusten-skl> From: Jordan Justen In-Reply-To: <20170603154203.29907-2-lersek@redhat.com> Cc: Ard Biesheuvel , Gerd Hoffmann , Igor Mammedov References: <20170603154203.29907-1-lersek@redhat.com> <20170603154203.29907-2-lersek@redhat.com> User-Agent: alot/0.5.1 Date: Mon, 05 Jun 2017 10:47:44 -0700 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: Mon, 05 Jun 2017 17:46:39 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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'? 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/AcpiPlatfo= rmDxe/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 =3D OrderedCollectionMin (RestrictedAllocations); > + Entry !=3D NULL; > + Entry =3D Entry2) { > + Entry2 =3D 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 field= s, such > + that QEMU_LOADER_ADD_POINTER.PointerSize is less than 8. This means th= at 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 li= nking > + (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 t= he > + 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 =3D OrderedCollectionInit (AsciiStringCompare, AsciiStringC= ompare); > + if (Collection =3D=3D NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + for (LoaderEntry =3D LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEnt= ry) { > + CONST QEMU_LOADER_ADD_POINTER *AddPointer; > + > + if (LoaderEntry->Type !=3D QemuLoaderCmdAddPointer) { > + continue; > + } > + AddPointer =3D &LoaderEntry->Command.AddPointer; > + > + if (AddPointer->PointerSize >=3D 8) { > + continue; > + } > + > + if (AddPointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] !=3D '\0') { > + DEBUG ((DEBUG_ERROR, "%a: malformed file name\n", __FUNCTION__)); > + Status =3D EFI_PROTOCOL_ERROR; > + goto RollBack; > + } > + > + Status =3D 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 =3D 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(), nami= ng the > + fw_cfg blobs that must not be alloca= ted > + 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 =3D EFI_SIZE_TO_PAGES (FwCfgSize); > - Address =3D 0xFFFFFFFF; > + Address =3D MAX_UINT64; > + if (OrderedCollectionFind (RestrictedAllocations, Allocate->File) !=3D= NULL) { > + Address =3D MAX_UINT32; > + } > Status =3D gBS->AllocatePages (AllocateMaxAddress, EfiACPIMemoryNVS, N= umPages, > &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 =3D LoaderStart + FwCfgSize / sizeof *LoaderEntry; > = > + Status =3D CollectRestrictedAllocations ( > + &RestrictedAllocations, > + LoaderStart, > + LoaderEnd > + ); > + if (EFI_ERROR (Status)) { > + goto FreeLoader; > + } > + > S3Context =3D NULL; > if (QemuFwCfgS3Enabled ()) { > // > // Size the allocation pessimistically, assuming that all commands i= n the > // script are QEMU_LOADER_WRITE_POINTER commands. > // > Status =3D AllocateS3Context (&S3Context, LoaderEnd - LoaderStart); > if (EFI_ERROR (Status)) { > - goto FreeLoader; > + goto FreeRestrictedAllocations; > } > } > = > Tracker =3D OrderedCollectionInit (BlobCompare, BlobKeyCompare); > @@ -862,9 +1031,13 @@ InstallQemuFwCfgTables ( > WritePointerSubsetEnd =3D LoaderStart; > for (LoaderEntry =3D LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEnt= ry) { > switch (LoaderEntry->Type) { > case QemuLoaderCmdAllocate: > - Status =3D ProcessCmdAllocate (&LoaderEntry->Command.Allocate, Tra= cker); > + Status =3D ProcessCmdAllocate ( > + &LoaderEntry->Command.Allocate, > + Tracker, > + RestrictedAllocations > + ); > break; > = > case QemuLoaderCmdAddPointer: > Status =3D ProcessCmdAddPointer (&LoaderEntry->Command.AddPointer, > @@ -1009,8 +1182,11 @@ FreeS3Context: > if (S3Context !=3D NULL) { > ReleaseS3Context (S3Context); > } > = > +FreeRestrictedAllocations: > + ReleaseRestrictedAllocations (RestrictedAllocations); > + > FreeLoader: > FreePool (LoaderStart); > = > return Status; > -- = > 2.9.3 >=20