Re: [PATCH 2/2] MdeModulePkg/PiSmmCpuDxeSmm: Check RegisterCpuInterruptHandler status

["Fan, Jeff" <jeff.fan@intel.com>]

Mike,

I think it's not necessary to move ASSERT() to SmmRegisterExceptionHandler() implementation.

Because PiSmmCpuDxeSmm will invoke SmmRegisterExceptionHandler() at module entry point function as my patch showed, it could make sure PiSmmCpuDxeSmm choose the correct instance. 

Other module invoke may check SmmRegisterExceptionHandler the return status for other choice per design.
For example:  
    @retval EFI_ALREADY_STARTED   InterruptHandler is not NULL, and a handler for InterruptType was previously installed.

Thanks!
Jeff


-----Original Message-----
From: Kinney, Michael D 
Sent: Thursday, November 17, 2016 12:29 AM
To: Fan, Jeff; edk2-devel@lists.01.org; Kinney, Michael D
Cc: Tian, Feng
Subject: RE: [PATCH 2/2] MdeModulePkg/PiSmmCpuDxeSmm: Check RegisterCpuInterruptHandler status

Jeff,

Shouldn't this ASSERT() be moved into the implementation of SmmRegisterExceptionHandler()?  This will make sure the PiSmmCpuDxeSmm driver uses the right lib instance for exceptions that module registers and for exceptions other SMM modules register through the EFI_SMM_CPU_SERVICE_PROTOCOL.

Mike

> -----Original Message-----
> From: Fan, Jeff
> Sent: Wednesday, November 16, 2016 6:34 AM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>
> Subject: [PATCH 2/2] MdeModulePkg/PiSmmCpuDxeSmm: Check 
> RegisterCpuInterruptHandler status
> 
> Once platform selects the incorrect instance, the caller could know it 
> from return status and ASSERT().
> 
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 4 +++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c   | 5 ++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c  | 4 +++-
>  3 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index a871bef..f2e2ceb 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -27,6 +27,7 @@ SmmInitPageTable (
>  {
>    UINTN                             PageFaultHandlerHookAddress;
>    IA32_IDT_GATE_DESCRIPTOR          *IdtEntry;
> +  EFI_STATUS                        Status;
> 
>    //
>    // Initialize spin lock
> @@ -49,7 +50,8 @@ SmmInitPageTable (
>      //
>      // Register SMM Page Fault Handler
>      //
> -    SmmRegisterExceptionHandler (&mSmmCpuService, EXCEPT_IA32_PAGE_FAULT,
> SmiPFHandler);
> +    Status = SmmRegisterExceptionHandler (&mSmmCpuService, 
> + EXCEPT_IA32_PAGE_FAULT,
> SmiPFHandler);
> +    ASSERT_EFI_ERROR (Status);
>    }
> 
>    //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> index 329574e..350a6a2 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> @@ -1384,5 +1384,8 @@ InitIdtr (
>    VOID
>    )
>  {
> -  SmmRegisterExceptionHandler (&mSmmCpuService, EXCEPT_IA32_DEBUG, 
> DebugExceptionHandler);
> +  EFI_STATUS                        Status;
> +
> +  Status = SmmRegisterExceptionHandler (&mSmmCpuService, 
> + EXCEPT_IA32_DEBUG,
> DebugExceptionHandler);
> +  ASSERT_EFI_ERROR (Status);
>  }
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 9cee784..c49fb26 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -102,6 +102,7 @@ SmmInitPageTable (
>    UINTN                             Index;
>    UINTN                             PageFaultHandlerHookAddress;
>    IA32_IDT_GATE_DESCRIPTOR          *IdtEntry;
> +  EFI_STATUS                        Status;
> 
>    //
>    // Initialize spin lock
> @@ -160,7 +161,8 @@ SmmInitPageTable (
>      //
>      // Register Smm Page Fault Handler
>      //
> -    SmmRegisterExceptionHandler (&mSmmCpuService, EXCEPT_IA32_PAGE_FAULT,
> SmiPFHandler);
> +    Status = SmmRegisterExceptionHandler (&mSmmCpuService, 
> + EXCEPT_IA32_PAGE_FAULT,
> SmiPFHandler);
> +    ASSERT_EFI_ERROR (Status);
>    }
> 
>    //
> --
> 2.9.3.windows.2