[PATCH 0/4] Remove AP waking vector in Reset Vector

[PATCH 0/4] Remove AP waking vector in Reset Vector

[Zhiguang Liu]

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

Today for SEC core(not VTF-0), GenFv finds free 4K aligned space in
FV for AP waking vector and JMP to 4G-30h in the waking vector.
There is no usage of this today. Remove the logic to avoid confusing
and save spaces in reset vector.


Zhiguang Liu (4):
  BaseTools: Remove logic to create AP waking vector in GenFv
  UefiCpuPkg/SecCore: Remove AP waking Vector logic in SecCore
  OvmfPkg: Remove applicationProcessorEntryPoint
  UefiCpuPk/ResetVector: Remove AP waking vector from ResetVector

 BaseTools/Source/C/GenFv/GenFvInternalLib.c   | 199 ------------------
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm  |  15 +-
 .../XenResetVector/Ia16/ResetVectorVtf0.asm   |  16 +-
 UefiCpuPkg/ResetVector/Vtf0/Ia16/Init16.asm   |   7 -
 .../ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm |  15 +-
 UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb        |  36 +---
 6 files changed, 12 insertions(+), 276 deletions(-)

-- 
2.31.1.windows.1

[PATCH 1/4] BaseTools: Remove logic to create AP waking vector in GenFv

[Zhiguang Liu]

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

Today for SEC core(not VTF-0), GenFv finds free 4K aligned space in
FV for AP waking vector and JMP to 4G-30h in the waking vector.
There is no usage of this today. Remove the logic to avoid confusing
and save spaces in reset vector.

Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 BaseTools/Source/C/GenFv/GenFvInternalLib.c | 199 --------------------
 1 file changed, 199 deletions(-)

diff --git a/BaseTools/Source/C/GenFv/GenFvInternalLib.c b/BaseTools/Source/C/GenFv/GenFvInternalLib.c
index f466324d61..29c3363a50 100644
--- a/BaseTools/Source/C/GenFv/GenFvInternalLib.c
+++ b/BaseTools/Source/C/GenFv/GenFvInternalLib.c
@@ -116,63 +116,6 @@ CHAR8      *mFvbAlignmentName[] = {
   EFI_FVB2_ALIGNMENT_2G_STRING
 };
 
-//
-// This data array will be located at the base of the Firmware Volume Header (FVH)
-// in the boot block.  It must not exceed 14 bytes of code.  The last 2 bytes
-// will be used to keep the FVH checksum consistent.
-// This code will be run in response to a startup IPI for HT-enabled systems.
-//
-#define SIZEOF_STARTUP_DATA_ARRAY 0x10
-
-UINT8                                   m128kRecoveryStartupApDataArray[SIZEOF_STARTUP_DATA_ARRAY] = {
-  //
-  // EA D0 FF 00 F0               ; far jmp F000:FFD0
-  // 0, 0, 0, 0, 0, 0, 0, 0, 0,   ; Reserved bytes
-  // 0, 0                         ; Checksum Padding
-  //
-  0xEA,
-  0xD0,
-  0xFF,
-  0x0,
-  0xF0,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00
-};
-
-UINT8                                   m64kRecoveryStartupApDataArray[SIZEOF_STARTUP_DATA_ARRAY] = {
-  //
-  // EB CE                               ; jmp short ($-0x30)
-  // ; (from offset 0x0 to offset 0xFFD0)
-  // 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ; Reserved bytes
-  // 0, 0                                ; Checksum Padding
-  //
-  0xEB,
-  0xCE,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00,
-  0x00
-};
-
 FV_INFO                     mFvDataInfo;
 CAP_INFO                    mCapDataInfo;
 BOOLEAN                     mIsLargeFfs = FALSE;
@@ -1568,12 +1511,6 @@ Returns:
   EFI_PHYSICAL_ADDRESS      SecCorePhysicalAddress;
   INT32                     Ia32SecEntryOffset;
   UINT32                    *Ia32ResetAddressPtr;
-  UINT8                     *BytePointer;
-  UINT8                     *BytePointer2;
-  UINT16                    *WordPointer;
-  UINT16                    CheckSum;
-  UINT32                    IpiVector;
-  UINTN                     Index;
   EFI_FFS_FILE_STATE        SavedState;
   BOOLEAN                   Vtf0Detected;
   UINT32                    FfsHeaderSize;
