Date   

Re: [edk2-platforms][PATCH V2 2/7] Platform/ARM/Morello: Add support for PciHostBridgeLib

Sami Mujawar
 

Hi Chandni,

This patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 01/04/2021, 15:36, "Chandni Cherukuri" <chandni.cherukuri@arm.com> wrote:

Morello FVP platform supports a PCIe root complex.
This patch implements PciHostBridgeLib to support PCIe.

Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
---
Platform/ARM/Morello/Library/PciHostBridgeLib/PciHostBridgeLibFvp.inf | 48 ++++++
Platform/ARM/Morello/Library/PciHostBridgeLib/PciHostBridgeLibFvp.c | 180 ++++++++++++++++++++
2 files changed, 228 insertions(+)

diff --git a/Platform/ARM/Morello/Library/PciHostBridgeLib/PciHostBridgeLibFvp.inf b/Platform/ARM/Morello/Library/PciHostBridgeLib/PciHostBridgeLibFvp.inf
new file mode 100644
index 000000000000..1d6c5b01d13d
--- /dev/null
+++ b/Platform/ARM/Morello/Library/PciHostBridgeLib/PciHostBridgeLibFvp.inf
@@ -0,0 +1,48 @@
+## @file
+# PCI Host Bridge Library instance for ARM Morello FVP platform.
+#
+# Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x0001001B
+ BASE_NAME = PciHostBridgeLib
+ FILE_GUID = 6879CEAD-DC94-42EB-895C-096D36B8083C
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = PciHostBridgeLib|DXE_DRIVER
+
+#
+# The following information is for reference only and not required by the build
+# tools.
+#
+# VALID_ARCHITECTURES = AARCH64
+#
+
+[Sources]
+ PciHostBridgeLibFvp.c
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Platform/ARM/Morello/MorelloPlatform.dec
+
+[FixedPcd]
+ gArmMorelloTokenSpaceGuid.PcdPciBusMax
+ gArmMorelloTokenSpaceGuid.PcdPciBusMin
+ gArmMorelloTokenSpaceGuid.PcdPciIoBase
+ gArmMorelloTokenSpaceGuid.PcdPciIoSize
+ gArmMorelloTokenSpaceGuid.PcdPciMmio32Base
+ gArmMorelloTokenSpaceGuid.PcdPciMmio32Size
+ gArmMorelloTokenSpaceGuid.PcdPciMmio64Base
+ gArmMorelloTokenSpaceGuid.PcdPciMmio64Size
+
+[Protocols]
+ gEfiCpuIo2ProtocolGuid ## CONSUMES
+
+[Depex]
+ gEfiCpuIo2ProtocolGuid
diff --git a/Platform/ARM/Morello/Library/PciHostBridgeLib/PciHostBridgeLibFvp.c b/Platform/ARM/Morello/Library/PciHostBridgeLib/PciHostBridgeLibFvp.c
new file mode 100644
index 000000000000..042cbbdca202
--- /dev/null
+++ b/Platform/ARM/Morello/Library/PciHostBridgeLib/PciHostBridgeLibFvp.c
@@ -0,0 +1,180 @@
+/** @file
+ PCI Host Bridge Library instance for ARM Morello FVP platform.
+
+ Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/DebugLib.h>
+#include <Library/DevicePathLib.h>
+#include <Library/PciHostBridgeLib.h>
+#include <Protocol/PciHostBridgeResourceAllocation.h>
+
+#define ROOT_COMPLEX_NUM 1
+
+GLOBAL_REMOVE_IF_UNREFERENCED
+STATIC CHAR16 CONST * CONST mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
+ L"Mem", L"I/O", L"Bus"
+};
+
+#pragma pack(1)
+typedef struct {
+ ACPI_HID_DEVICE_PATH AcpiDevicePath;
+ EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
+} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
+#pragma pack ()
+
+STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath[ROOT_COMPLEX_NUM] = {
+ {
+ {
+ {
+ ACPI_DEVICE_PATH,
+ ACPI_DP,
+ {
+ (UINT8)sizeof (ACPI_HID_DEVICE_PATH),
+ (UINT8)(sizeof (ACPI_HID_DEVICE_PATH) >> 8)
+ }
+ },
+ EISA_PNP_ID(0x0A08),
+ 0
+ },
+ {
+ END_DEVICE_PATH_TYPE,
+ END_ENTIRE_DEVICE_PATH_SUBTYPE,
+ {
+ END_DEVICE_PATH_LENGTH,
+ 0
+ }
+ }
+ },
+};
+
+STATIC PCI_ROOT_BRIDGE mPciRootBridge[ROOT_COMPLEX_NUM] = {
+ {
+ 0, // Segment
+ 0, // Supports
+ 0, // Attributes
+ TRUE, // DmaAbove4G
+ FALSE, // NoExtendedConfigSpace
+ FALSE, // ResourceAssigned
+ EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM | // AllocationAttributes
+ EFI_PCI_HOST_BRIDGE_MEM64_DECODE,
+ {
+ // Bus
+ FixedPcdGet32 (PcdPciBusMin),
+ FixedPcdGet32 (PcdPciBusMax)
+ }, {
+ // Io
+ FixedPcdGet64 (PcdPciIoBase),
+ FixedPcdGet64 (PcdPciIoBase) + FixedPcdGet64 (PcdPciIoSize) - 1
+ }, {
+ // Mem
+ FixedPcdGet32 (PcdPciMmio32Base),
+ FixedPcdGet32 (PcdPciMmio32Base) + FixedPcdGet32 (PcdPciMmio32Size) - 1
+ }, {
+ // MemAbove4G
+ FixedPcdGet64 (PcdPciMmio64Base),
+ FixedPcdGet64 (PcdPciMmio64Base) + FixedPcdGet64 (PcdPciMmio64Size) - 1
+ }, {
+ // PMem
+ MAX_UINT64,
+ 0
+ }, {
+ // PMemAbove4G
+ MAX_UINT64,
+ 0
+ },
+ (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath[0]
+ },
+};
+
+/**
+ Return all the root bridge instances in an array.
+
+ @param Count Return the count of root bridge instances.
+
+ @return All the root bridge instances in an array.
+ The array should be passed into PciHostBridgeFreeRootBridges()
+ when it's not used.
+**/
+PCI_ROOT_BRIDGE *
+EFIAPI
+PciHostBridgeGetRootBridges (
+ UINTN *Count
+ )
+{
+ *Count = ARRAY_SIZE (mPciRootBridge);
+ return mPciRootBridge;
+}
+
+/**
+ Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
+
+ @param Bridges The root bridge instances array.
+ @param Count The count of the array.
+**/
+VOID
+EFIAPI
+PciHostBridgeFreeRootBridges (
+ PCI_ROOT_BRIDGE *Bridges,
+ UINTN Count
+ )
+{
+}
+
+/**
+ Inform the platform that the resource conflict happens.
+
+ @param HostBridgeHandle Handle of the Host Bridge.
+ @param Configuration Pointer to PCI I/O and PCI memory resource
+ descriptors. The Configuration contains the resources
+ for all the root bridges. The resource for each root
+ bridge is terminated with END descriptor and an
+ additional END is appended indicating the end of the
+ entire resources. The resource descriptor field
+ values follow the description in
+ EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL
+ .SubmitResources().
+**/
+VOID
+EFIAPI
+PciHostBridgeResourceConflict (
+ EFI_HANDLE HostBridgeHandle,
+ VOID *Configuration
+ )
+{
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
+ UINTN RootBridgeIndex;
+ DEBUG ((DEBUG_ERROR, "PciHostBridge: Resource conflict happens!\n"));
+
+ RootBridgeIndex = 0;
+ Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
+ while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
+ DEBUG ((DEBUG_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
+ for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
+ ASSERT (Descriptor->ResType <
+ (ARRAY_SIZE (mPciHostBridgeLibAcpiAddressSpaceTypeStr))
+ );
+ DEBUG ((DEBUG_ERROR, "%s: Length/Alignment = 0x%lx / 0x%lx\n",
+ mPciHostBridgeLibAcpiAddressSpaceTypeStr[Descriptor->ResType],
+ Descriptor->AddrLen, Descriptor->AddrRangeMax
+ ));
+ if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
+ DEBUG ((DEBUG_ERROR, "Granularity/SpecificFlag = %ld / %02x%s\n",
+ Descriptor->AddrSpaceGranularity, Descriptor->SpecificFlag,
+ ((Descriptor->SpecificFlag &
+ EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE
+ ) != 0) ? L" (Prefetchable)" : L""
+ ));
+ }
+ }
+ //
+ // Skip the END descriptor for root bridge
+ //
+ ASSERT (Descriptor->Desc == ACPI_END_TAG_DESCRIPTOR);
+ Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)(
+ (EFI_ACPI_END_TAG_DESCRIPTOR *)Descriptor + 1
+ );
+ }
+}
--
2.17.1


Re: [edk2-platforms][PATCH V2 1/7] Platform/ARM/Morello: Add Platform library implementation

Sami Mujawar
 

Hi Chandni,

Please find my response inline marked [SAMI].


With that changed.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 01/04/2021, 15:36, "Chandni Cherukuri" <chandni.cherukuri@arm.com> wrote:

From: Anurag Koul <anurag.koul@arm.com>

This patch adds initial Morello Platform Library support.
It includes virtual memory map and helper functions for
platform initialization.

Co-authored-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
---
Platform/ARM/Morello/Library/PlatformLib/PlatformLib.inf | 52 +++++
Platform/ARM/Morello/Include/MorelloPlatform.h | 63 +++++++
Platform/ARM/Morello/Library/PlatformLib/PlatformLib.c | 99 ++++++++++
Platform/ARM/Morello/Library/PlatformLib/PlatformLibMem.c | 198 ++++++++++++++++++++
Platform/ARM/Morello/Library/PlatformLib/AArch64/Helper.S | 83 ++++++++
5 files changed, 495 insertions(+)

