Re: [edk2-devel] [PATCH 1/5] CpuException: Avoid allocating code pages for DXE instance

["Dong, Eric" <eric.dong@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 8632 bytes --]

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

From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, May 24, 2022 4:02 PM
To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/5] CpuException: Avoid allocating code pages for DXE instance

Jian,
Ia32/ExceptionHandlerAsm.nasm is used by 32bit DxeCpuExceptionHandlerLib instance.

I agree the commit message is not correct. The commit message says
SEC still creates 32 entries but 32bit SEC creates 256 entries.

I will update the commit message to align to code behavior.

Thanks,
Ray

________________________________________
From: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Sent: Monday, May 23, 2022 0:40
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray
Cc: Dong, Eric
Subject: RE: [edk2-devel] [PATCH 1/5] CpuException: Avoid allocating code pages for DXE instance

Ray,

You changed "%rep 32" to "%rep 256" in Ia32/ExceptionHandlerAsm.nasm.
According to my understanding and your comments, this should be done
only to X64 code, right?

Regards,
Jian

> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Ni, Ray
> Sent: Friday, May 20, 2022 10:16 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Subject: [edk2-devel] [PATCH 1/5] CpuException: Avoid allocating code pages
> for DXE instance
>
> Today the DXE instance allocates code page and then copies the IDT
> vectors to the allocated code page. Then it fixes up the vector number
> in the IDT vector.
>
> But if we update the NASM file to generate 256 IDT vectors, there is
> no need to do the copy and fix-up.
>
> A side effect is up to 4096 bytes (HOOKAFTER_STUB_SIZE * 256) is
> used for 256 IDT vectors. While 32 IDT vectors only require 512 bytes.
>
> But considering the code logic simplification, 3.5K space is not a big
> deal. SEC instance still generates 32 IDT vectors so no impact to SEC.
> If 3.5K is too much a waste in PEI phase, we can enhance the code
> further to generate 32 vectors for PEI.
>
> Signed-off-by: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> ---
>  .../CpuExceptionHandlerLib/DxeException.c     | 22 -------------------
>  .../Ia32/ExceptionHandlerAsm.nasm             |  4 ++--
>  .../X64/ExceptionHandlerAsm.nasm              |  2 ++
>  .../X64/Xcode5ExceptionHandlerAsm.nasm        |  9 ++++----
>  4 files changed, 9 insertions(+), 28 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
> index 61f11e98f8..5083c4b8e8 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
> @@ -95,9 +95,6 @@ InitializeCpuInterruptHandlers (
>    IA32_DESCRIPTOR                 IdtDescriptor;
>
>    UINTN                           IdtEntryCount;
>
>    EXCEPTION_HANDLER_TEMPLATE_MAP  TemplateMap;
>
> -  UINTN                           Index;
>
> -  UINTN                           InterruptEntry;
>
> -  UINT8                           *InterruptEntryCode;
>
>    RESERVED_VECTORS_DATA           *ReservedVectors;
>
>    EFI_CPU_INTERRUPT_HANDLER       *ExternalInterruptHandler;
>
>
>
> @@ -138,25 +135,6 @@ InitializeCpuInterruptHandlers (
>    AsmGetTemplateAddressMap (&TemplateMap);
>
>    ASSERT (TemplateMap.ExceptionStubHeaderSize <= HOOKAFTER_STUB_SIZE);
>
>
>
> -  Status = gBS->AllocatePool (
>
> -                  EfiBootServicesCode,
>
> -                  TemplateMap.ExceptionStubHeaderSize * CPU_INTERRUPT_NUM,
>
> -                  (VOID **)&InterruptEntryCode
>
> -                  );
>
> -  ASSERT (!EFI_ERROR (Status) && InterruptEntryCode != NULL);
>
> -
>
> -  InterruptEntry = (UINTN)InterruptEntryCode;
>
> -  for (Index = 0; Index < CPU_INTERRUPT_NUM; Index++) {
>
> -    CopyMem (
>
> -      (VOID *)InterruptEntry,
>
> -      (VOID *)TemplateMap.ExceptionStart,
>
> -      TemplateMap.ExceptionStubHeaderSize
>
> -      );
>
> -    AsmVectorNumFixup ((VOID *)InterruptEntry, (UINT8)Index, (VOID
> *)TemplateMap.ExceptionStart);
>
> -    InterruptEntry += TemplateMap.ExceptionStubHeaderSize;
>
> -  }
>
> -
>
> -  TemplateMap.ExceptionStart                     = (UINTN)InterruptEntryCode;
>
>    mExceptionHandlerData.IdtEntryCount            = CPU_INTERRUPT_NUM;
>
>    mExceptionHandlerData.ReservedVectors          = ReservedVectors;
>
>    mExceptionHandlerData.ExternalInterruptHandler = ExternalInterruptHandler;
>
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nas
> m
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nas
> m
> index 3fe9aed1e8..8ed2b8f455 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nas
> m
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nas
> m
> @@ -33,7 +33,7 @@ ALIGN   8
>  ;
>
>  AsmIdtVectorBegin:
>
>  %assign Vector 0
>
> -%rep  32
>
> +%rep  256
>
>      push    byte %[Vector];
>
>      push    eax
>
>      mov     eax, ASM_PFX(CommonInterruptEntry)
>
> @@ -439,7 +439,7 @@ ASM_PFX(AsmGetTemplateAddressMap):
>
>
>      mov ebx, dword [ebp + 0x8]
>
>      mov dword [ebx],      AsmIdtVectorBegin
>
> -    mov dword [ebx + 0x4], (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
>
> +    mov dword [ebx + 0x4], (AsmIdtVectorEnd - AsmIdtVectorBegin) / 256
>
>      mov dword [ebx + 0x8], HookAfterStubBegin
>
>
>
>      popad
>
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> index 9a806d1f86..aaf8d622e6 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> @@ -31,6 +31,8 @@ SECTION .text
>
>
>  ALIGN   8
>
>
>
> +; Generate 32 IDT vectors.
>
> +; 32 IDT vectors are enough because interrupts (32+) are not enabled in SEC and
> PEI phase.
>
>  AsmIdtVectorBegin:
>
>  %assign Vector 0
>
>  %rep  32
>
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAs
> m.nasm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAs
> m.nasm
> index 9c72fa5815..7c0e3d3b0b 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAs
> m.nasm
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAs
> m.nasm
> @@ -53,9 +53,10 @@ SECTION .text
>
>
>  ALIGN   8
>
>
>
> +; Generate 256 IDT vectors.
>
>  AsmIdtVectorBegin:
>
>  %assign Vector 0
>
> -%rep  32
>
> +%rep  256
>
>      push    byte %[Vector]
>
>      push    rax
>
>      mov     rax, strict qword 0 ;    mov     rax, ASM_PFX(CommonInterruptEntry)
>
> @@ -453,16 +454,16 @@ global ASM_PFX(AsmGetTemplateAddressMap)
>  ASM_PFX(AsmGetTemplateAddressMap):
>
>      lea     rax, [AsmIdtVectorBegin]
>
>      mov     qword [rcx], rax
>
> -    mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
>
> +    mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 256
>
>      lea     rax, [HookAfterStubHeaderBegin]
>
>      mov     qword [rcx + 0x10], rax
>
>
>
>  ; Fix up CommonInterruptEntry address
>
>      lea    rax, [ASM_PFX(CommonInterruptEntry)]
>
>      lea    rcx, [AsmIdtVectorBegin]
>
> -%rep  32
>
> +%rep  256
>
>      mov    qword [rcx + (JmpAbsoluteAddress - 8 - HookAfterStubHeaderBegin)],
> rax
>
> -    add    rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
>
> +    add    rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 256
>
>  %endrep
>
>  ; Fix up HookAfterStubHeaderEnd
>
>      lea    rax, [HookAfterStubHeaderEnd]
>
> --
> 2.35.1.windows.2
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#89916): https://edk2.groups.io/g/devel/message/89916
> Mute This Topic: https://groups.io/mt/91231767/1768734
> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jian.j.wang@intel.com]
> -=-=-=-=-=-=
>

[-- Attachment #2: Type: text/html, Size: 29064 bytes --]