@@ -1745,65 +1682,6 @@ if (MachineType == IMAGE_FILE_MACHINE_I386 || MachineType == IMAGE_FILE_MACHINE_
     Ia32ResetAddressPtr   = (UINT32 *) ((UINTN) FvImage->Eof - 4);
     *Ia32ResetAddressPtr  = (UINT32) (FvInfo->BaseAddress);
     DebugMsg (NULL, 0, 9, "update BFV base address in the top FV image", "BFV base address = 0x%llX.", (unsigned long long) FvInfo->BaseAddress);
-
-    //
-    // Update the Startup AP in the FVH header block ZeroVector region.
-    //
-    BytePointer   = (UINT8 *) ((UINTN) FvImage->FileImage);
-    if (FvInfo->Size <= 0x10000) {
-      BytePointer2 = m64kRecoveryStartupApDataArray;
-    } else if (FvInfo->Size <= 0x20000) {
-      BytePointer2 = m128kRecoveryStartupApDataArray;
-    } else {
-      BytePointer2 = m128kRecoveryStartupApDataArray;
-      //
-      // Find the position to place Ap reset vector, the offset
-      // between the position and the end of Fvrecovery.fv file
-      // should not exceed 128kB to prevent Ap reset vector from
-      // outside legacy E and F segment
-      //
-      Status = FindApResetVectorPosition (FvImage, &BytePointer);
-      if (EFI_ERROR (Status)) {
-        Error (NULL, 0, 3000, "Invalid", "FV image does not have enough space to place AP reset vector. The FV image needs to reserve at least 4KB of unused space.");
-        return EFI_ABORTED;
-      }
-    }
-
-    for (Index = 0; Index < SIZEOF_STARTUP_DATA_ARRAY; Index++) {
-      BytePointer[Index] = BytePointer2[Index];
-    }
-    //
-    // Calculate the checksum
-    //
-    CheckSum              = 0x0000;
-    WordPointer = (UINT16 *) (BytePointer);
-    for (Index = 0; Index < SIZEOF_STARTUP_DATA_ARRAY / 2; Index++) {
-      CheckSum = (UINT16) (CheckSum + ((UINT16) *WordPointer));
-      WordPointer++;
-    }
-    //
-    // Update the checksum field
-    //
-    WordPointer   = (UINT16 *) (BytePointer + SIZEOF_STARTUP_DATA_ARRAY - 2);
-    *WordPointer  = (UINT16) (0x10000 - (UINT32) CheckSum);
-
-    //
-    // IpiVector at the 4k aligned address in the top 2 blocks in the PEI FV.
-    //
-    IpiVector  = (UINT32) (FV_IMAGES_TOP_ADDRESS - ((UINTN) FvImage->Eof - (UINTN) BytePointer));
-    DebugMsg (NULL, 0, 9, "Startup AP Vector address", "IpiVector at 0x%X", (unsigned) IpiVector);
-    if ((IpiVector & 0xFFF) != 0) {
-      Error (NULL, 0, 3000, "Invalid", "Startup AP Vector address are not 4K aligned, because the FV size is not 4K aligned");
-      return EFI_ABORTED;
-    }
-    IpiVector  = IpiVector >> 12;
-    IpiVector  = IpiVector & 0xFF;
-
-    //
-    // Write IPI Vector at Offset FvrecoveryFileSize - 8
-    //
-    Ia32ResetAddressPtr   = (UINT32 *) ((UINTN) FvImage->Eof - 8);
-    *Ia32ResetAddressPtr  = IpiVector;
   } else if (MachineType == IMAGE_FILE_MACHINE_ARMTHUMB_MIXED) {
     //
     // Since the ARM reset vector is in the FV Header you really don't need a
@@ -4190,83 +4068,6 @@ Returns:
   return EFI_SUCCESS;
 }
 
