Date   

Re: [edk2-platforms: PATCH V9] Platform/Intel: Correct CPU APIC IDs

Ni, Ray
 

+ //
+ // 5. Re-assigen AcpiProcessorId for AcpiProcessorUId uses purpose.
+ //
+ for (Socket = 0; Socket < MAX_SOCKET; Socket++) {
+ for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus; CurrProcessor++) {
+ if (mCpuApicIdOrderTable[CurrProcessor].Flags && (mCpuApicIdOrderTable[CurrProcessor].SocketNum == Socket)) {
+ mCpuApicIdOrderTable[CurrProcessor].AcpiProcessorId = (ProcessorInfoBuffer.Location.Package << mNumOfBitShift) +
Index;
+ Index++;
}
}
+ }
1. I think you need to change above code to as below?
UINTN IndexInSocket[MAX_SOCKET];

ZeroMem (IndexInSocket, sizeof (IndexInSocket));

for (Socket = 0; Socket < MAX_SOCKET; Socket++) {
for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus; CurrProcessor++) {
if (mCpuApicIdOrderTable[CurrProcessor].Flags && (mCpuApicIdOrderTable[CurrProcessor].SocketNum == Socket)) {
mCpuApicIdOrderTable[CurrProcessor].AcpiProcessorId = (ProcessorInfoBuffer.Location.Package << mNumOfBitShift) + IndexInSocket[Socket];
IndexInSocket[Socket]++;
}
}

2. Can you separate the code refinement change (looks like most of the changes below) in a separate patch?
(No more comments)


- //keep for debug purpose
- DEBUG ((EFI_D_ERROR, "APIC ID Order Table ReOrdered\n"));
- DebugDisplayReOrderTable();
+ //keep for debug purpose
+ DEBUG ((DEBUG_INFO, "APIC ID Order Table ReOrdered\n"));
+ DebugDisplayReOrderTable (mCpuApicIdOrderTable);

- mCpuOrderSorted = TRUE;
- }
+ mCpuOrderSorted = TRUE;

return Status;
}
@@ -602,11 +493,11 @@ InitializeMadtHeader (
}

Status = InitializeHeader (
- &MadtHeader->Header,
- EFI_ACPI_4_0_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
- EFI_ACPI_4_0_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION,
- 0
- );
+ &MadtHeader->Header,
+ EFI_ACPI_4_0_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
+ EFI_ACPI_4_0_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION,
+ 0
+ );
if (EFI_ERROR (Status)) {
return Status;
}
@@ -784,11 +675,11 @@ BuildAcpiTable (
// Allocate the memory needed for the table.
//
Status = AllocateTable (
- TableSpecificHdrLength,
- Structures,
- StructureCount,
- &InternalTable
- );
+ TableSpecificHdrLength,
+ Structures,
+ StructureCount,
+ &InternalTable
+ );
if (EFI_ERROR (Status)) {
return Status;
}
@@ -871,18 +762,22 @@ InstallMadtFromScratch (
NewMadtTable = NULL;
MaxMadtStructCount = 0;

- DetectApicIdMap();
+ mCpuApicIdOrderTable = AllocateZeroPool (mNumberOfCpus * sizeof (EFI_CPU_ID_ORDER_MAP));
+ if (mCpuApicIdOrderTable == NULL) {
+ DEBUG ((DEBUG_ERROR, "Could not allocate mCpuApicIdOrderTable structure pointer array\n"));
+ return EFI_OUT_OF_RESOURCES;
+ }

// Call for Local APIC ID Reorder
Status = SortCpuLocalApicInTable ();
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "SortCpuLocalApicInTable failed: %r\n", Status));
+ DEBUG ((DEBUG_ERROR, "SortCpuLocalApicInTable failed: %r\n", Status));
goto Done;
}

MaxMadtStructCount = (UINT32) (
- MAX_CPU_NUM + // processor local APIC structures
- MAX_CPU_NUM + // processor local x2APIC structures
+ mNumberOfCpus + // processor local APIC structures
+ mNumberOfCpus + // processor local x2APIC structures
1 + PcdGet8(PcdPcIoApicCount) + // I/O APIC structures
2 + // interrupt source override structures
1 + // local APIC NMI structures
@@ -906,11 +801,11 @@ InstallMadtFromScratch (
//
Status = InitializeMadtHeader (&MadtTableHeader);
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "InitializeMadtHeader failed: %r\n", Status));
+ DEBUG ((DEBUG_ERROR, "InitializeMadtHeader failed: %r\n", Status));
goto Done;
}

- DEBUG ((EFI_D_INFO, "Number of CPUs detected = %d \n", mNumberOfCPUs));
+ DEBUG ((DEBUG_INFO, "Number of CPUs detected = %d \n", mNumberOfCpus));

//
// Build Processor Local APIC Structures and Processor Local X2APIC Structures
@@ -923,7 +818,7 @@ InstallMadtFromScratch (
ProcLocalX2ApicStruct.Reserved[0] = 0;
ProcLocalX2ApicStruct.Reserved[1] = 0;

- for (Index = 0; Index < MAX_CPU_NUM; Index++) {
+ for (Index = 0; Index < mNumberOfCpus; Index++) {
//
// If x2APIC mode is not enabled, and if it is possible to express the
// APIC ID as a UINT8, use a processor local APIC structure. Otherwise,
@@ -936,10 +831,10 @@ InstallMadtFromScratch (

ASSERT (MadtStructsIndex < MaxMadtStructCount);
Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &ProcLocalApicStruct,
- &MadtStructs[MadtStructsIndex++]
- );
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &ProcLocalApicStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
} else if (mCpuApicIdOrderTable[Index].ApicId != 0xFFFFFFFF) {
ProcLocalX2ApicStruct.Flags = (UINT8) mCpuApicIdOrderTable[Index].Flags;
ProcLocalX2ApicStruct.X2ApicId = mCpuApicIdOrderTable[Index].ApicId;
@@ -947,13 +842,13 @@ InstallMadtFromScratch (

ASSERT (MadtStructsIndex < MaxMadtStructCount);
Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &ProcLocalX2ApicStruct,
- &MadtStructs[MadtStructsIndex++]
- );
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &ProcLocalX2ApicStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
}
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "CopyMadtStructure (local APIC/x2APIC) failed: %r\n", Status));
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (local APIC/x2APIC) failed: %r\n", Status));
goto Done;
}
}
@@ -965,44 +860,44 @@ InstallMadtFromScratch (
IoApicStruct.Length = sizeof (EFI_ACPI_4_0_IO_APIC_STRUCTURE);
IoApicStruct.Reserved = 0;

- PcIoApicEnable = PcdGet32(PcdPcIoApicEnable);
+ PcIoApicEnable = PcdGet32 (PcdPcIoApicEnable);

- if (FixedPcdGet32(PcdMaxCpuSocketCount) <= 4) {
+ if (FixedPcdGet32 (PcdMaxCpuSocketCount) <= 4) {
IoApicStruct.IoApicId = PcdGet8(PcdIoApicId);
IoApicStruct.IoApicAddress = PcdGet32(PcdIoApicAddress);
IoApicStruct.GlobalSystemInterruptBase = 0;
ASSERT (MadtStructsIndex < MaxMadtStructCount);
Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &IoApicStruct,
- &MadtStructs[MadtStructsIndex++]
- );
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &IoApicStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "CopyMadtStructure (I/O APIC) failed: %r\n", Status));
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (I/O APIC) failed: %r\n", Status));
goto Done;
}
}

for (PcIoApicIndex = 0; PcIoApicIndex < PcdGet8(PcdPcIoApicCount); PcIoApicIndex++) {
- PcIoApicMask = (1 << PcIoApicIndex);
- if ((PcIoApicEnable & PcIoApicMask) == 0) {
- continue;
- }
+ PcIoApicMask = (1 << PcIoApicIndex);
+ if ((PcIoApicEnable & PcIoApicMask) == 0) {
+ continue;
+ }

- IoApicStruct.IoApicId = (UINT8)(PcdGet8(PcdPcIoApicIdBase) + PcIoApicIndex);
- IoApicStruct.IoApicAddress = CurrentIoApicAddress;
- CurrentIoApicAddress = (CurrentIoApicAddress & 0xFFFF8000) + 0x8000;
- IoApicStruct.GlobalSystemInterruptBase = (UINT32)(24 + (PcIoApicIndex * 8));
- ASSERT (MadtStructsIndex < MaxMadtStructCount);
- Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &IoApicStruct,
- &MadtStructs[MadtStructsIndex++]
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "CopyMadtStructure (I/O APIC) failed: %r\n", Status));
- goto Done;
- }
+ IoApicStruct.IoApicId = (UINT8)(PcdGet8(PcdPcIoApicIdBase) + PcIoApicIndex);
+ IoApicStruct.IoApicAddress = CurrentIoApicAddress;
+ CurrentIoApicAddress = (CurrentIoApicAddress & 0xFFFF8000) + 0x8000;
+ IoApicStruct.GlobalSystemInterruptBase = (UINT32)(24 + (PcIoApicIndex * 8));
+ ASSERT (MadtStructsIndex < MaxMadtStructCount);
+ Status = CopyStructure (
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &IoApicStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (I/O APIC) failed: %r\n", Status));
+ goto Done;
+ }
}

//
@@ -1021,12 +916,12 @@ InstallMadtFromScratch (

ASSERT (MadtStructsIndex < MaxMadtStructCount);
Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &IntSrcOverrideStruct,
- &MadtStructs[MadtStructsIndex++]
- );
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &IntSrcOverrideStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "CopyMadtStructure (IRQ2 source override) failed: %r\n", Status));
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (IRQ2 source override) failed: %r\n", Status));
goto Done;
}

@@ -1040,12 +935,12 @@ InstallMadtFromScratch (

ASSERT (MadtStructsIndex < MaxMadtStructCount);
Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &IntSrcOverrideStruct,
- &MadtStructs[MadtStructsIndex++]
- );
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &IntSrcOverrideStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "CopyMadtStructure (IRQ9 source override) failed: %r\n", Status));
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (IRQ9 source override) failed: %r\n", Status));
goto Done;
}

@@ -1060,12 +955,12 @@ InstallMadtFromScratch (

ASSERT (MadtStructsIndex < MaxMadtStructCount);
Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &LocalApciNmiStruct,
- &MadtStructs[MadtStructsIndex++]
- );
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &LocalApciNmiStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n", Status));
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n", Status));
goto Done;
}

@@ -1084,10 +979,10 @@ InstallMadtFromScratch (

ASSERT (MadtStructsIndex < MaxMadtStructCount);
Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &LocalX2ApicNmiStruct,
- &MadtStructs[MadtStructsIndex++]
- );
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &LocalX2ApicNmiStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "CopyMadtStructure (x2APIC NMI) failed: %r\n", Status));
goto Done;
@@ -1098,14 +993,14 @@ InstallMadtFromScratch (
// Build Madt Structure from the Madt Header and collection of pointers in MadtStructs[]
//
Status = BuildAcpiTable (
- (EFI_ACPI_DESCRIPTION_HEADER *) &MadtTableHeader,
- sizeof (EFI_ACPI_4_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER),
- MadtStructs,
- MadtStructsIndex,
- (UINT8 **)&NewMadtTable
- );
+ (EFI_ACPI_DESCRIPTION_HEADER *) &MadtTableHeader,
+ sizeof (EFI_ACPI_4_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER),
+ MadtStructs,
+ MadtStructsIndex,
+ (UINT8 **) &NewMadtTable
+ );
if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "BuildAcpiTable failed: %r\n", Status));
+ DEBUG ((DEBUG_ERROR, "BuildAcpiTable failed: %r\n", Status));
goto Done;
}

@@ -1113,11 +1008,11 @@ InstallMadtFromScratch (
// Publish Madt Structure to ACPI
//
Status = mAcpiTable->InstallAcpiTable (
- mAcpiTable,
- NewMadtTable,
- NewMadtTable->Header.Length,
- &TableHandle
- );
+ mAcpiTable,
+ NewMadtTable,
+ NewMadtTable->Header.Length,
+ &TableHandle
+ );

Done:
//
@@ -1136,6 +1031,10 @@ Done:
FreePool (NewMadtTable);
}

+ if (mCpuApicIdOrderTable != NULL) {
+ FreePool (mCpuApicIdOrderTable);
+ }
+
return Status;
}

