From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=supreeth.venkatesh@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 2EA3B211CFFEB for ; Thu, 7 Mar 2019 13:25:00 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id CCACEEBD; Thu, 7 Mar 2019 13:25:00 -0800 (PST) Received: from supven01-thinkstation-p720.austin.arm.com (supven01-thinkstation-p720.austin.arm.com [10.118.28.44]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 9BC283F575; Thu, 7 Mar 2019 13:25:00 -0800 (PST) Message-ID: <042f0558da5483ac6e95189a5bded172b28b914b.camel@arm.com> From: Supreeth Venkatesh To: Eric Jin , edk2-devel@lists.01.org Date: Thu, 07 Mar 2019 15:25:00 -0600 In-Reply-To: <20190307012349.2176-1-eric.jin@intel.com> References: <20190307012349.2176-1-eric.jin@intel.com> X-Mailer: Evolution 3.28.5-0ubuntu0.18.04.1 Mime-Version: 1.0 Subject: Re: [edk2-test][Patch 1/1] uefi-sct/SctPkg:Fix flaw in BBTestCreateEventEx_Func_Sub3 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Mar 2019 21:25:01 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit 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 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Jin Minor comment about parenthesis to make multiple conditions priority more readable. Please fix this. With that Reviewed-by: Supreeth Venkatesh > --- > 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.
> - Copyright (c) 2010 - 2016, Intel Corporation. All rights > reserved.
> + Copyright (c) 2010 - 2019, Intel Corporation. All rights > reserved.
> > 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.
> - Copyright (c) 2010 - 2016, Intel Corporation. All rights > reserved.
> + Copyright (c) 2010 - 2019, Intel Corporation. All rights > reserved.
> > 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.
> - Copyright (c) 2010, Intel Corporation. All rights reserved.
> + Copyright (c) 2010 - 2019, Intel Corporation. All rights > reserved.
> > 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 + Buffer[Index] = (UINTN)(0xFF); > + } > + } > + > Index = 3-EventIndex; > > while (1) { > - if (Buffer[Index] == (UINTN)(-1)) { > + if (Buffer[Index] == (UINTN)(0xFF)) { > break; > } else { > Index += 2;