Re: [edk2-test][Patch 1/1] uefi-sct/SctPkg:Fix flaw in BBTestCreateEventEx_Func_Sub3

["Jin, Eric" <eric.jin@intel.com>]

Supreeth,

Thank for the comment and add buffer layout description in committed code for better readable. Thanks.

Best Regards
Eric

-----Original Message-----
From: Supreeth Venkatesh [mailto:supreeth.venkatesh@arm.com] 
Sent: Friday, March 8, 2019 5:25 AM
To: Jin, Eric <eric.jin@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2-test][Patch 1/1] uefi-sct/SctPkg:Fix flaw in BBTestCreateEventEx_Func_Sub3

On Thu, 2019-03-07 at 09:23 +0800, Eric Jin wrote:
> The intention of test is to validate the signal sequence among three 
> events with gEfiEventMemoryMapChangeGuid and different Tpl. The call 
> of AllocatePages() causes memorymap change and trigger event Notify.
> But the test has an assumption that CreateEventEx() will not cause 
> memorymap change itself, which is not true. When memory pool is out of 
> resource, the memory service will be called to reserve more memory 
> pool.
> The fix is to filter the exception and only leave memorymap change 
> caused by AllocatePages() in test.
> 
> Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Jin <eric.jin@intel.com>


Minor comment about parenthesis to make multiple conditions priority more readable. Please fix this. With that
Reviewed-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>