@@ -1155,8 +1054,8 @@ InstallMcfgFromScratch (
PciSegmentInfo = GetPciSegmentInfo (&SegmentCount);

McfgTable = AllocateZeroPool (
- sizeof(EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_BASE_ADDRESS_TABLE_HEADER) +
-
sizeof(EFI_ACPI_MEMORY_MAPPED_ENHANCED_CONFIGURATION_SPACE_BASE_ADDRESS_ALLOCATION_STRUCTURE) *
SegmentCount
+ sizeof (EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_BASE_ADDRESS_TABLE_HEADER) +
+ sizeof
(EFI_ACPI_MEMORY_MAPPED_ENHANCED_CONFIGURATION_SPACE_BASE_ADDRESS_ALLOCATION_STRUCTURE) *
SegmentCount
);
if (McfgTable == NULL) {
DEBUG ((DEBUG_ERROR, "Could not allocate MCFG structure\n"));
@@ -1164,11 +1063,11 @@ InstallMcfgFromScratch (
}

Status = InitializeHeader (
- &McfgTable->Header,
-
EFI_ACPI_3_0_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
- EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_SPACE_ACCESS_TABLE_REVISION,
- 0
- );
+ &McfgTable->Header,
+
EFI_ACPI_3_0_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
+ EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_SPACE_ACCESS_TABLE_REVISION,
+ 0
+ );
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1192,11 +1091,11 @@ InstallMcfgFromScratch (
// Publish Madt Structure to ACPI
//
Status = mAcpiTable->InstallAcpiTable (
- mAcpiTable,
- McfgTable,
- McfgTable->Header.Length,
- &TableHandle
- );
+ mAcpiTable,
+ McfgTable,
+ McfgTable->Header.Length,
+ &TableHandle
+ );

return Status;
}
@@ -1280,7 +1179,7 @@ PlatformUpdateTables (
switch (Table->Signature) {

case EFI_ACPI_4_0_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
- ASSERT(FALSE);
+ ASSERT (FALSE);
break;

case EFI_ACPI_5_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
@@ -1324,9 +1223,9 @@ PlatformUpdateTables (
FadtHeader->XGpe1Blk.AccessSize = 0;
}

- DEBUG(( EFI_D_ERROR, "ACPI FADT table @ address 0x%x\n", Table ));
- DEBUG(( EFI_D_ERROR, " IaPcBootArch 0x%x\n", FadtHeader->IaPcBootArch ));
- DEBUG(( EFI_D_ERROR, " Flags 0x%x\n", FadtHeader->Flags ));
+ DEBUG ((DEBUG_INFO, "ACPI FADT table @ address 0x%x\n", Table));
+ DEBUG ((DEBUG_INFO, " IaPcBootArch 0x%x\n", FadtHeader->IaPcBootArch));
+ DEBUG ((DEBUG_INFO, " Flags 0x%x\n", FadtHeader->Flags));
break;

case EFI_ACPI_3_0_HIGH_PRECISION_EVENT_TIMER_TABLE_SIGNATURE:
@@ -1346,12 +1245,12 @@ PlatformUpdateTables (
HpetBlockId.Bits.VendorId = HpetCapabilities.Bits.VendorId;
HpetTable->EventTimerBlockId = HpetBlockId.Uint32;
HpetTable->MainCounterMinimumClockTickInPeriodicMode = (UINT16)HpetCapabilities.Bits.CounterClockPeriod;
- DEBUG(( EFI_D_ERROR, "ACPI HPET table @ address 0x%x\n", Table ));
- DEBUG(( EFI_D_ERROR, " HPET base 0x%x\n", PcdGet32 (PcdHpetBaseAddress) ));
+ DEBUG ((DEBUG_INFO, "ACPI HPET table @ address 0x%x\n", Table));
+ DEBUG ((DEBUG_INFO, " HPET base 0x%x\n", PcdGet32 (PcdHpetBaseAddress)));
break;

case
EFI_ACPI_3_0_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE:
- ASSERT(FALSE);
+ ASSERT (FALSE);
break;

default:
@@ -1403,8 +1302,8 @@ IsHardwareChange (
// pFADT->XDsdt
//
HWChangeSize = HandleCount + 1;
- HWChange = AllocateZeroPool( sizeof(UINT32) * HWChangeSize );
- ASSERT( HWChange != NULL );
+ HWChange = AllocateZeroPool (sizeof(UINT32) * HWChangeSize);
+ ASSERT(HWChange != NULL);

if (HWChange == NULL) return;

@@ -1445,14 +1344,14 @@ IsHardwareChange (
// Calculate CRC value with HWChange data.
//
Status = gBS->CalculateCrc32(HWChange, HWChangeSize, &CRC);
- DEBUG((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));
+ DEBUG ((DEBUG_INFO, "CRC = %x and Status = %r\n", CRC, Status));

//
// Set HardwareSignature value based on CRC value.
//
FacsPtr = (EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE *)(UINTN)pFADT->FirmwareCtrl;
FacsPtr->HardwareSignature = CRC;
- FreePool( HWChange );
+ FreePool (HWChange);
}

VOID
@@ -1475,17 +1374,16 @@ UpdateLocalTable (

if (Version != EFI_ACPI_TABLE_VERSION_NONE) {
Status = mAcpiTable->InstallAcpiTable (
- mAcpiTable,
- CurrentTable,
- CurrentTable->Length,
- &TableHandle
- );
+ mAcpiTable,
+ CurrentTable,
+ CurrentTable->Length,
+ &TableHandle
+ );
ASSERT_EFI_ERROR (Status);
}
}
}

-
VOID
EFIAPI
AcpiEndOfDxeEvent (
@@ -1493,16 +1391,14 @@ AcpiEndOfDxeEvent (
VOID *ParentImageHandle
)
{
-
if (Event != NULL) {
- gBS->CloseEvent(Event);
+ gBS->CloseEvent (Event);
}

-
//
// Calculate Hardware Signature value based on current platform configurations
//
- IsHardwareChange();
+ IsHardwareChange ();
}

/**
@@ -1526,7 +1422,6 @@ InstallAcpiPlatform (
EFI_STATUS Status;
EFI_EVENT EndOfDxeEvent;

-
Status = gBS->LocateProtocol (&gEfiMpServiceProtocolGuid, NULL, (VOID **)&mMpService);
ASSERT_EFI_ERROR (Status);

@@ -1550,19 +1445,19 @@ InstallAcpiPlatform (
// Determine the number of processors
//
mMpService->GetNumberOfProcessors (
- mMpService,
- &mNumberOfCPUs,
- &mNumberOfEnabledCPUs
- );
- ASSERT (mNumberOfCPUs <= MAX_CPU_NUM && mNumberOfEnabledCPUs >= 1);
- DEBUG ((DEBUG_INFO, "mNumberOfCPUs - %d\n", mNumberOfCPUs));
+ mMpService,
+ &mNumberOfCpus,
+ &mNumberOfEnabledCPUs
+ );
+
+ DEBUG ((DEBUG_INFO, "mNumberOfCpus - %d\n", mNumberOfCpus));
DEBUG ((DEBUG_INFO, "mNumberOfEnabledCPUs - %d\n", mNumberOfEnabledCPUs));

DEBUG ((DEBUG_INFO, "mX2ApicEnabled - 0x%x\n", mX2ApicEnabled));
DEBUG ((DEBUG_INFO, "mForceX2ApicId - 0x%x\n", mForceX2ApicId));

// support up to 64 threads/socket
- AsmCpuidEx(CPUID_EXTENDED_TOPOLOGY, 1, &mNumOfBitShift, NULL, NULL, NULL);
+ AsmCpuidEx (CPUID_EXTENDED_TOPOLOGY, 1, &mNumOfBitShift, NULL, NULL, NULL);
mNumOfBitShift &= 0x1F;
DEBUG ((DEBUG_INFO, "mNumOfBitShift - 0x%x\n", mNumOfBitShift));

diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.h
b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.h
index bd11f9e988..61f7470f80 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.h
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.h
@@ -1,7 +1,7 @@
/** @file
This is an implementation of the ACPI platform driver.

-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -35,6 +35,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/MemoryAllocationLib.h>
#include <Library/AslUpdateLib.h>
#include <Library/PciSegmentInfoLib.h>
+#include <Library/SortLib.h>
+#include <Library/LocalApicLib.h>

#include <Protocol/AcpiTable.h>
#include <Protocol/MpService.h>
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
index 5d9c8cab50..95f6656af0 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.inf
@@ -1,7 +1,7 @@
### @file
# Component information file for AcpiPlatform module
#
-# Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -43,6 +43,8 @@
PciSegmentInfoLib
AslUpdateLib
BoardAcpiTableLib
+ SortLib
+ LocalApicLib

[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId
--
2.32.0.windows.2


Re: [RFC] Expose HII package list via C variables

Marvin Häuser
 

Hey Mike,

On 26/08/2021 18:44, Michael D Kinney wrote:
Marvin,

The main reason most components in the EDK II repos continue to use the variables is
because there are incomplete tools to generate PE/COFF resource sections for all
the OS/compiler combinations that EDK II supports.

The preference would be to use PE/COFF resource sections and we would have converted
all components to that style long ago if the tools existed to be aligned with the
UEFI Specification instead of an EDK II specific implementation feature.
If it's not broken... :)

Also, it is not a good idea to only look at the open source EDK II repos to
understand how a feature is used. There are many downstream consumers of the
EDK II repos.
Well, that is why it is a RFC. I find it unfortunate that there are many features present (e.g. fix-address loading) with no explanation of why they were introduced.

The UEFI Driver Writer's Guide *only* documents the PE/COFF resource section
method.

https://tianocore-docs.github.io/edk2-UefiDriverWritersGuide/draft/7_driver_entry_point/74_adding_hii_packages_feature.html#74-adding-hii-packages-feature
Yes, that is part of the reason I did not know about the current variable feature.

Looking at the current feedback from others, there are (still) proprietary use-cases that cannot be updated easily. New proposal:
1. Import my new code to generate package lists into C variables
2. Remove the current HII variable code and update the modules accordingly
3. Introduce a FixedPcd to disable HII lookup in DxeCore like suggested, lookup enabled by default
4. Do not deprecate the PE/COFF section on spec-level, at least for now

1. + 2. gives us easy unified code paths, as the data will be equivalent for both methods, and there can be a library class with constructor/destructor to register the HII data with HiiDatabase, no matter whether a module uses PE/COFF or C HII information. The platform can then decide on its own whether to support the mechanism or not, and modules can easily be toggled between them. In my opinion, variables should be preferred unless there is a need for external parsing of the data. This is not really "EDK II vs spec", because this is not about an interface, but about private module data lookup.
3. Allows trees like Project Amaranth to disable the additional parsing at their own discretion. This technically does violate the spec, and should be documented accordingly, but if it is known ahead of time which software is run, this is a safe decision to make, and it reduces the attack surface.
4. If it will ever be deprecated, it can be a natural decision from shifted use-cases.

Comments? :)

Best regards,
Marvin


Best regards,

Mike

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Thursday, August 26, 2021 9:07 AM
To: tim.lewis@insyde.com; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: 'Andrew Fish' <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
<eric.dong@intel.com>; 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Vitaly Cheptsov' <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Hey Tim,

Interesting, do you happen to have some tool or code that performs such
edits at hand to take a look at? Seems like most modules already use
variables and thus cannot be modified in such a way even right now?

That's the kind of information I am looking for, thanks a lot!

Best regards,
Marvin

On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
Hi Marvin --

I would like to add some historical perspective on this. One of the design requirements back when HII was first
introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be
able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not
affect the offsets of the code and data in the binary.
While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for
possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource
sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary
format (resource fork?).
Thanks,

Tim Lewis
CTO, Insyde Software
www.insyde.com

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Thursday, August 26, 2021 7:37 AM
To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
<eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Marvin,

One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI
Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image
are allowed to use this feature, This means the system FW PE/COFF loader must support loading HII content from this
PE/COFF resource section to be UEFI conformant. So we cannot remove this feature from the PE/COFF loader without changes
to the UEFI Specification. Even if changes to the UEFI Specification we made, we would have to continue to support this
feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.
Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
Häuser
Sent: Thursday, August 26, 2021 1:51 AM
To: devel@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
<eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
variables

Hey Mike,

Thanks for your reply!

Well, this switch is not well-documented. Looking now at both the
generation code and the emitted code, it does not generate a package
list like my code does, but separate data variables (strings and
images) that cannot easily be passed to HiiDatabase as-is. However
apparently there are drivers that use this functionality successfully
by composing the package list at runtime [1].

Looking with this information now at the pattern of using HII C
variables (which I did not know existed before) vs the PE/COFF HII
section, most that use latter are Shell applications, which I guess
means the section has actually been introduced to resolve D.? There
are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
is not a problem, hence I got confused, sorry. I think these modules
should be updated in any case. Do you agree?

So, for modules that use C variables already, my patch would save some
runtime generation code and dynamic memory allocation for the HII
package list. This was not my goal (as I said, I didn't realise HII C
variables already were a thing in the first place), but the changes
are small enough that it might be worth considering anyway, in my opinion.
If a HII package list is generated for both Shell and non-Shell apps,
this also means code paths can be unified. For example, there could be
a library class with constructor and destructor to add/remove packages
from the HII database for all modules that use such, Shell or not. For
BaseTools it means that there is no real need for separate Python and
C paths as ideally they just generate the exact same data.

Now to D., the only usage for this seems to be that Shell can locate
the help text in the executable without executing it, yet it is fully
loaded anyway [3]. To be honest, I find it hard to justify loading an
executable (PE/COFF loading, memory permission application, the full
process) to retrieve a help text and then unloading it again,
especially with the HII code being on a core dispatcher level. 1. to
7. still hold true in my opinion. Was there any discussion I could
read through why Shell apps cannot simply support a "--help" or "-?"
command and output the string themselves? Pushing the burden to the
Shell apps does preserve the "drawback" that a full loading is
required (which honestly does not matter for a debugging application
like Shell), however it does relieve the burden of PE/COFF HII parsing
from the core dispatcher (which matters a lot in my opinion to keep
the core simple). It would simply be a normal Shell app execution as
any other however. If someone wants to avoid the PE/COFF burden
altogether, they can still provide a .man file.

As for my points 6. and 7., maybe I should provide some context. Due
to many issues with TE files, platforms started abandoning them and
returned to PE/COFF Images. I think a big reason for this is that TE
is not really a sound and complete format, but a stripped version of
PE/COFF with none of the necessary fixups applied. I'm currently
sketching a possible alternative [4], and I would really like to not
having to specify a HII section type, while still preserving
compatibility with all of the UEFI Image types and use-cases [4].

Thanks again!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
p/BootManagerMenu.c#L929-L934

[2]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
c#L646-L671

[4]
https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustrySt
andard/UeImage.h

26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:

Hi Marvin,

I think this feature is already there and supported.

HII can either be in a global variable or in a PE/COFF resource section.
The default is a global variable because HII was implemented before
the PE/COFF resource section feature was added to the UEFI Specification.

There is an INF [Defines] section statement to enable the PE/COFF
section. See UefiHiiResource in the following link.

https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
i_inf_file_format/34_[defines]_section.html#34-
defines-section
How is your proposal different than this existing capability?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Marvin Häuser
Sent: Wednesday, August 25, 2021 2:21 PM
To: devel@edk2.groups.io
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: [edk2-devel] [RFC] Expose HII package list via C variables

Good day everyone,

Currently, the HII package list is stored in a PE/COFF resource
section [1]. I propose to store it in a C variable (byte array with
a pointer to it and its size exposed) instead. DxeCore would have a
guard to toggle the deprecated support for the automatic protocol
installation. This has the following advantages:

1. Fixes BZ (incl. future toolchains):
https://bugzilla.tianocore.org/show_bug.cgi?id=557
2. Universal method across all toolchains and output file formats
3. Saves error-prone parsing work 4. Saves protocol install/locate
work, the data is available right away 5. The omission of a
dedicated section can save space 6. Terse file formats can support
this and remain terse :) 7. Removes a dependency on the PE/COFF
format specifically

A *very rough* PoC diff can be found here:
https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
If the feedback is positive, I will clean it up of course. OVMF
boots with everything working fine.

I'd explicitly like feedback on the following:
A. Is this an acceptable solution to BZ 557 (Andrew?)?
B. Is this an acceptable solution for the "HII workflow" (MdeModule
maintainers?)?
C. Is it acceptable to make support UEFI-side support for the old
mechanism optional (Stewards?)?
D. Can an acceptable alternative be found for the removed ShellPkg
code (Shell maintainers?)?

As you can see the BaseTools part also is rough, but that is more a
question of "how" rather than "whether", so I'll postpone asking about it.

Thanks for your time and feedback!

Best regards,
Marvin


[1] "Once the image is loaded, LoadImage() installs
EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a
custom PE/COFF resource with the type 'HII'."
- UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"









Re: [RFC] Expose HII package list via C variables

Tim Lewis
 

Hi Marvin --

Sorry, nothing I can share. Thanks,

Tim

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Thursday, August 26, 2021 9:07 AM
To: tim.lewis@insyde.com; devel@edk2.groups.io; michael.d.kinney@intel.com
Cc: 'Andrew Fish' <afish@apple.com>; leif@nuviainc.com; 'Ni, Ray' <ray.ni@intel.com>; 'Gao, Zhichao' <zhichao.gao@intel.com>; 'Wang, Jian J' <jian.j.wang@intel.com>; 'Wu, Hao A' <hao.a.wu@intel.com>; 'Bi, Dandan' <dandan.bi@intel.com>; 'Dong, Eric' <eric.dong@intel.com>; 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Vitaly Cheptsov' <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Hey Tim,

Interesting, do you happen to have some tool or code that performs such edits at hand to take a look at? Seems like most modules already use variables and thus cannot be modified in such a way even right now?

That's the kind of information I am looking for, thanks a lot!

Best regards,
Marvin

On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
Hi Marvin --

I would like to add some historical perspective on this. One of the design requirements back when HII was first introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not affect the offsets of the code and data in the binary.

While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary format (resource fork?).

Thanks,

Tim Lewis
CTO, Insyde Software
www.insyde.com

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
D Kinney
Sent: Thursday, August 26, 2021 7:37 AM
To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D
<michael.d.kinney@intel.com>
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray
<ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
<Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
variables

Marvin,

One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image are allowed to use this feature, This means the system FW PE/COFF loader must support loading HII content from this PE/COFF resource section to be UEFI conformant. So we cannot remove this feature from the PE/COFF loader without changes to the UEFI Specification. Even if changes to the UEFI Specification we made, we would have to continue to support this feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
Häuser
Sent: Thursday, August 26, 2021 1:51 AM
To: devel@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong,
Eric <eric.dong@intel.com>; Bret Barkelew
<Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
variables

Hey Mike,

Thanks for your reply!

Well, this switch is not well-documented. Looking now at both the
generation code and the emitted code, it does not generate a package
list like my code does, but separate data variables (strings and
images) that cannot easily be passed to HiiDatabase as-is. However
apparently there are drivers that use this functionality successfully
by composing the package list at runtime [1].

Looking with this information now at the pattern of using HII C
variables (which I did not know existed before) vs the PE/COFF HII
section, most that use latter are Shell applications, which I guess
means the section has actually been introduced to resolve D.? There
are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
is not a problem, hence I got confused, sorry. I think these modules
should be updated in any case. Do you agree?

So, for modules that use C variables already, my patch would save
some runtime generation code and dynamic memory allocation for the
HII package list. This was not my goal (as I said, I didn't realise
HII C variables already were a thing in the first place), but the
changes are small enough that it might be worth considering anyway, in my opinion.
If a HII package list is generated for both Shell and non-Shell apps,
this also means code paths can be unified. For example, there could
be a library class with constructor and destructor to add/remove
packages from the HII database for all modules that use such, Shell
or not. For BaseTools it means that there is no real need for
separate Python and C paths as ideally they just generate the exact same data.

Now to D., the only usage for this seems to be that Shell can locate
the help text in the executable without executing it, yet it is fully
loaded anyway [3]. To be honest, I find it hard to justify loading an
executable (PE/COFF loading, memory permission application, the full
process) to retrieve a help text and then unloading it again,
especially with the HII code being on a core dispatcher level. 1. to
7. still hold true in my opinion. Was there any discussion I could
read through why Shell apps cannot simply support a "--help" or "-?"
command and output the string themselves? Pushing the burden to the
Shell apps does preserve the "drawback" that a full loading is
required (which honestly does not matter for a debugging application
like Shell), however it does relieve the burden of PE/COFF HII
parsing from the core dispatcher (which matters a lot in my opinion
to keep the core simple). It would simply be a normal Shell app
execution as any other however. If someone wants to avoid the PE/COFF
burden altogether, they can still provide a .man file.

As for my points 6. and 7., maybe I should provide some context. Due
to many issues with TE files, platforms started abandoning them and
returned to PE/COFF Images. I think a big reason for this is that TE
is not really a sound and complete format, but a stripped version of
PE/COFF with none of the necessary fixups applied. I'm currently
sketching a possible alternative [4], and I would really like to not
having to specify a HII section type, while still preserving
compatibility with all of the UEFI Image types and use-cases [4].

Thanks again!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
a b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
p/BootManagerMenu.c#L929-L934

[2]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
a
b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
c#L646-L671

[4]
https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustryS
t
andard/UeImage.h

26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:

Hi Marvin,

I think this feature is already there and supported.

HII can either be in a global variable or in a PE/COFF resource section.
The default is a global variable because HII was implemented before
the PE/COFF resource section feature was added to the UEFI Specification.

There is an INF [Defines] section statement to enable the PE/COFF
section. See UefiHiiResource in the following link.

https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
i_inf_file_format/34_[defines]_section.html#34-
defines-section
How is your proposal different than this existing capability?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Marvin Häuser
Sent: Wednesday, August 25, 2021 2:21 PM
To: devel@edk2.groups.io
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: [edk2-devel] [RFC] Expose HII package list via C variables

Good day everyone,

Currently, the HII package list is stored in a PE/COFF resource
section [1]. I propose to store it in a C variable (byte array with
a pointer to it and its size exposed) instead. DxeCore would have a
guard to toggle the deprecated support for the automatic protocol
installation. This has the following advantages:

1. Fixes BZ (incl. future toolchains):
https://bugzilla.tianocore.org/show_bug.cgi?id=557
2. Universal method across all toolchains and output file formats
3. Saves error-prone parsing work 4. Saves protocol install/locate
work, the data is available right away 5. The omission of a
dedicated section can save space 6. Terse file formats can support
this and remain terse :) 7. Removes a dependency on the PE/COFF
format specifically

A *very rough* PoC diff can be found here:
https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
If the feedback is positive, I will clean it up of course. OVMF
boots with everything working fine.

I'd explicitly like feedback on the following:
A. Is this an acceptable solution to BZ 557 (Andrew?)?
B. Is this an acceptable solution for the "HII workflow" (MdeModule
maintainers?)?
C. Is it acceptable to make support UEFI-side support for the old
mechanism optional (Stewards?)?
D. Can an acceptable alternative be found for the removed ShellPkg
code (Shell maintainers?)?

As you can see the BaseTools part also is rough, but that is more a
question of "how" rather than "whether", so I'll postpone asking about it.

Thanks for your time and feedback!

Best regards,
Marvin


[1] "Once the image is loaded, LoadImage() installs
EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a
custom PE/COFF resource with the type 'HII'."
- UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"










Re: [RFC] Expose HII package list via C variables

Marvin Häuser
 

Good day Samer,

Thanks a lot for your reply!

On 26/08/2021 18:49, Samer El-Haj-Mahmoud wrote:
I am aware of some proprietary tools (not open source) that depend on this functionality.

The feature has been used, especially on some server designs, to allow exporting HII packages and consuming them offline by out-of-band or OS-based management applications for instance.
OS-based, so we are talking about DXE Runtime Drivers with HII support? Can you share how they retrieve the data, i.e. do they parse the PE/COFF header at OS runtime, or is there maybe some OS loader support that collects all installed packages before kernel handover? As I mentioned, part of the rationale is to support formats other than PE/COFF (especially a new terse format), so the former mechanisms would break for the long-term future no matter what (if adoption is progressing of course). In the latter case, functionality should not really be affected I believe, or am I missing something?

The goal is not only to reduce core dispatcher parsing work, but also to abstract away file format specifics with UEFI concepts (e.g. I replaced all parsing of PE/COFF to retrieve the PDB with a modification of the Debug Image Info Table to store the PDB file path). Publishing to the OS could work with some OS-proprietary mechanism, or maybe with the help of the SystemTable ConfigTable?

The need for this seems to be declining though, as implementations move to using standard Redfish data instead, which in turn can be produced from HII packages, either during boot (like what is done in EDK2 RedfishPkg), or at build / release time.
How is this data retrieved then, some dynamic database the OS loader passes along?

Thanks!

Best regards,
Marvin


Thanks,
--Samer


-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
Häuser via groups.io
Sent: Thursday, August 26, 2021 12:07 PM
To: tim.lewis@insyde.com; devel@edk2.groups.io;
michael.d.kinney@intel.com
Cc: 'Andrew Fish' <afish@apple.com>; leif@nuviainc.com; 'Ni, Ray'
<ray.ni@intel.com>; 'Gao, Zhichao' <zhichao.gao@intel.com>; 'Wang, Jian J'
<jian.j.wang@intel.com>; 'Wu, Hao A' <hao.a.wu@intel.com>; 'Bi, Dandan'
<dandan.bi@intel.com>; 'Dong, Eric' <eric.dong@intel.com>; 'Bret Barkelew'
<Bret.Barkelew@microsoft.com>; 'Vitaly Cheptsov'
<vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Hey Tim,

Interesting, do you happen to have some tool or code that performs such
edits at hand to take a look at? Seems like most modules already use
variables and thus cannot be modified in such a way even right now?

That's the kind of information I am looking for, thanks a lot!

Best regards,
Marvin

On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
Hi Marvin --

I would like to add some historical perspective on this. One of the design
requirements back when HII was first introduced into the UEFI specification
after Intel's initial contribution was that of binary editability. In order to be
able to reliably find, extract and then re-insert the HII data into the binary, it
needed to be discoverable and not affect the offsets of the code and data in
the binary.
While it was possible to put some sort of signature in the read-only data
sections of the binary and leave padding for possible future edits, it was felt
that using a PE/COFF section similar to the resource sections was better.
Resource sections are commonly in use for PE/COFF files (in Windows) and
the similar idea existed in the then-extant Mac binary format (resource
fork?).
Thanks,

Tim Lewis
CTO, Insyde Software
www.insyde.com

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Michael D Kinney
Sent: Thursday, August 26, 2021 7:37 AM
To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D
<michael.d.kinney@intel.com>
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray
<ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
<Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Marvin,

One constraint in this discussion is that the HII content in a PE/COFF
resource section is defined in the UEFI Specification, Which means UEFI Apps
and UEFI Drivers from media and option ROMs that are not part of the
system FW image are allowed to use this feature, This means the system FW
PE/COFF loader must support loading HII content from this PE/COFF resource
section to be UEFI conformant. So we cannot remove this feature from the
PE/COFF loader without changes to the UEFI Specification. Even if changes to
the UEFI Specification we made, we would have to continue to support this
feature for backward compatibility with existing UEFI Apps/Drivers that may
be using this feature.
Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Marvin
Häuser
Sent: Thursday, August 26, 2021 1:51 AM
To: devel@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
Hao
A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
<eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
variables

Hey Mike,

Thanks for your reply!

Well, this switch is not well-documented. Looking now at both the
generation code and the emitted code, it does not generate a package
list like my code does, but separate data variables (strings and
images) that cannot easily be passed to HiiDatabase as-is. However
apparently there are drivers that use this functionality successfully
by composing the package list at runtime [1].

Looking with this information now at the pattern of using HII C
variables (which I did not know existed before) vs the PE/COFF HII
section, most that use latter are Shell applications, which I guess
means the section has actually been introduced to resolve D.? There
are exceptions such as LogoDxe [2], which use the PE/COFF section while
D.
is not a problem, hence I got confused, sorry. I think these modules
should be updated in any case. Do you agree?

So, for modules that use C variables already, my patch would save some
runtime generation code and dynamic memory allocation for the HII
package list. This was not my goal (as I said, I didn't realise HII C
variables already were a thing in the first place), but the changes
are small enough that it might be worth considering anyway, in my
opinion.
If a HII package list is generated for both Shell and non-Shell apps,
this also means code paths can be unified. For example, there could be
a library class with constructor and destructor to add/remove packages
from the HII database for all modules that use such, Shell or not. For
BaseTools it means that there is no real need for separate Python and
C paths as ideally they just generate the exact same data.

Now to D., the only usage for this seems to be that Shell can locate
the help text in the executable without executing it, yet it is fully
loaded anyway [3]. To be honest, I find it hard to justify loading an
executable (PE/COFF loading, memory permission application, the full
process) to retrieve a help text and then unloading it again,
especially with the HII code being on a core dispatcher level. 1. to
7. still hold true in my opinion. Was there any discussion I could
read through why Shell apps cannot simply support a "--help" or "-?"
command and output the string themselves? Pushing the burden to the
Shell apps does preserve the "drawback" that a full loading is
required (which honestly does not matter for a debugging application
like Shell), however it does relieve the burden of PE/COFF HII parsing
from the core dispatcher (which matters a lot in my opinion to keep
the core simple). It would simply be a normal Shell app execution as
any other however. If someone wants to avoid the PE/COFF burden
altogether, they can still provide a .man file.

As for my points 6. and 7., maybe I should provide some context. Due
to many issues with TE files, platforms started abandoning them and
returned to PE/COFF Images. I think a big reason for this is that TE
is not really a sound and complete format, but a stripped version of
PE/COFF with none of the necessary fixups applied. I'm currently
sketching a possible alternative [4], and I would really like to not
having to specify a HII section type, while still preserving
compatibility with all of the UEFI Image types and use-cases [4].

Thanks again!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
a
b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
p/BootManagerMenu.c#L929-L934

[2]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
a
b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
ab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
c#L646-L671

[4]
https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/Industr
ySt
andard/UeImage.h

26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:

Hi Marvin,

I think this feature is already there and supported.

HII can either be in a global variable or in a PE/COFF resource section.
The default is a global variable because HII was implemented before
the PE/COFF resource section feature was added to the UEFI
Specification.
There is an INF [Defines] section statement to enable the PE/COFF
section. See UefiHiiResource in the following link.

https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
i_inf_file_format/34_[defines]_section.html#34-
defines-section
How is your proposal different than this existing capability?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Marvin Häuser
Sent: Wednesday, August 25, 2021 2:21 PM
To: devel@edk2.groups.io
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi,
Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: [edk2-devel] [RFC] Expose HII package list via C variables

Good day everyone,

Currently, the HII package list is stored in a PE/COFF resource
section [1]. I propose to store it in a C variable (byte array with
a pointer to it and its size exposed) instead. DxeCore would have a
guard to toggle the deprecated support for the automatic protocol
installation. This has the following advantages:

1. Fixes BZ (incl. future toolchains):
https://bugzilla.tianocore.org/show_bug.cgi?id=557
2. Universal method across all toolchains and output file formats
3. Saves error-prone parsing work 4. Saves protocol install/locate
work, the data is available right away 5. The omission of a
dedicated section can save space 6. Terse file formats can support
this and remain terse :) 7. Removes a dependency on the PE/COFF
format specifically

A *very rough* PoC diff can be found here:
https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
If the feedback is positive, I will clean it up of course. OVMF
boots with everything working fine.

I'd explicitly like feedback on the following:
A. Is this an acceptable solution to BZ 557 (Andrew?)?
B. Is this an acceptable solution for the "HII workflow" (MdeModule
maintainers?)?
C. Is it acceptable to make support UEFI-side support for the old
mechanism optional (Stewards?)?
D. Can an acceptable alternative be found for the removed ShellPkg
code (Shell maintainers?)?

As you can see the BaseTools part also is rough, but that is more a
question of "how" rather than "whether", so I'll postpone asking about
it.
Thanks for your time and feedback!

Best regards,
Marvin


[1] "Once the image is loaded, LoadImage() installs
EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains
a
custom PE/COFF resource with the type 'HII'."
- UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"







IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Re: [PATCH 18/23] OvmfPkg: Enable Tdx in SecMain.c

Yao, Jiewen
 

Comment below:

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
Hoffmann
Sent: Thursday, August 26, 2021 4:32 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb@kernel.org>; Xu, Min M
<min.m.xu@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
Jordan L <jordan.l.justen@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
Erdem Aktas <erdemaktas@google.com>; James Bottomley
<jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@amd.com>;
Yamahata, Isaku <isaku.yamahata@intel.com>
Subject: Re: [edk2-devel] [PATCH 18/23] OvmfPkg: Enable Tdx in SecMain.c

Hi,

Some reference for QEMU:
https://lists.nongnu.org/archive/html/qemu-devel/2021-07/msg01682.html
Ah, good. /me adds an entry to the todo list.

The fw_cfg is still allowed in the TDVF design guide, just because we
feel it is a burden to convert everything suddenly.
What is the longer-term plan here?

Does it make sense to special-case the memory map?

If we want handle other fw_cfg items that way too later on, shouldn't we
better check how we can improve the fw_cfg interface so it works better
with confidential computing?
[Jiewen] So far, my hope is to limit the fw_cfg as much as possible.
My worry is that we have to measure fw_cfg everywhere. If we miss one place,
it will be a completeness vulnerability for trusted computing.

I also think if we can add measurement code inside of fw_cfg get function.
Then we need improve the FwCfg API - Current style: QemuFwCfgSelectItem()
+ QemuFwCfgReadxxx() is not friendly for measurement. For example, we can
combine them and do QemuFwCfgSelectRead ().

I was more thinking about a completely different way to pass (constant)
fw_cfg data. Something like defining a fw_cfg hob and adding that to the
td hob. QemuFwCfgLib could lookup the hob and use that when it finds
the needed entry there.

In case the entry is not there try use io instead. We'll continue to
need that for the acpi tables for example, these entries are not
constant. qemu will adapt them when the firmware maps hardware
resources referenced in acpi tables (mmconfig region, power management
registers, ...).
[Jiewen] That is great idea. I really like it.



The QemuFwCfgWritexxx() interface may also bring inconsistency issue.
If we use this API, we have 2 copy data.
Do you need any writable fw_cfg entries in TDX mode?
[Jiewen] I hope NOT to support writable fw_cfg.
In our TDX design, we even don't want to support SetVariable to NV Storage, just to reduce the risk.



'git grep' shows the ramfb driver, smi feature negotiation and s3
support use QemuFwCfgWrite()
[Jiewen] TDVF does not support SMM, and TDVF does not support S3.


One is in TDVF (trusted), and
the other is in VMM/QEMU (untrusted). What if the VMM modifies its
untrusted copy?
What I can see is many potential attack surfaces. :-(
Well, you have to trust VMM/QEMU to a certain degree. TDX can prevent
data leaking, but it can't prevent VMM misbehaving.
[Jiewen] Yes, you are right. It is "in certain degree".

The threat model is :
TD cannot resist the deny-of-service (DOS) attack from VMM/QEMU.
TD need maintain the integrity and confidentiality, to avoid tamper and information disclosure.


If VMM misbehaving causes the system hang or guest device error, it is OK.
But if VMM misbehaving causes a TD secret leak to QEMU or TD tampered without being detected by measurement register (MRTD or RTMR), that is NOT acceptable.

If we allow the misbehaving, then we have to do thorough analysis to understand the impact.
If we can think of a way to avoid the possibility of misbehaving, then we know we are good. :-) That is our preference so far.



Please let me know if you need any other information.
Sure. For now I have to read more docs and patches ...

take care,
Gerd





Re: [RFC] Expose HII package list via C variables

Michael D Kinney
 

Marvin,

The main reason most components in the EDK II repos continue to use the variables is
because there are incomplete tools to generate PE/COFF resource sections for all
the OS/compiler combinations that EDK II supports.

The preference would be to use PE/COFF resource sections and we would have converted
all components to that style long ago if the tools existed to be aligned with the
UEFI Specification instead of an EDK II specific implementation feature.

Also, it is not a good idea to only look at the open source EDK II repos to
understand how a feature is used. There are many downstream consumers of the
EDK II repos.

The UEFI Driver Writer's Guide *only* documents the PE/COFF resource section
method.

https://tianocore-docs.github.io/edk2-UefiDriverWritersGuide/draft/7_driver_entry_point/74_adding_hii_packages_feature.html#74-adding-hii-packages-feature

Best regards,

Mike

-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Thursday, August 26, 2021 9:07 AM
To: tim.lewis@insyde.com; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: 'Andrew Fish' <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
<eric.dong@intel.com>; 'Bret Barkelew' <Bret.Barkelew@microsoft.com>; 'Vitaly Cheptsov' <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Hey Tim,

Interesting, do you happen to have some tool or code that performs such
edits at hand to take a look at? Seems like most modules already use
variables and thus cannot be modified in such a way even right now?

That's the kind of information I am looking for, thanks a lot!

Best regards,
Marvin

On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
Hi Marvin --

I would like to add some historical perspective on this. One of the design requirements back when HII was first
introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be
able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not
affect the offsets of the code and data in the binary.

While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for
possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource
sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary
format (resource fork?).

Thanks,

Tim Lewis
CTO, Insyde Software
www.insyde.com

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Thursday, August 26, 2021 7:37 AM
To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
<eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Marvin,

One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI
Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image
are allowed to use this feature, This means the system FW PE/COFF loader must support loading HII content from this
PE/COFF resource section to be UEFI conformant. So we cannot remove this feature from the PE/COFF loader without changes
to the UEFI Specification. Even if changes to the UEFI Specification we made, we would have to continue to support this
feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
Häuser
Sent: Thursday, August 26, 2021 1:51 AM
To: devel@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
<eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
variables

Hey Mike,

Thanks for your reply!

Well, this switch is not well-documented. Looking now at both the
generation code and the emitted code, it does not generate a package
list like my code does, but separate data variables (strings and
images) that cannot easily be passed to HiiDatabase as-is. However
apparently there are drivers that use this functionality successfully
by composing the package list at runtime [1].

Looking with this information now at the pattern of using HII C
variables (which I did not know existed before) vs the PE/COFF HII
section, most that use latter are Shell applications, which I guess
means the section has actually been introduced to resolve D.? There
are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
is not a problem, hence I got confused, sorry. I think these modules
should be updated in any case. Do you agree?

So, for modules that use C variables already, my patch would save some
runtime generation code and dynamic memory allocation for the HII
package list. This was not my goal (as I said, I didn't realise HII C
variables already were a thing in the first place), but the changes
are small enough that it might be worth considering anyway, in my opinion.
If a HII package list is generated for both Shell and non-Shell apps,
this also means code paths can be unified. For example, there could be
a library class with constructor and destructor to add/remove packages
from the HII database for all modules that use such, Shell or not. For
BaseTools it means that there is no real need for separate Python and
C paths as ideally they just generate the exact same data.

Now to D., the only usage for this seems to be that Shell can locate
the help text in the executable without executing it, yet it is fully
loaded anyway [3]. To be honest, I find it hard to justify loading an
executable (PE/COFF loading, memory permission application, the full
process) to retrieve a help text and then unloading it again,
especially with the HII code being on a core dispatcher level. 1. to
7. still hold true in my opinion. Was there any discussion I could
read through why Shell apps cannot simply support a "--help" or "-?"
command and output the string themselves? Pushing the burden to the
Shell apps does preserve the "drawback" that a full loading is
required (which honestly does not matter for a debugging application
like Shell), however it does relieve the burden of PE/COFF HII parsing
from the core dispatcher (which matters a lot in my opinion to keep
the core simple). It would simply be a normal Shell app execution as
any other however. If someone wants to avoid the PE/COFF burden
altogether, they can still provide a .man file.

As for my points 6. and 7., maybe I should provide some context. Due
to many issues with TE files, platforms started abandoning them and
returned to PE/COFF Images. I think a big reason for this is that TE
is not really a sound and complete format, but a stripped version of
PE/COFF with none of the necessary fixups applied. I'm currently
sketching a possible alternative [4], and I would really like to not
having to specify a HII section type, while still preserving
compatibility with all of the UEFI Image types and use-cases [4].

Thanks again!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
p/BootManagerMenu.c#L929-L934

[2]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
c#L646-L671

[4]
https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustrySt
andard/UeImage.h

26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:

Hi Marvin,

I think this feature is already there and supported.

HII can either be in a global variable or in a PE/COFF resource section.
The default is a global variable because HII was implemented before
the PE/COFF resource section feature was added to the UEFI Specification.

There is an INF [Defines] section statement to enable the PE/COFF
section. See UefiHiiResource in the following link.

https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
i_inf_file_format/34_[defines]_section.html#34-
defines-section
How is your proposal different than this existing capability?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Marvin Häuser
Sent: Wednesday, August 25, 2021 2:21 PM
To: devel@edk2.groups.io
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: [edk2-devel] [RFC] Expose HII package list via C variables

Good day everyone,

Currently, the HII package list is stored in a PE/COFF resource
section [1]. I propose to store it in a C variable (byte array with
a pointer to it and its size exposed) instead. DxeCore would have a
guard to toggle the deprecated support for the automatic protocol
installation. This has the following advantages:

1. Fixes BZ (incl. future toolchains):
https://bugzilla.tianocore.org/show_bug.cgi?id=557
2. Universal method across all toolchains and output file formats
3. Saves error-prone parsing work 4. Saves protocol install/locate
work, the data is available right away 5. The omission of a
dedicated section can save space 6. Terse file formats can support
this and remain terse :) 7. Removes a dependency on the PE/COFF
format specifically

A *very rough* PoC diff can be found here:
https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
If the feedback is positive, I will clean it up of course. OVMF
boots with everything working fine.

I'd explicitly like feedback on the following:
A. Is this an acceptable solution to BZ 557 (Andrew?)?
B. Is this an acceptable solution for the "HII workflow" (MdeModule
maintainers?)?
C. Is it acceptable to make support UEFI-side support for the old
mechanism optional (Stewards?)?
D. Can an acceptable alternative be found for the removed ShellPkg
code (Shell maintainers?)?

As you can see the BaseTools part also is rough, but that is more a
question of "how" rather than "whether", so I'll postpone asking about it.

Thanks for your time and feedback!

Best regards,
Marvin


[1] "Once the image is loaded, LoadImage() installs
EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a
custom PE/COFF resource with the type 'HII'."
- UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"










Re: [RFC] Expose HII package list via C variables

Samer El-Haj-Mahmoud
 

I am aware of some proprietary tools (not open source) that depend on this functionality.

The feature has been used, especially on some server designs, to allow exporting HII packages and consuming them offline by out-of-band or OS-based management applications for instance. The need for this seems to be declining though, as implementations move to using standard Redfish data instead, which in turn can be produced from HII packages, either during boot (like what is done in EDK2 RedfishPkg), or at build / release time.

Thanks,
--Samer

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
Häuser via groups.io
Sent: Thursday, August 26, 2021 12:07 PM
To: tim.lewis@insyde.com; devel@edk2.groups.io;
michael.d.kinney@intel.com
Cc: 'Andrew Fish' <afish@apple.com>; leif@nuviainc.com; 'Ni, Ray'
<ray.ni@intel.com>; 'Gao, Zhichao' <zhichao.gao@intel.com>; 'Wang, Jian J'
<jian.j.wang@intel.com>; 'Wu, Hao A' <hao.a.wu@intel.com>; 'Bi, Dandan'
<dandan.bi@intel.com>; 'Dong, Eric' <eric.dong@intel.com>; 'Bret Barkelew'
<Bret.Barkelew@microsoft.com>; 'Vitaly Cheptsov'
<vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Hey Tim,

Interesting, do you happen to have some tool or code that performs such
edits at hand to take a look at? Seems like most modules already use
variables and thus cannot be modified in such a way even right now?

That's the kind of information I am looking for, thanks a lot!

Best regards,
Marvin

On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
Hi Marvin --

I would like to add some historical perspective on this. One of the design
requirements back when HII was first introduced into the UEFI specification
after Intel's initial contribution was that of binary editability. In order to be
able to reliably find, extract and then re-insert the HII data into the binary, it
needed to be discoverable and not affect the offsets of the code and data in
the binary.

While it was possible to put some sort of signature in the read-only data
sections of the binary and leave padding for possible future edits, it was felt
that using a PE/COFF section similar to the resource sections was better.
Resource sections are commonly in use for PE/COFF files (in Windows) and
the similar idea existed in the then-extant Mac binary format (resource
fork?).

Thanks,

Tim Lewis
CTO, Insyde Software
www.insyde.com

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Michael D Kinney
Sent: Thursday, August 26, 2021 7:37 AM
To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D
<michael.d.kinney@intel.com>
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray
<ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
<Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Marvin,

One constraint in this discussion is that the HII content in a PE/COFF
resource section is defined in the UEFI Specification, Which means UEFI Apps
and UEFI Drivers from media and option ROMs that are not part of the
system FW image are allowed to use this feature, This means the system FW
PE/COFF loader must support loading HII content from this PE/COFF resource
section to be UEFI conformant. So we cannot remove this feature from the
PE/COFF loader without changes to the UEFI Specification. Even if changes to
the UEFI Specification we made, we would have to continue to support this
feature for backward compatibility with existing UEFI Apps/Drivers that may
be using this feature.

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Marvin
Häuser
Sent: Thursday, August 26, 2021 1:51 AM
To: devel@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
Hao
A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
<eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
variables

Hey Mike,

Thanks for your reply!

Well, this switch is not well-documented. Looking now at both the
generation code and the emitted code, it does not generate a package
list like my code does, but separate data variables (strings and
images) that cannot easily be passed to HiiDatabase as-is. However
apparently there are drivers that use this functionality successfully
by composing the package list at runtime [1].

Looking with this information now at the pattern of using HII C
variables (which I did not know existed before) vs the PE/COFF HII
section, most that use latter are Shell applications, which I guess
means the section has actually been introduced to resolve D.? There
are exceptions such as LogoDxe [2], which use the PE/COFF section while
D.
is not a problem, hence I got confused, sorry. I think these modules
should be updated in any case. Do you agree?

So, for modules that use C variables already, my patch would save some
runtime generation code and dynamic memory allocation for the HII
package list. This was not my goal (as I said, I didn't realise HII C
variables already were a thing in the first place), but the changes
are small enough that it might be worth considering anyway, in my
opinion.
If a HII package list is generated for both Shell and non-Shell apps,
this also means code paths can be unified. For example, there could be
a library class with constructor and destructor to add/remove packages
from the HII database for all modules that use such, Shell or not. For
BaseTools it means that there is no real need for separate Python and
C paths as ideally they just generate the exact same data.

Now to D., the only usage for this seems to be that Shell can locate
the help text in the executable without executing it, yet it is fully
loaded anyway [3]. To be honest, I find it hard to justify loading an
executable (PE/COFF loading, memory permission application, the full
process) to retrieve a help text and then unloading it again,
especially with the HII code being on a core dispatcher level. 1. to
7. still hold true in my opinion. Was there any discussion I could
read through why Shell apps cannot simply support a "--help" or "-?"
command and output the string themselves? Pushing the burden to the
Shell apps does preserve the "drawback" that a full loading is
required (which honestly does not matter for a debugging application
like Shell), however it does relieve the burden of PE/COFF HII parsing
from the core dispatcher (which matters a lot in my opinion to keep
the core simple). It would simply be a normal Shell app execution as
any other however. If someone wants to avoid the PE/COFF burden
altogether, they can still provide a .man file.

As for my points 6. and 7., maybe I should provide some context. Due
to many issues with TE files, platforms started abandoning them and
returned to PE/COFF Images. I think a big reason for this is that TE
is not really a sound and complete format, but a stripped version of
PE/COFF with none of the necessary fixups applied. I'm currently
sketching a possible alternative [4], and I would really like to not
having to specify a HII section type, while still preserving
compatibility with all of the UEFI Image types and use-cases [4].

Thanks again!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
a
b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
p/BootManagerMenu.c#L929-L934

[2]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
a
b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0e
ab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
c#L646-L671

[4]
https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/Industr
ySt
andard/UeImage.h

26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:

Hi Marvin,

I think this feature is already there and supported.

HII can either be in a global variable or in a PE/COFF resource section.
The default is a global variable because HII was implemented before
the PE/COFF resource section feature was added to the UEFI
Specification.

There is an INF [Defines] section statement to enable the PE/COFF
section. See UefiHiiResource in the following link.

https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
i_inf_file_format/34_[defines]_section.html#34-
defines-section
How is your proposal different than this existing capability?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Marvin Häuser
Sent: Wednesday, August 25, 2021 2:21 PM
To: devel@edk2.groups.io
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi,
Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: [edk2-devel] [RFC] Expose HII package list via C variables

Good day everyone,

Currently, the HII package list is stored in a PE/COFF resource
section [1]. I propose to store it in a C variable (byte array with
a pointer to it and its size exposed) instead. DxeCore would have a
guard to toggle the deprecated support for the automatic protocol
installation. This has the following advantages:

1. Fixes BZ (incl. future toolchains):
https://bugzilla.tianocore.org/show_bug.cgi?id=557
2. Universal method across all toolchains and output file formats
3. Saves error-prone parsing work 4. Saves protocol install/locate
work, the data is available right away 5. The omission of a
dedicated section can save space 6. Terse file formats can support
this and remain terse :) 7. Removes a dependency on the PE/COFF
format specifically

A *very rough* PoC diff can be found here:
https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
If the feedback is positive, I will clean it up of course. OVMF
boots with everything working fine.

I'd explicitly like feedback on the following:
A. Is this an acceptable solution to BZ 557 (Andrew?)?
B. Is this an acceptable solution for the "HII workflow" (MdeModule
maintainers?)?
C. Is it acceptable to make support UEFI-side support for the old
mechanism optional (Stewards?)?
D. Can an acceptable alternative be found for the removed ShellPkg
code (Shell maintainers?)?

As you can see the BaseTools part also is rough, but that is more a
question of "how" rather than "whether", so I'll postpone asking about
it.

Thanks for your time and feedback!

Best regards,
Marvin


[1] "Once the image is loaded, LoadImage() installs
EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains
a
custom PE/COFF resource with the type 'HII'."
- UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"












IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


[edk2-platforms][PATCH v1] KabylakeOpenBoardPkg/AspireVn7Dash572G: Correct my name on copyrights

Benjamin Doron
 

Use my correct, legal name for copyright purposes.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/BoardAcpiTables=
.inf | 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/BoardSsdt.asl =
| 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/ac.asl =
| 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/battery.asl =
| 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/ec.asl =
| 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/mainboard.asl =
| 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/thermal.asl =
| 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/FspWrapper/Library/P=
eiSiliconPolicyUpdateLibFsp/PeiBoardPolicyUpdate.c | 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Include/Library/Boar=
dEcLib.h | 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEcLib/B=
oardEcLib.inf | 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEcLib/E=
cCommands.c | 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardInitLib=
/DxeBoardInitLib.c | 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardInitLib=
/DxeBoardInitLib.inf | 2 +-
Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Policy/Library/PeiSi=
liconPolicyUpdateLib/PeiBoardPolicyUpdate.c | 2 +-
14 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/Boa=
rdAcpiTables.inf b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Ac=
pi/BoardAcpiTables.inf
index 2bfad5c86403..806c0d2575c8 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/BoardAcpiT=
ables.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/BoardAcpiT=
ables.inf
@@ -1,7 +1,7 @@
## @file=0D
# Component description file for the Acer Aspire VN7-572G board ACPI tabl=
es=0D
#=0D
-# Copyright (c) 2021, Benjamin Doran=0D
+# Copyright (c) 2021, Baruch Binyamin Doron=0D
# SPDX-License-Identifier: BSD-2-Clause-Patent=0D
#=0D
##=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/Boa=
rdSsdt.asl b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/Boa=
rdSsdt.asl
index 77f869492989..cdec0434883e 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/BoardSsdt.=
asl
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/BoardSsdt.=
asl
@@ -1,7 +1,7 @@
/** @file=0D
This file contains the Aspire VN7-572G SSDT Table ASL code.=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/ac.=
asl b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/ac.asl
index 7253915d6cc1..126c5b53a470 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/ac.asl
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/ac.asl
@@ -1,6 +1,6 @@
/** @file=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/bat=
tery.asl b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/batte=
ry.asl
index 72dc036eb7aa..5ae4bdca89d5 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/battery.asl
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/battery.asl
@@ -1,6 +1,6 @@
/** @file=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/ec.=
asl b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/ec.asl
index bc1e612a92fc..df71dd69b491 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/ec.asl
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/ec.asl
@@ -1,6 +1,6 @@
/** @file=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/mai=
nboard.asl b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/mai=
nboard.asl
index 9febe7c385a8..7a73d37429d0 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/mainboard.=
asl
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/mainboard.=
asl
@@ -1,6 +1,6 @@
/** @file=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/the=
rmal.asl b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/therm=
al.asl
index f829c62020d2..805ee0700cd0 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/thermal.asl
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Acpi/thermal.asl
@@ -1,6 +1,6 @@
/** @file=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/FspWrapp=
er/Library/PeiSiliconPolicyUpdateLibFsp/PeiBoardPolicyUpdate.c b/Platform/I=
ntel/KabylakeOpenBoardPkg/AspireVn7Dash572G/FspWrapper/Library/PeiSiliconPo=
licyUpdateLibFsp/PeiBoardPolicyUpdate.c
index c8ab26eaf645..81cd8b940f05 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/FspWrapper/Libr=
ary/PeiSiliconPolicyUpdateLibFsp/PeiBoardPolicyUpdate.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/FspWrapper/Libr=
ary/PeiSiliconPolicyUpdateLibFsp/PeiBoardPolicyUpdate.c
@@ -1,7 +1,7 @@
/** @file=0D
This file configures Aspire VN7-572G board-specific FSP UPDs.=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Include/=
Library/BoardEcLib.h b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572=
G/Include/Library/BoardEcLib.h
index c99adc189745..2e7e0573900a 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Include/Library=
/BoardEcLib.h
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Include/Library=
/BoardEcLib.h
@@ -1,7 +1,7 @@
/** @file=0D
Board-specific EC library=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/=
BoardEcLib/BoardEcLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Da=
sh572G/Library/BoardEcLib/BoardEcLib.inf
index 01fee8916b36..56527c3b9a3c 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEc=
Lib/BoardEcLib.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEc=
Lib/BoardEcLib.inf
@@ -1,7 +1,7 @@
## @file=0D
# Component information file for Aspire VN7-572G EC library=0D
#=0D
-# Copyright (c) 2021, Benjamin Doran=0D
+# Copyright (c) 2021, Baruch Binyamin Doron=0D
# SPDX-License-Identifier: BSD-2-Clause-Patent=0D
#=0D
##=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/=
BoardEcLib/EcCommands.c b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash=
572G/Library/BoardEcLib/EcCommands.c
index 7650a6b21575..09b2b5ee9180 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEc=
Lib/EcCommands.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEc=
Lib/EcCommands.c
@@ -1,7 +1,7 @@
/** @file=0D
Board-specific EC commands.=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/=
BoardInitLib/DxeBoardInitLib.c b/Platform/Intel/KabylakeOpenBoardPkg/Aspire=
Vn7Dash572G/Library/BoardInitLib/DxeBoardInitLib.c
index 1281391ed36f..906b2d265092 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardIn=
itLib/DxeBoardInitLib.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardIn=
itLib/DxeBoardInitLib.c
@@ -1,7 +1,7 @@
/** @file=0D
Aspire VN7-572G Board Initialization DXE library=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/=
BoardInitLib/DxeBoardInitLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/Aspi=
reVn7Dash572G/Library/BoardInitLib/DxeBoardInitLib.inf
index 000ec8a1b56d..9a868ee15fb2 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardIn=
itLib/DxeBoardInitLib.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardIn=
itLib/DxeBoardInitLib.inf
@@ -1,7 +1,7 @@
## @file=0D
# Component information file for AspireVn7Dash572GInitLib in DXE phase.=0D
#=0D
-# Copyright (c) 2021, Benjamin Doran=0D
+# Copyright (c) 2021, Baruch Binyamin Doron=0D
# SPDX-License-Identifier: BSD-2-Clause-Patent=0D
#=0D
##=0D
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Policy/L=
ibrary/PeiSiliconPolicyUpdateLib/PeiBoardPolicyUpdate.c b/Platform/Intel/Ka=
bylakeOpenBoardPkg/AspireVn7Dash572G/Policy/Library/PeiSiliconPolicyUpdateL=
ib/PeiBoardPolicyUpdate.c
index b72bc56f1d0d..95b7c4ad5f77 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Policy/Library/=
PeiSiliconPolicyUpdateLib/PeiBoardPolicyUpdate.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Policy/Library/=
PeiSiliconPolicyUpdateLib/PeiBoardPolicyUpdate.c
@@ -1,7 +1,7 @@
/** @file=0D
This file configures Aspire VN7-572G board-specific policies.=0D
=0D
- Copyright (c) 2021, Benjamin Doran=0D
+ Copyright (c) 2021, Baruch Binyamin Doron=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
--=20
2.31.1


Re: [PATCH v2 3/3] RFC: OvmfPkg/PlatformPei: stop using cmos for memory detection

Yao, Jiewen
 

Good idea, Ard.

ASSERT_EFI_ERROR (Status);
ASSERT (LowerMemorySize > 0);
I like this suggestion.


Thank you
Yao Jiewen


-----Original Message-----
From: Ard Biesheuvel <ardb@kernel.org>
Sent: Thursday, August 26, 2021 7:15 PM
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Yao, Jiewen <jiewen.yao@intel.com>; Justen,
Jordan L <jordan.l.justen@intel.com>; Philippe Mathieu-Daude
<philmd@redhat.com>
Subject: Re: [PATCH v2 3/3] RFC: OvmfPkg/PlatformPei: stop using cmos for
memory detection

On Thu, 26 Aug 2021 at 11:55, Gerd Hoffmann <kraxel@redhat.com> wrote:

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3593

Not needed for qemu 1.7 (released in 2013) and newer.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
OvmfPkg/PlatformPei/MemDetect.c | 58 ++-------------------------------
1 file changed, 3 insertions(+), 55 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c
b/OvmfPkg/PlatformPei/MemDetect.c
index 20154255324b..8d2c7f432472 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -37,7 +37,6 @@ Module Name:
#include <Library/QemuFwCfgSimpleParserLib.h>

#include "Platform.h"
-#include "Cmos.h"

UINT8 mPhysMemAddressWidth;

@@ -295,52 +294,11 @@ GetSystemMemorySizeBelow4gb (
{
EFI_STATUS Status;
UINT64 LowerMemorySize = 0;
- UINT8 Cmos0x34;
- UINT8 Cmos0x35;

Status = ScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
- if (Status == EFI_SUCCESS && LowerMemorySize > 0) {
- return LowerMemorySize;
- }
+ ASSERT (Status == EFI_SUCCESS && LowerMemorySize > 0);
Nit: better to do

ASSERT_EFI_ERROR (Status);
ASSERT (LowerMemorySize > 0);

so that you can tell from the debug log which condition triggered, and
if it is the first one, what the actual error code was.


+ return LowerMemorySize;

- //
- // CMOS 0x34/0x35 specifies the system memory above 16 MB.
- // * CMOS(0x35) is the high byte
- // * CMOS(0x34) is the low byte
- // * The size is specified in 64kb chunks
- // * Since this is memory above 16MB, the 16MB must be added
- // into the calculation to get the total memory size.
- //
-
- Cmos0x34 = (UINT8) CmosRead8 (0x34);
- Cmos0x35 = (UINT8) CmosRead8 (0x35);
-
- return (UINT32) (((UINTN)((Cmos0x35 << 8) + Cmos0x34) << 16) +
SIZE_16MB);
-}
-
-
-STATIC
-UINT64
-GetSystemMemorySizeAbove4gb (
- )
-{
- UINT32 Size;
- UINTN CmosIndex;
-
- //
- // CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
- // * CMOS(0x5d) is the most significant size byte
- // * CMOS(0x5c) is the middle size byte
- // * CMOS(0x5b) is the least significant size byte
- // * The size is specified in 64kb chunks
- //
-
- Size = 0;
- for (CmosIndex = 0x5d; CmosIndex >= 0x5b; CmosIndex--) {
- Size = (UINT32) (Size << 8) + (UINT32) CmosRead8 (CmosIndex);
- }
-
- return LShiftU64 (Size, 16);
}


@@ -371,12 +329,9 @@ GetFirstNonAddress (
// If QEMU presents an E820 map, then get the highest exclusive >=4GB RAM
// address from it. This can express an address >= 4GB+1TB.
//
- // Otherwise, get the flat size of the memory above 4GB from the CMOS
(which
- // can only express a size smaller than 1TB), and add it to 4GB.
- //
Status = ScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
if (EFI_ERROR (Status)) {
- FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
+ FirstNonAddress = BASE_4GB;
}

//
@@ -719,7 +674,6 @@ QemuInitializeRam (
)
{
UINT64 LowerMemorySize;
- UINT64 UpperMemorySize;
MTRR_SETTINGS MtrrSettings;
EFI_STATUS Status;

@@ -775,12 +729,6 @@ QemuInitializeRam (
// memory size read from the CMOS.
//
Status = ScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
- if (EFI_ERROR (Status)) {
- UpperMemorySize = GetSystemMemorySizeAbove4gb ();
- if (UpperMemorySize != 0) {
- AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
- }
- }
}

//
--
2.31.1


Re: [RFC] Expose HII package list via C variables

Marvin Häuser
 

Hey Tim,

Interesting, do you happen to have some tool or code that performs such edits at hand to take a look at? Seems like most modules already use variables and thus cannot be modified in such a way even right now?

That's the kind of information I am looking for, thanks a lot!

Best regards,
Marvin

On 26/08/2021 18:01, tim.lewis@insyde.com wrote:
Hi Marvin --

I would like to add some historical perspective on this. One of the design requirements back when HII was first introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not affect the offsets of the code and data in the binary.

While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary format (resource fork?).

Thanks,

Tim Lewis
CTO, Insyde Software
www.insyde.com

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Thursday, August 26, 2021 7:37 AM
To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Marvin,

One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image are allowed to use this feature, This means the system FW PE/COFF loader must support loading HII content from this PE/COFF resource section to be UEFI conformant. So we cannot remove this feature from the PE/COFF loader without changes to the UEFI Specification. Even if changes to the UEFI Specification we made, we would have to continue to support this feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
Häuser
Sent: Thursday, August 26, 2021 1:51 AM
To: devel@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
<eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
variables

Hey Mike,

Thanks for your reply!

Well, this switch is not well-documented. Looking now at both the
generation code and the emitted code, it does not generate a package
list like my code does, but separate data variables (strings and
images) that cannot easily be passed to HiiDatabase as-is. However
apparently there are drivers that use this functionality successfully
by composing the package list at runtime [1].

Looking with this information now at the pattern of using HII C
variables (which I did not know existed before) vs the PE/COFF HII
section, most that use latter are Shell applications, which I guess
means the section has actually been introduced to resolve D.? There
are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
is not a problem, hence I got confused, sorry. I think these modules
should be updated in any case. Do you agree?

So, for modules that use C variables already, my patch would save some
runtime generation code and dynamic memory allocation for the HII
package list. This was not my goal (as I said, I didn't realise HII C
variables already were a thing in the first place), but the changes
are small enough that it might be worth considering anyway, in my opinion.
If a HII package list is generated for both Shell and non-Shell apps,
this also means code paths can be unified. For example, there could be
a library class with constructor and destructor to add/remove packages
from the HII database for all modules that use such, Shell or not. For
BaseTools it means that there is no real need for separate Python and
C paths as ideally they just generate the exact same data.

Now to D., the only usage for this seems to be that Shell can locate
the help text in the executable without executing it, yet it is fully
loaded anyway [3]. To be honest, I find it hard to justify loading an
executable (PE/COFF loading, memory permission application, the full
process) to retrieve a help text and then unloading it again,
especially with the HII code being on a core dispatcher level. 1. to
7. still hold true in my opinion. Was there any discussion I could
read through why Shell apps cannot simply support a "--help" or "-?"
command and output the string themselves? Pushing the burden to the
Shell apps does preserve the "drawback" that a full loading is
required (which honestly does not matter for a debugging application
like Shell), however it does relieve the burden of PE/COFF HII parsing
from the core dispatcher (which matters a lot in my opinion to keep
the core simple). It would simply be a normal Shell app execution as
any other however. If someone wants to avoid the PE/COFF burden
altogether, they can still provide a .man file.

As for my points 6. and 7., maybe I should provide some context. Due
to many issues with TE files, platforms started abandoning them and
returned to PE/COFF Images. I think a big reason for this is that TE
is not really a sound and complete format, but a stripped version of
PE/COFF with none of the necessary fixups applied. I'm currently
sketching a possible alternative [4], and I would really like to not
having to specify a HII section type, while still preserving
compatibility with all of the UEFI Image types and use-cases [4].

Thanks again!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
p/BootManagerMenu.c#L929-L934

[2]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
c#L646-L671

[4]
https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustrySt
andard/UeImage.h

26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:

Hi Marvin,

I think this feature is already there and supported.

HII can either be in a global variable or in a PE/COFF resource section.
The default is a global variable because HII was implemented before
the PE/COFF resource section feature was added to the UEFI Specification.

There is an INF [Defines] section statement to enable the PE/COFF
section. See UefiHiiResource in the following link.

https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
i_inf_file_format/34_[defines]_section.html#34-
defines-section
How is your proposal different than this existing capability?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Marvin Häuser
Sent: Wednesday, August 25, 2021 2:21 PM
To: devel@edk2.groups.io
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: [edk2-devel] [RFC] Expose HII package list via C variables

Good day everyone,

Currently, the HII package list is stored in a PE/COFF resource
section [1]. I propose to store it in a C variable (byte array with
a pointer to it and its size exposed) instead. DxeCore would have a
guard to toggle the deprecated support for the automatic protocol
installation. This has the following advantages:

1. Fixes BZ (incl. future toolchains):
https://bugzilla.tianocore.org/show_bug.cgi?id=557
2. Universal method across all toolchains and output file formats
3. Saves error-prone parsing work 4. Saves protocol install/locate
work, the data is available right away 5. The omission of a
dedicated section can save space 6. Terse file formats can support
this and remain terse :) 7. Removes a dependency on the PE/COFF
format specifically

A *very rough* PoC diff can be found here:
https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
If the feedback is positive, I will clean it up of course. OVMF
boots with everything working fine.

I'd explicitly like feedback on the following:
A. Is this an acceptable solution to BZ 557 (Andrew?)?
B. Is this an acceptable solution for the "HII workflow" (MdeModule
maintainers?)?
C. Is it acceptable to make support UEFI-side support for the old
mechanism optional (Stewards?)?
D. Can an acceptable alternative be found for the removed ShellPkg
code (Shell maintainers?)?

As you can see the BaseTools part also is rough, but that is more a
question of "how" rather than "whether", so I'll postpone asking about it.

Thanks for your time and feedback!

Best regards,
Marvin


[1] "Once the image is loaded, LoadImage() installs
EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a
custom PE/COFF resource with the type 'HII'."
- UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"









Re: [RFC] Expose HII package list via C variables

Tim Lewis
 

Hi Marvin --

I would like to add some historical perspective on this. One of the design requirements back when HII was first introduced into the UEFI specification after Intel's initial contribution was that of binary editability. In order to be able to reliably find, extract and then re-insert the HII data into the binary, it needed to be discoverable and not affect the offsets of the code and data in the binary.

While it was possible to put some sort of signature in the read-only data sections of the binary and leave padding for possible future edits, it was felt that using a PE/COFF section similar to the resource sections was better. Resource sections are commonly in use for PE/COFF files (in Windows) and the similar idea existed in the then-extant Mac binary format (resource fork?).

Thanks,

Tim Lewis
CTO, Insyde Software
www.insyde.com

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Thursday, August 26, 2021 7:37 AM
To: devel@edk2.groups.io; mhaeuser@posteo.de; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Marvin,

One constraint in this discussion is that the HII content in a PE/COFF resource section is defined in the UEFI Specification, Which means UEFI Apps and UEFI Drivers from media and option ROMs that are not part of the system FW image are allowed to use this feature, This means the system FW PE/COFF loader must support loading HII content from this PE/COFF resource section to be UEFI conformant. So we cannot remove this feature from the PE/COFF loader without changes to the UEFI Specification. Even if changes to the UEFI Specification we made, we would have to continue to support this feature for backward compatibility with existing UEFI Apps/Drivers that may be using this feature.

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
Häuser
Sent: Thursday, August 26, 2021 1:51 AM
To: devel@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric
<eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C
variables

Hey Mike,

Thanks for your reply!

Well, this switch is not well-documented. Looking now at both the
generation code and the emitted code, it does not generate a package
list like my code does, but separate data variables (strings and
images) that cannot easily be passed to HiiDatabase as-is. However
apparently there are drivers that use this functionality successfully
by composing the package list at runtime [1].

Looking with this information now at the pattern of using HII C
variables (which I did not know existed before) vs the PE/COFF HII
section, most that use latter are Shell applications, which I guess
means the section has actually been introduced to resolve D.? There
are exceptions such as LogoDxe [2], which use the PE/COFF section while D.
is not a problem, hence I got confused, sorry. I think these modules
should be updated in any case. Do you agree?

So, for modules that use C variables already, my patch would save some
runtime generation code and dynamic memory allocation for the HII
package list. This was not my goal (as I said, I didn't realise HII C
variables already were a thing in the first place), but the changes
are small enough that it might be worth considering anyway, in my opinion.
If a HII package list is generated for both Shell and non-Shell apps,
this also means code paths can be unified. For example, there could be
a library class with constructor and destructor to add/remove packages
from the HII database for all modules that use such, Shell or not. For
BaseTools it means that there is no real need for separate Python and
C paths as ideally they just generate the exact same data.

Now to D., the only usage for this seems to be that Shell can locate
the help text in the executable without executing it, yet it is fully
loaded anyway [3]. To be honest, I find it hard to justify loading an
executable (PE/COFF loading, memory permission application, the full
process) to retrieve a help text and then unloading it again,
especially with the HII code being on a core dispatcher level. 1. to
7. still hold true in my opinion. Was there any discussion I could
read through why Shell apps cannot simply support a "--help" or "-?"
command and output the string themselves? Pushing the burden to the
Shell apps does preserve the "drawback" that a full loading is
required (which honestly does not matter for a debugging application
like Shell), however it does relieve the burden of PE/COFF HII parsing
from the core dispatcher (which matters a lot in my opinion to keep
the core simple). It would simply be a normal Shell app execution as
any other however. If someone wants to avoid the PE/COFF burden
altogether, they can still provide a .man file.

As for my points 6. and 7., maybe I should provide some context. Due
to many issues with TE files, platforms started abandoning them and
returned to PE/COFF Images. I think a big reason for this is that TE
is not really a sound and complete format, but a stripped version of
PE/COFF with none of the necessary fixups applied. I'm currently
sketching a possible alternative [4], and I would really like to not
having to specify a HII section type, while still preserving
compatibility with all of the UEFI Image types and use-cases [4].

Thanks again!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
b4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
p/BootManagerMenu.c#L929-L934

[2]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0ea
b4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
c#L646-L671

[4]
https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustrySt
andard/UeImage.h

26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:

Hi Marvin,

I think this feature is already there and supported.

HII can either be in a global variable or in a PE/COFF resource section.
The default is a global variable because HII was implemented before
the PE/COFF resource section feature was added to the UEFI Specification.

There is an INF [Defines] section statement to enable the PE/COFF
section. See UefiHiiResource in the following link.

https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_i
i_inf_file_format/34_[defines]_section.html#34-
defines-section

How is your proposal different than this existing capability?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Marvin Häuser
Sent: Wednesday, August 25, 2021 2:21 PM
To: devel@edk2.groups.io
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney,
Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret
Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: [edk2-devel] [RFC] Expose HII package list via C variables

Good day everyone,

Currently, the HII package list is stored in a PE/COFF resource
section [1]. I propose to store it in a C variable (byte array with
a pointer to it and its size exposed) instead. DxeCore would have a
guard to toggle the deprecated support for the automatic protocol
installation. This has the following advantages:

1. Fixes BZ (incl. future toolchains):
https://bugzilla.tianocore.org/show_bug.cgi?id=557
2. Universal method across all toolchains and output file formats
3. Saves error-prone parsing work 4. Saves protocol install/locate
work, the data is available right away 5. The omission of a
dedicated section can save space 6. Terse file formats can support
this and remain terse :) 7. Removes a dependency on the PE/COFF
format specifically

A *very rough* PoC diff can be found here:
https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
If the feedback is positive, I will clean it up of course. OVMF
boots with everything working fine.

I'd explicitly like feedback on the following:
A. Is this an acceptable solution to BZ 557 (Andrew?)?
B. Is this an acceptable solution for the "HII workflow" (MdeModule
maintainers?)?
C. Is it acceptable to make support UEFI-side support for the old
mechanism optional (Stewards?)?
D. Can an acceptable alternative be found for the removed ShellPkg
code (Shell maintainers?)?

As you can see the BaseTools part also is rough, but that is more a
question of "how" rather than "whether", so I'll postpone asking about it.

Thanks for your time and feedback!

Best regards,
Marvin


[1] "Once the image is loaded, LoadImage() installs
EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if the image contains a
custom PE/COFF resource with the type 'HII'."
- UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"








Re: [edk2-platforms] [PATCH V1] KabylakeOpenBoardPkg: Document EcLib return value

Michael Kubacki
 

Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 8/25/2021 11:55 PM, Nate DeSimone wrote:
Added EFI_INVALID_PARAMETER to the EcRead()
function's list of return values.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Cc: Benjamin Doron <benjamin.doron00@gmail.com>
Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
---
.../Intel/KabylakeOpenBoardPkg/Include/Library/EcLib.h | 9 +++++----
.../KabylakeOpenBoardPkg/Library/BaseEcLib/EcCommands.c | 9 +++++----
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Include/Library/EcLib.h b/Platform/Intel/KabylakeOpenBoardPkg/Include/Library/EcLib.h
index 7c58e592d9..e95accc465 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Include/Library/EcLib.h
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Include/Library/EcLib.h
@@ -7,7 +7,7 @@
Make sure you meet the requirements for the library (protocol dependencies, use
restrictions, etc).
-Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2019 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -109,9 +109,10 @@ LpcEcInterface (
@param[in] Address Address to read
@param[out] Data Data received
- @retval EFI_SUCCESS Command success
- @retval EFI_DEVICE_ERROR Command error
- @retval EFI_TIMEOUT Command timeout
+ @retval EFI_SUCCESS Command success
+ @retval EFI_INVALID_PARAMETER Data is NULL
+ @retval EFI_DEVICE_ERROR Command error
+ @retval EFI_TIMEOUT Command timeout
**/
EFI_STATUS
EcRead (
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Library/BaseEcLib/EcCommands.c b/Platform/Intel/KabylakeOpenBoardPkg/Library/BaseEcLib/EcCommands.c
index d14edb75de..14a746172b 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Library/BaseEcLib/EcCommands.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Library/BaseEcLib/EcCommands.c
@@ -1,7 +1,7 @@
/** @file
Common EC commands.
-Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2019 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -16,9 +16,10 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
@param[in] Address Address to read
@param[out] Data Data received
- @retval EFI_SUCCESS Command success
- @retval EFI_DEVICE_ERROR Command error
- @retval EFI_TIMEOUT Command timeout
+ @retval EFI_SUCCESS Command success
+ @retval EFI_INVALID_PARAMETER Data is NULL
+ @retval EFI_DEVICE_ERROR Command error
+ @retval EFI_TIMEOUT Command timeout
**/
EFI_STATUS
EcRead (


Re: [RFC] Expose HII package list via C variables

Marvin Häuser
 

Hey Mike,

Yes, I am aware. My goal for that would be to declare support as optional to support drivers up until UEFI vX (whatever would be the spec to endorse drivers to move away from the PE/COFF section model), much like EFI 1.10 backwards-compatibility worked with UEFI 2.x, and have some enabled-by-default FixedPcd for HII PE/COFF support. Project Amaranth at ISP RAS for example would disable support for this right away, as there is no urgent dependency on any external software to backwards-support. Production consumer platforms would drop support eventually in the far future, just like with all the EFI 1.10 specifics. Possibly a DEBUG_WARN could be issued whenever such a section is encountered, to help identify modules relying on this model.

Considering the nature of the usage right now, i.e. Shell manual queries, I sure hope there aren't actually any dependencies on this. The new PE/COFF loader is yet to undergo extensive real-world platform testing on full-UEFI scale (it is already used to load OS loaders on a wider scale, but not e.g. Option ROMs), and so are the UEFI spec changes that are to be proposed for hardening the parser, so I hope this can just be squeezed into both. If you can give a sort of general vote whether this change can be endorsed or not, ignoring any real-world dependencies for the moment, I will clean up the changes, integrate them into any upcoming PE/COFF loader testing, and provide findings whenever they arrive.

Best regards,
Marvin

On 26/08/2021 16:37, Michael D Kinney wrote:
Marvin,

One constraint in this discussion is that the HII content in
a PE/COFF resource section is defined in the UEFI Specification,
Which means UEFI Apps and UEFI Drivers from media and option ROMs
that are not part of the system FW image are allowed to use this
feature, This means the system FW PE/COFF loader must support
loading HII content from this PE/COFF resource section to be UEFI
conformant. So we cannot remove this feature from the PE/COFF
loader without changes to the UEFI Specification. Even if
changes to the UEFI Specification we made, we would have to
continue to support this feature for backward compatibility
with existing UEFI Apps/Drivers that may be using this
feature.

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
Sent: Thursday, August 26, 2021 1:51 AM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Hey Mike,

Thanks for your reply!

Well, this switch is not well-documented. Looking now at both the
generation code and the emitted code, it does not generate a package
list like my code does, but separate data variables (strings and images)
that cannot easily be passed to HiiDatabase as-is. However apparently
there are drivers that use this functionality successfully by composing
the package list at runtime [1].

Looking with this information now at the pattern of using HII C
variables (which I did not know existed before) vs the PE/COFF HII
section, most that use latter are Shell applications, which I guess
means the section has actually been introduced to resolve D.? There are
exceptions such as LogoDxe [2], which use the PE/COFF section while D.
is not a problem, hence I got confused, sorry. I think these modules
should be updated in any case. Do you agree?

So, for modules that use C variables already, my patch would save some
runtime generation code and dynamic memory allocation for the HII
package list. This was not my goal (as I said, I didn't realise HII C
variables already were a thing in the first place), but the changes are
small enough that it might be worth considering anyway, in my opinion.
If a HII package list is generated for both Shell and non-Shell apps,
this also means code paths can be unified. For example, there could be a
library class with constructor and destructor to add/remove packages
from the HII database for all modules that use such, Shell or not. For
BaseTools it means that there is no real need for separate Python and C
paths as ideally they just generate the exact same data.

Now to D., the only usage for this seems to be that Shell can locate the
help text in the executable without executing it, yet it is fully loaded
anyway [3]. To be honest, I find it hard to justify loading an
executable (PE/COFF loading, memory permission application, the full
process) to retrieve a help text and then unloading it again, especially
with the HII code being on a core dispatcher level. 1. to 7. still hold
true in my opinion. Was there any discussion I could read through why
Shell apps cannot simply support a "--help" or "-?" command and output
the string themselves? Pushing the burden to the Shell apps does
preserve the "drawback" that a full loading is required (which honestly
does not matter for a debugging application like Shell), however it does
relieve the burden of PE/COFF HII parsing from the core dispatcher
(which matters a lot in my opinion to keep the core simple). It would
simply be a normal Shell app execution as any other however. If someone
wants to avoid the PE/COFF burden altogether, they can still provide a
.man file.

As for my points 6. and 7., maybe I should provide some context. Due to
many issues with TE files, platforms started abandoning them and
returned to PE/COFF Images. I think a big reason for this is that TE is
not really a sound and complete format, but a stripped version of
PE/COFF with none of the necessary fixups applied. I'm currently
sketching a possible alternative [4], and I would really like to not
having to specify a HII section type, while still preserving
compatibility with all of the UEFI Image types and use-cases [4].

Thanks again!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
p/BootManagerMenu.c#L929-L934

[2]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
c#L646-L671

[4]
https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustryStandard/UeImage.h

26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:

Hi Marvin,

I think this feature is already there and supported.

HII can either be in a global variable or in a PE/COFF resource section.
The default is a global variable because HII was implemented before the
PE/COFF resource section feature was added to the UEFI Specification.

There is an INF [Defines] section statement to enable the PE/COFF
section. See UefiHiiResource in the following link.

https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_ii_inf_file_format/34_[defines]_section.html#34-
defines-section
How is your proposal different than this existing capability?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
Sent: Wednesday, August 25, 2021 2:21 PM
To: devel@edk2.groups.io
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
<ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
<hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
<Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: [edk2-devel] [RFC] Expose HII package list via C variables

Good day everyone,

Currently, the HII package list is stored in a PE/COFF resource section
[1]. I propose to store it in a C variable (byte array with a pointer to
it and its size exposed) instead. DxeCore would have a guard to toggle
the deprecated support for the automatic protocol installation. This has
the following advantages:

1. Fixes BZ (incl. future toolchains):
https://bugzilla.tianocore.org/show_bug.cgi?id=557
2. Universal method across all toolchains and output file formats
3. Saves error-prone parsing work
4. Saves protocol install/locate work, the data is available right away
5. The omission of a dedicated section can save space
6. Terse file formats can support this and remain terse :)
7. Removes a dependency on the PE/COFF format specifically

A *very rough* PoC diff can be found here:
https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
If the feedback is positive, I will clean it up of course. OVMF boots
with everything working fine.

I'd explicitly like feedback on the following:
A. Is this an acceptable solution to BZ 557 (Andrew?)?
B. Is this an acceptable solution for the "HII workflow" (MdeModule
maintainers?)?
C. Is it acceptable to make support UEFI-side support for the old
mechanism optional (Stewards?)?
D. Can an acceptable alternative be found for the removed ShellPkg code
(Shell maintainers?)?

As you can see the BaseTools part also is rough, but that is more a
question of "how" rather than "whether", so I'll postpone asking about it.

Thanks for your time and feedback!

Best regards,
Marvin


[1] "Once the image is loaded, LoadImage() installs
EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if
the image contains a custom PE/COFF resource with the type 'HII'."
- UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"








Re: [RFC] Expose HII package list via C variables

Michael D Kinney
 

Marvin,

One constraint in this discussion is that the HII content in
a PE/COFF resource section is defined in the UEFI Specification,
Which means UEFI Apps and UEFI Drivers from media and option ROMs
that are not part of the system FW image are allowed to use this
feature, This means the system FW PE/COFF loader must support
loading HII content from this PE/COFF resource section to be UEFI
conformant. So we cannot remove this feature from the PE/COFF
loader without changes to the UEFI Specification. Even if
changes to the UEFI Specification we made, we would have to
continue to support this feature for backward compatibility
with existing UEFI Apps/Drivers that may be using this
feature.

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
Sent: Thursday, August 26, 2021 1:51 AM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; leif@nuviainc.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
<dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
<vit9696@protonmail.com>
Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables

Hey Mike,

Thanks for your reply!

Well, this switch is not well-documented. Looking now at both the
generation code and the emitted code, it does not generate a package
list like my code does, but separate data variables (strings and images)
that cannot easily be passed to HiiDatabase as-is. However apparently
there are drivers that use this functionality successfully by composing
the package list at runtime [1].

Looking with this information now at the pattern of using HII C
variables (which I did not know existed before) vs the PE/COFF HII
section, most that use latter are Shell applications, which I guess
means the section has actually been introduced to resolve D.? There are
exceptions such as LogoDxe [2], which use the PE/COFF section while D.
is not a problem, hence I got confused, sorry. I think these modules
should be updated in any case. Do you agree?

So, for modules that use C variables already, my patch would save some
runtime generation code and dynamic memory allocation for the HII
package list. This was not my goal (as I said, I didn't realise HII C
variables already were a thing in the first place), but the changes are
small enough that it might be worth considering anyway, in my opinion.
If a HII package list is generated for both Shell and non-Shell apps,
this also means code paths can be unified. For example, there could be a
library class with constructor and destructor to add/remove packages
from the HII database for all modules that use such, Shell or not. For
BaseTools it means that there is no real need for separate Python and C
paths as ideally they just generate the exact same data.

Now to D., the only usage for this seems to be that Shell can locate the
help text in the executable without executing it, yet it is fully loaded
anyway [3]. To be honest, I find it hard to justify loading an
executable (PE/COFF loading, memory permission application, the full
process) to retrieve a help text and then unloading it again, especially
with the HII code being on a core dispatcher level. 1. to 7. still hold
true in my opinion. Was there any discussion I could read through why
Shell apps cannot simply support a "--help" or "-?" command and output
the string themselves? Pushing the burden to the Shell apps does
preserve the "drawback" that a full loading is required (which honestly
does not matter for a debugging application like Shell), however it does
relieve the burden of PE/COFF HII parsing from the core dispatcher
(which matters a lot in my opinion to keep the core simple). It would
simply be a normal Shell app execution as any other however. If someone
wants to avoid the PE/COFF burden altogether, they can still provide a
.man file.

As for my points 6. and 7., maybe I should provide some context. Due to
many issues with TE files, platforms started abandoning them and
returned to PE/COFF Images. I think a big reason for this is that TE is
not really a sound and complete format, but a stripped version of
PE/COFF with none of the necessary fixups applied. I'm currently
sketching a possible alternative [4], and I would really like to not
having to specify a HII section type, while still preserving
compatibility with all of the UEFI Image types and use-cases [4].

Thanks again!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Application/BootManagerMenuAp
p/BootManagerMenu.c#L929-L934

[2]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Logo/LogoDxe.inf#L23

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/ShellPkg/Application/Shell/ShellManParser.
c#L646-L671

[4]
https://github.com/mhaeuser/edk2/blob/ue_poc/MdePkg/Include/IndustryStandard/UeImage.h

26.08.2021 00:34:12 Michael D Kinney <michael.d.kinney@intel.com>:

Hi Marvin,

I think this feature is already there and supported.

HII can either be in a global variable or in a PE/COFF resource section.
The default is a global variable because HII was implemented before the
PE/COFF resource section feature was added to the UEFI Specification.

There is an INF [Defines] section statement to enable the PE/COFF
section. See UefiHiiResource in the following link.

https://tianocore-docs.github.io/edk2-InfSpecification/draft/3_edk_ii_inf_file_format/34_[defines]_section.html#34-
defines-section

How is your proposal different than this existing capability?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
Sent: Wednesday, August 25, 2021 2:21 PM
To: devel@edk2.groups.io
Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
<ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
<hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew
<Bret.Barkelew@microsoft.com>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: [edk2-devel] [RFC] Expose HII package list via C variables

Good day everyone,

Currently, the HII package list is stored in a PE/COFF resource section
[1]. I propose to store it in a C variable (byte array with a pointer to
it and its size exposed) instead. DxeCore would have a guard to toggle
the deprecated support for the automatic protocol installation. This has
the following advantages:

1. Fixes BZ (incl. future toolchains):
https://bugzilla.tianocore.org/show_bug.cgi?id=557
2. Universal method across all toolchains and output file formats
3. Saves error-prone parsing work
4. Saves protocol install/locate work, the data is available right away
5. The omission of a dedicated section can save space
6. Terse file formats can support this and remain terse :)
7. Removes a dependency on the PE/COFF format specifically

A *very rough* PoC diff can be found here:
https://github.com/mhaeuser/edk2/compare/master...wip_hii_cvar
If the feedback is positive, I will clean it up of course. OVMF boots
with everything working fine.

I'd explicitly like feedback on the following:
A. Is this an acceptable solution to BZ 557 (Andrew?)?
B. Is this an acceptable solution for the "HII workflow" (MdeModule
maintainers?)?
C. Is it acceptable to make support UEFI-side support for the old
mechanism optional (Stewards?)?
D. Can an acceptable alternative be found for the removed ShellPkg code
(Shell maintainers?)?

As you can see the BaseTools part also is rough, but that is more a
question of "how" rather than "whether", so I'll postpone asking about it.

Thanks for your time and feedback!

Best regards,
Marvin


[1] "Once the image is loaded, LoadImage() installs
EFI_HII_PACKAGE_LIST_PROTOCOL on the handle if
the image contains a custom PE/COFF resource with the type 'HII'."
- UEFI 2.9, 7.4, "EFI_BOOT_SERVICES.LoadImage()"








[edk2-platforms PATCH v3 1/1] Maintainers.txt: Add maintainer of Ext4Pkg

Pedro Falcato
 

Add myself as a maintainter for Features/Ext4Pkg.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
Maintainers.txt | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index 9b8d6aead923..e5ddaa8d2284 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -141,6 +141,10 @@ M: Leif Lindholm <leif@nuviainc.com>
R: Ard Biesheuvel <ardb+tianocore@kernel.org>=0D
R: Wenyi Xie <xiewenyi2@huawei.com>=0D
=0D
+Features/Ext4Pkg=0D
+F: Features/Ext4Pkg/=0D
+M: Pedro Falcato <pedro.falcato@gmail.com>=0D
+=0D
Features/Intel=0D
F: Features/Intel/=0D
M: Sai Chaganty <rangasai.v.chaganty@intel.com>=0D
--=20
2.33.0


Re: [edk2-platforms PATCH v2 1/1] Maintainers.txt: Add maintainer of Ext4Pkg

Leif Lindholm
 

On Thu, Aug 26, 2021 at 00:08:50 +0100, Pedro Falcato wrote:
Add myself as a maintainter for Features/Ext4Pkg.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
Maintainers.txt | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index 9b8d6aead923..56b167447eb7 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -141,6 +141,10 @@ M: Leif Lindholm <leif@nuviainc.com>
R: Ard Biesheuvel <ardb+tianocore@kernel.org>
R: Wenyi Xie <xiewenyi2@huawei.com>

+Features/Ext4Pkg
+F: Features/Ext4Pkg
Missing a trailing /

+M: Pedro Falcato <pedro.falcato@gmail.com>
+
Features/Intel
F: Features/Intel/
M: Sai Chaganty <rangasai.v.chaganty@intel.com>
--
2.33.0


Re: [PATCH v2 3/3] RFC: OvmfPkg/PlatformPei: stop using cmos for memory detection

Ard Biesheuvel
 

On Thu, 26 Aug 2021 at 11:55, Gerd Hoffmann <kraxel@redhat.com> wrote:

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3593

Not needed for qemu 1.7 (released in 2013) and newer.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
OvmfPkg/PlatformPei/MemDetect.c | 58 ++-------------------------------
1 file changed, 3 insertions(+), 55 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 20154255324b..8d2c7f432472 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -37,7 +37,6 @@ Module Name:
#include <Library/QemuFwCfgSimpleParserLib.h>

#include "Platform.h"
-#include "Cmos.h"

UINT8 mPhysMemAddressWidth;

@@ -295,52 +294,11 @@ GetSystemMemorySizeBelow4gb (
{
EFI_STATUS Status;
UINT64 LowerMemorySize = 0;
- UINT8 Cmos0x34;
- UINT8 Cmos0x35;

Status = ScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
- if (Status == EFI_SUCCESS && LowerMemorySize > 0) {
- return LowerMemorySize;
- }
+ ASSERT (Status == EFI_SUCCESS && LowerMemorySize > 0);
Nit: better to do

ASSERT_EFI_ERROR (Status);
ASSERT (LowerMemorySize > 0);

so that you can tell from the debug log which condition triggered, and
if it is the first one, what the actual error code was.


+ return LowerMemorySize;

- //
- // CMOS 0x34/0x35 specifies the system memory above 16 MB.
- // * CMOS(0x35) is the high byte
- // * CMOS(0x34) is the low byte
- // * The size is specified in 64kb chunks
- // * Since this is memory above 16MB, the 16MB must be added
- // into the calculation to get the total memory size.
- //
-
- Cmos0x34 = (UINT8) CmosRead8 (0x34);
- Cmos0x35 = (UINT8) CmosRead8 (0x35);
-
- return (UINT32) (((UINTN)((Cmos0x35 << 8) + Cmos0x34) << 16) + SIZE_16MB);
-}
-
-
-STATIC
-UINT64
-GetSystemMemorySizeAbove4gb (
- )
-{
- UINT32 Size;
- UINTN CmosIndex;
-
- //
- // CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
- // * CMOS(0x5d) is the most significant size byte
- // * CMOS(0x5c) is the middle size byte
- // * CMOS(0x5b) is the least significant size byte
- // * The size is specified in 64kb chunks
- //
-
- Size = 0;
- for (CmosIndex = 0x5d; CmosIndex >= 0x5b; CmosIndex--) {
- Size = (UINT32) (Size << 8) + (UINT32) CmosRead8 (CmosIndex);
- }
-
- return LShiftU64 (Size, 16);
}


@@ -371,12 +329,9 @@ GetFirstNonAddress (
// If QEMU presents an E820 map, then get the highest exclusive >=4GB RAM
// address from it. This can express an address >= 4GB+1TB.
//
- // Otherwise, get the flat size of the memory above 4GB from the CMOS (which
- // can only express a size smaller than 1TB), and add it to 4GB.
- //
Status = ScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
if (EFI_ERROR (Status)) {
- FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
+ FirstNonAddress = BASE_4GB;
}

//
@@ -719,7 +674,6 @@ QemuInitializeRam (
)
{
UINT64 LowerMemorySize;
- UINT64 UpperMemorySize;
MTRR_SETTINGS MtrrSettings;
EFI_STATUS Status;

@@ -775,12 +729,6 @@ QemuInitializeRam (
// memory size read from the CMOS.
//
Status = ScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
- if (EFI_ERROR (Status)) {
- UpperMemorySize = GetSystemMemorySizeAbove4gb ();
- if (UpperMemorySize != 0) {
- AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
- }
- }
}

//
--
2.31.1


[PATCH v2 3/3] RFC: OvmfPkg/PlatformPei: stop using cmos for memory detection

Gerd Hoffmann
 

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3593

Not needed for qemu 1.7 (released in 2013) and newer.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
OvmfPkg/PlatformPei/MemDetect.c | 58 ++-------------------------------
1 file changed, 3 insertions(+), 55 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 20154255324b..8d2c7f432472 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -37,7 +37,6 @@ Module Name:
#include <Library/QemuFwCfgSimpleParserLib.h>

#include "Platform.h"
-#include "Cmos.h"

UINT8 mPhysMemAddressWidth;

@@ -295,52 +294,11 @@ GetSystemMemorySizeBelow4gb (
{
EFI_STATUS Status;
UINT64 LowerMemorySize = 0;
- UINT8 Cmos0x34;
- UINT8 Cmos0x35;

Status = ScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
- if (Status == EFI_SUCCESS && LowerMemorySize > 0) {
- return LowerMemorySize;
- }
+ ASSERT (Status == EFI_SUCCESS && LowerMemorySize > 0);
+ return LowerMemorySize;

- //
- // CMOS 0x34/0x35 specifies the system memory above 16 MB.
- // * CMOS(0x35) is the high byte
- // * CMOS(0x34) is the low byte
- // * The size is specified in 64kb chunks
- // * Since this is memory above 16MB, the 16MB must be added
- // into the calculation to get the total memory size.
- //
-
- Cmos0x34 = (UINT8) CmosRead8 (0x34);
- Cmos0x35 = (UINT8) CmosRead8 (0x35);
-
- return (UINT32) (((UINTN)((Cmos0x35 << 8) + Cmos0x34) << 16) + SIZE_16MB);
-}
-
-
-STATIC
-UINT64
-GetSystemMemorySizeAbove4gb (
- )
-{
- UINT32 Size;
- UINTN CmosIndex;
-
- //
- // CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
- // * CMOS(0x5d) is the most significant size byte
- // * CMOS(0x5c) is the middle size byte
- // * CMOS(0x5b) is the least significant size byte
- // * The size is specified in 64kb chunks
- //
-
- Size = 0;
- for (CmosIndex = 0x5d; CmosIndex >= 0x5b; CmosIndex--) {
- Size = (UINT32) (Size << 8) + (UINT32) CmosRead8 (CmosIndex);
- }
-
- return LShiftU64 (Size, 16);
}


@@ -371,12 +329,9 @@ GetFirstNonAddress (
// If QEMU presents an E820 map, then get the highest exclusive >=4GB RAM
// address from it. This can express an address >= 4GB+1TB.
//
- // Otherwise, get the flat size of the memory above 4GB from the CMOS (which
- // can only express a size smaller than 1TB), and add it to 4GB.
- //
Status = ScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
if (EFI_ERROR (Status)) {
- FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
+ FirstNonAddress = BASE_4GB;
}

//
@@ -719,7 +674,6 @@ QemuInitializeRam (
)
{
UINT64 LowerMemorySize;
- UINT64 UpperMemorySize;
MTRR_SETTINGS MtrrSettings;
EFI_STATUS Status;

@@ -775,12 +729,6 @@ QemuInitializeRam (
// memory size read from the CMOS.
//
Status = ScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
- if (EFI_ERROR (Status)) {
- UpperMemorySize = GetSystemMemorySizeAbove4gb ();
- if (UpperMemorySize != 0) {
- AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
- }
- }
}

//
--
2.31.1


[PATCH v2 2/3] OvmfPkg/PlatformPei: prefer etc/e820 for memory detection

Gerd Hoffmann
 

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3593

Prefer the e820 map provided via qemu firmware config interface
for memory detection. Use rtc cmos only as fallback, which should
be rarely needed these days as qemu supports etc/e820 since 2013.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
OvmfPkg/PlatformPei/MemDetect.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index d7fb3e742be3..20154255324b 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -293,9 +293,16 @@ GetSystemMemorySizeBelow4gb (
VOID
)
{
+ EFI_STATUS Status;
+ UINT64 LowerMemorySize = 0;
UINT8 Cmos0x34;
UINT8 Cmos0x35;

+ Status = ScanOrAdd64BitE820Ram (FALSE, &LowerMemorySize, NULL);
+ if (Status == EFI_SUCCESS && LowerMemorySize > 0) {
+ return LowerMemorySize;
+ }
+
//
// CMOS 0x34/0x35 specifies the system memory above 16 MB.
// * CMOS(0x35) is the high byte
@@ -722,7 +729,6 @@ QemuInitializeRam (
// Determine total memory size available
//
LowerMemorySize = GetSystemMemorySizeBelow4gb ();
- UpperMemorySize = GetSystemMemorySizeAbove4gb ();

if (mBootMode == BOOT_ON_S3_RESUME) {
//
@@ -769,8 +775,11 @@ QemuInitializeRam (
// memory size read from the CMOS.
//
Status = ScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
- if (EFI_ERROR (Status) && UpperMemorySize != 0) {
- AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
+ if (EFI_ERROR (Status)) {
+ UpperMemorySize = GetSystemMemorySizeAbove4gb ();
+ if (UpperMemorySize != 0) {
+ AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
+ }
}
}

--
2.31.1


[PATCH v2 1/3] OvmfPkg/PlatformPei: ScanOrAdd64BitE820Ram improvements

Gerd Hoffmann
 

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3593

Add a bool parameter to ScanOrAdd64BitE820Ram to explicitly specify
whenever ScanOrAdd64BitE820Ram should add HOBs for high memory (above
4G) or scan only.

Also add a lowmem parameter so ScanOrAdd64BitE820Ram
can report the memory size below 4G.

This allows a more flexible usage of ScanOrAdd64BitE820Ram,
a followup patch will use it for all memory detection.

No functional change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
OvmfPkg/PlatformPei/MemDetect.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 2deec128f464..d7fb3e742be3 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -200,6 +200,8 @@ QemuUc32BaseInitialization (
STATIC
EFI_STATUS
ScanOrAdd64BitE820Ram (
+ IN BOOLEAN AddHighHob,
+ OUT UINT64 *LowMemory OPTIONAL,
OUT UINT64 *MaxAddress OPTIONAL
)
{
@@ -217,6 +219,9 @@ ScanOrAdd64BitE820Ram (
return EFI_PROTOCOL_ERROR;
}

+ if (LowMemory != NULL) {
+ *LowMemory = 0;
+ }
if (MaxAddress != NULL) {
*MaxAddress = BASE_4GB;
}
@@ -232,9 +237,8 @@ ScanOrAdd64BitE820Ram (
E820Entry.Length,
E820Entry.Type
));
- if (E820Entry.Type == EfiAcpiAddressRangeMemory &&
- E820Entry.BaseAddr >= BASE_4GB) {
- if (MaxAddress == NULL) {
+ if (E820Entry.Type == EfiAcpiAddressRangeMemory) {
+ if (AddHighHob && E820Entry.BaseAddr >= BASE_4GB) {
UINT64 Base;
UINT64 End;

@@ -254,11 +258,12 @@ ScanOrAdd64BitE820Ram (
End
));
}
- } else {
+ }
+ if (MaxAddress || LowMemory) {
UINT64 Candidate;

Candidate = E820Entry.BaseAddr + E820Entry.Length;
- if (Candidate > *MaxAddress) {
+ if (MaxAddress && Candidate > *MaxAddress) {
*MaxAddress = Candidate;
DEBUG ((
DEBUG_VERBOSE,
@@ -267,6 +272,15 @@ ScanOrAdd64BitE820Ram (
*MaxAddress
));
}
+ if (LowMemory && Candidate > *LowMemory && Candidate < BASE_4GB) {
+ *LowMemory = Candidate;
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a: LowMemory=0x%Lx\n",
+ __FUNCTION__,
+ *LowMemory
+ ));
+ }
}
}
}
@@ -353,7 +367,7 @@ GetFirstNonAddress (
// Otherwise, get the flat size of the memory above 4GB from the CMOS (which
// can only express a size smaller than 1TB), and add it to 4GB.
//
- Status = ScanOrAdd64BitE820Ram (&FirstNonAddress);
+ Status = ScanOrAdd64BitE820Ram (FALSE, NULL, &FirstNonAddress);
if (EFI_ERROR (Status)) {
FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
}
@@ -754,7 +768,7 @@ QemuInitializeRam (
// entries. Otherwise, create a single memory HOB with the flat >=4GB
// memory size read from the CMOS.
//
- Status = ScanOrAdd64BitE820Ram (NULL);
+ Status = ScanOrAdd64BitE820Ram (TRUE, NULL, NULL);
if (EFI_ERROR (Status) && UpperMemorySize != 0) {
AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize);
}
--
2.31.1

5941 - 5960 of 85723