-EFI_STATUS
-FindApResetVectorPosition (
-  IN  MEMORY_FILE  *FvImage,
-  OUT UINT8        **Pointer
-  )
-/*++
-
-Routine Description:
-
-  Find the position in this FvImage to place Ap reset vector.
-
-Arguments:
-
-  FvImage       Memory file for the FV memory image.
-  Pointer       Pointer to pointer to position.
-
-Returns:
-
-  EFI_NOT_FOUND   - No satisfied position is found.
-  EFI_SUCCESS     - The suitable position is return.
-
---*/
-{
-  EFI_FFS_FILE_HEADER   *PadFile;
-  UINT32                Index;
-  EFI_STATUS            Status;
-  UINT8                 *FixPoint;
-  UINT32                FileLength;
-
-  for (Index = 1; ;Index ++) {
-    //
-    // Find Pad File to add ApResetVector info
-    //
-    Status = GetFileByType (EFI_FV_FILETYPE_FFS_PAD, Index, &PadFile);
-    if (EFI_ERROR (Status) || (PadFile == NULL)) {
-      //
-      // No Pad file to be found.
-      //
-      break;
-    }
-    //
-    // Get Pad file size.
-    //
-    FileLength = GetFfsFileLength(PadFile);
-    FileLength = (FileLength + EFI_FFS_FILE_HEADER_ALIGNMENT - 1) & ~(EFI_FFS_FILE_HEADER_ALIGNMENT - 1);
-    //
-    // FixPoint must be align on 0x1000 relative to FvImage Header
-    //
-    FixPoint = (UINT8*) PadFile + GetFfsHeaderLength(PadFile);
-    FixPoint = FixPoint + 0x1000 - (((UINTN) FixPoint - (UINTN) FvImage->FileImage) & 0xFFF);
-    //
-    // FixPoint be larger at the last place of one fv image.
-    //
-    while (((UINTN) FixPoint + SIZEOF_STARTUP_DATA_ARRAY - (UINTN) PadFile) <= FileLength) {
-      FixPoint += 0x1000;
-    }
-    FixPoint -= 0x1000;
-
-    if ((UINTN) FixPoint < ((UINTN) PadFile + GetFfsHeaderLength(PadFile))) {
-      //
-      // No alignment FixPoint in this Pad File.
-      //
-      continue;
-    }
-
-    if ((UINTN) FvImage->Eof - (UINTN)FixPoint <= 0x20000) {
-      //
-      // Find the position to place ApResetVector
-      //
-      *Pointer = FixPoint;
-      return EFI_SUCCESS;
-    }
-  }
-
-  return EFI_NOT_FOUND;
-}
-
 EFI_STATUS
 ParseCapInf (
   IN  MEMORY_FILE  *InfFile,
-- 
2.31.1.windows.1

[PATCH 2/4] UefiCpuPkg/SecCore: Remove AP waking Vector logic in SecCore

[Zhiguang Liu]

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

There are two part of AP waking Vector logic in SecCore.
The first one working with GenFv to find a free 4K aligned space,
use the 4K aligned address as AP waking Vector and jump to 4G-30h,
and finally jump to ApStartup..
The second one hard code uses 4G-1000h as AP waking Vector and
jump to ApStartup.
Both usages are no longer used. Remove them.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb | 36 +++-----------------------
 1 file changed, 3 insertions(+), 33 deletions(-)

diff --git a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
index 1dfc4efe4c..df5f439c4e 100644
--- a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
+++ b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
@@ -24,18 +24,6 @@ USE16
 
     ORG     0h
 
-;
-; 0xFFFFF000
-;
-; We enter here with CS:IP = 0xFF00:0x0000. Do a far-jump to change CS to 0xF000
-; and IP to ApStartup.
-;
-ApVector:
-    mov     di, "AP"
-    jmp     0xF000:0xF000+ApStartup
-
-    TIMES 0xFC0-($-$$) nop
-
 ;
 ; This should be at 0xFFFFFFC0
 ;
@@ -45,14 +33,7 @@ ApVector:
 ;
 ReservedData:            DD 0eeeeeeeeh, 0eeeeeeeeh
 
-    TIMES 0xFD0-($-$$) nop
-;
-; This is located at 0xFFFFFFD0
-;
-    mov     di, "PA"
-    jmp     ApStartup
-
-    TIMES 0xFE0-($-$$) nop
+    TIMES 0x20-($-$$) nop
 ;
 ; Pointer to the entry point of the PEI core
 ; It is located at 0xFFFFFFE0, and is fixed up by some build tool
@@ -70,7 +51,7 @@ ASM_PFX(InterruptHandler):
     jmp     $
     iret
 
-    TIMES 0xFF0-($-$$) nop
+    TIMES 0x30-($-$$) nop
 ;
 ; For IA32, the reset vector must be at 0xFFFFFFF0, i.e., 4G-16 byte
 ; Execution starts here upon power-on/platform-reset.
@@ -78,7 +59,6 @@ ASM_PFX(InterruptHandler):
 ResetHandler:
     nop
     nop
-ApStartup:
     ;
     ; Jmp Rel16 instruction
     ; Use machine code directly in case of the assembler optimization
@@ -90,17 +70,7 @@ ApStartup:
     DB      0e9h
     DW      -3
 
-
-    TIMES 0xFF8-($-$$) nop
-;
-; Ap reset vector segment address is at 0xFFFFFFF8
-; This will be fixed up by some build tool,
-; so if the value 1..8 appears in the final FD image,
-; tool failure occurs
-;
-ApSegAddress:    dd      12345678h
-
-    TIMES 0xFFC-($-$$) nop
+    TIMES 0x3C-($-$$) nop
 ;
 ; BFV Base is at 0xFFFFFFFC
 ; This will be fixed up by some build tool,
-- 
2.31.1.windows.1

[PATCH 3/4] OvmfPkg: Remove applicationProcessorEntryPoint

[Zhiguang Liu]

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

Current reset vector uses 0xffffffe0 as AP waking vector, and expects
GenFv generates code aligned on a 4k boundary which will jump to this
location. However, some issues are listed below
1. GenFV doesn't generate code as the comment expects, because GenFv
assumes no modifications are required to the VTF-0 'Volume Top File'.
2. Even if removing VFT0 signature and let GenFv to modify, Genfv is
hard-code using another flash address 0xffffffd0.
3. In the same patch series, AP waking vector code is removed from
GenFv, because no such usage anymore. The existing of first two issues
also approve the usage is not available for a long time.

Therefore, remove AP waking vector related code.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm    | 15 +++------------
 OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm | 16 +++-------------
 2 files changed, 6 insertions(+), 25 deletions(-)

diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 12f2cedd67..8f94da89f7 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -160,22 +160,13 @@ guidedStructureEnd:
 
 ALIGN   16
 
-applicationProcessorEntryPoint:
 ;
-; Application Processors entry point
+; 0xffffffe0
 ;
-; GenFv generates code aligned on a 4k boundary which will jump to this
-; location.  (0xffffffe0)  This allows the Local APIC Startup IPI to be
-; used to wake up the application processors.
-;
-    jmp     EarlyApInitReal16
-
-ALIGN   8
-
-    DD      0
+    DD      0, 0, 0
 
 ;
-; The VTF signature
+; The VTF signature (0xffffffec)
 ;
 ; VTF-0 means that the VTF (Volume Top File) code does not require
 ; any fixups.
diff --git a/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
index 56749bdbc9..67156d8252 100644
--- a/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
@@ -39,23 +39,13 @@ xenPVHEntryPoint:
 
 BITS    16
 ALIGN   16
-
-applicationProcessorEntryPoint:
-;
-; Application Processors entry point
 ;
-; GenFv generates code aligned on a 4k boundary which will jump to this
-; location.  (0xffffffe0)  This allows the Local APIC Startup IPI to be
-; used to wake up the application processors.
+; 0xffffffe0
 ;
-    jmp     EarlyApInitReal16
-
-ALIGN   8
-
-    DD      0
+   DD      0, 0, 0
 
 ;
-; The VTF signature
+; The VTF signature (0xffffffec)
 ;
 ; VTF-0 means that the VTF (Volume Top File) code does not require
 ; any fixups.
-- 
2.31.1.windows.1

[PATCH 4/4] UefiCpuPk/ResetVector: Remove AP waking vector from ResetVector

[Zhiguang Liu]

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

Current reset vector uses 0xffffffe0 as AP waking vector, and expects
GenFv generates code aligned on a 4k boundary which will jump to this
location. However, some issues are listed below
1. GenFV doesn't generate code as the comment expects, because GenFv
assumes no modifications are required to the VTF-0 'Volume Top File'.
2. Even if removing VFT0 signature and let GenFv to modify, Genfv is
hard-code using another flash address 0xffffffd0.
3. In the same patch series, AP waking vector code is removed from
GenFv, because no such usage anymore. The existing of first two issues
also approve the usage is not available for a long time.

Therefore, remove AP waking vector related code.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiCpuPkg/ResetVector/Vtf0/Ia16/Init16.asm       |  7 -------
 .../ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm     | 15 +++------------
 2 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia16/Init16.asm b/UefiCpuPkg/ResetVector/Vtf0/Ia16/Init16.asm
index cbdadee166..02b9a0303c 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Ia16/Init16.asm
+++ b/UefiCpuPkg/ResetVector/Vtf0/Ia16/Init16.asm
@@ -17,13 +17,6 @@ EarlyBspInitReal16:
     mov     di, 'BP'
     jmp     short Main16
 
-;
-; @param[out] DI    'AP' to indicate application processor
-;
-EarlyApInitReal16:
-    mov     di, 'AP'
-    jmp     short Main16
-
 ;
 ; Modified:  EAX
 ;
diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm b/UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm
index fe5bbea803..384b1d4d98 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm
+++ b/UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm
@@ -32,22 +32,13 @@ ALIGN   16
     TIMES (0x1000 - 0x20) DB 0
 %endif
 
-applicationProcessorEntryPoint:
 ;
-; Application Processors entry point
+; 0xffffffe0
 ;
-; GenFv generates code aligned on a 4k boundary which will jump to this
-; location.  (0xffffffe0)  This allows the Local APIC Startup IPI to be
-; used to wake up the application processors.
-;
-    jmp     EarlyApInitReal16
-
-ALIGN   8
-
-    DD      0
+    DD      0, 0, 0
 
 ;
-; The VTF signature
+; The VTF signature (0xffffffec)
 ;
 ; VTF-0 means that the VTF (Volume Top File) code does not require
 ; any fixups.
-- 
2.31.1.windows.1

Re: [edk2-devel] [PATCH 0/4] Remove AP waking vector in Reset Vector

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhiguang Liu
> Sent: Monday, July 10, 2023 11:17 AM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: [edk2-devel] [PATCH 0/4] Remove AP waking vector in Reset Vector
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4494
> 
> Today for SEC core(not VTF-0), GenFv finds free 4K aligned space in
> FV for AP waking vector and JMP to 4G-30h in the waking vector.
> There is no usage of this today. Remove the logic to avoid confusing
> and save spaces in reset vector.
> 
> 
> Zhiguang Liu (4):
>   BaseTools: Remove logic to create AP waking vector in GenFv
>   UefiCpuPkg/SecCore: Remove AP waking Vector logic in SecCore
>   OvmfPkg: Remove applicationProcessorEntryPoint
>   UefiCpuPk/ResetVector: Remove AP waking vector from ResetVector
> 
>  BaseTools/Source/C/GenFv/GenFvInternalLib.c   | 199 ------------------
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm  |  15 +-
>  .../XenResetVector/Ia16/ResetVectorVtf0.asm   |  16 +-
>  UefiCpuPkg/ResetVector/Vtf0/Ia16/Init16.asm   |   7 -
>  .../ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm |  15 +-
>  UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb        |  36 +---
>  6 files changed, 12 insertions(+), 276 deletions(-)
> 
> --
> 2.31.1.windows.1
> 
> 
> 
> 
> 

回复: [edk2-devel] [PATCH 1/4] BaseTools: Remove logic to create AP waking vector in GenFv

[gaoliming]

Zhiguang:
  If there is no such usage any more, I agree to remove this logic.
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Zhiguang Liu
> 发送时间: 2023年7月10日 11:17
> 收件人: devel@edk2.groups.io
> 抄送: Zhiguang Liu <zhiguang.liu@intel.com>; Rebecca Cran
> <rebecca@bsdio.com>; Liming Gao <gaoliming@byosoft.com.cn>; Bob Feng
> <bob.c.feng@intel.com>; Yuwei Chen <yuwei.chen@intel.com>
> 主题: [edk2-devel] [PATCH 1/4] BaseTools: Remove logic to create AP waking
> vector in GenFv
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4494
> 
> Today for SEC core(not VTF-0), GenFv finds free 4K aligned space in
> FV for AP waking vector and JMP to 4G-30h in the waking vector.
> There is no usage of this today. Remove the logic to avoid confusing
> and save spaces in reset vector.
> 
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  BaseTools/Source/C/GenFv/GenFvInternalLib.c | 199 --------------------
>  1 file changed, 199 deletions(-)
> 
> diff --git a/BaseTools/Source/C/GenFv/GenFvInternalLib.c
> b/BaseTools/Source/C/GenFv/GenFvInternalLib.c
> index f466324d61..29c3363a50 100644
> --- a/BaseTools/Source/C/GenFv/GenFvInternalLib.c
> +++ b/BaseTools/Source/C/GenFv/GenFvInternalLib.c
> @@ -116,63 +116,6 @@ CHAR8      *mFvbAlignmentName[] = {
>    EFI_FVB2_ALIGNMENT_2G_STRING
>  };
> 
> -//
> -// This data array will be located at the base of the Firmware Volume
Header
> (FVH)
> -// in the boot block.  It must not exceed 14 bytes of code.  The last 2
bytes
> -// will be used to keep the FVH checksum consistent.
> -// This code will be run in response to a startup IPI for HT-enabled
systems.
> -//
> -#define SIZEOF_STARTUP_DATA_ARRAY 0x10
> -
> -UINT8
> m128kRecoveryStartupApDataArray[SIZEOF_STARTUP_DATA_ARRAY] = {
> -  //
> -  // EA D0 FF 00 F0               ; far jmp F000:FFD0
> -  // 0, 0, 0, 0, 0, 0, 0, 0, 0,   ; Reserved bytes
> -  // 0, 0                         ; Checksum Padding
> -  //
> -  0xEA,
> -  0xD0,
> -  0xFF,
> -  0x0,
> -  0xF0,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00
> -};
> -
> -UINT8
> m64kRecoveryStartupApDataArray[SIZEOF_STARTUP_DATA_ARRAY] = {
> -  //
> -  // EB CE                               ; jmp short ($-0x30)
> -  // ; (from offset 0x0 to offset 0xFFD0)
> -  // 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ; Reserved bytes
> -  // 0, 0                                ; Checksum Padding
> -  //
> -  0xEB,
> -  0xCE,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00,
> -  0x00
> -};
> -
>  FV_INFO                     mFvDataInfo;
>  CAP_INFO                    mCapDataInfo;
>  BOOLEAN                     mIsLargeFfs = FALSE;
> @@ -1568,12 +1511,6 @@ Returns:
>    EFI_PHYSICAL_ADDRESS      SecCorePhysicalAddress;
>    INT32                     Ia32SecEntryOffset;
>    UINT32                    *Ia32ResetAddressPtr;
> -  UINT8                     *BytePointer;
> -  UINT8                     *BytePointer2;
> -  UINT16                    *WordPointer;
> -  UINT16                    CheckSum;
> -  UINT32                    IpiVector;
> -  UINTN                     Index;
>    EFI_FFS_FILE_STATE        SavedState;
>    BOOLEAN                   Vtf0Detected;
>    UINT32                    FfsHeaderSize;
> @@ -1745,65 +1682,6 @@ if (MachineType == IMAGE_FILE_MACHINE_I386
> || MachineType == IMAGE_FILE_MACHINE_
>      Ia32ResetAddressPtr   = (UINT32 *) ((UINTN) FvImage->Eof - 4);
>      *Ia32ResetAddressPtr  = (UINT32) (FvInfo->BaseAddress);
>      DebugMsg (NULL, 0, 9, "update BFV base address in the top FV image",
> "BFV base address = 0x%llX.", (unsigned long long) FvInfo->BaseAddress);
> -
> -    //
> -    // Update the Startup AP in the FVH header block ZeroVector region.
> -    //
> -    BytePointer   = (UINT8 *) ((UINTN) FvImage->FileImage);
> -    if (FvInfo->Size <= 0x10000) {
> -      BytePointer2 = m64kRecoveryStartupApDataArray;
> -    } else if (FvInfo->Size <= 0x20000) {
> -      BytePointer2 = m128kRecoveryStartupApDataArray;
> -    } else {
> -      BytePointer2 = m128kRecoveryStartupApDataArray;
> -      //
> -      // Find the position to place Ap reset vector, the offset
> -      // between the position and the end of Fvrecovery.fv file
> -      // should not exceed 128kB to prevent Ap reset vector from
> -      // outside legacy E and F segment
> -      //
> -      Status = FindApResetVectorPosition (FvImage, &BytePointer);
> -      if (EFI_ERROR (Status)) {
> -        Error (NULL, 0, 3000, "Invalid", "FV image does not have enough
> space to place AP reset vector. The FV image needs to reserve at least 4KB
of
> unused space.");
> -        return EFI_ABORTED;
> -      }
> -    }
> -
> -    for (Index = 0; Index < SIZEOF_STARTUP_DATA_ARRAY; Index++) {
> -      BytePointer[Index] = BytePointer2[Index];
> -    }
> -    //
> -    // Calculate the checksum
> -    //
> -    CheckSum              = 0x0000;
> -    WordPointer = (UINT16 *) (BytePointer);
> -    for (Index = 0; Index < SIZEOF_STARTUP_DATA_ARRAY / 2; Index++) {
> -      CheckSum = (UINT16) (CheckSum + ((UINT16) *WordPointer));
> -      WordPointer++;
> -    }
> -    //
> -    // Update the checksum field
> -    //
> -    WordPointer   = (UINT16 *) (BytePointer +
> SIZEOF_STARTUP_DATA_ARRAY - 2);
> -    *WordPointer  = (UINT16) (0x10000 - (UINT32) CheckSum);
> -
> -    //
> -    // IpiVector at the 4k aligned address in the top 2 blocks in the PEI
FV.
> -    //
> -    IpiVector  = (UINT32) (FV_IMAGES_TOP_ADDRESS - ((UINTN)
> FvImage->Eof - (UINTN) BytePointer));
> -    DebugMsg (NULL, 0, 9, "Startup AP Vector address", "IpiVector at
0x%X",
> (unsigned) IpiVector);
> -    if ((IpiVector & 0xFFF) != 0) {
> -      Error (NULL, 0, 3000, "Invalid", "Startup AP Vector address are not
4K
> aligned, because the FV size is not 4K aligned");
> -      return EFI_ABORTED;
> -    }
> -    IpiVector  = IpiVector >> 12;
> -    IpiVector  = IpiVector & 0xFF;
> -
> -    //
> -    // Write IPI Vector at Offset FvrecoveryFileSize - 8
> -    //
> -    Ia32ResetAddressPtr   = (UINT32 *) ((UINTN) FvImage->Eof - 8);
> -    *Ia32ResetAddressPtr  = IpiVector;
>    } else if (MachineType == IMAGE_FILE_MACHINE_ARMTHUMB_MIXED) {
>      //
>      // Since the ARM reset vector is in the FV Header you really don't
need
> a
> @@ -4190,83 +4068,6 @@ Returns:
>    return EFI_SUCCESS;
>  }
> 
> -EFI_STATUS
> -FindApResetVectorPosition (
> -  IN  MEMORY_FILE  *FvImage,
> -  OUT UINT8        **Pointer
> -  )
> -/*++
> -
> -Routine Description:
> -
> -  Find the position in this FvImage to place Ap reset vector.
> -
> -Arguments:
> -
> -  FvImage       Memory file for the FV memory image.
> -  Pointer       Pointer to pointer to position.
> -
> -Returns:
> -
> -  EFI_NOT_FOUND   - No satisfied position is found.
> -  EFI_SUCCESS     - The suitable position is return.
> -
> ---*/
> -{
> -  EFI_FFS_FILE_HEADER   *PadFile;
> -  UINT32                Index;
> -  EFI_STATUS            Status;
> -  UINT8                 *FixPoint;
> -  UINT32                FileLength;
> -
> -  for (Index = 1; ;Index ++) {
> -    //
> -    // Find Pad File to add ApResetVector info
> -    //
> -    Status = GetFileByType (EFI_FV_FILETYPE_FFS_PAD, Index, &PadFile);
> -    if (EFI_ERROR (Status) || (PadFile == NULL)) {
> -      //
> -      // No Pad file to be found.
> -      //
> -      break;
> -    }
> -    //
> -    // Get Pad file size.
> -    //
> -    FileLength = GetFfsFileLength(PadFile);
> -    FileLength = (FileLength + EFI_FFS_FILE_HEADER_ALIGNMENT - 1) &
> ~(EFI_FFS_FILE_HEADER_ALIGNMENT - 1);
> -    //
> -    // FixPoint must be align on 0x1000 relative to FvImage Header
> -    //
> -    FixPoint = (UINT8*) PadFile + GetFfsHeaderLength(PadFile);
> -    FixPoint = FixPoint + 0x1000 - (((UINTN) FixPoint - (UINTN)
> FvImage->FileImage) & 0xFFF);
> -    //
> -    // FixPoint be larger at the last place of one fv image.
> -    //
> -    while (((UINTN) FixPoint + SIZEOF_STARTUP_DATA_ARRAY - (UINTN)
> PadFile) <= FileLength) {
> -      FixPoint += 0x1000;
> -    }
> -    FixPoint -= 0x1000;
> -
> -    if ((UINTN) FixPoint < ((UINTN) PadFile +
GetFfsHeaderLength(PadFile)))
> {
> -      //
> -      // No alignment FixPoint in this Pad File.
> -      //
> -      continue;
> -    }
> -
> -    if ((UINTN) FvImage->Eof - (UINTN)FixPoint <= 0x20000) {
> -      //
> -      // Find the position to place ApResetVector
> -      //
> -      *Pointer = FixPoint;
> -      return EFI_SUCCESS;
> -    }
> -  }
> -
> -  return EFI_NOT_FOUND;
> -}
> -
>  EFI_STATUS
>  ParseCapInf (
>    IN  MEMORY_FILE  *InfFile,
> --
> 2.31.1.windows.1
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 3/4] OvmfPkg: Remove applicationProcessorEntryPoint

[Zhiguang Liu]

Hi OvmfPkg maintainer & reviewer,

Please help review this patch.

Thanks
Zhiguang

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Zhiguang Liu
> Sent: Monday, July 10, 2023 11:17 AM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Yao, Jiewen <jiewen.yao@intel.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Anthony Perard <anthony.perard@citrix.com>; Julien
> Grall <julien@xen.org>
> Subject: [edk2-devel] [PATCH 3/4] OvmfPkg: Remove
> applicationProcessorEntryPoint
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4494
> 
> Current reset vector uses 0xffffffe0 as AP waking vector, and expects GenFv
> generates code aligned on a 4k boundary which will jump to this location.
> However, some issues are listed below 1. GenFV doesn't generate code as
> the comment expects, because GenFv assumes no modifications are
> required to the VTF-0 'Volume Top File'.
> 2. Even if removing VFT0 signature and let GenFv to modify, Genfv is hard-
> code using another flash address 0xffffffd0.
> 3. In the same patch series, AP waking vector code is removed from GenFv,
> because no such usage anymore. The existing of first two issues also approve
> the usage is not available for a long time.
> 
> Therefore, remove AP waking vector related code.
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm    | 15 +++------------
>  OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm | 16 +++-------------
>  2 files changed, 6 insertions(+), 25 deletions(-)
> 
> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> index 12f2cedd67..8f94da89f7 100644
> --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> @@ -160,22 +160,13 @@ guidedStructureEnd:
> 
>  ALIGN   16
> 
> -applicationProcessorEntryPoint:
>  ;
> -; Application Processors entry point
> +; 0xffffffe0
>  ;
> -; GenFv generates code aligned on a 4k boundary which will jump to this -;
> location.  (0xffffffe0)  This allows the Local APIC Startup IPI to be -; used to
> wake up the application processors.
> -;
> -    jmp     EarlyApInitReal16
> -
> -ALIGN   8
> -
> -    DD      0
> +    DD      0, 0, 0
> 
>  ;
> -; The VTF signature
> +; The VTF signature (0xffffffec)
>  ;
>  ; VTF-0 means that the VTF (Volume Top File) code does not require  ; any
> fixups.
> diff --git a/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> index 56749bdbc9..67156d8252 100644
> --- a/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> +++ b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> @@ -39,23 +39,13 @@ xenPVHEntryPoint:
> 
>  BITS    16
>  ALIGN   16
> -
> -applicationProcessorEntryPoint:
> -;
> -; Application Processors entry point
>  ;
> -; GenFv generates code aligned on a 4k boundary which will jump to this -;
> location.  (0xffffffe0)  This allows the Local APIC Startup IPI to be -; used to
> wake up the application processors.
> +; 0xffffffe0
>  ;
> -    jmp     EarlyApInitReal16
> -
> -ALIGN   8
> -
> -    DD      0
> +   DD      0, 0, 0
> 
>  ;
> -; The VTF signature
> +; The VTF signature (0xffffffec)
>  ;
>  ; VTF-0 means that the VTF (Volume Top File) code does not require  ; any
> fixups.
> --
> 2.31.1.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107155): https://edk2.groups.io/g/devel/message/107155
Mute This Topic: https://groups.io/mt/100322593/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/4] OvmfPkg: Remove applicationProcessorEntryPoint

[Anthony PERARD via groups.io]

On Mon, Jul 10, 2023 at 11:17:05AM +0800, Zhiguang Liu wrote:
> diff --git a/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> index 56749bdbc9..67156d8252 100644
> --- a/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> +++ b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> @@ -39,23 +39,13 @@ xenPVHEntryPoint:
>  
>  BITS    16
>  ALIGN   16
> -
> -applicationProcessorEntryPoint:
> -;
> -; Application Processors entry point
>  ;
> -; GenFv generates code aligned on a 4k boundary which will jump to this
> -; location.  (0xffffffe0)  This allows the Local APIC Startup IPI to be
> -; used to wake up the application processors.
> +; 0xffffffe0
>  ;
> -    jmp     EarlyApInitReal16
> -
> -ALIGN   8
> -
> -    DD      0
> +   DD      0, 0, 0
>  
>  ;
> -; The VTF signature
> +; The VTF signature (0xffffffec)
>  ;
>  ; VTF-0 means that the VTF (Volume Top File) code does not require
>  ; any fixups.


I've tested that part of the patch. OvmfXen seems to work fine with it:

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107167): https://edk2.groups.io/g/devel/message/107167
Mute This Topic: https://groups.io/mt/100051790/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-