> ---
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestMain.h          
> |  5 +++--
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEventEx.c
> | 26 +++++++++++++++-----------
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/Support.c                                           
> | 23 +++++++++++++++++++++--
>  3 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestMain.h b/uefi- 
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestMain.h
> index 4431b5b22888..87451f9f9a91 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestMain.h
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestMain.h
> @@ -1,7 +1,7 @@
>  /** @file
>  
>    Copyright 2006 - 2016 Unified EFI, Inc.<BR>
> -  Copyright (c) 2010 - 2016, Intel Corporation. All rights 
> reserved.<BR>
> +  Copyright (c) 2010 - 2019, Intel Corporation. All rights
> reserved.<BR>
>  
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of 
> the BSD License @@ -49,7 +49,8 @@ Abstract:
>                   Status                                     \
>                   );
>  
> -#define MAX_TEST_EVENT_NUM 3
> +#define MAX_TEST_EVENT_NUM    3
> +#define SIGNAL_CONTEXT        0xAA    //To check the buffer content
> is modifed by exception or not
>  //
>  // Prototypes: Test Cases
>  //
> diff --git a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEventEx.c
> b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEventEx.c
> index e2e173ab04c0..4a8e44e29b4e 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEventEx.c
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEventEx.c
> @@ -1,7 +1,7 @@
>  /** @file
>  
>    Copyright 2006 - 2016 Unified EFI, Inc.<BR>
> -  Copyright (c) 2010 - 2016, Intel Corporation. All rights 
> reserved.<BR>
> +  Copyright (c) 2010 - 2019, Intel Corporation. All rights
> reserved.<BR>
>  
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of 
> the BSD License @@ -192,6 +192,10 @@ BBTestCreateEventEx_Func (
>    BBTestCreateEventEx_Func_Sub2 (StandardLib);  #endif
>  
> +  //
> +  // The test for the EFI_EVENT_GROUP_MEMORY_MAP_CHANGE  // This 
> + event group is notified by the system when the memory map
> has changed.
> +  //
>    BBTestCreateEventEx_Func_Sub3 (StandardLib);
>  
>    //
> @@ -599,12 +603,12 @@ BBTestCreateEventEx_Func_Sub1 (
>    UINTN               Buffer[MAX_TEST_EVENT_NUM +
> MAX_TEST_EVENT_NUM*2];
>  
>    //
> -  // Initialize Buffer
> +  // Initialize Buffer as SIGNAL_CONTEXT
>    //
>    for (Index = 0; Index < MAX_TEST_EVENT_NUM; Index ++) {
>      Buffer[Index] = Index;
> -    Buffer[Index + MAX_TEST_EVENT_NUM + Index] = (UINTN)(-1);
> -    Buffer[Index + MAX_TEST_EVENT_NUM + 1 + Index] = (UINTN)(-1);
> +    Buffer[Index + MAX_TEST_EVENT_NUM + Index] =
> (UINTN)(SIGNAL_CONTEXT);
> +    Buffer[Index + MAX_TEST_EVENT_NUM + 1 + Index] =
> (UINTN)(SIGNAL_CONTEXT);
>    }
>  
>    //
> @@ -755,12 +759,12 @@ BBTestCreateEventEx_Func_Sub2 (
>    UINTN               Buffer[MAX_TEST_EVENT_NUM +
> MAX_TEST_EVENT_NUM*2];
>  
>    //
> -  // Initialize Buffer
> +  // Initialize Buffer as SIGNAL_CONTEXT
>    //
>    for (Index = 0; Index < MAX_TEST_EVENT_NUM; Index ++) {
>      Buffer[Index] = Index;
> -    Buffer[Index + MAX_TEST_EVENT_NUM + Index] = (UINTN)(-1);
> -    Buffer[Index + MAX_TEST_EVENT_NUM + 1 + Index] = (UINTN)(-1);
> +    Buffer[Index + MAX_TEST_EVENT_NUM + Index] =
> (UINTN)(SIGNAL_CONTEXT);
> +    Buffer[Index + MAX_TEST_EVENT_NUM + 1 + Index] =
> (UINTN)(SIGNAL_CONTEXT);
>    }
>  
>    //
> @@ -914,12 +918,12 @@ BBTestCreateEventEx_Func_Sub3 (
>    UINTN               Buffer[MAX_TEST_EVENT_NUM +
> MAX_TEST_EVENT_NUM*2];
>  
>    //
> -  // Initialize Buffer
> +  // Initialize Buffer as SIGNAL_CONTEXT
>    //
>    for (Index = 0; Index < MAX_TEST_EVENT_NUM; Index ++) {
>      Buffer[Index] = Index;
> -    Buffer[Index + MAX_TEST_EVENT_NUM + Index] = (UINTN)(-1);
> -    Buffer[Index + MAX_TEST_EVENT_NUM + 1 + Index] = (UINTN)(-1);
> +    Buffer[Index + MAX_TEST_EVENT_NUM + Index] =
> (UINTN)(SIGNAL_CONTEXT);
> +    Buffer[Index + MAX_TEST_EVENT_NUM + 1 + Index] =
> (UINTN)(SIGNAL_CONTEXT);
>    }
>  
>    //
> @@ -974,7 +978,7 @@ BBTestCreateEventEx_Func_Sub3 (
>    }
>    
>    //
> -  // Install a configuration table at TPL_NOTIFY
> +  // Call AllocatePage to change the memorymap
>    //
>    OldTpl = gtBS->RaiseTPL (TPL_NOTIFY);
>    
> diff --git a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/Support.c b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/Support.c
> index aa02383ed2c6..6013c75ff6cf 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/Support.c
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServi
> ces/BlackBoxTest/Support.c
> @@ -1,7 +1,7 @@
>  /** @file
>  
>    Copyright 2006 - 2010 Unified EFI, Inc.<BR>
> -  Copyright (c) 2010, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2010 - 2019, Intel Corporation. All rights
> reserved.<BR>
>  
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of 
> the BSD License @@ -64,10 +64,29 @@ NotifyFunctionTplEx(
>  
>      EventIndex = Buffer[0];
>  
> +    //
> +    // The special code check for the BBTestCreateEventEx_Func_Sub3
> +    // Besides AllocatePages(), CreateEventEx() may trigger the
> memorymap
> +    // change when it is out of resource in memory pool
> +    // Use SIGNAL_CONTEXT to block possible enter triggered by
> CreateEventEx
> +    //
> +    if (EventIndex != 2 && Buffer[4] == (UINTN)(SIGNAL_CONTEXT))
> +      return;
> +
> +    //
> +    // It is the code execution path as expect
> +    // To initial the Buffer to 0xFF
> +    //
> +    if (EventIndex == 2 && Buffer[1] == (UINTN)(SIGNAL_CONTEXT)) {
Priority order isnt readable. Better to add parenthesis.

> +      for (Index=1; Index<MAX_TEST_EVENT_NUM*2+1; Index++) {
> +        Buffer[Index] = (UINTN)(0xFF);
> +      }
> +    }
> +
>      Index = 3-EventIndex;
>  
>      while (1) {     
> -      if (Buffer[Index] == (UINTN)(-1)) {
> +      if (Buffer[Index] == (UINTN)(0xFF)) {
>          break;
>        } else {
>          Index += 2;