diff --git a/Platform/ARM/Morello/Library/PlatformLib/PlatformLib.inf b/Platform/ARM/Morello/Library/PlatformLib/PlatformLib.inf
new file mode 100644
index 000000000000..c2d7da3701d2
--- /dev/null
+++ b/Platform/ARM/Morello/Library/PlatformLib/PlatformLib.inf
@@ -0,0 +1,52 @@
+## @file
+# Platform Library for Morello platform.
+#
+# Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x0001001B
+ BASE_NAME = ArmMorelloLib
+ FILE_GUID = 36853D86-7200-47B4-9408-E962A00963FD
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmPlatformLib
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ ArmPlatformPkg/ArmPlatformPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Platform/ARM/Morello/MorelloPlatform.dec
+
+[Sources.common]
+ PlatformLib.c
+ PlatformLibMem.c
+
+[Sources.AARCH64]
+ AArch64/Helper.S | GCC
+
+[FixedPcd]
+ gArmMorelloTokenSpaceGuid.PcdDramBlock2Base
+ gArmMorelloTokenSpaceGuid.PcdPciBusMax
+ gArmMorelloTokenSpaceGuid.PcdPciBusMin
+ gArmMorelloTokenSpaceGuid.PcdPciExpressBaseAddress
+ gArmMorelloTokenSpaceGuid.PcdPciIoSize
+ gArmMorelloTokenSpaceGuid.PcdPciMmio32Base
+ gArmMorelloTokenSpaceGuid.PcdPciMmio32Size
+ gArmMorelloTokenSpaceGuid.PcdPciMmio64Base
+ gArmMorelloTokenSpaceGuid.PcdPciMmio64Size
+
+ gArmTokenSpaceGuid.PcdArmPrimaryCore
+ gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
+ gArmTokenSpaceGuid.PcdSystemMemoryBase
+ gArmTokenSpaceGuid.PcdSystemMemorySize
+
+[Guids]
+ gEfiHobListGuid ## CONSUMES ## SystemTable
+
+[Ppis]
+ gArmMpCoreInfoPpiGuid
diff --git a/Platform/ARM/Morello/Include/MorelloPlatform.h b/Platform/ARM/Morello/Include/MorelloPlatform.h
new file mode 100644
index 000000000000..8b3233025958
--- /dev/null
+++ b/Platform/ARM/Morello/Include/MorelloPlatform.h
@@ -0,0 +1,63 @@
+/** @file
+
+ Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef MORELLO_PLATFORM_H_
+#define MORELLO_PLATFORM_H_
+
+#define MORELLO_DRAM_BLOCK1_SIZE SIZE_2GB
+
+// ****************************************************************************
+// Platform Memory Map
+// ****************************************************************************
+
+// SubSystem Peripherals - UART0
+#define MORELLO_UART0_BASE 0x2A400000
+#define MORELLO_UART0_SZ SIZE_64KB
+
+// SubSystem Peripherals - UART1
+#define MORELLO_UART1_BASE 0x2A410000
+#define MORELLO_UART1_SZ SIZE_64KB
+
+// SubSystem Peripherals - Generic Watchdog
+#define MORELLO_GENERIC_WDOG_BASE 0x2A440000
+#define MORELLO_GENERIC_WDOG_SZ SIZE_128KB
+
+// SubSystem Peripherals - GIC(600)
+#define MORELLO_GIC_BASE 0x30000000
+#define MORELLO_GICR_BASE 0x300C0000
+#define MORELLO_GIC_SZ SIZE_256KB
+#define MORELLO_GICR_SZ SIZE_1MB
+
+// SubSystem non-secure SRAM
+#define MORELLO_NON_SECURE_SRAM_BASE 0x06000000
+#define MORELLO_NON_SECURE_SRAM_SZ SIZE_64KB
+
+// AXI Expansion peripherals
+#define MORELLO_EXP_PERIPH_BASE 0x1C000000
+#define MORELLO_EXP_PERIPH_BASE_SZ 0x1300000
+
+// Platform information structure base address
+#define MORELLO_PLAT_INFO_STRUCT_BASE MORELLO_NON_SECURE_SRAM_BASE
+
+/*
+ * Platform information structure stored in Non-secure SRAM. Platform
+ * information are passed from the trusted firmware with the below structure
+ * format. The elements of MORELLO_PLAT_INFO should be always in sync with
+ * the lower level firmware.
+ */
+#pragma pack(1)
+
+typedef struct {
+ UINT64 LocalDdrSize; ///< Local DDR memory size in Bytes
+ UINT64 RemoteDdrSize; ///< Remote DDR memory size in Bytes
+ UINT8 SlaveCount; ///< Slave count in C2C mode
+ UINT8 Mode; ///< 0 - Single Chip, 1 - Chip to Chip (C2C)
+} MORELLO_PLAT_INFO;
+
+#pragma pack()
+
+#endif //MORELLO_PLATFORM_H_
diff --git a/Platform/ARM/Morello/Library/PlatformLib/PlatformLib.c b/Platform/ARM/Morello/Library/PlatformLib/PlatformLib.c
new file mode 100644
index 000000000000..52318a62911a
--- /dev/null
+++ b/Platform/ARM/Morello/Library/PlatformLib/PlatformLib.c
@@ -0,0 +1,99 @@
+/** @file
+
+ Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/ArmPlatformLib.h>
+#include <Library/BaseLib.h>
+#include <Ppi/ArmMpCoreInfo.h>
+
+STATIC ARM_CORE_INFO mCoreInfoTable[] = {
+ { 0x0, 0x0 }, // Cluster 0, Core 0
+ { 0x0, 0x1 }, // Cluster 0, Core 1
+ { 0x1, 0x0 }, // Cluster 1, Core 0
+ { 0x1, 0x1 } // Cluster 1, Core 1
+};
+
+/**
+ Return the current Boot Mode.
+
+ This function returns the boot reason on the platform.
+
+**/
+EFI_BOOT_MODE
+ArmPlatformGetBootMode (
+ VOID
+ )
+{
+ return BOOT_WITH_FULL_CONFIGURATION;
+}
+
+/**
+ Initialize controllers that must be setup in the normal world.
+
+ This function is called by the ArmPlatformPkg/Pei or ArmPlatformPkg/Pei/PlatformPeim
+ in the PEI phase.
+
+ @param[in] MpId Processor ID
+
+**/
+RETURN_STATUS
+ArmPlatformInitialize (
+ IN UINTN MpId
+ )
+{
+ return RETURN_SUCCESS;
+}
+
+/**
+ Populate the Platform core information.
+
+ This function populates the ARM_MP_CORE_INFO_PPI with information about the cores.
+
+ @param[out] CoreCount Number of cores
+ @param[out] ArmCoreTable Table containing information about the cores
+
+**/
+EFI_STATUS
+PrePeiCoreGetMpCoreInfo (
+ OUT UINTN *CoreCount,
+ OUT ARM_CORE_INFO **ArmCoreTable
+ )
+{
+ *CoreCount = sizeof (mCoreInfoTable) / sizeof (ARM_CORE_INFO);
+ *ArmCoreTable = mCoreInfoTable;
+ return EFI_SUCCESS;
+}
+
+STATIC ARM_MP_CORE_INFO_PPI mMpCoreInfoPpi = {
+ PrePeiCoreGetMpCoreInfo
+};
+
+STATIC EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
+ {
+ EFI_PEI_PPI_DESCRIPTOR_PPI,
+ &gArmMpCoreInfoPpiGuid,
+ &mMpCoreInfoPpi
+ }
+};
+
+/**
+ Return the Platform specific PPIs
+
+ This function exposes the Morello Platform Specific PPIs.
+
+ @param[out] PpiListSize Size in Bytes of the Platform PPI List
+ @param[out] PpiList Platform PPI List
+
+**/
+VOID
+ArmPlatformGetPlatformPpiList (
+ OUT UINTN *PpiListSize,
+ OUT EFI_PEI_PPI_DESCRIPTOR **PpiList
+ )
+{
+ *PpiListSize = sizeof (gPlatformPpiTable);
+ *PpiList = gPlatformPpiTable;
+}
diff --git a/Platform/ARM/Morello/Library/PlatformLib/PlatformLibMem.c b/Platform/ARM/Morello/Library/PlatformLib/PlatformLibMem.c
new file mode 100644
index 000000000000..54a870cfb3ba
--- /dev/null
+++ b/Platform/ARM/Morello/Library/PlatformLib/PlatformLibMem.c
@@ -0,0 +1,198 @@
+/** @file
+
+ Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/ArmPlatformLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <MorelloPlatform.h>
+
+// The total number of descriptors, including the final "end-of-table" descriptor.
+#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 12
+
+#if !defined(MDEPKG_NDEBUG)
+STATIC CONST CHAR8 *gTblAttrDesc[] = {
+ "UNCACHED_UNBUFFERED ",
+ "NONSECURE_UNCACHED_UNBUFFERED",
+ "WRITE_BACK ",
+ "NONSECURE_WRITE_BACK ",
+ "WB_NONSHAREABLE ",
+ "NONSECURE_WB_NONSHAREABLE ",
+ "WRITE_THROUGH ",
+ "NONSECURE_WRITE_THROUGH ",
+ "DEVICE ",
+ "NONSECURE_DEVICE "
+};
+#endif
+
+#define LOG_MEM(desc) DEBUG (( \
+ DEBUG_ERROR, \
+ desc, \
+ VirtualMemoryTable[Index].PhysicalBase, \
+ (VirtualMemoryTable[Index].PhysicalBase + \
+ VirtualMemoryTable[Index].Length - 1), \
+ VirtualMemoryTable[Index].Length, \
+ gTblAttrDesc[VirtualMemoryTable[Index].Attributes] \
+ ));
+
+/**
+ Returns the Virtual Memory Map of the platform.
+
+ This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
+ on your platform.
+
+ @param[out] VirtualMemoryMap Array of ARM_MEMORY_REGION_DESCRIPTOR describing
+ a Physical-to-Virtual Memory mapping. This array
+ must be ended by a zero-filled entry.
+**/
+VOID
+ArmPlatformGetVirtualMemoryMap (
+ OUT ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap
+ )
+{
+ UINTN Index;
+ ARM_MEMORY_REGION_DESCRIPTOR * VirtualMemoryTable;
+ EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
+ MORELLO_PLAT_INFO * PlatInfo;
+ UINT64 DramBlock2Size;
+
+ Index = 0;
+ DramBlock2Size = 0;
+
+ PlatInfo = (MORELLO_PLAT_INFO *)MORELLO_PLAT_INFO_STRUCT_BASE;
+ if (PlatInfo->LocalDdrSize > MORELLO_DRAM_BLOCK1_SIZE) {
+ DramBlock2Size = PlatInfo->LocalDdrSize - MORELLO_DRAM_BLOCK1_SIZE;
+ }
+
+ if (DramBlock2Size != 0) {
+ ResourceAttributes =
+ EFI_RESOURCE_ATTRIBUTE_PRESENT |
+ EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
+ EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
+ EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
+ EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
+ EFI_RESOURCE_ATTRIBUTE_TESTED;
+
+ BuildResourceDescriptorHob (
+ EFI_RESOURCE_SYSTEM_MEMORY,
+ ResourceAttributes,
+ FixedPcdGet64 (PcdDramBlock2Base),
+ DramBlock2Size
+ );
+ }
+
+ ASSERT (VirtualMemoryMap != NULL);
+
+ VirtualMemoryTable = AllocatePool (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
+ MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
+ if (VirtualMemoryTable == NULL) {
+ return;
+ }
+
+ DEBUG ((
+ DEBUG_ERROR,
+ " Memory Map\n----------------------------------------------------------\n"
+ ));
+ DEBUG ((
+ DEBUG_ERROR,
+ "Description : START - END " \
+ "[ SIZE ] { ATTR }\n"
+ ));
+
+ // SubSystem Peripherals - Generic Watchdog
+ VirtualMemoryTable[Index].PhysicalBase = MORELLO_GENERIC_WDOG_BASE;
+ VirtualMemoryTable[Index].VirtualBase = MORELLO_GENERIC_WDOG_BASE;
+ VirtualMemoryTable[Index].Length = MORELLO_GENERIC_WDOG_SZ;
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+ LOG_MEM ("Generic Watchdog : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+
+ // SubSystem Peripherals - GIC-600
+ VirtualMemoryTable[++Index].PhysicalBase = MORELLO_GIC_BASE;
+ VirtualMemoryTable[Index].VirtualBase = MORELLO_GIC_BASE;
+ VirtualMemoryTable[Index].Length = MORELLO_GIC_SZ;
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+ LOG_MEM ("GIC-600 : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+
+ // SubSystem Peripherals - GICR-600
+ VirtualMemoryTable[++Index].PhysicalBase = MORELLO_GICR_BASE;
+ VirtualMemoryTable[Index].VirtualBase = MORELLO_GICR_BASE;
+ VirtualMemoryTable[Index].Length = MORELLO_GICR_SZ;
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+ LOG_MEM ("GICR-600 : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+
+ // SubSystem non-secure SRAM
+ VirtualMemoryTable[++Index].PhysicalBase = MORELLO_NON_SECURE_SRAM_BASE;
+ VirtualMemoryTable[Index].VirtualBase = MORELLO_NON_SECURE_SRAM_BASE;
+ VirtualMemoryTable[Index].Length = MORELLO_NON_SECURE_SRAM_SZ;
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
+ LOG_MEM ("non-secure SRAM : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+
+ // SubSystem Pheripherals - UART0
+ VirtualMemoryTable[++Index].PhysicalBase = MORELLO_UART0_BASE;
+ VirtualMemoryTable[Index].VirtualBase = MORELLO_UART0_BASE;
+ VirtualMemoryTable[Index].Length = MORELLO_UART0_SZ;
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+ LOG_MEM ("UART0 : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+
+ // DDR Primary
+ VirtualMemoryTable[++Index].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
+ VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdSystemMemoryBase);
+ VirtualMemoryTable[Index].Length = PcdGet64 (PcdSystemMemorySize);
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+ LOG_MEM ("DDR Primary : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+
+ // DDR Secondary
+ if (DramBlock2Size != 0) {
+ VirtualMemoryTable[++Index].PhysicalBase = PcdGet64 (PcdDramBlock2Base);
+ VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdDramBlock2Base);
+ VirtualMemoryTable[Index].Length = DramBlock2Size;
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+ LOG_MEM ("DDR Secondary : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+ }
+
+ // Expansion Peripherals
+ VirtualMemoryTable[++Index].PhysicalBase = MORELLO_EXP_PERIPH_BASE;
+ VirtualMemoryTable[Index].VirtualBase = MORELLO_EXP_PERIPH_BASE;
+ VirtualMemoryTable[Index].Length = MORELLO_EXP_PERIPH_BASE_SZ;
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+ LOG_MEM ("Expansion Peripherals : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+
+ // PCI Configuration Space
+ VirtualMemoryTable[++Index].PhysicalBase = PcdGet64 (PcdPciExpressBaseAddress);
+ VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdPciExpressBaseAddress);
+ VirtualMemoryTable[Index].Length = (FixedPcdGet32 (PcdPciBusMax) -
+ FixedPcdGet32 (PcdPciBusMin) + 1) *
+ SIZE_1MB;
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+ LOG_MEM ("PCI Configuration Space : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+
+ // PCI MMIO32/IO Space
+ VirtualMemoryTable[++Index].PhysicalBase = PcdGet32 (PcdPciMmio32Base);
+ VirtualMemoryTable[Index].VirtualBase = PcdGet32 (PcdPciMmio32Base);
+ VirtualMemoryTable[Index].Length = PcdGet32 (PcdPciMmio32Size) +
+ PcdGet32 (PcdPciIoSize);
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+ LOG_MEM ("PCI MMIO32 & IO Region : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+
+ // PCI MMIO64 Space
+ VirtualMemoryTable[++Index].PhysicalBase = PcdGet64 (PcdPciMmio64Base);
+ VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdPciMmio64Base);
+ VirtualMemoryTable[Index].Length = PcdGet64 (PcdPciMmio64Size);
+ VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+ LOG_MEM ("PCI MMIO64 Region : 0x%016lx - 0x%016lx [ 0x%016lx ] { %a }\n");
+
+ // End of Table
+ VirtualMemoryTable[++Index].PhysicalBase = 0;
+ VirtualMemoryTable[Index].VirtualBase = 0;
+ VirtualMemoryTable[Index].Length = 0;
+ VirtualMemoryTable[Index].Attributes = (ARM_MEMORY_REGION_ATTRIBUTES)0;
+
+ ASSERT ((Index) < MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
+ DEBUG ((DEBUG_INIT, "Virtual Memory Table setup complete.\n"));
+
+ *VirtualMemoryMap = VirtualMemoryTable;
+}
diff --git a/Platform/ARM/Morello/Library/PlatformLib/AArch64/Helper.S b/Platform/ARM/Morello/Library/PlatformLib/AArch64/Helper.S
new file mode 100644
index 000000000000..f6cc087a132c
--- /dev/null
+++ b/Platform/ARM/Morello/Library/PlatformLib/AArch64/Helper.S
@@ -0,0 +1,83 @@
+/** @file
+
+ Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <AsmMacroIoLibV8.h>
+#include <Library/ArmLib.h>
+
+.text
+.align 3
+
+GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
+GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
+GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
+GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
+
+//
+// First platform specific function to be called in the PEI phase
+//
+// This function is actually the first function called by the PrePi
+// or PrePeiCore modules. It allows to retrieve arguments passed to
+// the UEFI firmware through the CPU registers.
+//
+ASM_PFX(ArmPlatformPeiBootAction):
+ ret
+
+//
+// Return the core position from the value of its MpId register
+//
+// This function returns core position from the position 0 in the processor.
+// This function might be called from assembler before any stack is set.
+//
+// @return Return the core position
+//
+//UINTN
+//ArmPlatformGetCorePosition (
+// IN UINTN MpId
+// );
+// With this function: CorePos = (ClusterId * 2) + CoreId
+ASM_PFX(ArmPlatformGetCorePosition):
[SAMI] Can ASM_FUNC be used instead of ASM_PFX?
[/SAMI]
+ and x1, x0, #ARM_CORE_MASK
+ and x0, x0, #ARM_CLUSTER_MASK
+ add x0, x1, x0, LSR #7
+ ret
+
+//
+// Return the MpId of the primary core
+//
+// This function returns the MpId of the primary core.
+// This function might be called from assembler before any stack is set.
+//
+// @return Return the MpId of the primary core
+//
+//UINTN
+//ArmPlatformGetPrimaryCoreMpId (
+// VOID
+// );
+ASM_PFX(ArmPlatformGetPrimaryCoreMpId):
+ MOV32 (w0, FixedPcdGet32(PcdArmPrimaryCore))
[SAMI] Please add a space between FixedPcdGet32 and opening (. Same at other places in this file.
[/SAMI]
+ ret
+
+//
+// Return a non-zero value if the callee is the primary core
+//
+// This function returns a non-zero value if the callee is the primary core.
+// Primary core is the core responsible to initialize hardware and run UEFI.
+// This function might be called from assembler before any stack is set.
+//
+// @return Return a non-zero value if the callee is the primary core.
+//
+//UINTN
+//ArmPlatformIsPrimaryCore (
+// IN UINTN MpId
+// );
+ASM_PFX(ArmPlatformIsPrimaryCore):
+ MOV32 (w1, FixedPcdGet32(PcdArmPrimaryCoreMask))
+ and x0, x0, x1
+ MOV32 (w1, FixedPcdGet32(PcdArmPrimaryCore))
+ cmp w0, w1
+ cset x0, eq
+ ret
--
2.17.1


Re: [edk2-platforms][Patch] Maintainers.txt: Update Vlv2* and Quark* reviewers

Nate DeSimone
 

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Thursday, April 8, 2021 10:11 AM
To: devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Subject: [edk2-platforms][Patch] Maintainers.txt: Update Vlv2* and Quark*
reviewers

Add Nate DeSimone as a reviewer for the following packages:
* Silicon/Intel/Vlv2DeviceRefCodePkg
* Platform/Intel/Vlv2TbltDevicePkg
* Silicon/Intel/QuarkSocPkg
* Platform/Intel/QuarkPlatformPkg

Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
Maintainers.txt | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/Maintainers.txt b/Maintainers.txt index 4c39be8d7f..f6313b430c
100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -166,11 +166,13 @@ Platform/Intel/QuarkPlatformPkg
F: Platform/Intel/QuarkPlatformPkg/
M: Michael D Kinney <michael.d.kinney@intel.com>
M: Kelly Steele <kelly.steele@intel.com>
+R: Nate DeSimone <nathaniel.l.desimone@intel.com>

Platform/Intel/Vlv2TbltDevicePkg
F: Platform/Intel/Vlv2TbltDevicePkg/
M: Zailiang Sun <zailiang.sun@intel.com>
M: Yi Qian <yi.qian@intel.com>
+R: Nate DeSimone <nathaniel.l.desimone@intel.com>

Platform/Intel/BoardModulePkg
F: Platform/Intel/BoardModulePkg/
@@ -243,11 +245,13 @@ Silicon/Intel/QuarkSocPkg
F: Silicon/Intel/QuarkSocPkg/
M: Michael D Kinney <michael.d.kinney@intel.com>
M: Kelly Steele <kelly.steele@intel.com>
+R: Nate DeSimone <nathaniel.l.desimone@intel.com>

Silicon/Intel/Vlv2DeviceRefCodePkg
F: Silicon/Intel/Vlv2DeviceRefCodePkg/
M: Zailiang Sun <zailiang.sun@intel.com>
M: Yi Qian <yi.qian@intel.com>
+R: Nate DeSimone <nathaniel.l.desimone@intel.com>

Silicon/Intel/CoffeelakeSiliconPkg
F: Silicon/Intel/CoffeelakeSiliconPkg/
--
2.31.1.windows.1


Re: [GSoC proposal] Secure Image Loader

Andrew Fish
 

On Apr 8, 2021, at 10:02 AM, Marvin Häuser <mhaeuser@posteo.de> wrote:

On 08.04.21 18:44, Andrew Fish via groups.io wrote:


On Apr 8, 2021, at 9:06 AM, Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>> wrote:

We use the loader code in userspace anyway for fuzzing and such. I also want to build a database of all sorts of UEFI binaries some time before the merge to confirm they are all accepted (Windows / macOS / Linux bootloaders, tools like memtest, drivers like iPXE). As part of that, I'm sure we can have a userspace tool that uses the code to emit parsing information.

But as the EDK II build system is very... not so userspace friendly, I will not promise it will be very nice. :)
Marvin,

The BaseTools can easily build C command line tools that are cross platform?

Actually GenFw [1] already does a lot of PE/COFF magic, so it should be relatively easy to add a -I, —info, and dump out an overview of a PE/COFF image, and make comments on things that are not secure. It would also probably be useful to dump out information about the Debug Directory entries, His sections, etc. for general debug.
I did not look at the code much, but I do know that BaseTools duplicates the PE/COFF code from MdePkg. Whether it was changed or not I cannot tell.
GenFw does the ELF to PE/COFF conversion, zeroing out Debug Directory Entries etc. so it should be correct. It is not like the PE/COFF spec is a moving target.

Thanks,

Andrew Fish

Best regards,
Marvin


[1] https://github.com/tianocore/edk2/tree/master/BaseTools/Source/C/GenFw <https://github.com/tianocore/edk2/tree/master/BaseTools/Source/C/GenFw>
/Volumes/Case/edk2-github(eng/PR-557-XcodeResourceSections)>. edksetup.sh
Loading previous configuration from /Volumes/Case/edk2-github/Conf/BuildEnv.sh
WORKSPACE: /Volumes/Case/edk2-github
EDK_TOOLS_PATH: /Volumes/Case/edk2-github/BaseTools
CONF_PATH: /Volumes/Case/edk2-github/Conf
/Volumes/Case/edk2-github(eng/PR-557-XcodeResourceSections)>GenFw -h
GenFw Version 0.2 Developer Build based on Revision: Unknown

Usage: GenFw [options] <input_file>

Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.

Options:
-o FileName, --outputfile FileName
File will be created to store the output content.
-e EFI_FILETYPE, --efiImage EFI_FILETYPE
Create Efi Image. EFI_FILETYPE is one of BASE,SMM_CORE,
PEI_CORE, PEIM, DXE_CORE, DXE_DRIVER, UEFI_APPLICATION,
SEC, DXE_SAL_DRIVER, UEFI_DRIVER, DXE_RUNTIME_DRIVER,
DXE_SMM_DRIVER, SECURITY_CORE, COMBINED_PEIM_DRIVER,
MM_STANDALONE, MM_CORE_STANDALONE,
PIC_PEIM, RELOCATABLE_PEIM, BS_DRIVER, RT_DRIVER,
APPLICATION, SAL_RT_DRIVER to support all module types
It can only be used together with --keepexceptiontable,
--keepzeropending, --keepoptionalheader, -r, -o option.
It is a action option. If it is combined with other action options,
the later input action option will override the previous one.
-c, --acpi Create Acpi table.
It can't be combined with other action options
except for -o, -r option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
-t, --terse Create Te Image.
It can only be used together with --keepexceptiontable,
--keepzeropending, --keepoptionalheader, -r, -o option.
It is a action option. If it is combined with other action options,
the later input action option will override the previous one.
-u, --dump Dump TeImage Header.
It can't be combined with other action options
except for -o, -r option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
-z, --zero Zero the Debug Data Fields in the PE input image file.
It also zeros the time stamp fields.
This option can be used to compare the binary efi image.
It can't be combined with other action options
except for -o, -r option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
-b, --exe2bin Convert the input EXE to the output BIN file.
It can't be combined with other action options
except for -o, -r option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
-l, --stripped Strip off the relocation info from PE or TE image.
It can't be combined with other action options
except for -o, -r option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
-s timedate, --stamp timedate
timedate format is "yyyy-mm-dd 00:00:00". if timedata
is set to NOW, current system time is used. The support
date scope is 1970-01-01 00+timezone:00:00
~ 2038-01-19 03+timezone:14:07
The scope is adjusted according to the different zones.
It can't be combined with other action options
except for -o, -r option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
-m, --mcifile Convert input microcode txt file to microcode bin file.
It can't be combined with other action options
except for -o option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
-j, --join Combine multi microcode bin files to one file.
It can be specified with -a, -p, -o option.
No other options can be combined with it.
If it is combined with other action options, the later
input action option will override the previous one.
-a NUM, --align NUM NUM is one HEX or DEC format alignment value.
This option is only used together with -j option.
-p NUM, --pad NUM NUM is one HEX or DEC format padding value.
This option is only used together with -j option.
--keepexceptiontable Don't clear exception table.
This option can be used together with -e or -t.
It doesn't work for other options.
--keepoptionalheader Don't zero PE/COFF optional header fields.
This option can be used together with -e or -t.
It doesn't work for other options.
--keepzeropending Don't strip zero pending of .reloc.
This option can be used together with -e or -t.
It doesn't work for other options.
-r, --replace Overwrite the input file with the output content.
If more input files are specified,
the last input file will be as the output file.
-g HiiPackageListGuid, --hiiguid HiiPackageListGuid
Guid is used to specify hii package list guid.
Its format is xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
If not specified, the first Form FormSet guid is used.
--hiipackage Combine all input binary hii packages into
a single package list as the text resource data(RC).
It can't be combined with other action options
except for -o option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
--hiibinpackage Combine all input binary hii packages into
a single package list as the binary resource section.
It can't be combined with other action options
except for -o option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
--rc FlieName Append a Hii resource section to the
last PE/COFF section. The FileName is the resource section to append
If FileName does not exist this operation is skipped. This feature is
only intended for toolchains, like XCODE, that don't suport $(RC).
This option can only be combined with -e
--rebase NewAddress Rebase image to new base address. New address
is also set to the first none code section header.
It can't be combined with other action options
except for -o or -r option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
--address NewAddress Set new address into the first none code
section header of the input image.
It can't be combined with other action options
except for -o or -r option. It is a action option.
If it is combined with other action options, the later
input action option will override the previous one.
-v, --verbose Turn on verbose output with informational messages.
-q, --quiet Disable all messages except key message and fatal error
-d, --debug level Enable debug messages, at input debug level.
--version Show program's version number and exit
-h, --help Show this help message and exit

Thanks,

Andrew Fish

Best regards,
Marvin

On 08.04.21 16:13, Andrew (EFI) Fish wrote:
At a minimum it would be nice if we had a tool that would point out the security faults with a given PE/COFF file layout.



On Apr 8, 2021, at 4:16 AM, Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> wrote:

On 04/06/21 12:06, Marvin Häuser wrote:
Good day Nate,

Comments are inline.

Best regards,
Marvin

On 06.04.21 11:41, Nate DeSimone wrote:
Hi Marvin,

Great to meet you and welcome back! Glad you hear you are interested!
Completing a formal verification of a PE/COFF loader is certainly
impressive. Was this done with some sort of automated theorem proving?
It would seem a rather arduous task doing an inductive proof for an
algorithm like that by hand!
I would call it "semi-automated", a great deal of intermediate goals
(preconditions, postconditions, invariants, assertions, ...) were
required to show all interesting properties. But yes, the actual proof
steps are automated by common SMT solvers. It was done using the
AstraVer Toolset and ACSL, latter basically a language to express logic
statements with C-like syntax.

I completely agree with you that getting a formally verified PE/COFF
loader into mainline is undoubtably valuable and would pay security
dividends for years to come.
I'm glad to hear that. :)

Admittedly, this is an area of computer science that I don't have a
great deal of experience with. The furthest I have gone on this topic
is writing out proofs for simple algorithms on exams in my Algorithms
class in college. Regardless you have a much better idea of what the
current status is of the work that you and Vitaly have done. I guess
my only question is do you think there is sufficient work remaining to
fill the 10 week GSoC development window?
Please don't get me wrong, but I would be surprised if the UEFI
specification changes I'd like to discuss alone would be completed
within 10 weeks, let alone implementation throughout the codebase. While
I think the plain amount of code may be a bit less than say a
MinPlatform port, the changes are much deeper and require much more
caution to avoid regressions (e.g. by invalidating undocumented
assertions). This sadly is not a matter of just replacing the underlying
library implementation or "plug-in and play" at all. It furthermore
affects many parts of the stack, the core dispatchers used for all
platforms, image emulation (EBC), UEFI userland emulation (EmuPkg), and
so on. I was rather worried the scope is too broad time-wise, but it can
be narrowed/widened as you see fit really. This is one of *the* core
components used on millions of device, and many package maintainers need
to review and validate the changes, this must really be done right the
first try. :)

Certainly we can use some of that time to perform the code reviews you
mention and write up formal ECRs for the UEFI spec changes that you
believe are needed.
I believed that was part of the workload, yes, but even without it I
think there is plenty to do.

Thank you for sending the application and alerting us to the great
work you and Vitaly have done! I'll read your paper more closely and
come back with any questions I still have.
Thank you, I will gladly explain anything unclear. Just try to not give
Laszlo too many flashbacks. :)
I haven't commented yet in this thread, as I thought my stance on this
undertaking was (or should be) obvious.

I very much welcome a replacement for the PE/COFF parser (as I consider
its security issues unfixable in an incremental manner). From my reading
of Marvin's and Vitaly's paper (draft), they have my full trust, and I'm
ready to put their upcoming code to use in ArmVirtPkg and OvmfPkg with
minimal actual code review. If fixing the pervasive security problems
around this area cannot avoid spiraling out to other core code in edk2,
such as dispatchers, and even to the PI / UEFI specs, so be it.

Regarding GSoC itself: as I stated elsewhere previously, I support
edk2's participation in GSoC, while at the same time I'm not
volunteering for mentorship at all. I'm uncertain if GSoC is the best
framework for upstreaming such a large undertaking, but if it can help,
we should use it as much as possible.

Thanks
Laszlo





With Best Regards,
Nate

-----Original Message-----
From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Marvin
Häuser
Sent: Sunday, April 4, 2021 4:02 PM
To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Andrew Fish
<afish@apple.com <mailto:afish@apple.com>>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
Subject: [edk2-devel] [GSoC proposal] Secure Image Loader

Good day everyone,

I'll keep the introduction brief because I've been around for a while
now. :) I'm
Marvin Häuser, a third-year Computer Science student from TU
Kaiserslautern,
Germany. Late last year, my colleague Vitaly from ISP RAS and me
introduced a
formally verified Image Loader for UEFI usage at ISP RAS Open[1] due
to various
defects we outlined in the corresponding paper. Thank you once again
Laszlo
for your *incredible* review work on the publication part.

I now want to make an effort to mainline it, preferably as part of
the current
Google Summer of Code event. To be clear, my internship at ISP RAS has
concluded, and while Vitaly will be available for design discussion,
he has other
priorities at the moment and the practical part will be on me. I have
previously
submitted a proposal via the GSoC website for your review.

There are many things to consider:
1. The Image Loader is a core component, and there needs to be a
significant
level of quality and security assurance.
2. Being consumed by many packages, the proposed patch set will take
a lot of
time to review and integrate.
3. During my initial exploration, I discovered defective PPIs and
protocols (e.g.
returning data with no corresponding size) originating from the UEFI
PI and
UEFI specifications. Changes need to be discussed, settled on, and
submitted to
the UEFI Forum.
4. Some UEFI APIs like the Security Architecture protocols are
inconveniently
abstract, see 5.
5. Some of the current code does not use the existing context, or
accesses it
outside of the exposed APIs. The control flow of the dispatchers may
need to be
adapted to make the context available to appropriate APIs.

But obviously there are not only unpleasant considerations:
A. The Image Loader is mostly formally verified, and only very few
changes will
be required from the last proven state. This gives a lot of trust in
its correctness
and safety.
B. All outlined defects that are of critical nature have been fixed
successfully.
C. The Image Loader has been tested with real-world code loading
real-world
OSes on thousands of machines in the past few months, including
rejecting
malformed images (configurable by PCD).
D. The new APIs will centralise everything PE, reducing code
duplication and
potentially unsafe operations.
E. Centralising and reduced parse duplication may improve overall boot
performance.
F. The code has been coverage-tested to not contain dead code.
G. The code has been fuzz-tested including sanitizers to not invoke
undefined
behaviour.
H. I already managed to identify a malformed image in OVMF with its help
(incorrectly reported section alignment of an Intel IPXE driver). A
fix will be
submitted shortly.
I. I plan to support PE section permissions, allowing for read-only data
segments when enabled.

There are likely more points for both lists, but I hope this gives a
decent
starting point for discussion. What are your thoughts on the matter?
I strongly
encourage everyone to read the section regarding defects of our
publication[2]
to better understand the motivation. The vague points above can of
course be
elaborated in due time, as you see fit.

Thank you for your time!

Best regards,
Marvin


[1] https://github.com/mhaeuser/ISPRASOpen-SecurePE <https://github.com/mhaeuser/ISPRASOpen-SecurePE>
[2] https://arxiv.org/pdf/2012.05471.pdf <https://arxiv.org/pdf/2012.05471.pdf>









Re: MinPlatform Board port (GSoC 2021)

Benjamin Doron
 

On Tue, Apr 6, 2021 at 04:54 AM, Nate DeSimone wrote:
...but this might not be relevant to anyone after all. :-)

Is that how this normally works? Who would organise it? (It sounds needlessly expensive, for someone... *shrug*)
I think it is a little harsh to say this is not a relevant project. Sure, not many people have an exact Acer Aspire VN7-572G. In fact, the large OEMs like Acer on average produce 60 new laptop designs every 6 months. So yes it is true that keeping up with every single laptop design out there in the wild isn't really feasible. However, the important thing to keep in mind is that while there may be ~100 Acer laptop designs with Sky Lake, they are all mostly identical. They differ in screen sizes, keyboards, colors, amount of RAM, etc. but they are all built on common templates. A typical OEM will make maybe 1-5 motherboard designs for a given chipset (say Sky Lake-U) and then ship a ton of minor variations. Sometimes those variations will show up with different brand names on the lid. The important thing to keep in mind is that while you may be currently focused on the VN7-572G, it is extremely likely that your work would be 95% of what is needed to get 30-50% of Acer's Sky Lake laptops working.

To be completely honest with you, this is the first time I have done GSoC as well so this is a learning experience for me as well. I'll look into the feasibility of acquiring a second VN7-572G.
Sorry, I meant that the specifics of flashing might not be relevant if no TianoCore mentor/developer will have one of these boards.

Regarding the various board models produced per generation, I see your point. For instance, the schematics and boardview I have for this board apply to all VN7-572 models (VN7-572, VN7-572G, VN7-572TG) and could somewhat describe the so-called "Black Edition" (VN7-592 boards). However, those have a Skylake PCH-H, so perhaps some GPIO/HSIO/memory configuration differs.

If anyone wants to know, the specific model I have is the Aspire VN7-572G-72NB.
Thanks for these pointers and references, I will look at them more.

As I understand it, the objective of dispatch mode was to remove code duplication in flash and possibly save boot time by minimising phase transitions. But it makes the PeiCore module non-updateable. From a board initialisation perspective, they're probably the same, so this can be figured out later.
You are talking to the guy that invented FSP dispatch mode, so I can definitively say that yes those were all goals for dispatch mode. However, by far the biggest goal was to make FSP integrate nicely with EDK II based firmware. There are two ways to use dispatch mode actually. The first way as you mention uses the PeiCore included with FSP-M. The other method uses a PeiCore provided by the platform firmware. This method is described in Section 7.2.3 - "Alternate Boot Flow Description" of the FSP v2.2 specification. While it does result in 2 copies of PeiCore, it keeps PeiCore updatable. It is still better than API mode because even though there are 2 copies of PeiCore, only 1 of them is actually used. So you only have a single instance of PEI at runtime, whereas in API mode you have two separate instances of PEI in memory at runtime. My understanding is that most OEMs choose to use FSP dispatch mode in this way. From a board initialization perspective, they are mostly the same but there are some differences. Specifically, in dispatch mode one does not use FSP UPDs at all. Instead, one passes the native configuration policy data structures that FSP uses internally, in the case of Kaby Lake that is Config Blocks stored inside a PPI.
Okay, nice! I suppose within EDK2, dispatch mode is more intuitive/natural. In other system firmware, well, the "Firmware Support Package" still has API mode, so it's not really my concern.

I've shared a draft of the project proposal, feel free to take a look at it at your convenience. I'd appreciate any feedback.

Best regards,
Benjamin Doron


Re: [edk2-platforms] [PATCH v5 0/4] Add Large Variable Libraries

Oram, Isaac W
 

Series Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>

-----Original Message-----
From: Chiu, Chasel <chasel.chiu@intel.com>
Sent: Wednesday, April 7, 2021 11:28 PM
To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>; Michael Kubacki <michael.kubacki@microsoft.com>; Oram, Isaac W <isaac.w.oram@intel.com>
Subject: RE: [edk2-platforms] [PATCH v5 0/4] Add Large Variable Libraries


Reviewed-by: Chasel Chiu <chasel.chiu@intel.com>

-----Original Message-----
From: Nate DeSimone <nathaniel.l.desimone@intel.com>
Sent: Thursday, April 8, 2021 3:17 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Liming Gao
<gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>; Michael
Kubacki <michael.kubacki@microsoft.com>; Oram, Isaac W
<isaac.w.oram@intel.com>
Subject: [edk2-platforms] [PATCH v5 0/4] Add Large Variable Libraries

Changes from V4:
- Added LibraryClass interface definitions to
MinPlatformPkg.dec

Changes from V3:
- Added header guards
- Documented the possibility of returning EFI_UNSUPPORTED
- Added MM_CORE_STANDALONE to the StandaloneMm library
implementations
- Documented library constructor return status
- Moved LargeVariableReadLib and LargeVariableWriteLib into
a single BaseLargeVariableLib folder
- Added LargeVariableCommon.h for shared macro definitions
- Converted some debug macros from DEBUG_INFO to DEBUG_VERBOSE

Changes from V2:
- Added comment to LargeVariableLib INF and header files
describing the usage for drivers that cannot assume that
PcdMaxVariableSize has been set to a certain minimum value.

Changes from V1:
- Changed prefix from "Min" to "VarLib"
- Better comments
- Added more whitespace for readability
- Removed unused INF sections
- Better debug messages

This patch series introduces libaries that enable large data sets to
be stored using the UEFI Variable Services. At present, most UEFI
Variable Services implementations have a maximum variable size of
<=64KB. The exact value varies depending on platform.

These libaries enable a data set to use as much space as needed, up to
the remaining space in the UEFI Variable non-volatile storage.

To implement this, I have broken the problem down into two parts:

1. Phase angostic UEFI Variable access.
2. Storage of data across multiple UEFI Variables.

For the first part, I have created two new LibraryClasses:
VariableReadLib and VariableWriteLib. I have provided implementation
instances of VariableReadLib for PEI, DXE, and SMM.
For VariableWriteLib, I have provided implementation instances for DXE
and SMM. This enables code that accesses UEFI variables to be written
in a matter than is phase agnostic, so the same code can be used in
PEI, DXE, or SMM without modification.

The second part involves another two new LibaryClasses:
LargeVariableReadLib and LargeVariableWriteLib. Only one BASE
implementation is needed for both of these as the phase dependent code
was seperated out in the first piece. These libraries provide logic to
calculate the maximum size of an individual UEFI variable and split
the data into as many smaller pieces as needed to store the entire data set in the UEFI Variable storage.
They also provide the ability to stitch the data back together when it is read.
Deleting the data will delete all variables used to store it.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

Nate DeSimone (4):
MinPlatformPkg: Add VariableReadLib
MinPlatformPkg: Add VariableWriteLib
MinPlatformPkg: Add LargeVariableReadLib
MinPlatformPkg: Add LargeVariableWriteLib

.../Include/Dsc/CoreCommonLib.dsc | 6 +-
.../MinPlatformPkg/Include/Dsc/CoreDxeLib.dsc | 12 +-
.../MinPlatformPkg/Include/Dsc/CorePeiLib.dsc | 9 +-
.../Include/Library/LargeVariableReadLib.h | 61 +++
.../Include/Library/LargeVariableWriteLib.h | 69 +++
.../Include/Library/VariableReadLib.h | 94 ++++
.../Include/Library/VariableWriteLib.h | 138 ++++++
.../BaseLargeVariableReadLib.inf | 51 ++
.../BaseLargeVariableWriteLib.inf | 51 ++
.../LargeVariableCommon.h | 47 ++
.../LargeVariableReadLib.c | 176 +++++++
.../LargeVariableWriteLib.c | 450 ++++++++++++++++++
.../DxeRuntimeVariableReadLib.c | 117 +++++
.../DxeRuntimeVariableReadLib.inf | 41 ++
.../DxeRuntimeVariableWriteLib.c | 265 +++++++++++
.../DxeRuntimeVariableWriteLib.inf | 49 ++
.../PeiVariableReadLib/PeiVariableReadLib.c | 155 ++++++
.../PeiVariableReadLib/PeiVariableReadLib.inf | 42 ++
.../SmmVariableReadCommon.c | 116 +++++
.../StandaloneMmVariableReadLib.inf | 50 ++
.../StandaloneMmVariableReadLibConstructor.c | 51 ++
.../TraditionalMmVariableReadLib.inf | 49 ++
.../TraditionalMmVariableReadLibConstructor.c | 51 ++
.../SmmVariableWriteCommon.c | 171 +++++++
.../StandaloneMmVariableWriteLib.inf | 45 ++
.../StandaloneMmVariableWriteLibConstructor.c | 51 ++
.../TraditionalMmVariableWriteLib.inf | 44 ++
...TraditionalMmVariableWriteLibConstructor.c | 51 ++
.../Intel/MinPlatformPkg/MinPlatformPkg.dec | 5 +
.../Intel/MinPlatformPkg/MinPlatformPkg.dsc | 4 +-
30 files changed, 2511 insertions(+), 10 deletions(-) create mode
100644
Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableReadLib.h
create mode 100644
Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableWriteLib.h
create mode 100644
Platform/Intel/MinPlatformPkg/Include/Library/VariableReadLib.h
create mode 100644
Platform/Intel/MinPlatformPkg/Include/Library/VariableWriteLib.h
create mode 100644
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVa
riabl
eReadLib.inf
create mode 100644
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVa
riabl
eWriteLib.inf
create mode 100644
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariab
leCo
mmon.h
create mode 100644
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariab
leRea
dLib.c
create mode 100644
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariab
leWri
teLib.c
create mode 100644
Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableReadLib/DxeRun
tim
eVariableReadLib.c
create mode 100644
Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableReadLib/DxeRun
tim
eVariableReadLib.inf
create mode 100644
Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRu
nti
meVariableWriteLib.c
create mode 100644
Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRu
nti
meVariableWriteLib.inf
create mode 100644
Platform/Intel/MinPlatformPkg/Library/PeiVariableReadLib/PeiVariableReadLib.
c
create mode 100644
Platform/Intel/MinPlatformPkg/Library/PeiVariableReadLib/PeiVariableReadLib.
inf
create mode 100644
Platform/Intel/MinPlatformPkg/Library/SmmVariableReadLib/SmmVariableRe
ad
Common.c
create mode 100644
Platform/Intel/MinPlatformPkg/Library/SmmVariableReadLib/StandaloneMmV
a
riableReadLib.inf
create mode 100644
Platform/Intel/MinPlatformPkg/Library/SmmVariableReadLib/StandaloneMmV
a
riableReadLibConstructor.c
create mode 100644
Platform/Intel/MinPlatformPkg/Library/SmmVariableReadLib/TraditionalMm
Var
iableReadLib.inf
create mode 100644
Platform/Intel/MinPlatformPkg/Library/SmmVariableReadLib/TraditionalMm
Var
iableReadLibConstructor.c
create mode 100644
Platform/Intel/MinPlatformPkg/Library/SmmVariableWriteLib/SmmVariableW
rit
eCommon.c
create mode 100644
Platform/Intel/MinPlatformPkg/Library/SmmVariableWriteLib/StandaloneMm
Va
riableWriteLib.inf
create mode 100644
Platform/Intel/MinPlatformPkg/Library/SmmVariableWriteLib/StandaloneMm
Va
riableWriteLibConstructor.c
create mode 100644
Platform/Intel/MinPlatformPkg/Library/SmmVariableWriteLib/TraditionalM
mVa
riableWriteLib.inf
create mode 100644
Platform/Intel/MinPlatformPkg/Library/SmmVariableWriteLib/TraditionalM
mVa
riableWriteLibConstructor.c

--
2.27.0.windows.1


[edk2-platforms][Patch] Maintainers.txt: Update Vlv2* and Quark* reviewers

Michael D Kinney
 

Add Nate DeSimone as a reviewer for the following packages:
* Silicon/Intel/Vlv2DeviceRefCodePkg
* Platform/Intel/Vlv2TbltDevicePkg
* Silicon/Intel/QuarkSocPkg
* Platform/Intel/QuarkPlatformPkg

Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
Maintainers.txt | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index 4c39be8d7f..f6313b430c 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -166,11 +166,13 @@ Platform/Intel/QuarkPlatformPkg
F: Platform/Intel/QuarkPlatformPkg/
M: Michael D Kinney <michael.d.kinney@intel.com>
M: Kelly Steele <kelly.steele@intel.com>
+R: Nate DeSimone <nathaniel.l.desimone@intel.com>

Platform/Intel/Vlv2TbltDevicePkg
F: Platform/Intel/Vlv2TbltDevicePkg/
M: Zailiang Sun <zailiang.sun@intel.com>
M: Yi Qian <yi.qian@intel.com>
+R: Nate DeSimone <nathaniel.l.desimone@intel.com>

Platform/Intel/BoardModulePkg
F: Platform/Intel/BoardModulePkg/
@@ -243,11 +245,13 @@ Silicon/Intel/QuarkSocPkg
F: Silicon/Intel/QuarkSocPkg/
M: Michael D Kinney <michael.d.kinney@intel.com>
M: Kelly Steele <kelly.steele@intel.com>
+R: Nate DeSimone <nathaniel.l.desimone@intel.com>

Silicon/Intel/Vlv2DeviceRefCodePkg
F: Silicon/Intel/Vlv2DeviceRefCodePkg/
M: Zailiang Sun <zailiang.sun@intel.com>
M: Yi Qian <yi.qian@intel.com>
+R: Nate DeSimone <nathaniel.l.desimone@intel.com>

Silicon/Intel/CoffeelakeSiliconPkg
F: Silicon/Intel/CoffeelakeSiliconPkg/
--
2.31.1.windows.1


Re: [GSoC proposal] Secure Image Loader

Marvin Häuser
 

On 08.04.21 18:44, Andrew Fish via groups.io wrote:


On Apr 8, 2021, at 9:06 AM, Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>> wrote:

We use the loader code in userspace anyway for fuzzing and such. I also want to build a database of all sorts of UEFI binaries some time before the merge to confirm they are all accepted (Windows / macOS / Linux bootloaders, tools like memtest, drivers like iPXE). As part of that, I'm sure we can have a userspace tool that uses the code to emit parsing information.

But as the EDK II build system is very... not so userspace friendly, I will not promise it will be very nice. :)
Marvin,

The BaseTools can easily build C command line tools that are cross platform?

Actually GenFw [1] already does a lot of PE/COFF magic, so it should be relatively easy to add a -I, —info, and dump out an overview of a PE/COFF image, and make comments on things that are not secure. It would also probably be useful to dump out information about the Debug Directory entries, His sections, etc. for general debug.
I did not look at the code much, but I do know that BaseTools duplicates the PE/COFF code from MdePkg. Whether it was changed or not I cannot tell.

Best regards,
Marvin


[1] https://github.com/tianocore/edk2/tree/master/BaseTools/Source/C/GenFw <https://github.com/tianocore/edk2/tree/master/BaseTools/Source/C/GenFw>
/Volumes/Case/edk2-github(eng/PR-557-XcodeResourceSections)>. edksetup.sh
Loading previous configuration from /Volumes/Case/edk2-github/Conf/BuildEnv.sh
WORKSPACE: /Volumes/Case/edk2-github
EDK_TOOLS_PATH: /Volumes/Case/edk2-github/BaseTools
CONF_PATH: /Volumes/Case/edk2-github/Conf
/Volumes/Case/edk2-github(eng/PR-557-XcodeResourceSections)>GenFw -h
GenFw Version 0.2 Developer Build based on Revision: Unknown

Usage: GenFw [options] <input_file>

Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.

Options:
  -o FileName, --outputfile FileName
                        File will be created to store the output content.
  -e EFI_FILETYPE, --efiImage EFI_FILETYPE
                        Create Efi Image. EFI_FILETYPE is one of BASE,SMM_CORE,
                        PEI_CORE, PEIM, DXE_CORE, DXE_DRIVER, UEFI_APPLICATION,
                        SEC, DXE_SAL_DRIVER, UEFI_DRIVER, DXE_RUNTIME_DRIVER,
                        DXE_SMM_DRIVER, SECURITY_CORE, COMBINED_PEIM_DRIVER,
                        MM_STANDALONE, MM_CORE_STANDALONE,
                        PIC_PEIM, RELOCATABLE_PEIM, BS_DRIVER, RT_DRIVER,
                        APPLICATION, SAL_RT_DRIVER to support all module types
                        It can only be used together with --keepexceptiontable,
                        --keepzeropending, --keepoptionalheader, -r, -o option.
                        It is a action option. If it is combined with other action options,
                        the later input action option will override the previous one.
  -c, --acpi            Create Acpi table.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -t, --terse           Create Te Image.
                        It can only be used together with --keepexceptiontable,
                        --keepzeropending, --keepoptionalheader, -r, -o option.
                        It is a action option. If it is combined with other action options,
                        the later input action option will override the previous one.
  -u, --dump            Dump TeImage Header.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -z, --zero            Zero the Debug Data Fields in the PE input image file.
                        It also zeros the time stamp fields.
                        This option can be used to compare the binary efi image.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -b, --exe2bin         Convert the input EXE to the output BIN file.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -l, --stripped        Strip off the relocation info from PE or TE image.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -s timedate, --stamp timedate
                        timedate format is "yyyy-mm-dd 00:00:00". if timedata
                        is set to NOW, current system time is used. The support
                        date scope is 1970-01-01 00+timezone:00:00
                        ~ 2038-01-19 03+timezone:14:07
                        The scope is adjusted according to the different zones.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -m, --mcifile         Convert input microcode txt file to microcode bin file.
                        It can't be combined with other action options
                        except for -o option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -j, --join            Combine multi microcode bin files to one file.
                        It can be specified with -a, -p, -o option.
                        No other options can be combined with it.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -a NUM, --align NUM   NUM is one HEX or DEC format alignment value.
                        This option is only used together with -j option.
  -p NUM, --pad NUM     NUM is one HEX or DEC format padding value.
                        This option is only used together with -j option.
  --keepexceptiontable  Don't clear exception table.
                        This option can be used together with -e or -t.
                        It doesn't work for other options.
  --keepoptionalheader  Don't zero PE/COFF optional header fields.
                        This option can be used together with -e or -t.
                        It doesn't work for other options.
  --keepzeropending     Don't strip zero pending of .reloc.
                        This option can be used together with -e or -t.
                        It doesn't work for other options.
  -r, --replace         Overwrite the input file with the output content.
                        If more input files are specified,
                        the last input file will be as the output file.
  -g HiiPackageListGuid, --hiiguid HiiPackageListGuid
                        Guid is used to specify hii package list guid.
                        Its format is xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
                        If not specified, the first Form FormSet guid is used.
  --hiipackage          Combine all input binary hii packages into
                        a single package list as the text resource data(RC).
                        It can't be combined with other action options
                        except for -o option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  --hiibinpackage       Combine all input binary hii packages into
                        a single package list as the binary resource section.
                        It can't be combined with other action options
                        except for -o option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  --rc FlieName         Append a Hii resource section to the
                        last PE/COFF section. The FileName is the resource section to append
                        If FileName does not exist this operation is skipped. This feature is
                        only intended for toolchains, like XCODE, that don't suport $(RC).
                        This option can only be combined with -e
  --rebase NewAddress   Rebase image to new base address. New address
                        is also set to the first none code section header.
                        It can't be combined with other action options
                        except for -o or -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  --address NewAddress  Set new address into the first none code
                        section header of the input image.
                        It can't be combined with other action options
                        except for -o or -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -v, --verbose         Turn on verbose output with informational messages.
  -q, --quiet           Disable all messages except key message and fatal error
  -d, --debug level     Enable debug messages, at input debug level.
  --version             Show program's version number and exit
  -h, --help            Show this help message and exit

Thanks,

Andrew Fish

Best regards,
Marvin

On 08.04.21 16:13, Andrew (EFI) Fish wrote:
At a minimum it would be nice if we had a tool that would point out the security faults with a given PE/COFF file layout.



On Apr 8, 2021, at 4:16 AM, Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> wrote:

On 04/06/21 12:06, Marvin Häuser wrote:
Good day Nate,

Comments are inline.

Best regards,
Marvin

On 06.04.21 11:41, Nate DeSimone wrote:
Hi Marvin,

Great to meet you and welcome back! Glad you hear you are interested!
Completing a formal verification of a PE/COFF loader is certainly
impressive. Was this done with some sort of automated theorem proving?
It would seem a rather arduous task doing an inductive proof for an
algorithm like that by hand!
I would call it "semi-automated", a great deal of intermediate goals
(preconditions, postconditions, invariants, assertions, ...) were
required to show all interesting properties. But yes, the actual proof
steps are automated by common SMT solvers. It was done using the
AstraVer Toolset and ACSL, latter basically a language to express logic
statements with C-like syntax.

I completely agree with you that getting a formally verified PE/COFF
loader into mainline is undoubtably valuable and would pay security
dividends for years to come.
I'm glad to hear that. :)

Admittedly, this is an area of computer science that I don't have a
great deal of experience with. The furthest I have gone on this topic
is writing out proofs for simple algorithms on exams in my Algorithms
class in college. Regardless you have a much better idea of what the
current status is of the work that you and Vitaly have done. I guess
my only question is do you think there is sufficient work remaining to
fill the 10 week GSoC development window?
Please don't get me wrong, but I would be surprised if the UEFI
specification changes I'd like to discuss alone would be completed
within 10 weeks, let alone implementation throughout the codebase. While
I think the plain amount of code may be a bit less than say a
MinPlatform port, the changes are much deeper and require much more
caution to avoid regressions (e.g. by invalidating undocumented
assertions). This sadly is not a matter of just replacing the underlying
library implementation or "plug-in and play" at all. It furthermore
affects many parts of the stack, the core dispatchers used for all
platforms, image emulation (EBC), UEFI userland emulation (EmuPkg), and
so on. I was rather worried the scope is too broad time-wise, but it can
be narrowed/widened as you see fit really. This is one of *the* core
components used on millions of device, and many package maintainers need
to review and validate the changes, this must really be done right the
first try. :)

Certainly we can use some of that time to perform the code reviews you
mention and write up formal ECRs for the UEFI spec changes that you
believe are needed.
I believed that was part of the workload, yes, but even without it I
think there is plenty to do.

Thank you for sending the application and alerting us to the great
work you and Vitaly have done! I'll read your paper more closely and
come back with any questions I still have.
Thank you, I will gladly explain anything unclear. Just try to not give
Laszlo too many flashbacks. :)
I haven't commented yet in this thread, as I thought my stance on this
undertaking was (or should be) obvious.

I very much welcome a replacement for the PE/COFF parser (as I consider
its security issues unfixable in an incremental manner). From my reading
of Marvin's and Vitaly's paper (draft), they have my full trust, and I'm
ready to put their upcoming code to use in ArmVirtPkg and OvmfPkg with
minimal actual code review. If fixing the pervasive security problems
around this area cannot avoid spiraling out to other core code in edk2,
such as dispatchers, and even to the PI / UEFI specs, so be it.

Regarding GSoC itself: as I stated elsewhere previously, I support
edk2's participation in GSoC, while at the same time I'm not
volunteering for mentorship at all. I'm uncertain if GSoC is the best
framework for upstreaming such a large undertaking, but if it can help,
we should use it as much as possible.

Thanks
Laszlo





With Best Regards,
Nate

-----Original Message-----
From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Marvin
Häuser
Sent: Sunday, April 4, 2021 4:02 PM
To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Andrew Fish
<afish@apple.com <mailto:afish@apple.com>>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
Subject: [edk2-devel] [GSoC proposal] Secure Image Loader

Good day everyone,

I'll keep the introduction brief because I've been around for a while
now. :) I'm
Marvin Häuser, a third-year Computer Science student from TU
Kaiserslautern,
Germany. Late last year, my colleague Vitaly from ISP RAS and me
introduced a
formally verified Image Loader for UEFI usage at ISP RAS Open[1] due
to various
defects we outlined in the corresponding paper. Thank you once again
Laszlo
for your *incredible* review work on the publication part.

I now want to make an effort to mainline it, preferably as part of
the current
Google Summer of Code event. To be clear, my internship at ISP RAS has
concluded, and while Vitaly will be available for design discussion,
he has other
priorities at the moment and the practical part will be on me. I have
previously
submitted a proposal via the GSoC website for your review.

There are many things to consider:
1. The Image Loader is a core component, and there needs to be a
significant
level of quality and security assurance.
2. Being consumed by many packages, the proposed patch set will take
a lot of
time to review and integrate.
3. During my initial exploration, I discovered defective PPIs and
protocols (e.g.
returning data with no corresponding size) originating from the UEFI
PI and
UEFI specifications. Changes need to be discussed, settled on, and
submitted to
the UEFI Forum.
4. Some UEFI APIs like the Security Architecture protocols are
inconveniently
abstract, see 5.
5. Some of the current code does not use the existing context, or
accesses it
outside of the exposed APIs. The control flow of the dispatchers may
need to be
adapted to make the context available to appropriate APIs.

But obviously there are not only unpleasant considerations:
A. The Image Loader is mostly formally verified, and only very few
changes will
be required from the last proven state. This gives a lot of trust in
its correctness
and safety.
B. All outlined defects that are of critical nature have been fixed
successfully.
C. The Image Loader has been tested with real-world code loading
real-world
OSes on thousands of machines in the past few months, including
rejecting
malformed images (configurable by PCD).
D. The new APIs will centralise everything PE, reducing code
duplication and
potentially unsafe operations.
E. Centralising and reduced parse duplication may improve overall boot
performance.
F. The code has been coverage-tested to not contain dead code.
G. The code has been fuzz-tested including sanitizers to not invoke
undefined
behaviour.
H. I already managed to identify a malformed image in OVMF with its help
(incorrectly reported section alignment of an Intel IPXE driver). A
fix will be
submitted shortly.
I. I plan to support PE section permissions, allowing for read-only data
segments when enabled.

There are likely more points for both lists, but I hope this gives a
decent
starting point for discussion. What are your thoughts on the matter?
I strongly
encourage everyone to read the section regarding defects of our
publication[2]
to better understand the motivation. The vague points above can of
course be
elaborated in due time, as you see fit.

Thank you for your time!

Best regards,
Marvin


[1] https://github.com/mhaeuser/ISPRASOpen-SecurePE <https://github.com/mhaeuser/ISPRASOpen-SecurePE>
[2] https://arxiv.org/pdf/2012.05471.pdf <https://arxiv.org/pdf/2012.05471.pdf>









Re: [GSoC proposal] Secure Image Loader

Andrew Fish
 



On Apr 8, 2021, at 9:06 AM, Marvin Häuser <mhaeuser@...> wrote:

We use the loader code in userspace anyway for fuzzing and such. I also want to build a database of all sorts of UEFI binaries some time before the merge to confirm they are all accepted (Windows / macOS / Linux bootloaders, tools like memtest, drivers like iPXE). As part of that, I'm sure we can have a userspace tool that uses the code to emit parsing information.

But as the EDK II build system is very... not so userspace friendly, I will not promise it will be very nice. :)


Marvin,

The BaseTools can easily build C command line tools that are cross platform?

Actually GenFw [1] already does a lot of PE/COFF magic, so it should be relatively easy to add a -I, —info, and dump out an overview of a PE/COFF image, and make comments on things that are not secure. It would also probably be useful to dump out information about the Debug Directory entries, His sections, etc. for general debug.

/Volumes/Case/edk2-github(eng/PR-557-XcodeResourceSections)>. edksetup.sh
Loading previous configuration from /Volumes/Case/edk2-github/Conf/BuildEnv.sh
WORKSPACE: /Volumes/Case/edk2-github
EDK_TOOLS_PATH: /Volumes/Case/edk2-github/BaseTools
CONF_PATH: /Volumes/Case/edk2-github/Conf
/Volumes/Case/edk2-github(eng/PR-557-XcodeResourceSections)>GenFw -h
GenFw Version 0.2 Developer Build based on Revision: Unknown 

Usage: GenFw [options] <input_file>

Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.

Options:
  -o FileName, --outputfile FileName
                        File will be created to store the output content.
  -e EFI_FILETYPE, --efiImage EFI_FILETYPE
                        Create Efi Image. EFI_FILETYPE is one of BASE,SMM_CORE,
                        PEI_CORE, PEIM, DXE_CORE, DXE_DRIVER, UEFI_APPLICATION,
                        SEC, DXE_SAL_DRIVER, UEFI_DRIVER, DXE_RUNTIME_DRIVER,
                        DXE_SMM_DRIVER, SECURITY_CORE, COMBINED_PEIM_DRIVER,
                        MM_STANDALONE, MM_CORE_STANDALONE,
                        PIC_PEIM, RELOCATABLE_PEIM, BS_DRIVER, RT_DRIVER,
                        APPLICATION, SAL_RT_DRIVER to support all module types
                        It can only be used together with --keepexceptiontable,
                        --keepzeropending, --keepoptionalheader, -r, -o option.
                        It is a action option. If it is combined with other action options,
                        the later input action option will override the previous one.
  -c, --acpi            Create Acpi table.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -t, --terse           Create Te Image.
                        It can only be used together with --keepexceptiontable,
                        --keepzeropending, --keepoptionalheader, -r, -o option.
                        It is a action option. If it is combined with other action options,
                        the later input action option will override the previous one.
  -u, --dump            Dump TeImage Header.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -z, --zero            Zero the Debug Data Fields in the PE input image file.
                        It also zeros the time stamp fields.
                        This option can be used to compare the binary efi image.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -b, --exe2bin         Convert the input EXE to the output BIN file.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -l, --stripped        Strip off the relocation info from PE or TE image.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -s timedate, --stamp timedate
                        timedate format is "yyyy-mm-dd 00:00:00". if timedata 
                        is set to NOW, current system time is used. The support
                        date scope is 1970-01-01 00+timezone:00:00
                        ~ 2038-01-19 03+timezone:14:07
                        The scope is adjusted according to the different zones.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -m, --mcifile         Convert input microcode txt file to microcode bin file.
                        It can't be combined with other action options
                        except for -o option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -j, --join            Combine multi microcode bin files to one file.
                        It can be specified with -a, -p, -o option.
                        No other options can be combined with it.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -a NUM, --align NUM   NUM is one HEX or DEC format alignment value.
                        This option is only used together with -j option.
  -p NUM, --pad NUM     NUM is one HEX or DEC format padding value.
                        This option is only used together with -j option.
  --keepexceptiontable  Don't clear exception table.
                        This option can be used together with -e or -t.
                        It doesn't work for other options.
  --keepoptionalheader  Don't zero PE/COFF optional header fields.
                        This option can be used together with -e or -t.
                        It doesn't work for other options.
  --keepzeropending     Don't strip zero pending of .reloc.
                        This option can be used together with -e or -t.
                        It doesn't work for other options.
  -r, --replace         Overwrite the input file with the output content.
                        If more input files are specified,
                        the last input file will be as the output file.
  -g HiiPackageListGuid, --hiiguid HiiPackageListGuid
                        Guid is used to specify hii package list guid.
                        Its format is xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
                        If not specified, the first Form FormSet guid is used.
  --hiipackage          Combine all input binary hii packages into 
                        a single package list as the text resource data(RC).
                        It can't be combined with other action options
                        except for -o option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  --hiibinpackage       Combine all input binary hii packages into 
                        a single package list as the binary resource section.
                        It can't be combined with other action options
                        except for -o option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  --rc FlieName         Append a Hii resource section to the
                        last PE/COFF section. The FileName is the resource section to append
                        If FileName does not exist this operation is skipped. This feature is
                        only intended for toolchains, like XCODE, that don't suport $(RC).
                        This option can only be combined with -e
  --rebase NewAddress   Rebase image to new base address. New address 
                        is also set to the first none code section header.
                        It can't be combined with other action options
                        except for -o or -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  --address NewAddress  Set new address into the first none code 
                        section header of the input image.
                        It can't be combined with other action options
                        except for -o or -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.
  -v, --verbose         Turn on verbose output with informational messages.
  -q, --quiet           Disable all messages except key message and fatal error
  -d, --debug level     Enable debug messages, at input debug level.
  --version             Show program's version number and exit
  -h, --help            Show this help message and exit

Thanks,

Andrew Fish

Best regards,
Marvin

On 08.04.21 16:13, Andrew (EFI) Fish wrote:
At a minimum it would be nice if we had a tool that would point out the security faults with a given PE/COFF file layout.



On Apr 8, 2021, at 4:16 AM, Laszlo Ersek <lersek@...> wrote:

On 04/06/21 12:06, Marvin Häuser wrote:
Good day Nate,

Comments are inline.

Best regards,
Marvin

On 06.04.21 11:41, Nate DeSimone wrote:
Hi Marvin,

Great to meet you and welcome back! Glad you hear you are interested!
Completing a formal verification of a PE/COFF loader is certainly
impressive. Was this done with some sort of automated theorem proving?
It would seem a rather arduous task doing an inductive proof for an
algorithm like that by hand!
I would call it "semi-automated", a great deal of intermediate goals
(preconditions, postconditions, invariants, assertions, ...) were
required to show all interesting properties. But yes, the actual proof
steps are automated by common SMT solvers. It was done using the
AstraVer Toolset and ACSL, latter basically a language to express logic
statements with C-like syntax.

I completely agree with you that getting a formally verified PE/COFF
loader into mainline is undoubtably valuable and would pay security
dividends for years to come.
I'm glad to hear that. :)

Admittedly, this is an area of computer science that I don't have a
great deal of experience with. The furthest I have gone on this topic
is writing out proofs for simple algorithms on exams in my Algorithms
class in college. Regardless you have a much better idea of what the
current status is of the work that you and Vitaly have done. I guess
my only question is do you think there is sufficient work remaining to
fill the 10 week GSoC development window?
Please don't get me wrong, but I would be surprised if the UEFI
specification changes I'd like to discuss alone would be completed
within 10 weeks, let alone implementation throughout the codebase. While
I think the plain amount of code may be a bit less than say a
MinPlatform port, the changes are much deeper and require much more
caution to avoid regressions (e.g. by invalidating undocumented
assertions). This sadly is not a matter of just replacing the underlying
library implementation or "plug-in and play" at all. It furthermore
affects many parts of the stack, the core dispatchers used for all
platforms, image emulation (EBC), UEFI userland emulation (EmuPkg), and
so on. I was rather worried the scope is too broad time-wise, but it can
be narrowed/widened as you see fit really. This is one of *the* core
components used on millions of device, and many package maintainers need
to review and validate the changes, this must really be done right the
first try. :)

Certainly we can use some of that time to perform the code reviews you
mention and write up formal ECRs for the UEFI spec changes that you
believe are needed.
I believed that was part of the workload, yes, but even without it I
think there is plenty to do.

Thank you for sending the application and alerting us to the great
work you and Vitaly have done! I'll read your paper more closely and
come back with any questions I still have.
Thank you, I will gladly explain anything unclear. Just try to not give
Laszlo too many flashbacks. :)
I haven't commented yet in this thread, as I thought my stance on this
undertaking was (or should be) obvious.

I very much welcome a replacement for the PE/COFF parser (as I consider
its security issues unfixable in an incremental manner). From my reading
of Marvin's and Vitaly's paper (draft), they have my full trust, and I'm
ready to put their upcoming code to use in ArmVirtPkg and OvmfPkg with
minimal actual code review. If fixing the pervasive security problems
around this area cannot avoid spiraling out to other core code in edk2,
such as dispatchers, and even to the PI / UEFI specs, so be it.

Regarding GSoC itself: as I stated elsewhere previously, I support
edk2's participation in GSoC, while at the same time I'm not
volunteering for mentorship at all. I'm uncertain if GSoC is the best
framework for upstreaming such a large undertaking, but if it can help,
we should use it as much as possible.

Thanks
Laszlo





With Best Regards,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
Häuser
Sent: Sunday, April 4, 2021 4:02 PM
To: devel@edk2.groups.io; Laszlo Ersek <lersek@...>; Andrew Fish
<afish@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: [edk2-devel] [GSoC proposal] Secure Image Loader

Good day everyone,

I'll keep the introduction brief because I've been around for a while
now. :) I'm
Marvin Häuser, a third-year Computer Science student from TU
Kaiserslautern,
Germany. Late last year, my colleague Vitaly from ISP RAS and me
introduced a
formally verified Image Loader for UEFI usage at ISP RAS Open[1] due
to various
defects we outlined in the corresponding paper. Thank you once again
Laszlo
for your *incredible* review work on the publication part.

I now want to make an effort to mainline it, preferably as part of
the current
Google Summer of Code event. To be clear, my internship at ISP RAS has
concluded, and while Vitaly will be available for design discussion,
he has other
priorities at the moment and the practical part will be on me. I have
previously
submitted a proposal via the GSoC website for your review.

There are many things to consider:
1. The Image Loader is a core component, and there needs to be a
significant
level of quality and security assurance.
2. Being consumed by many packages, the proposed patch set will take
a lot of
time to review and integrate.
3. During my initial exploration, I discovered defective PPIs and
protocols (e.g.
returning data with no corresponding size) originating from the UEFI
PI and
UEFI specifications. Changes need to be discussed, settled on, and
submitted to
the UEFI Forum.
4. Some UEFI APIs like the Security Architecture protocols are
inconveniently
abstract, see 5.
5. Some of the current code does not use the existing context, or
accesses it
outside of the exposed APIs. The control flow of the dispatchers may
need to be
adapted to make the context available to appropriate APIs.

But obviously there are not only unpleasant considerations:
A. The Image Loader is mostly formally verified, and only very few
changes will
be required from the last proven state. This gives a lot of trust in
its correctness
and safety.
B. All outlined defects that are of critical nature have been fixed
successfully.
C. The Image Loader has been tested with real-world code loading
real-world
OSes on thousands of machines in the past few months, including
rejecting
malformed images (configurable by PCD).
D. The new APIs will centralise everything PE, reducing code
duplication and
potentially unsafe operations.
E. Centralising and reduced parse duplication may improve overall boot
performance.
F. The code has been coverage-tested to not contain dead code.
G. The code has been fuzz-tested including sanitizers to not invoke
undefined
behaviour.
H. I already managed to identify a malformed image in OVMF with its help
(incorrectly reported section alignment of an Intel IPXE driver). A
fix will be
submitted shortly.
I. I plan to support PE section permissions, allowing for read-only data
segments when enabled.

There are likely more points for both lists, but I hope this gives a
decent
starting point for discussion. What are your thoughts on the matter?
I strongly
encourage everyone to read the section regarding defects of our
publication[2]
to better understand the motivation. The vague points above can of
course be
elaborated in due time, as you see fit.

Thank you for your time!

Best regards,
Marvin


[1] https://github.com/mhaeuser/ISPRASOpen-SecurePE
[2] https://arxiv.org/pdf/2012.05471.pdf

















Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] BaseTools/Ecc: Update structpcd parsing method.

Bret Barkelew
 

Ah! Gotcha. I missed that this was specific to ECC. Thanks!

 

- Bret

 

From: Chen, Christine
Sent: Wednesday, April 7, 2021 6:42 PM
To: Bret Barkelew; devel@edk2.groups.io
Cc: Liang, MingyueX; Feng, Bob C; Liming Gao
Subject: RE: [EXTERNAL] [edk2-devel] [PATCH 1/1] BaseTools/Ecc: Update structpcd parsing method.

 

Hi Bret,

 

This patch adds the ECC check for structurePcd written in des/dsc file, as the origin check is only for non-structured Pcd.

 

Best Regards,

Yuwei (Christine)

 

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Friday, April 2, 2021 12:43 AM
To: devel@edk2.groups.io; Chen, Christine <yuwei.chen@...>
Cc: Liang, MingyueX <mingyuex.liang@...>; Feng, Bob C <bob.c.feng@...>; Liming Gao <gaoliming@...>
Subject: RE: [EXTERNAL] [edk2-devel] [PATCH 1/1] BaseTools/Ecc: Update structpcd parsing method.

 

What does “update” mean in this context? What behavior is changing?

 

- Bret

 

From: Yuwei Chen via groups.io
Sent: Thursday, April 1, 2021 12:04 AM
To: devel@edk2.groups.io
Cc: mliang2x; Feng, Bob C; Liming Gao
Subject: [EXTERNAL] [edk2-devel] [PATCH 1/1] BaseTools/Ecc: Update structpcd parsing method.

 

From: mliang2x <mingyuex.liang@...>

Update the pcdparser method in Dec and DSC files.

Signed-off-by: Mingyue Liang <mingyuex.liang@...>
Cc: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <gaoliming@...>
Cc: Yuwei Chen <yuwei.chen@...>
---
 .../Ecc/MetaFileWorkspace/MetaFileParser.py   | 464 ++++++++++--------
 1 file changed, 265 insertions(+), 199 deletions(-)

diff --git a/BaseTools/Source/Python/Ecc/MetaFileWorkspace/MetaFileParser.py b/BaseTools/Source/Python/Ecc/MetaFileWorkspace/MetaFileParser.py
index 9c27c8e16a05..588d3dbe6ed5 100644
--- a/BaseTools/Source/Python/Ecc/MetaFileWorkspace/MetaFileParser.py
+++ b/BaseTools/Source/Python/Ecc/MetaFileWorkspace/MetaFileParser.py
@@ -22,7 +22,7 @@ import Ecc.EccToolError as EccToolError
 from CommonDataClass.DataClass import *
 from Common.DataType import *
 from Common.StringUtils import *
-from Common.Misc import GuidStructureStringToGuidString, CheckPcdDatum, PathClass, AnalyzePcdData
+from Common.Misc import GuidStructureStringToGuidString, CheckPcdDatum, PathClass, AnalyzePcdData, AnalyzeDscPcd, AnalyzePcdExpression, ParseFieldValue, StructPattern
 from Common.Expression import *
 from CommonDataClass.Exceptions import *
 
@@ -31,6 +31,8 @@ from GenFds.FdfParser import FdfParser
 from Common.LongFilePathSupport import OpenLongFilePath as open
 from Common.LongFilePathSupport import CodecOpenLongFilePath
 
+CODEPattern = re.compile(r"{CODE\([a-fA-F0-9Xx\{\},\s]*\)}")
+
 ## A decorator used to parse macro definition
 def ParseMacro(Parser):
     def MacroParser(self):
@@ -174,6 +176,11 @@ class MetaFileParser(object):
         # UNI object and extra UNI object
         self._UniObj = None
         self._UniExtraObj = None
+        # StructPcd var
+        self._PcdCodeValue = ""
+        self._PcdDataTypeCODE = False
+        self._CurrentPcdName = ""
+        self._GuidDict = {}  # for Parser PCD value {GUID(gTokeSpaceGuidName)}
 
     ## Store the parsed data in table
     def _Store(self, *Args):
@@ -395,6 +402,40 @@ class MetaFileParser(object):
                 Macros.update(self._SectionsMacroDict[(self._SectionType, Scope1, Scope2)])
         return Macros
 
+    def ProcessMultipleLineCODEValue(self, Content):
+        CODEBegin = False
+        CODELine = ""
+        continuelinecount = 0
+        newContent = []
+        for Index in range(0, len(Content)):
+            Line = Content[Index]
+            if CODEBegin:
+                CODELine = CODELine + Line
+                continuelinecount +=1
+                if ")}" in Line:
+                    newContent.append(CODELine)
+                    for _ in range(continuelinecount):
+                        newContent.append("")
+                    CODEBegin = False
+                    CODELine = ""
+                    continuelinecount = 0
+            else:
+                if not Line:
+                    newContent.append(Line)
+                    continue
+                if "{CODE(" not in Line:
+                    newContent.append(Line)
+                    continue
+                elif CODEPattern.findall(Line):
+                    newContent.append(Line)
+                    continue
+                else:
+                    CODEBegin = True
+                    CODELine = Line
+
+        return newContent
+
+
     _SectionParser  = {}
     Finished        = property(_GetFinished, _SetFinished)
     _Macros         = property(_GetMacros)
@@ -812,6 +853,8 @@ class DscParser(MetaFileParser):
             Content = open(str(self.MetaFile.Path), 'r').readlines()
         except:
             EdkLogger.error("Parser", FILE_READ_FAILURE, ExtraData=self.MetaFile)
+
+        Content = self.ProcessMultipleLineCODEValue(Content)
         #
         # Insert a record for file
         #
@@ -1018,24 +1061,71 @@ class DscParser(MetaFileParser):
     #
     @ParseMacro
     def _PcdParser(self):
+        if self._PcdDataTypeCODE:
+            self._PcdCodeValue = self._PcdCodeValue + "\n " + self._CurrentLine
+            if self._CurrentLine.endswith(")}"):
+                self._CurrentLine = "|".join((self._CurrentPcdName, self._PcdCodeValue))
+                self._PcdDataTypeCODE = False
+                self._PcdCodeValue = ""
+            else:
+                self._ValueList = None
+                return
         TokenList = GetSplitValueList(self._CurrentLine, TAB_VALUE_SPLIT, 1)
+        self._CurrentPcdName = TokenList[0]
+        if len(TokenList) == 2 and TokenList[1].strip().startswith("{CODE"):
+            self._PcdDataTypeCODE = True
+            self._PcdCodeValue = TokenList[1].strip()
+
+        if self._PcdDataTypeCODE:
+            if self._CurrentLine.endswith(")}"):
+                self._PcdDataTypeCODE = False
+                self._PcdCodeValue = ""
+            else:
+                self._ValueList = None
+                return
         self._ValueList[0:1] = GetSplitValueList(TokenList[0], TAB_SPLIT)
+        PcdNameTockens = GetSplitValueList(TokenList[0], TAB_SPLIT)
+        if len(PcdNameTockens) == 2:
+            self._ValueList[0], self._ValueList[1] = PcdNameTockens[0], PcdNameTockens[1]
+        elif len(PcdNameTockens) == 3:
+            self._ValueList[0], self._ValueList[1] = ".".join((PcdNameTockens[0], PcdNameTockens[1])), PcdNameTockens[2]
+        elif len(PcdNameTockens) > 3:
+            self._ValueList[0], self._ValueList[1] = ".".join((PcdNameTockens[0], PcdNameTockens[1])), ".".join(PcdNameTockens[2:])
         if len(TokenList) == 2:
             self._ValueList[2] = TokenList[1]
         if self._ValueList[0] == '' or self._ValueList[1] == '':
             EdkLogger.error('Parser', FORMAT_INVALID, "No token space GUID or PCD name specified",
                             ExtraData=self._CurrentLine + " (<TokenSpaceGuidCName>.<TokenCName>|<PcdValue>)",
-                            File=self.MetaFile, Line=self._LineIndex+1)
+                            File=self.MetaFile, Line=self._LineIndex + 1)
         if self._ValueList[2] == '':
+            #
+            # The PCD values are optional for FIXEDATBUILD, PATCHABLEINMODULE, Dynamic/DynamicEx default
+            #
+            if self._SectionType in (MODEL_PCD_FIXED_AT_BUILD, MODEL_PCD_PATCHABLE_IN_MODULE, MODEL_PCD_DYNAMIC_DEFAULT, MODEL_PCD_DYNAMIC_EX_DEFAULT):
+                return
             EdkLogger.error('Parser', FORMAT_INVALID, "No PCD value given",
                             ExtraData=self._CurrentLine + " (<TokenSpaceGuidCName>.<TokenCName>|<PcdValue>)",
-                            File=self.MetaFile, Line=self._LineIndex+1)
+                            File=self.MetaFile, Line=self._LineIndex + 1)
+
+        # Validate the datum type of Dynamic Defaul PCD and DynamicEx Default PCD
+        ValueList = GetSplitValueList(self._ValueList[2])
+        if len(ValueList) > 1 and ValueList[1] in [TAB_UINT8, TAB_UINT16, TAB_UINT32, TAB_UINT64] \
+                              and self._ItemType in [MODEL_PCD_DYNAMIC_DEFAULT, MODEL_PCD_DYNAMIC_EX_DEFAULT]:
+            EdkLogger.error('Parser', FORMAT_INVALID, "The datum type '%s' of PCD is wrong" % ValueList[1],
+                            ExtraData=self._CurrentLine, File=self.MetaFile, Line=self._LineIndex + 1)
+
+        # Validate the VariableName of DynamicHii and DynamicExHii for PCD Entry must not be an empty string
+        if self._ItemType in [MODEL_PCD_DYNAMIC_HII, MODEL_PCD_DYNAMIC_EX_HII]:
+            DscPcdValueList = GetSplitValueList(TokenList[1], TAB_VALUE_SPLIT, 1)
+            if len(DscPcdValueList[0].replace('L', '').replace('"', '').strip()) == 0:
+                EdkLogger.error('Parser', FORMAT_INVALID, "The VariableName field in the HII format PCD entry must not be an empty string",
+                            ExtraData=self._CurrentLine, File=self.MetaFile, Line=self._LineIndex + 1)
         # if value are 'True', 'true', 'TRUE' or 'False', 'false', 'FALSE', replace with integer 1 or 0.
         DscPcdValueList = GetSplitValueList(TokenList[1], TAB_VALUE_SPLIT, 1)
         if DscPcdValueList[0] in ['True', 'true', 'TRUE']:
-            self._ValueList[2] = TokenList[1].replace(DscPcdValueList[0], '1', 1);
+            self._ValueList[2] = TokenList[1].replace(DscPcdValueList[0], '1', 1)
         elif DscPcdValueList[0] in ['False', 'false', 'FALSE']:
-            self._ValueList[2] = TokenList[1].replace(DscPcdValueList[0], '0', 1);
+            self._ValueList[2] = TokenList[1].replace(DscPcdValueList[0], '0', 1)
 
     ## [components] section parser
     @ParseMacro
@@ -1502,6 +1592,10 @@ class DecParser(MetaFileParser):
         self._include_flag = False
         self._package_flag = False
 
+        self._AllPCDs = [] # Only for check duplicate PCD
+        self._AllPcdDict = {}
+
+
     ## Parser starter
     def Start(self):
         Content = ''
@@ -1510,6 +1604,7 @@ class DecParser(MetaFileParser):
         except:
             EdkLogger.error("Parser", FILE_READ_FAILURE, ExtraData=self.MetaFile)
 
+        Content = self.ProcessMultipleLineCODEValue(Content)
         #
         # Insert a record for file
         #
@@ -1707,51 +1802,6 @@ class DecParser(MetaFileParser):
                 namelist[2] = ".".join((arrayindex,namelist[2]))
         return namelist
 
-    def StructPcdParser(self):
-        self._ValueList[0] = self._CurrentStructurePcdName
-
-        if "|" not in self._CurrentLine:
-            if "<HeaderFiles>" == self._CurrentLine:
-                self._include_flag = True
-                self._package_flag = False
-                self._ValueList = None
-                return
-            if "<Packages>" == self._CurrentLine:
-                self._package_flag = True
-                self._ValueList = None
-                self._include_flag = False
-                return
-
-            if self._include_flag:
-                self._ValueList[1] = "<HeaderFiles>_" + md5(self._CurrentLine.encode('utf-8')).hexdigest()
-                self._ValueList[2] = self._CurrentLine
-            if self._package_flag and "}" != self._CurrentLine:
-                self._ValueList[1] = "<Packages>_" + md5(self._CurrentLine.encode('utf-8')).hexdigest()
-                self._ValueList[2] = self._CurrentLine
-            if self._CurrentLine == "}":
-                self._package_flag = False
-                self._include_flag = False
-                self._ValueList = None
-        else:
-            PcdTockens = self._CurrentLine.split(TAB_VALUE_SPLIT)
-            PcdNames = self.ParsePcdName(PcdTockens[0].split(TAB_SPLIT))
-            if len(PcdNames) == 2:
-                if PcdNames[1].strip().endswith("]"):
-                    PcdName = PcdNames[1][:PcdNames[1].index('[')]
-                    Index = PcdNames[1][PcdNames[1].index('['):]
-                    self._ValueList[0] = TAB_SPLIT.join((PcdNames[0], PcdName))
-                    self._ValueList[1] = Index
-                    self._ValueList[2] = PcdTockens[1]
-                else:
-                    self._CurrentStructurePcdName = ""
-            else:
-                if self._CurrentStructurePcdName != TAB_SPLIT.join(PcdNames[:2]):
-                    EdkLogger.error('Parser', FORMAT_INVALID, "Pcd Name does not match: %s and %s " % (
-                    self._CurrentStructurePcdName, TAB_SPLIT.join(PcdNames[:2])),
-                                    File=self.MetaFile, Line=self._LineIndex + 1)
-                self._ValueList[1] = TAB_SPLIT.join(PcdNames[2:])
-                self._ValueList[2] = PcdTockens[1]
-
     ## PCD sections parser
     #
     #   [PcdsFixedAtBuild]
@@ -1763,159 +1813,175 @@ class DecParser(MetaFileParser):
     @ParseMacro
     def _PcdParser(self):
         if self._CurrentStructurePcdName:
-            self.StructPcdParser()
-            return
-        TokenList = GetSplitValueList(self._CurrentLine, TAB_VALUE_SPLIT, 1)
-        self._ValueList[0:1] = GetSplitValueList(TokenList[0], TAB_SPLIT)
-        # check PCD information
-        if self._ValueList[0] == '' or self._ValueList[1] == '':
-            EdkLogger.error('Parser', FORMAT_INVALID, "No token space GUID or PCD name specified",
-                            ExtraData=self._CurrentLine + \
-                                      " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
-                            File=self.MetaFile, Line=self._LineIndex+1)
-        # check PCD datum information
-        if len(TokenList) < 2 or TokenList[1] == '':
-            EdkLogger.error('Parser', FORMAT_INVALID, "No PCD Datum information given",
-                            ExtraData=self._CurrentLine + \
-                                      " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
-                            File=self.MetaFile, Line=self._LineIndex+1)
-
-
-        ValueRe  = re.compile(r'^\s*L?\".*\|.*\"')
-        PtrValue = ValueRe.findall(TokenList[1])
-
-        # Has VOID* type string, may contain "|" character in the string.
-        if len(PtrValue) != 0:
-            ptrValueList = re.sub(ValueRe, '', TokenList[1])
-            ValueList    = GetSplitValueList(ptrValueList)
-            ValueList[0] = PtrValue[0]
-        else:
-            ValueList = GetSplitValueList(TokenList[1])
-
-
-        # check if there's enough datum information given
-        if len(ValueList) != 3:
-            EdkLogger.error('Parser', FORMAT_INVALID, "Invalid PCD Datum information given",
-                            ExtraData=self._CurrentLine + \
-                                      " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
-                            File=self.MetaFile, Line=self._LineIndex+1)
-        # check default value
-        if ValueList[0] == '':
-            EdkLogger.error('Parser', FORMAT_INVALID, "Missing DefaultValue in PCD Datum information",
-                            ExtraData=self._CurrentLine + \
-                                      " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
-                            File=self.MetaFile, Line=self._LineIndex+1)
-        # check datum type
-        if ValueList[1] == '':
-            EdkLogger.error('Parser', FORMAT_INVALID, "Missing DatumType in PCD Datum information",
-                            ExtraData=self._CurrentLine + \
-                                      " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
-                            File=self.MetaFile, Line=self._LineIndex+1)
-        # check token of the PCD
-        if ValueList[2] == '':
-            EdkLogger.error('Parser', FORMAT_INVALID, "Missing Token in PCD Datum information",
-                            ExtraData=self._CurrentLine + \
-                                      " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
-                            File=self.MetaFile, Line=self._LineIndex+1)
-        # check format of default value against the datum type
-        IsValid, Cause = CheckPcdDatum(ValueList[1], ValueList[0])
-        if not IsValid:
-            EdkLogger.error('Parser', FORMAT_INVALID, Cause, ExtraData=self._CurrentLine,
-                            File=self.MetaFile, Line=self._LineIndex+1)
-        if Cause == "StructurePcd":
-            self._CurrentStructurePcdName = TAB_SPLIT.join(self._ValueList[0:2])
             self._ValueList[0] = self._CurrentStructurePcdName
-            self._ValueList[1] = ValueList[1].strip()
 
-        if EccGlobalData.gConfig.UniCheckPCDInfo == '1' or EccGlobalData.gConfig.UniCheckAll == '1' or EccGlobalData.gConfig.CheckAll == '1':
-            # check Description, Prompt information
-            PatternDesc = re.compile('##\s*([\x21-\x7E\s]*)', re.S)
-            PatternPrompt = re.compile('#\s+@Prompt\s+([\x21-\x7E\s]*)', re.S)
-            Description = None
-            Prompt = None
-            # check @ValidRange, @ValidList and @Expression format valid
-            ErrorCodeValid = '0x0 <= %s <= 0xFFFFFFFF'
-            PatternValidRangeIn = '(NOT)?\s*(\d+\s*-\s*\d+|0[xX][a-fA-F0-9]+\s*-\s*0[xX][a-fA-F0-9]+|LT\s*\d+|LT\s*0[xX][a-fA-F0-9]+|GT\s*\d+|GT\s*0[xX][a-fA-F0-9]+|LE\s*\d+|LE\s*0[xX][a-fA-F0-9]+|GE\s*\d+|GE\s*0[xX][a-fA-F0-9]+|XOR\s*\d+|XOR\s*0[xX][a-fA-F0-9]+|EQ\s*\d+|EQ\s*0[xX][a-fA-F0-9]+)'
-            PatternValidRng = re.compile('^' + '(NOT)?\s*' + PatternValidRangeIn + '$')
-            for Comment in self._Comments:
-                Comm = Comment[0].strip()
-                if not Comm:
-                    continue
-                if not Description:
-                    Description = PatternDesc.findall(Comm)
-                if not Prompt:
-                    Prompt = PatternPrompt.findall(Comm)
-                if Comm[0] == '#':
-                    ValidFormt = Comm.lstrip('#')
-                    ValidFormt = ValidFormt.lstrip()
-                    if ValidFormt[0:11] == '@ValidRange':
-                        ValidFormt = ValidFormt[11:]
-                        ValidFormt = ValidFormt.lstrip()
-                        try:
-                            ErrorCode, Expression = ValidFormt.split('|', 1)
-                        except ValueError:
-                            ErrorCode = '0x0'
-                            Expression = ValidFormt
-                        ErrorCode, Expression = ErrorCode.strip(), Expression.strip()
-                        try:
-                            if not eval(ErrorCodeValid % ErrorCode):
-                                EdkLogger.warn('Parser', '@ValidRange ErrorCode(%s) of PCD %s is not valid UINT32 value.' % (ErrorCode, TokenList[0]))
-                        except:
-                            EdkLogger.warn('Parser', '@ValidRange ErrorCode(%s) of PCD %s is not valid UINT32 value.' % (ErrorCode, TokenList[0]))
-                        if not PatternValidRng.search(Expression):
-                            EdkLogger.warn('Parser', '@ValidRange Expression(%s) of PCD %s is incorrect format.' % (Expression, TokenList[0]))
-                    if ValidFormt[0:10] == '@ValidList':
-                        ValidFormt = ValidFormt[10:]
-                        ValidFormt = ValidFormt.lstrip()
-                        try:
-                            ErrorCode, Expression = ValidFormt.split('|', 1)
-                        except ValueError:
-                            ErrorCode = '0x0'
-                            Expression = ValidFormt
-                        ErrorCode, Expression = ErrorCode.strip(), Expression.strip()
-                        try:
-                            if not eval(ErrorCodeValid % ErrorCode):
-                                EdkLogger.warn('Parser', '@ValidList ErrorCode(%s) of PCD %s is not valid UINT32 value.' % (ErrorCode, TokenList[0]))
-                        except:
-                            EdkLogger.warn('Parser', '@ValidList ErrorCode(%s) of PCD %s is not valid UINT32 value.' % (ErrorCode, TokenList[0]))
-                        Values = Expression.split(',')
-                        for Value in Values:
-                            Value = Value.strip()
-                            try:
-                                eval(Value)
-                            except:
-                                EdkLogger.warn('Parser', '@ValidList Expression of PCD %s include a invalid value(%s).' % (TokenList[0], Value))
-                                break
-                    if ValidFormt[0:11] == '@Expression':
-                        ValidFormt = ValidFormt[11:]
-                        ValidFormt = ValidFormt.lstrip()
-                        try:
-                            ErrorCode, Expression = ValidFormt.split('|', 1)
-                        except ValueError:
-                            ErrorCode = '0x0'
-                            Expression = ValidFormt
-                        ErrorCode, Expression = ErrorCode.strip(), Expression.strip()
-                        try:
-                            if not eval(ErrorCodeValid % ErrorCode):
-                                EdkLogger.warn('Parser', '@Expression ErrorCode(%s) of PCD %s is not valid UINT32 value.' % (ErrorCode, TokenList[0]))
-                        except:
-                            EdkLogger.warn('Parser', '@Expression ErrorCode(%s) of PCD %s is not valid UINT32 value.' % (ErrorCode, TokenList[0]))
-                        if not Expression:
-                            EdkLogger.warn('Parser', '@Expression Expression of PCD %s is incorrect format.' % TokenList[0])
-            if not Description:
-                EdkLogger.warn('Parser', 'PCD %s Description information is not provided.' % TokenList[0])
-            if not Prompt:
-                EdkLogger.warn('Parser', 'PCD %s Prompt information is not provided.' % TokenList[0])
-            # check Description, Prompt localization information
-            if self._UniObj:
-                self._UniObj.CheckPcdInfo(TokenList[0])
+            if "|" not in self._CurrentLine:
+                if "<HeaderFiles>" == self._CurrentLine:
+                    self._include_flag = True
+                    self._package_flag = False
+                    self._ValueList = None
+                    return
+                if "<Packages>" == self._CurrentLine:
+                    self._package_flag = True
+                    self._ValueList = None
+                    self._include_flag = False
+                    return
 
-        if ValueList[0] in ['True', 'true', 'TRUE']:
-            ValueList[0] = '1'
-        elif ValueList[0] in ['False', 'false', 'FALSE']:
-            ValueList[0] = '0'
+                if self._include_flag:
+                    self._ValueList[1] = "<HeaderFiles>_" + md5(self._CurrentLine.encode('utf-8')).hexdigest()
+                    self._ValueList[2] = self._CurrentLine
+                if self._package_flag and "}" != self._CurrentLine:
+                    self._ValueList[1] = "<Packages>_" + md5(self._CurrentLine.encode('utf-8')).hexdigest()
+                    self._ValueList[2] = self._CurrentLine
+                if self._CurrentLine == "}":
+                    self._package_flag = False
+                    self._include_flag = False
+                    self._ValueList = None
+                    return
+            else:
+                PcdTockens = self._CurrentLine.split(TAB_VALUE_SPLIT)
+                PcdNames = self.ParsePcdName(PcdTockens[0].split(TAB_SPLIT))
+                if len(PcdNames) == 2:
+                    if PcdNames[1].strip().endswith("]"):
+                        PcdName = PcdNames[1][:PcdNames[1].index('[')]
+                        Index = PcdNames[1][PcdNames[1].index('['):]
+                        self._ValueList[0] = TAB_SPLIT.join((PcdNames[0],PcdName))
+                        self._ValueList[1] = Index
+                        self._ValueList[2] = PcdTockens[1]
+                    else:
+                        self._CurrentStructurePcdName = ""
+                else:
+                    if self._CurrentStructurePcdName != TAB_SPLIT.join(PcdNames[:2]):
+                        EdkLogger.error('Parser', FORMAT_INVALID, "Pcd Name does not match: %s and %s " % (self._CurrentStructurePcdName, TAB_SPLIT.join(PcdNames[:2])),
+                                File=self.MetaFile, Line=self._LineIndex + 1)
+                    self._ValueList[1] = TAB_SPLIT.join(PcdNames[2:])
+                    self._ValueList[2] = PcdTockens[1]
 
-        self._ValueList[2] = ValueList[0].strip() + '|' + ValueList[1].strip() + '|' + ValueList[2].strip()
+        if not self._CurrentStructurePcdName:
+            if self._PcdDataTypeCODE:
+                if ")}" in self._CurrentLine:
+                    ValuePart, RestofValue = self._CurrentLine.split(")}")
+                    self._PcdCodeValue = self._PcdCodeValue + "\n " + ValuePart
+                    self._CurrentLine = "|".join((self._CurrentPcdName, self._PcdCodeValue, RestofValue))
+                    self._PcdDataTypeCODE = False
+                    self._PcdCodeValue = ""
+                else:
+                    self._PcdCodeValue = self._PcdCodeValue + "\n " + self._CurrentLine
+                    self._ValueList = None
+                    return
+            TokenList = GetSplitValueList(self._CurrentLine, TAB_VALUE_SPLIT, 1)
+            self._CurrentPcdName = TokenList[0]
+            if len(TokenList) == 2 and TokenList[1].strip().startswith("{CODE"):
+                if ")}" in self._CurrentLine:
+                    self._PcdDataTypeCODE = False
+                    self._PcdCodeValue = ""
+                else:
+                    self._PcdDataTypeCODE = True
+                    self._PcdCodeValue = TokenList[1].strip()
+                    self._ValueList = None
+                    return
+
+            self._ValueList[0:1] = GetSplitValueList(TokenList[0], TAB_SPLIT)
+            ValueRe = re.compile(r'^[a-zA-Z_][a-zA-Z0-9_]*')
+            # check PCD information
+            if self._ValueList[0] == '' or self._ValueList[1] == '':
+                EdkLogger.error('Parser', FORMAT_INVALID, "No token space GUID or PCD name specified",
+                                ExtraData=self._CurrentLine + \
+                                          " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
+                                File=self.MetaFile, Line=self._LineIndex+1)
+            # check format of token space GUID CName
+            if not ValueRe.match(self._ValueList[0]):
+                EdkLogger.error('Parser', FORMAT_INVALID,
+                                "The format of the token space GUID CName is invalid. The correct format is '(a-zA-Z_)[a-zA-Z0-9_]*'",
+                                ExtraData=self._CurrentLine + \
+                                          " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
+                                File=self.MetaFile, Line=self._LineIndex + 1)
+            # check format of PCD CName
+            if not ValueRe.match(self._ValueList[1]):
+                EdkLogger.error('Parser', FORMAT_INVALID,
+                                "The format of the PCD CName is invalid. The correct format is '(a-zA-Z_)[a-zA-Z0-9_]*'",
+                                ExtraData=self._CurrentLine + \
+                                          " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
+                                File=self.MetaFile, Line=self._LineIndex + 1)
+
+            # check PCD datum information
+            if len(TokenList) < 2 or TokenList[1] == '':
+                EdkLogger.error('Parser', FORMAT_INVALID, "No PCD Datum information given",
+                                ExtraData=self._CurrentLine + \
+                                          " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
+                                File=self.MetaFile, Line=self._LineIndex+1)
+
+
+            ValueRe = re.compile(r'^\s*L?\".*\|.*\"')
+            PtrValue = ValueRe.findall(TokenList[1])
+
+            # Has VOID* type string, may contain "|" character in the string.
+            if len(PtrValue) != 0:
+                ptrValueList = re.sub(ValueRe, '', TokenList[1])
+                ValueList = AnalyzePcdExpression(ptrValueList)
+                ValueList[0] = PtrValue[0]
+            else:
+                ValueList = AnalyzePcdExpression(TokenList[1])
+
+
+            # check if there's enough datum information given
+            if len(ValueList) != 3:
+                EdkLogger.error('Parser', FORMAT_INVALID, "Invalid PCD Datum information given",
+                                ExtraData=self._CurrentLine + \
+                                          " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
+                                File=self.MetaFile, Line=self._LineIndex + 1)
+            # check default value
+            if ValueList[0] == '':
+                EdkLogger.error('Parser', FORMAT_INVALID, "Missing DefaultValue in PCD Datum information",
+                                ExtraData=self._CurrentLine + \
+                                          " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
+                                File=self.MetaFile, Line=self._LineIndex + 1)
+            # check datum type
+            if ValueList[1] == '':
+                EdkLogger.error('Parser', FORMAT_INVALID, "Missing DatumType in PCD Datum information",
+                                ExtraData=self._CurrentLine + \
+                                          " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
+                                File=self.MetaFile, Line=self._LineIndex + 1)
+            # check token of the PCD
+            if ValueList[2] == '':
+                EdkLogger.error('Parser', FORMAT_INVALID, "Missing Token in PCD Datum information",
+                                ExtraData=self._CurrentLine + \
+                                          " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
+                                File=self.MetaFile, Line=self._LineIndex + 1)
+
+            PcdValue = ValueList[0]
+            if PcdValue:
+                try:
+                    self._GuidDict.update(self._AllPcdDict)
+                    ValueList[0] = ValueExpressionEx(ValueList[0], ValueList[1], self._GuidDict)(True)
+                except BadExpression as Value:
+                    EdkLogger.error('Parser', FORMAT_INVALID, Value, ExtraData=self._CurrentLine, File=self.MetaFile, Line=self._LineIndex + 1)
+            # check format of default value against the datum type
+            IsValid, Cause = CheckPcdDatum(ValueList[1], ValueList[0])
+            if not IsValid:
+                EdkLogger.error('Parser', FORMAT_INVALID, Cause, ExtraData=self._CurrentLine,
+                                File=self.MetaFile, Line=self._LineIndex + 1)
+
+            if Cause == "StructurePcd":
+                self._CurrentStructurePcdName = TAB_SPLIT.join(self._ValueList[0:2])
+                self._ValueList[0] = self._CurrentStructurePcdName
+                self._ValueList[1] = ValueList[1].strip()
+
+            if ValueList[0] in ['True', 'true', 'TRUE']:
+                ValueList[0] = '1'
+            elif ValueList[0] in ['False', 'false', 'FALSE']:
+                ValueList[0] = '0'
+
+            # check for duplicate PCD definition
+            if (self._Scope[0], self._ValueList[0], self._ValueList[1]) in self._AllPCDs:
+                EdkLogger.error('Parser', FORMAT_INVALID,
+                                "The same PCD name and GUID have been already defined",
+                                ExtraData=self._CurrentLine, File=self.MetaFile, Line=self._LineIndex + 1)
+            else:
+                self._AllPCDs.append((self._Scope[0], self._ValueList[0], self._ValueList[1]))
+                self._AllPcdDict[TAB_SPLIT.join(self._ValueList[0:2])] = ValueList[0]
+
+            self._ValueList[2] = ValueList[0].strip() + '|' + ValueList[1].strip() + '|' + ValueList[2].strip()
 
     _SectionParser = {
         MODEL_META_DATA_HEADER          :   MetaFileParser._DefineParser,
--
2.26.2.windows.1



 

 


Re: [PATCH 3/3] IntelSiliconPkg/ShadowMicrocodePei: Consume MicrocodeLib

Chaganty, Rangasai V
 

Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Thursday, April 01, 2021 11:00 PM
To: devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
Subject: [PATCH 3/3] IntelSiliconPkg/ShadowMicrocodePei: Consume MicrocodeLib

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
---
.../ShadowMicrocode/ShadowMicrocodePei.c | 155 ++----------------
.../ShadowMicrocode/ShadowMicrocodePei.inf | 3 +-
2 files changed, 13 insertions(+), 145 deletions(-)

diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.c b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.c
index 98a7aed697..4e4b69a0ca 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.c
@@ -1,7 +1,7 @@
/** @file

FIT based microcode shadow PEIM.



-Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>

+Copyright (c) 2020 - 2021, Intel Corporation. All rights reserved.<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent



**/

@@ -13,6 +13,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/DebugLib.h>

#include <Library/BaseMemoryLib.h>

#include <Library/MemoryAllocationLib.h>

+#include <Library/MicrocodeLib.h>

#include <IndustryStandard/FirmwareInterfaceTable.h>

#include <Register/Intel/Microcode.h>

#include <Register/Intel/Cpuid.h>

@@ -70,118 +71,6 @@ EFI_PEI_PPI_DESCRIPTOR mPeiShadowMicrocodePpiList[] = {
}

};



-/**

- Determine if a microcode patch matchs the specific processor signature and flag.

-

- @param[in] CpuIdCount Number of elements in MicrocodeCpuId array.

- @param[in] MicrocodeCpuId A pointer to an array of EDKII_PEI_MICROCODE_CPU_ID

- structures.

- @param[in] ProcessorSignature The processor signature field value

- supported by a microcode patch.

- @param[in] ProcessorFlags The prcessor flags field value supported by

- a microcode patch.

-

- @retval TRUE The specified microcode patch will be loaded.

- @retval FALSE The specified microcode patch will not be loaded.

-**/

-BOOLEAN

-IsProcessorMatchedMicrocodePatch (

- IN UINTN CpuIdCount,

- IN EDKII_PEI_MICROCODE_CPU_ID *MicrocodeCpuId,

- IN UINT32 ProcessorSignature,

- IN UINT32 ProcessorFlags

- )

-{

- UINTN Index;

-

- for (Index = 0; Index < CpuIdCount; Index++) {

- if ((ProcessorSignature == MicrocodeCpuId[Index].ProcessorSignature) &&

- (ProcessorFlags & (1 << MicrocodeCpuId[Index].PlatformId)) != 0) {

- return TRUE;

- }

- }

-

- return FALSE;

-}

-

-/**

- Check the 'ProcessorSignature' and 'ProcessorFlags' of the microcode

- patch header with the CPUID and PlatformID of the processors within

- system to decide if it will be copied into memory.

-

- @param[in] CpuIdCount Number of elements in MicrocodeCpuId array.

- @param[in] MicrocodeCpuId A pointer to an array of EDKII_PEI_MICROCODE_CPU_ID

- structures.

- @param[in] MicrocodeEntryPoint The pointer to the microcode patch header.

-

- @retval TRUE The specified microcode patch need to be loaded.

- @retval FALSE The specified microcode patch dosen't need to be loaded.

-**/

-BOOLEAN

-IsMicrocodePatchNeedLoad (

- IN UINTN CpuIdCount,

- IN EDKII_PEI_MICROCODE_CPU_ID *MicrocodeCpuId,

- CPU_MICROCODE_HEADER *MicrocodeEntryPoint

- )

-{

- BOOLEAN NeedLoad;

- UINTN DataSize;

- UINTN TotalSize;

- CPU_MICROCODE_EXTENDED_TABLE_HEADER *ExtendedTableHeader;

- UINT32 ExtendedTableCount;

- CPU_MICROCODE_EXTENDED_TABLE *ExtendedTable;

- UINTN Index;

-

- if (FeaturePcdGet (PcdShadowAllMicrocode)) {

- return TRUE;

- }

-

- //

- // Check the 'ProcessorSignature' and 'ProcessorFlags' in microcode patch header.

- //

- NeedLoad = IsProcessorMatchedMicrocodePatch (

- CpuIdCount,

- MicrocodeCpuId,

- MicrocodeEntryPoint->ProcessorSignature.Uint32,

- MicrocodeEntryPoint->ProcessorFlags

- );

-

- //

- // If the Extended Signature Table exists, check if the processor is in the

- // support list

- //

- DataSize = MicrocodeEntryPoint->DataSize;

- TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;

- if ((!NeedLoad) && (DataSize != 0) &&

- (TotalSize - DataSize > sizeof (CPU_MICROCODE_HEADER) +

- sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER))) {

- ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) (MicrocodeEntryPoint)

- + DataSize + sizeof (CPU_MICROCODE_HEADER));

- ExtendedTableCount = ExtendedTableHeader->ExtendedSignatureCount;

- ExtendedTable = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);

-

- for (Index = 0; Index < ExtendedTableCount; Index ++) {

- //

- // Check the 'ProcessorSignature' and 'ProcessorFlag' of the Extended

- // Signature Table entry with the CPUID and PlatformID of the processors

- // within system to decide if it will be copied into memory

- //

- NeedLoad = IsProcessorMatchedMicrocodePatch (

- CpuIdCount,

- MicrocodeCpuId,

- ExtendedTable->ProcessorSignature.Uint32,

- ExtendedTable->ProcessorFlag

- );

- if (NeedLoad) {

- break;

- }

- ExtendedTable ++;

- }

- }

-

- return NeedLoad;

-}

-

/**

Actual worker function that shadows the required microcode patches into memory.



@@ -439,6 +328,11 @@ ShadowMicrocode (
return EFI_OUT_OF_RESOURCES;

}



+ if (FeaturePcdGet (PcdShadowAllMicrocode)) {

+ MicrocodeCpuId = NULL;

+ CpuIdCount = 0;

+ }

+

//

// Fill up microcode patch info buffer according to FIT table.

//

@@ -447,37 +341,10 @@ ShadowMicrocode (
for (Index = 0; Index < EntryNum; Index++) {

if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE) {

MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) FitEntry[Index].Address;

-

- if (*(UINT32 *) MicrocodeEntryPoint == 0xFFFFFFFF) {

- //

- // An empty slot for reserved microcode update, skip to check next entry.

- //

- continue;

- }

-

- if (MicrocodeEntryPoint->HeaderVersion != 0x1) {

- //

- // Not a valid microcode header, skip to check next entry.

- //

- continue;

- }

-

- DataSize = MicrocodeEntryPoint->DataSize;

- TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;

- if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||

- (DataSize & 0x3) != 0 ||

- (TotalSize & (SIZE_1KB - 1)) != 0 ||

- TotalSize < DataSize

- ) {

- //

- // Not a valid microcode header, skip to check next entry.

- //

- continue;

- }

-

- if (IsMicrocodePatchNeedLoad (CpuIdCount, MicrocodeCpuId, MicrocodeEntryPoint)) {

- PatchInfoBuffer[PatchCount].Address = (UINTN) MicrocodeEntryPoint;

- PatchInfoBuffer[PatchCount].Size = TotalSize;

+ TotalSize = GetMicrocodeLength (MicrocodeEntryPoint);

+ if (IsValidMicrocode (MicrocodeEntryPoint, TotalSize, MicrocodeCpuId, CpuIdCount, FALSE)) {

+ PatchInfoBuffer[PatchCount].Address = (UINTN) MicrocodeEntryPoint;

+ PatchInfoBuffer[PatchCount].Size = TotalSize;

TotalLoadSize += TotalSize;

PatchCount++;

}

diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.inf b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.inf
index 581780add8..5ee225297d 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.inf
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocodePei.inf
@@ -1,7 +1,7 @@
### @file

# FIT based microcode shadow PEIM.

#

-# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>

+# Copyright (c) 2020 - 2021, Intel Corporation. All rights reserved.<BR>

#

# SPDX-License-Identifier: BSD-2-Clause-Patent

#

@@ -25,6 +25,7 @@
BaseMemoryLib

HobLib

PeiServicesLib

+ MicrocodeLib



[Packages]

MdePkg/MdePkg.dec

--
2.27.0.windows.1


Re: [GSoC proposal] Secure Image Loader

Marvin Häuser
 

We use the loader code in userspace anyway for fuzzing and such. I also want to build a database of all sorts of UEFI binaries some time before the merge to confirm they are all accepted (Windows / macOS / Linux bootloaders, tools like memtest, drivers like iPXE). As part of that, I'm sure we can have a userspace tool that uses the code to emit parsing information.

But as the EDK II build system is very... not so userspace friendly, I will not promise it will be very nice. :)

Best regards,
Marvin

On 08.04.21 16:13, Andrew (EFI) Fish wrote:
At a minimum it would be nice if we had a tool that would point out the security faults with a given PE/COFF file layout.

Sent from my iPhone

On Apr 8, 2021, at 4:16 AM, Laszlo Ersek <lersek@redhat.com> wrote:

On 04/06/21 12:06, Marvin Häuser wrote:
Good day Nate,

Comments are inline.

Best regards,
Marvin

On 06.04.21 11:41, Nate DeSimone wrote:
Hi Marvin,

Great to meet you and welcome back! Glad you hear you are interested!
Completing a formal verification of a PE/COFF loader is certainly
impressive. Was this done with some sort of automated theorem proving?
It would seem a rather arduous task doing an inductive proof for an
algorithm like that by hand!
I would call it "semi-automated", a great deal of intermediate goals
(preconditions, postconditions, invariants, assertions, ...) were
required to show all interesting properties. But yes, the actual proof
steps are automated by common SMT solvers. It was done using the
AstraVer Toolset and ACSL, latter basically a language to express logic
statements with C-like syntax.

I completely agree with you that getting a formally verified PE/COFF
loader into mainline is undoubtably valuable and would pay security
dividends for years to come.
I'm glad to hear that. :)

Admittedly, this is an area of computer science that I don't have a
great deal of experience with. The furthest I have gone on this topic
is writing out proofs for simple algorithms on exams in my Algorithms
class in college. Regardless you have a much better idea of what the
current status is of the work that you and Vitaly have done. I guess
my only question is do you think there is sufficient work remaining to
fill the 10 week GSoC development window?
Please don't get me wrong, but I would be surprised if the UEFI
specification changes I'd like to discuss alone would be completed
within 10 weeks, let alone implementation throughout the codebase. While
I think the plain amount of code may be a bit less than say a
MinPlatform port, the changes are much deeper and require much more
caution to avoid regressions (e.g. by invalidating undocumented
assertions). This sadly is not a matter of just replacing the underlying
library implementation or "plug-in and play" at all. It furthermore
affects many parts of the stack, the core dispatchers used for all
platforms, image emulation (EBC), UEFI userland emulation (EmuPkg), and
so on. I was rather worried the scope is too broad time-wise, but it can
be narrowed/widened as you see fit really. This is one of *the* core
components used on millions of device, and many package maintainers need
to review and validate the changes, this must really be done right the
first try. :)

Certainly we can use some of that time to perform the code reviews you
mention and write up formal ECRs for the UEFI spec changes that you
believe are needed.
I believed that was part of the workload, yes, but even without it I
think there is plenty to do.

Thank you for sending the application and alerting us to the great
work you and Vitaly have done! I'll read your paper more closely and
come back with any questions I still have.
Thank you, I will gladly explain anything unclear. Just try to not give
Laszlo too many flashbacks. :)
I haven't commented yet in this thread, as I thought my stance on this
undertaking was (or should be) obvious.

I very much welcome a replacement for the PE/COFF parser (as I consider
its security issues unfixable in an incremental manner). From my reading
of Marvin's and Vitaly's paper (draft), they have my full trust, and I'm
ready to put their upcoming code to use in ArmVirtPkg and OvmfPkg with
minimal actual code review. If fixing the pervasive security problems
around this area cannot avoid spiraling out to other core code in edk2,
such as dispatchers, and even to the PI / UEFI specs, so be it.

Regarding GSoC itself: as I stated elsewhere previously, I support
edk2's participation in GSoC, while at the same time I'm not
volunteering for mentorship at all. I'm uncertain if GSoC is the best
framework for upstreaming such a large undertaking, but if it can help,
we should use it as much as possible.

Thanks
Laszlo





With Best Regards,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
Häuser
Sent: Sunday, April 4, 2021 4:02 PM
To: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Andrew Fish
<afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [edk2-devel] [GSoC proposal] Secure Image Loader

Good day everyone,

I'll keep the introduction brief because I've been around for a while
now. :) I'm
Marvin Häuser, a third-year Computer Science student from TU
Kaiserslautern,
Germany. Late last year, my colleague Vitaly from ISP RAS and me
introduced a
formally verified Image Loader for UEFI usage at ISP RAS Open[1] due
to various
defects we outlined in the corresponding paper. Thank you once again
Laszlo
for your *incredible* review work on the publication part.

I now want to make an effort to mainline it, preferably as part of
the current
Google Summer of Code event. To be clear, my internship at ISP RAS has
concluded, and while Vitaly will be available for design discussion,
he has other
priorities at the moment and the practical part will be on me. I have
previously
submitted a proposal via the GSoC website for your review.

There are many things to consider:
1. The Image Loader is a core component, and there needs to be a
significant
level of quality and security assurance.
2. Being consumed by many packages, the proposed patch set will take
a lot of
time to review and integrate.
3. During my initial exploration, I discovered defective PPIs and
protocols (e.g.
returning data with no corresponding size) originating from the UEFI
PI and
UEFI specifications. Changes need to be discussed, settled on, and
submitted to
the UEFI Forum.
4. Some UEFI APIs like the Security Architecture protocols are
inconveniently
abstract, see 5.
5. Some of the current code does not use the existing context, or
accesses it
outside of the exposed APIs. The control flow of the dispatchers may
need to be
adapted to make the context available to appropriate APIs.

But obviously there are not only unpleasant considerations:
A. The Image Loader is mostly formally verified, and only very few
changes will
be required from the last proven state. This gives a lot of trust in
its correctness
and safety.
B. All outlined defects that are of critical nature have been fixed
successfully.
C. The Image Loader has been tested with real-world code loading
real-world
OSes on thousands of machines in the past few months, including
rejecting
malformed images (configurable by PCD).
D. The new APIs will centralise everything PE, reducing code
duplication and
potentially unsafe operations.
E. Centralising and reduced parse duplication may improve overall boot
performance.
F. The code has been coverage-tested to not contain dead code.
G. The code has been fuzz-tested including sanitizers to not invoke
undefined
behaviour.
H. I already managed to identify a malformed image in OVMF with its help
(incorrectly reported section alignment of an Intel IPXE driver). A
fix will be
submitted shortly.
I. I plan to support PE section permissions, allowing for read-only data
segments when enabled.

There are likely more points for both lists, but I hope this gives a
decent
starting point for discussion. What are your thoughts on the matter?
I strongly
encourage everyone to read the section regarding defects of our
publication[2]
to better understand the motivation. The vague points above can of
course be
elaborated in due time, as you see fit.

Thank you for your time!

Best regards,
Marvin


[1] https://github.com/mhaeuser/ISPRASOpen-SecurePE
[2] https://arxiv.org/pdf/2012.05471.pdf








TianoCore Community Meeting - EMEA / NAMO - Thu, 04/08/2021 9:00am-10:00am #cal-reminder

devel@edk2.groups.io Calendar <devel@...>
 

Reminder: TianoCore Community Meeting - EMEA / NAMO

When: Thursday, 8 April 2021, 9:00am to 10:00am, (GMT-07:00) America/Los Angeles

Where:https://teams.microsoft.com/l/meetup-join/19%3ameeting_MTVhZTE3MDAtOWM2NS00YTgyLWFjNTMtNDVjZTdjOGY2NjJl%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%22b286b53a-1218-4db3-bfc9-3d4c5aa7669e%22%7d

View Event

Organizer: Soumya Guptha soumya.k.guptha@...

Description:

________________________________________________________________________________

Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 112 501 054 0

Alternate VTC dialing instructions

Or call in (audio only)

+1 916-245-6934,,318337837#   United States, Sacramento

Phone Conference ID: 318 337 837#

Find a local number | Reset PIN

Learn More | Meeting options

________________________________________________________________________________

 


TianoCore Community Meeting - EMEA / NAMO - Thu, 04/08/2021 9:00am-10:00am #cal-reminder

devel@edk2.groups.io Calendar <devel@...>
 

Reminder: TianoCore Community Meeting - EMEA / NAMO

When: Thursday, 8 April 2021, 9:00am to 10:00am, (GMT-07:00) America/Los Angeles

Where:https://teams.microsoft.com/l/meetup-join/19%3ameeting_MTVhZTE3MDAtOWM2NS00YTgyLWFjNTMtNDVjZTdjOGY2NjJl%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%22b286b53a-1218-4db3-bfc9-3d4c5aa7669e%22%7d

View Event

Organizer: Soumya Guptha soumya.k.guptha@...

Description:

________________________________________________________________________________

Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 112 501 054 0

Alternate VTC dialing instructions

Or call in (audio only)

+1 916-245-6934,,318337837#   United States, Sacramento

Phone Conference ID: 318 337 837#

Find a local number | Reset PIN

Learn More | Meeting options

________________________________________________________________________________

 


Re: [edk2-platforms] [PATCH v3 0/3] Add HMAT tables for RD multi-chip platforms

Sami Mujawar
 

Pushed as d2339f3c5f9a..a8278bd8bafd

Thanks.

Regards,

Sami Mujawar


[PATCH v3] MdePkg/Cpuid.h: Define new element in CPUID Leaf(07h) data structure.

Jason Lou
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3309

Define new element(Hybird) in CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS
(07h) data structure.

Signed-off-by: Jason Lou <yun.lou@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
---
MdePkg/Include/Register/Intel/Cpuid.h | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/MdePkg/Include/Register/Intel/Cpuid.h b/MdePkg/Include/Registe=
r/Intel/Cpuid.h
index 19af99b6af..25ec65a746 100644
--- a/MdePkg/Include/Register/Intel/Cpuid.h
+++ b/MdePkg/Include/Register/Intel/Cpuid.h
@@ -6,7 +6,7 @@
If a register returned is a single 32-bit value, then a data structure i=
s=0D
not provided for that register.=0D
=0D
- Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>=0D
+ Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
@par Specification Reference:=0D
@@ -1550,9 +1550,17 @@ typedef union {
///=0D
UINT32 AVX512_4FMAPS:1;=0D
///=0D
- /// [Bit 25:4] Reserved.=0D
+ /// [Bit 14:4] Reserved.=0D
///=0D
- UINT32 Reserved2:22;=0D
+ UINT32 Reserved4:11;=0D
+ ///=0D
+ /// [Bit 15] Hybrid. If 1, the processor is identified as a hybrid par=
t.=0D
+ ///=0D
+ UINT32 Hybrid:1;=0D
+ ///=0D
+ /// [Bit 25:16] Reserved.=0D
+ ///=0D
+ UINT32 Reserved5:10;=0D
///=0D
/// [Bit 26] Enumerates support for indirect branch restricted specula=
tion=0D
/// (IBRS) and the indirect branch pre-dictor barrier (IBPB). Processo=
rs=0D
@@ -1581,7 +1589,7 @@ typedef union {
///=0D
/// [Bit 30] Reserved.=0D
///=0D
- UINT32 Reserved3:1;=0D
+ UINT32 Reserved6:1;=0D
///=0D
/// [Bit 31] Enumerates support for Speculative Store Bypass Disable (=
SSBD).=0D
/// Processors that set this bit sup-port the IA32_SPEC_CTRL MSR. They=
allow=0D
--=20
2.28.0.windows.1


Re: [PATCH v1] MdePkg/Cpuid.h: Define new element in CPUID Leaf(07h) data structure.

Jason Lou
 

Got it, the update will be included in the new patch(v3).

Thanks!
Jason Lou


Re: [PATCH 0/3] SD+USB perf/DMA fixes

Andrei Warkentin
 

I think Linux's behavior needs to be reconciled with the ACPI spec, which uses _DMA with ResourceConsumer, not ResourceProducer.

A


From: Jeremy Linton <jeremy.linton@...>
Sent: Thursday, April 8, 2021 12:58 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: ard.biesheuvel@... <ard.biesheuvel@...>; leif@... <leif@...>; pete@... <pete@...>; samer.el-haj-mahmoud@... <samer.el-haj-mahmoud@...>; Andrei Warkentin <awarkentin@...>; Jeremy Linton <jeremy.linton@...>
Subject: [PATCH 0/3] SD+USB perf/DMA fixes
 
A large part of why the emmc & dwc2 usb
controllers haven't been working properly is
because the "bus" _DMA was incorrectly tagged
as a consumer, when it needs to be a producer.

That is why linux has been dropping the
translation value portions of _DMA().

Since the emmc2 dma (with the old B0 SoC), and the
dwc2 is expected to work, lets add matching 30 bit
IORT entries for them.

Finally, in the shuffle the high speed cap bit override
was dropped from the linux patches, and I failed
to add it back to the firmware values, this caused
the wifi perf to be lower than it should have been.

Jeremy Linton (3):
  Platform/RaspberryPi/Acpitables: Enable Arasan hispeed mode
  Platform/RaspberryPi/AcpiTables: Add further named components
  Platform/RaspberryPi/AcpiTables: Correct _DMA consumer

 Platform/RaspberryPi/AcpiTables/Dsdt.asl  |  2 +-
 Platform/RaspberryPi/AcpiTables/Emmc.asl  |  2 +-
 Platform/RaspberryPi/AcpiTables/Iort.aslc | 44 ++++++++++++++++++++++++++++++-
 Platform/RaspberryPi/AcpiTables/Sdhc.asl  |  2 +-
 4 files changed, 46 insertions(+), 4 deletions(-)

--
2.13.7


Re: [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code

Dong, Eric
 

Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Friday, April 2, 2021 1:58 PM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/Microcode.c | 484 ++++--------------
UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 +
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 1 +
4 files changed, 96 insertions(+), 391 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 860a9750e2..d34419c2a5 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -52,6 +52,7 @@ [LibraryClasses]
SynchronizationLib

PcdLib

VmgExitLib

+ MicrocodeLib



[Protocols]

gEfiTimerArchProtocolGuid ## SOMETIMES_CONSUMES

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 297c2abcd1..105a9f84bf 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -1,70 +1,16 @@
/** @file

Implementation of loading microcode on processors.



- Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>

+ Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>

SPDX-License-Identifier: BSD-2-Clause-Patent



**/



#include "MpLib.h"



-/**

- Get microcode update signature of currently loaded microcode update.

-

- @return Microcode signature.

-**/

-UINT32

-GetCurrentMicrocodeSignature (

- VOID

- )

-{

- MSR_IA32_BIOS_SIGN_ID_REGISTER BiosSignIdMsr;

-

- AsmWriteMsr64 (MSR_IA32_BIOS_SIGN_ID, 0);

- AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, NULL);

- BiosSignIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_BIOS_SIGN_ID);

- return BiosSignIdMsr.Bits.MicrocodeUpdateSignature;

-}

-

/**

Detect whether specified processor can find matching microcode patch and load it.



- Microcode Payload as the following format:

- +----------------------------------------+------------------+

- | CPU_MICROCODE_HEADER | |

- +----------------------------------------+ CheckSum Part1 |

- | Microcode Binary | |

- +----------------------------------------+------------------+

- | CPU_MICROCODE_EXTENDED_TABLE_HEADER | |

- +----------------------------------------+ CheckSum Part2 |

- | CPU_MICROCODE_EXTENDED_TABLE | |

- | ... | |

- +----------------------------------------+------------------+

-

- There may by multiple CPU_MICROCODE_EXTENDED_TABLE in this format.

- The count of CPU_MICROCODE_EXTENDED_TABLE is indicated by ExtendedSignatureCount

- of CPU_MICROCODE_EXTENDED_TABLE_HEADER structure.

-

- When we are trying to verify the CheckSum32 with extended table.

- We should use the fields of exnteded table to replace the corresponding

- fields in CPU_MICROCODE_HEADER structure, and recalculate the

- CheckSum32 with CPU_MICROCODE_HEADER + Microcode Binary. We named

- it as CheckSum Part3.

-

- The CheckSum Part2 is used to verify the CPU_MICROCODE_EXTENDED_TABLE_HEADER

- and CPU_MICROCODE_EXTENDED_TABLE parts. We should make sure CheckSum Part2

- is correct before we are going to verify each CPU_MICROCODE_EXTENDED_TABLE.

-

- Only ProcessorSignature, ProcessorFlag and CheckSum are different between

- CheckSum Part1 and CheckSum Part3. To avoid multiple computing CheckSum Part3.

- Save an in-complete CheckSum32 from CheckSum Part1 for common parts.

- When we are going to calculate CheckSum32, just should use the corresponding part

- of the ProcessorSignature, ProcessorFlag and CheckSum with in-complete CheckSum32.

-

- Notes: CheckSum32 is not a strong verification.

- It does not guarantee that the data has not been modified.

- CPU has its own mechanism to verify Microcode Binary part.

-

@param[in] CpuMpData The pointer to CPU MP Data structure.

@param[in] ProcessorNumber The handle number of the processor. The range is

from 0 to the total number of logical processors

@@ -76,26 +22,13 @@ MicrocodeDetect (
IN UINTN ProcessorNumber

)

{

- UINT32 ExtendedTableLength;

- UINT32 ExtendedTableCount;

- CPU_MICROCODE_EXTENDED_TABLE *ExtendedTable;

- CPU_MICROCODE_EXTENDED_TABLE_HEADER *ExtendedTableHeader;

- CPU_MICROCODE_HEADER *MicrocodeEntryPoint;

+ CPU_MICROCODE_HEADER *Microcode;

UINTN MicrocodeEnd;

- UINTN Index;

- UINT8 PlatformId;

- CPUID_VERSION_INFO_EAX Eax;

- CPU_AP_DATA *CpuData;

- UINT32 CurrentRevision;

+ CPU_AP_DATA *BspData;

UINT32 LatestRevision;

- UINTN TotalSize;

- UINT32 CheckSum32;

- UINT32 InCompleteCheckSum32;

- BOOLEAN CorrectMicrocode;

- VOID *MicrocodeData;

- MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;

+ CPU_MICROCODE_HEADER *LatestMicrocode;

UINT32 ThreadId;

- BOOLEAN IsBspCallIn;

+ EDKII_PEI_MICROCODE_CPU_ID MicrocodeCpuId;



if (CpuMpData->MicrocodePatchRegionSize == 0) {

//

@@ -104,9 +37,6 @@ MicrocodeDetect (
return;

}



- CurrentRevision = GetCurrentMicrocodeSignature ();

- IsBspCallIn = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ? TRUE : FALSE;

-

GetProcessorLocationByApicId (GetInitialApicId (), NULL, NULL, &ThreadId);

if (ThreadId != 0) {

//

@@ -115,156 +45,35 @@ MicrocodeDetect (
return;

}



- ExtendedTableLength = 0;

- //

- // Here data of CPUID leafs have not been collected into context buffer, so

- // GetProcessorCpuid() cannot be used here to retrieve CPUID data.

- //

- AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);

+ GetProcessorMicrocodeCpuId (&MicrocodeCpuId);



- //

- // The index of platform information resides in bits 50:52 of MSR IA32_PLATFORM_ID

- //

- PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);

- PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;

-

-

- //

- // Check whether AP has same processor with BSP.

- // If yes, direct use microcode info saved by BSP.

- //

- if (!IsBspCallIn) {

+ if (ProcessorNumber != (UINTN) CpuMpData->BspNumber) {

//

- // Get the CPU data for BSP

+ // Direct use microcode of BSP if AP is the same as BSP.

+ // Assume BSP calls this routine() before AP.

//

- CpuData = &(CpuMpData->CpuData[CpuMpData->BspNumber]);

- if ((CpuData->ProcessorSignature == Eax.Uint32) &&

- (CpuData->PlatformId == PlatformId) &&

- (CpuData->MicrocodeEntryAddr != 0)) {

- MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *)(UINTN) CpuData->MicrocodeEntryAddr;

- MicrocodeData = (VOID *) (MicrocodeEntryPoint + 1);

- LatestRevision = MicrocodeEntryPoint->UpdateRevision;

- goto Done;

+ BspData = &(CpuMpData->CpuData[CpuMpData->BspNumber]);

+ if ((BspData->ProcessorSignature == MicrocodeCpuId.ProcessorSignature) &&

+ (BspData->PlatformId == MicrocodeCpuId.PlatformId) &&

+ (BspData->MicrocodeEntryAddr != 0)) {

+ LatestMicrocode = (CPU_MICROCODE_HEADER *)(UINTN) BspData->MicrocodeEntryAddr;

+ LatestRevision = LatestMicrocode->UpdateRevision;

+ goto LoadMicrocode;

}

}



- LatestRevision = 0;

- MicrocodeData = NULL;

- MicrocodeEnd = (UINTN) (CpuMpData->MicrocodePatchAddress + CpuMpData->MicrocodePatchRegionSize);

- MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;

+ //

+ // BSP or AP which is different from BSP runs here

+ // Use 0 as the starting revision to search for microcode because MicrocodePatchInfo HOB needs

+ // the latest microcode location even it's loaded to the processor.

+ //

+ LatestRevision = 0;

+ LatestMicrocode = NULL;

+ Microcode = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;

+ MicrocodeEnd = (UINTN) Microcode + (UINTN) CpuMpData->MicrocodePatchRegionSize;



do {

- //

- // Check if the microcode is for the Cpu and the version is newer

- // and the update can be processed on the platform

- //

- CorrectMicrocode = FALSE;

-

- if (MicrocodeEntryPoint->DataSize == 0) {

- TotalSize = sizeof (CPU_MICROCODE_HEADER) + 2000;

- } else {

- TotalSize = sizeof (CPU_MICROCODE_HEADER) + MicrocodeEntryPoint->DataSize;

- }

-

- ///

- /// 0x0 MicrocodeBegin MicrocodeEntry MicrocodeEnd 0xffffffff

- /// |--------------|---------------|---------------|---------------|

- /// valid TotalSize

- /// TotalSize is only valid between 0 and (MicrocodeEnd - MicrocodeEntry).

- /// And it should be aligned with 4 bytes.

- /// If the TotalSize is invalid, skip 1KB to check next entry.

- ///

- if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||

- ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||

- (TotalSize & 0x3) != 0

- ) {

- MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);

- continue;

- }

-

- //

- // Save an in-complete CheckSum32 from CheckSum Part1 for common parts.

- //

- InCompleteCheckSum32 = CalculateSum32 (

- (UINT32 *) MicrocodeEntryPoint,

- TotalSize

- );

- InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorSignature.Uint32;

- InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorFlags;

- InCompleteCheckSum32 -= MicrocodeEntryPoint->Checksum;

-

- if (MicrocodeEntryPoint->HeaderVersion == 0x1) {

- //

- // It is the microcode header. It is not the padding data between microcode patches

- // because the padding data should not include 0x00000001 and it should be the repeated

- // byte format (like 0xXYXYXYXY....).

- //

- if (MicrocodeEntryPoint->ProcessorSignature.Uint32 == Eax.Uint32 &&

- MicrocodeEntryPoint->UpdateRevision > LatestRevision &&

- (MicrocodeEntryPoint->ProcessorFlags & (1 << PlatformId))

- ) {

- //

- // Calculate CheckSum Part1.

- //

- CheckSum32 = InCompleteCheckSum32;

- CheckSum32 += MicrocodeEntryPoint->ProcessorSignature.Uint32;

- CheckSum32 += MicrocodeEntryPoint->ProcessorFlags;

- CheckSum32 += MicrocodeEntryPoint->Checksum;

- if (CheckSum32 == 0) {

- CorrectMicrocode = TRUE;

- }

- } else if ((MicrocodeEntryPoint->DataSize != 0) &&

- (MicrocodeEntryPoint->UpdateRevision > LatestRevision)) {

- ExtendedTableLength = MicrocodeEntryPoint->TotalSize - (MicrocodeEntryPoint->DataSize +

- sizeof (CPU_MICROCODE_HEADER));

- if (ExtendedTableLength != 0) {

- //

- // Extended Table exist, check if the CPU in support list

- //

- ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) (MicrocodeEntryPoint)

- + MicrocodeEntryPoint->DataSize + sizeof (CPU_MICROCODE_HEADER));

- //

- // Calculate Extended Checksum

- //

- if ((ExtendedTableLength % 4) == 0) {

- //

- // Calculate CheckSum Part2.

- //

- CheckSum32 = CalculateSum32 ((UINT32 *) ExtendedTableHeader, ExtendedTableLength);

- if (CheckSum32 == 0) {

- //

- // Checksum correct

- //

- ExtendedTableCount = ExtendedTableHeader->ExtendedSignatureCount;

- ExtendedTable = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);

- for (Index = 0; Index < ExtendedTableCount; Index ++) {

- //

- // Calculate CheckSum Part3.

- //

- CheckSum32 = InCompleteCheckSum32;

- CheckSum32 += ExtendedTable->ProcessorSignature.Uint32;

- CheckSum32 += ExtendedTable->ProcessorFlag;

- CheckSum32 += ExtendedTable->Checksum;

- if (CheckSum32 == 0) {

- //

- // Verify Header

- //

- if ((ExtendedTable->ProcessorSignature.Uint32 == Eax.Uint32) &&

- (ExtendedTable->ProcessorFlag & (1 << PlatformId)) ) {

- //

- // Find one

- //

- CorrectMicrocode = TRUE;

- break;

- }

- }

- ExtendedTable ++;

- }

- }

- }

- }

- }

- } else {

+ if (!IsValidMicrocode (Microcode, MicrocodeEnd - (UINTN) Microcode, LatestRevision, &MicrocodeCpuId, 1, TRUE)) {

//

// It is the padding data between the microcode patches for microcode patches alignment.

// Because the microcode patch is the multiple of 1-KByte, the padding data should not

@@ -272,156 +81,40 @@ MicrocodeDetect (
// alignment value should be larger than 1-KByte. We could skip SIZE_1KB padding data to

// find the next possible microcode patch header.

//

- MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);

+ Microcode = (CPU_MICROCODE_HEADER *) ((UINTN) Microcode + SIZE_1KB);

continue;

}

- //

- // Get the next patch.

- //

- if (MicrocodeEntryPoint->DataSize == 0) {

- TotalSize = 2048;

- } else {

- TotalSize = MicrocodeEntryPoint->TotalSize;

- }

+ LatestMicrocode = Microcode;

+ LatestRevision = LatestMicrocode->UpdateRevision;



- if (CorrectMicrocode) {

- LatestRevision = MicrocodeEntryPoint->UpdateRevision;

- MicrocodeData = (VOID *) ((UINTN) MicrocodeEntryPoint + sizeof (CPU_MICROCODE_HEADER));

- }

-

- MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + TotalSize);

- } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));

+ Microcode = (CPU_MICROCODE_HEADER *) (((UINTN) Microcode) + GetMicrocodeLength (Microcode));

+ } while ((UINTN) Microcode < MicrocodeEnd);



-Done:

+LoadMicrocode:

if (LatestRevision != 0) {

//

- // Save the detected microcode patch entry address (including the

- // microcode patch header) for each processor.

+ // Save the detected microcode patch entry address (including the microcode

+ // patch header) for each processor even it's the same as the loaded one.

// It will be used when building the microcode patch cache HOB.

//

- CpuMpData->CpuData[ProcessorNumber].MicrocodeEntryAddr =

- (UINTN) MicrocodeData - sizeof (CPU_MICROCODE_HEADER);

+ CpuMpData->CpuData[ProcessorNumber].MicrocodeEntryAddr = (UINTN) LatestMicrocode;

}



- if (LatestRevision > CurrentRevision) {

+ if (LatestRevision > GetProcessorMicrocodeSignature ()) {

//

// BIOS only authenticate updates that contain a numerically larger revision

// than the currently loaded revision, where Current Signature < New Update

// Revision. A processor with no loaded update is considered to have a

// revision equal to zero.

//

- ASSERT (MicrocodeData != NULL);

- AsmWriteMsr64 (

- MSR_IA32_BIOS_UPDT_TRIG,

- (UINT64) (UINTN) MicrocodeData

- );

- }

- CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetCurrentMicrocodeSignature ();

-}

-

-/**

- Determine if a microcode patch matchs the specific processor signature and flag.

-

- @param[in] CpuMpData The pointer to CPU MP Data structure.

- @param[in] ProcessorSignature The processor signature field value

- supported by a microcode patch.

- @param[in] ProcessorFlags The prcessor flags field value supported by

- a microcode patch.

-

- @retval TRUE The specified microcode patch will be loaded.

- @retval FALSE The specified microcode patch will not be loaded.

-**/

-BOOLEAN

-IsProcessorMatchedMicrocodePatch (

- IN CPU_MP_DATA *CpuMpData,

- IN UINT32 ProcessorSignature,

- IN UINT32 ProcessorFlags

- )

-{

- UINTN Index;

- CPU_AP_DATA *CpuData;

-

- for (Index = 0; Index < CpuMpData->CpuCount; Index++) {

- CpuData = &CpuMpData->CpuData[Index];

- if ((ProcessorSignature == CpuData->ProcessorSignature) &&

- (ProcessorFlags & (1 << CpuData->PlatformId)) != 0) {

- return TRUE;

- }

+ LoadMicrocode (LatestMicrocode);

}

-

- return FALSE;

-}

-

-/**

- Check the 'ProcessorSignature' and 'ProcessorFlags' of the microcode

- patch header with the CPUID and PlatformID of the processors within

- system to decide if it will be copied into memory.

-

- @param[in] CpuMpData The pointer to CPU MP Data structure.

- @param[in] MicrocodeEntryPoint The pointer to the microcode patch header.

-

- @retval TRUE The specified microcode patch need to be loaded.

- @retval FALSE The specified microcode patch dosen't need to be loaded.

-**/

-BOOLEAN

-IsMicrocodePatchNeedLoad (

- IN CPU_MP_DATA *CpuMpData,

- CPU_MICROCODE_HEADER *MicrocodeEntryPoint

- )

-{

- BOOLEAN NeedLoad;

- UINTN DataSize;

- UINTN TotalSize;

- CPU_MICROCODE_EXTENDED_TABLE_HEADER *ExtendedTableHeader;

- UINT32 ExtendedTableCount;

- CPU_MICROCODE_EXTENDED_TABLE *ExtendedTable;

- UINTN Index;

-

//

- // Check the 'ProcessorSignature' and 'ProcessorFlags' in microcode patch header.

+ // It's possible that the microcode fails to load. Just capture the CPU microcode revision after loading.

//

- NeedLoad = IsProcessorMatchedMicrocodePatch (

- CpuMpData,

- MicrocodeEntryPoint->ProcessorSignature.Uint32,

- MicrocodeEntryPoint->ProcessorFlags

- );

-

- //

- // If the Extended Signature Table exists, check if the processor is in the

- // support list

- //

- DataSize = MicrocodeEntryPoint->DataSize;

- TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;

- if ((!NeedLoad) && (DataSize != 0) &&

- (TotalSize - DataSize > sizeof (CPU_MICROCODE_HEADER) +

- sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER))) {

- ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) (MicrocodeEntryPoint)

- + DataSize + sizeof (CPU_MICROCODE_HEADER));

- ExtendedTableCount = ExtendedTableHeader->ExtendedSignatureCount;

- ExtendedTable = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);

-

- for (Index = 0; Index < ExtendedTableCount; Index ++) {

- //

- // Check the 'ProcessorSignature' and 'ProcessorFlag' of the Extended

- // Signature Table entry with the CPUID and PlatformID of the processors

- // within system to decide if it will be copied into memory

- //

- NeedLoad = IsProcessorMatchedMicrocodePatch (

- CpuMpData,

- ExtendedTable->ProcessorSignature.Uint32,

- ExtendedTable->ProcessorFlag

- );

- if (NeedLoad) {

- break;

- }

- ExtendedTable ++;

- }

- }

-

- return NeedLoad;

+ CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetProcessorMicrocodeSignature ();

}



-

/**

Actual worker function that shadows the required microcode patches into memory.



@@ -491,14 +184,16 @@ ShadowMicrocodePatchByPcd (
IN OUT CPU_MP_DATA *CpuMpData

)

{

+ UINTN Index;

CPU_MICROCODE_HEADER *MicrocodeEntryPoint;

UINTN MicrocodeEnd;

- UINTN DataSize;

UINTN TotalSize;

MICROCODE_PATCH_INFO *PatchInfoBuffer;

UINTN MaxPatchNumber;

UINTN PatchCount;

UINTN TotalLoadSize;

+ EDKII_PEI_MICROCODE_CPU_ID *MicrocodeCpuIds;

+ BOOLEAN Valid;



//

// Initialize the microcode patch related fields in CpuMpData as the values

@@ -526,12 +221,34 @@ ShadowMicrocodePatchByPcd (
return;

}



+ MicrocodeCpuIds = AllocatePages (

+ EFI_SIZE_TO_PAGES (CpuMpData->CpuCount * sizeof (EDKII_PEI_MICROCODE_CPU_ID))

+ );

+ if (MicrocodeCpuIds == NULL) {

+ FreePool (PatchInfoBuffer);

+ return;

+ }

+

+ for (Index = 0; Index < CpuMpData->CpuCount; Index++) {

+ MicrocodeCpuIds[Index].PlatformId = CpuMpData->CpuData[Index].PlatformId;

+ MicrocodeCpuIds[Index].ProcessorSignature = CpuMpData->CpuData[Index].ProcessorSignature;

+ }

+

//

// Process the header of each microcode patch within the region.

// The purpose is to decide which microcode patch(es) will be loaded into memory.

+ // Microcode checksum is not verified because it's slow when performing on flash.

//

do {

- if (MicrocodeEntryPoint->HeaderVersion != 0x1) {

+ Valid = IsValidMicrocode (

+ MicrocodeEntryPoint,

+ MicrocodeEnd - (UINTN) MicrocodeEntryPoint,

+ 0,

+ MicrocodeCpuIds,

+ CpuMpData->CpuCount,

+ FALSE

+ );

+ if (!Valid) {

//

// Padding data between the microcode patches, skip 1KB to check next entry.

//

@@ -539,59 +256,44 @@ ShadowMicrocodePatchByPcd (
continue;

}



- DataSize = MicrocodeEntryPoint->DataSize;

- TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;

- if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||

- ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||

- (DataSize & 0x3) != 0 ||

- (TotalSize & (SIZE_1KB - 1)) != 0 ||

- TotalSize < DataSize

- ) {

+ PatchCount++;

+ if (PatchCount > MaxPatchNumber) {

//

- // Not a valid microcode header, skip 1KB to check next entry.

+ // Current 'PatchInfoBuffer' cannot hold the information, double the size

+ // and allocate a new buffer.

//

- MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);

- continue;

- }

-

- if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {

- PatchCount++;

- if (PatchCount > MaxPatchNumber) {

+ if (MaxPatchNumber > MAX_UINTN / 2 / sizeof (MICROCODE_PATCH_INFO)) {

//

- // Current 'PatchInfoBuffer' cannot hold the information, double the size

- // and allocate a new buffer.

+ // Overflow check for MaxPatchNumber

//

- if (MaxPatchNumber > MAX_UINTN / 2 / sizeof (MICROCODE_PATCH_INFO)) {

- //

- // Overflow check for MaxPatchNumber

- //

- goto OnExit;

- }

-

- PatchInfoBuffer = ReallocatePool (

- MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),

- 2 * MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),

- PatchInfoBuffer

- );

- if (PatchInfoBuffer == NULL) {

- goto OnExit;

- }

- MaxPatchNumber = MaxPatchNumber * 2;

+ goto OnExit;

}



- //

- // Store the information of this microcode patch

- //

- PatchInfoBuffer[PatchCount - 1].Address = (UINTN) MicrocodeEntryPoint;

- PatchInfoBuffer[PatchCount - 1].Size = TotalSize;

- TotalLoadSize += TotalSize;

+ PatchInfoBuffer = ReallocatePool (

+ MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),

+ 2 * MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),

+ PatchInfoBuffer

+ );

+ if (PatchInfoBuffer == NULL) {

+ goto OnExit;

+ }

+ MaxPatchNumber = MaxPatchNumber * 2;

}



+ TotalSize = GetMicrocodeLength (MicrocodeEntryPoint);

+

+ //

+ // Store the information of this microcode patch

+ //

+ PatchInfoBuffer[PatchCount - 1].Address = (UINTN) MicrocodeEntryPoint;

+ PatchInfoBuffer[PatchCount - 1].Size = TotalSize;

+ TotalLoadSize += TotalSize;

+

//

// Process the next microcode patch

//

- MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + TotalSize);

- } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));

+ MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) ((UINTN) MicrocodeEntryPoint + TotalSize);

+ } while ((UINTN) MicrocodeEntryPoint < MicrocodeEnd);



if (PatchCount != 0) {

DEBUG ((

@@ -607,7 +309,7 @@ OnExit:
if (PatchInfoBuffer != NULL) {

FreePool (PatchInfoBuffer);

}

- return;

+ FreePages (MicrocodeCpuIds, EFI_SIZE_TO_PAGES (CpuMpData->CpuCount * sizeof (EDKII_PEI_MICROCODE_CPU_ID)));

}



/**

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 66f9eb2304..e88a5355c9 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -32,6 +32,7 @@
#include <Library/MtrrLib.h>

#include <Library/HobLib.h>

#include <Library/PcdLib.h>

+#include <Library/MicrocodeLib.h>



#include <Guid/MicrocodePatchHob.h>



diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
index 49b0ffe8be..36fcb96b58 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
@@ -51,6 +51,7 @@ [LibraryClasses]
PeiServicesLib

PcdLib

VmgExitLib

+ MicrocodeLib



[Pcd]

gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## CONSUMES

--
2.27.0.windows.1


Re: [PATCH 3/3] Platform/RaspberryPi/AcpiTables: Correct _DMA consumer

Andrei Warkentin
 

I don't know... the ACPI spec is weird.


...lists ResourceConsumer for _DMA.

A


From: Jeremy Linton <jeremy.linton@...>
Sent: Thursday, April 8, 2021 12:58 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: ard.biesheuvel@... <ard.biesheuvel@...>; leif@... <leif@...>; pete@... <pete@...>; samer.el-haj-mahmoud@... <samer.el-haj-mahmoud@...>; Andrei Warkentin <awarkentin@...>; Jeremy Linton <jeremy.linton@...>
Subject: [PATCH 3/3] Platform/RaspberryPi/AcpiTables: Correct _DMA consumer
 
Bridge devices should be marked as producers so that their
children can consume the resources. In linux if this isn't
true then the translation gets ignored and the DMA values
are incorrect. This fixes DMA on all the devices that
need a translation.

Signed-off-by: Jeremy Linton <jeremy.linton@...>
---
 Platform/RaspberryPi/AcpiTables/Dsdt.asl | 2 +-
 Platform/RaspberryPi/AcpiTables/Emmc.asl | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Platform/RaspberryPi/AcpiTables/Dsdt.asl b/Platform/RaspberryPi/AcpiTables/Dsdt.asl
index d116f965e1..32cd5fc9f9 100644
--- a/Platform/RaspberryPi/AcpiTables/Dsdt.asl
+++ b/Platform/RaspberryPi/AcpiTables/Dsdt.asl
@@ -205,7 +205,7 @@ DefinitionBlock ("Dsdt.aml", "DSDT", 5, "RPIFDN", "RPI", 2)
         // Only the first GB is available.

         // Bus 0xC0000000 -> CPU 0x00000000.

         //

-        QWordMemory (ResourceConsumer,

+        QWordMemory (ResourceProducer,

           ,

           MinFixed,

           MaxFixed,

diff --git a/Platform/RaspberryPi/AcpiTables/Emmc.asl b/Platform/RaspberryPi/AcpiTables/Emmc.asl
index 179dd3ecdb..0fbc2a79ea 100644
--- a/Platform/RaspberryPi/AcpiTables/Emmc.asl
+++ b/Platform/RaspberryPi/AcpiTables/Emmc.asl
@@ -32,7 +32,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPI4EMMC", 2)
       }

 

       Name (_DMA, ResourceTemplate() {

-        QWordMemory (ResourceConsumer,

+        QWordMemory (ResourceProducer,

           ,

           MinFixed,

           MaxFixed,

--
2.13.7

1061 - 1080 of 74863