* [PATCH 0/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted
@ 2017-06-03 15:42 Laszlo Ersek
2017-06-03 15:42 ` [PATCH 1/1] " Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2017-06-03 15:42 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Gerd Hoffmann, Igor Mammedov, Jordan Justen
This patch implements the alternative, suggested by Igor & Gerd, to
<http://mid.mail-archive.com/c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com>.
As I ultimately wrote in that thread, the NOACPI content hint is
postponed for now, and we deal with the 64-bit allocation zone only.
Ard, can you test this please? I checked with the "virt" machine type,
and according to the firmware log, everything is allocated above 4GB.
Repo: https://github.com/lersek/edk2.git
Branch: restrict_alloc
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Thanks
Laszlo
Laszlo Ersek (1):
OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless
restricted
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 190 +++++++++++++++++++-
1 file changed, 183 insertions(+), 7 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted
2017-06-03 15:42 [PATCH 0/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted Laszlo Ersek
@ 2017-06-03 15:42 ` Laszlo Ersek
2017-06-03 18:33 ` Ard Biesheuvel
2017-06-05 17:47 ` Jordan Justen
0 siblings, 2 replies; 9+ messages in thread
From: Laszlo Ersek @ 2017-06-03 15:42 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Gerd Hoffmann, Igor Mammedov, Jordan Justen
... by narrower than 8-byte ADD_POINTER references.
Introduce the CollectRestrictedAllocations() function, which iterates over
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 <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted
2017-06-03 15:42 ` [PATCH 1/1] " Laszlo Ersek
@ 2017-06-03 18:33 ` Ard Biesheuvel
2017-06-05 17:47 ` Jordan Justen
1 sibling, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2017-06-03 18:33 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Gerd Hoffmann, Igor Mammedov, Jordan Justen
On 3 June 2017 at 15:42, Laszlo Ersek <lersek@redhat.com> wrote:
> ... by narrower than 8-byte ADD_POINTER references.
>
> Introduce the CollectRestrictedAllocations() function, which iterates over
> 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 <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Thanks Laszlo
This works as expected for me. The ACPI reclaim regions happily reside
beyond the 4 GB mark on mach-virt. I haven't tried the 'enterprise'
flavor machine, but given that it does not actually exist yet, there
are no compatibility concerns, and so the behavior observed on
mach-virt should extrapolate to that machine as well.
Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
I am quite pleased with this solution, given that it should seamlessly
interoperate with QEMU versions with or without patch cb51ac2ffe36.
One realization I did make due to my involvement in this discussion is
that ACPI reclaim regions are omitted from the linear mapping on arm64
for no good reason: even though we preserve such regions (for kexec or
other 'late' references to their contents), the fact that the firmware
allows them to be reclaimed implies that they have no special
significance to the firmware, and so there is really no good reason to
punch them out of the linear mapping. So while the [modest] TLB
footprint concern* does apply to mach-virt currently, I intend to
propose a change to the arm64 kernel to deal with ACPI reclaim regions
differently, after which this benefit no longer exists.
Thanks for all the work.
Ard.
* The default arm64 defconfig defines 4 levels of stage 1 page tables,
and KVM uses 3 levels with concatenation at level one, resulting in a
48-bit VA space and a 40-bit IPA space. Under virt, each level of
lookup at stage 1 may result in a full page table walk at stage 2, and
so it is generally wise to use as few levels as possible. ARM is
finicky about aliased mappings with mismatched attributes, and so we
omit 'special' memory regions from the linear mapping, which means the
remaining DRAM in the same 1 GB frame cannot be mapped as efficiently.
This is why it is preferable to keep all such regions close together.
> ---
> 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted
2017-06-03 15:42 ` [PATCH 1/1] " Laszlo Ersek
2017-06-03 18:33 ` Ard Biesheuvel
@ 2017-06-05 17:47 ` Jordan Justen
2017-06-06 18:16 ` Laszlo Ersek
1 sibling, 1 reply; 9+ messages in thread
From: Jordan Justen @ 2017-06-05 17:47 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Ard Biesheuvel, Gerd Hoffmann, Igor Mammedov
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 <jordan.l.justen@intel.com>
> 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 <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted
2017-06-05 17:47 ` Jordan Justen
@ 2017-06-06 18:16 ` Laszlo Ersek
2017-06-07 23:10 ` Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2017-06-06 18:16 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01; +Cc: Ard Biesheuvel, Gerd Hoffmann, Igor Mammedov
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 <jordan.l.justen@intel.com>
>
>> 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 <ard.biesheuvel@linaro.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> 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
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted
2017-06-06 18:16 ` Laszlo Ersek
@ 2017-06-07 23:10 ` Laszlo Ersek
2017-06-08 10:11 ` Ard Biesheuvel
0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2017-06-07 23:10 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01; +Cc: Ard Biesheuvel
On 06/06/17 20:16, Laszlo Ersek wrote:
> 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?
Ultimately I went with
s/RestrictedAllocations/AllocationsRestrictedTo32Bit/
in the patch body and in the commit message too. Cleaned up the line
lengths and such as well, plus retested the patch.
Commit 4275f38507a4.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted
2017-06-07 23:10 ` Laszlo Ersek
@ 2017-06-08 10:11 ` Ard Biesheuvel
2017-06-08 18:40 ` Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2017-06-08 10:11 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Jordan Justen, edk2-devel-01
On 7 June 2017 at 23:10, Laszlo Ersek <lersek@redhat.com> wrote:
> On 06/06/17 20:16, Laszlo Ersek wrote:
>> 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?
>
> Ultimately I went with
>
> s/RestrictedAllocations/AllocationsRestrictedTo32Bit/
>
> in the patch body and in the commit message too. Cleaned up the line
> lengths and such as well, plus retested the patch.
>
> Commit 4275f38507a4.
>
Hi Laszlo,
Thanks again for the effort. Sadly, though, this patch is breaking my CI build:
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c: In function 'InstallQemuFwCfgTables':
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c:357:29: error:
'AllocationsRestrictedTo32Bit' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
if (OrderedCollectionFind (
^
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c:975:23: note:
'AllocationsRestrictedTo32Bit' was declared here
ORDERED_COLLECTION *AllocationsRestrictedTo32Bit;
^
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted
2017-06-08 10:11 ` Ard Biesheuvel
@ 2017-06-08 18:40 ` Laszlo Ersek
2017-06-08 19:05 ` Ard Biesheuvel
0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2017-06-08 18:40 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Jordan Justen, edk2-devel-01
On 06/08/17 12:11, Ard Biesheuvel wrote:
> On 7 June 2017 at 23:10, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 06/06/17 20:16, Laszlo Ersek wrote:
>>> 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?
>>
>> Ultimately I went with
>>
>> s/RestrictedAllocations/AllocationsRestrictedTo32Bit/
>>
>> in the patch body and in the commit message too. Cleaned up the line
>> lengths and such as well, plus retested the patch.
>>
>> Commit 4275f38507a4.
>>
>
> Hi Laszlo,
>
> Thanks again for the effort. Sadly, though, this patch is breaking my CI build:
>
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c: In function 'InstallQemuFwCfgTables':
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c:357:29: error:
> 'AllocationsRestrictedTo32Bit' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> if (OrderedCollectionFind (
> ^
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c:975:23: note:
> 'AllocationsRestrictedTo32Bit' was declared here
> ORDERED_COLLECTION *AllocationsRestrictedTo32Bit;
> ^
>
Indeed, Gerd's Jenkins CI (using GCC49) reported the same (non-)issue.
It is a compiler bug. The compiler reports that the ProcessCmdAllocate()
function may be called with "AllocationsRestrictedTo32Bit"
uninitialized. That's not the case; this function call is only reached
if CollectAllocationsRestrictedTo32Bit() returns EFI_SUCCESS --
otherwise we jump to the "FreeLoader" label, way past the
ProcessCmdAllocate() call --, and when
CollectAllocationsRestrictedTo32Bit() succeeds, it will have
"AllocationsRestrictedTo32Bit" assigned.
In order to expedite things, could you please help me by submitting a
one-liner patch? Namely, please set "AllocationsRestrictedTo32Bit" to
NULL right before the CollectAllocationsRestrictedTo32Bit() function call.
(If you add a comment that the assignment exists to suppress an invalid
compiler warning, that's even better. :) )
Thank you!
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted
2017-06-08 18:40 ` Laszlo Ersek
@ 2017-06-08 19:05 ` Ard Biesheuvel
0 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2017-06-08 19:05 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Jordan Justen, edk2-devel-01
On 8 June 2017 at 18:40, Laszlo Ersek <lersek@redhat.com> wrote:
> On 06/08/17 12:11, Ard Biesheuvel wrote:
>> On 7 June 2017 at 23:10, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 06/06/17 20:16, Laszlo Ersek wrote:
>>>> 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?
>>>
>>> Ultimately I went with
>>>
>>> s/RestrictedAllocations/AllocationsRestrictedTo32Bit/
>>>
>>> in the patch body and in the commit message too. Cleaned up the line
>>> lengths and such as well, plus retested the patch.
>>>
>>> Commit 4275f38507a4.
>>>
>>
>> Hi Laszlo,
>>
>> Thanks again for the effort. Sadly, though, this patch is breaking my CI build:
>>
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c: In function 'InstallQemuFwCfgTables':
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c:357:29: error:
>> 'AllocationsRestrictedTo32Bit' may be used uninitialized in this
>> function [-Werror=maybe-uninitialized]
>> if (OrderedCollectionFind (
>> ^
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c:975:23: note:
>> 'AllocationsRestrictedTo32Bit' was declared here
>> ORDERED_COLLECTION *AllocationsRestrictedTo32Bit;
>> ^
>>
>
> Indeed, Gerd's Jenkins CI (using GCC49) reported the same (non-)issue.
>
> It is a compiler bug. The compiler reports that the ProcessCmdAllocate()
> function may be called with "AllocationsRestrictedTo32Bit"
> uninitialized. That's not the case; this function call is only reached
> if CollectAllocationsRestrictedTo32Bit() returns EFI_SUCCESS --
> otherwise we jump to the "FreeLoader" label, way past the
> ProcessCmdAllocate() call --, and when
> CollectAllocationsRestrictedTo32Bit() succeeds, it will have
> "AllocationsRestrictedTo32Bit" assigned.
>
> In order to expedite things, could you please help me by submitting a
> one-liner patch? Namely, please set "AllocationsRestrictedTo32Bit" to
> NULL right before the CollectAllocationsRestrictedTo32Bit() function call.
>
Done
Thanks,
Ard.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-06-08 19:04 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-03 15:42 [PATCH 0/1] OvmfPkg/AcpiPlatformDxe: alloc blobs from 64-bit space unless restricted Laszlo Ersek
2017-06-03 15:42 ` [PATCH 1/1] " Laszlo Ersek
2017-06-03 18:33 ` Ard Biesheuvel
2017-06-05 17:47 ` Jordan Justen
2017-06-06 18:16 ` Laszlo Ersek
2017-06-07 23:10 ` Laszlo Ersek
2017-06-08 10:11 ` Ard Biesheuvel
2017-06-08 18:40 ` Laszlo Ersek
2017-06-08 19:05 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox