From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: supreeth.venkatesh@arm.com) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by groups.io with SMTP; Fri, 23 Aug 2019 09:51:43 -0700 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D538928; Fri, 23 Aug 2019 09:51:42 -0700 (PDT) Received: from supven01-thinkstation-p720.austin.arm.com (supven01-thinkstation-p720.austin.arm.com [10.118.30.53]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id D08D33F246; Fri, 23 Aug 2019 09:51:42 -0700 (PDT) Message-ID: <57e19cdac7501ab96f27512260bc3b65acd9930e.camel@arm.com> Subject: Re: [edk2-test][Patch V2 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test From: "Supreeth Venkatesh" To: Eric Jin , "devel@edk2.groups.io" Date: Fri, 23 Aug 2019 11:51:42 -0500 In-Reply-To: <20190823054613.11376-1-eric.jin@intel.com> References: <20190823054613.11376-1-eric.jin@intel.com> X-Mailer: Evolution 3.28.5-0ubuntu0.18.04.1 Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit On Fri, 2019-08-23 at 00:46 -0500, Eric Jin wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2098 > > In the ExitBootServices() test, after ExitBootServices() call, all > boot services are forbidden. The original design is to save the > return > status value of ExitBootServices() in variable using variable service > and reset, but this needs one additional execution of the test to > retrieve the value from variable and this design was not > straightforward > from end user perspective. > This patch enhances the test by leveraging RecoveryLib to restore > execution after reset automatically, thus requiring only one > execution. > > Cc: Supreeth Venkatesh > Signed-off-by: Eric Jin Please re-order the statements like below to make it more clear since ResetData is a just a pointer to Buffer Array which is statically allocated to 1024 and ResetData is not being really initialized to Reset Record after reading reset record, but to Buffer Array. 1. ResetData = (RESET_DATA *)Buffer; 2. RecoveryLib->ReadResetRecord(RecoveryLib, Size, Buffer); while commiting it. With that, Reviewed-by: Supreeth Venkatesh > --- > uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTest.inf | 3 ++- > uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTest.h | 9 ++++++++- > uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTestConformance.c | 121 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > ++++++++++++++++++++++++++++++++++++---------------- > 3 files changed, 115 insertions(+), 18 deletions(-) > > diff --git a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTest.inf b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTest.inf > index 49ad79915934..3de43a20e8a4 100644 > --- a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTest.inf > +++ b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTest.inf > @@ -1,7 +1,7 @@ > ## @file > # > # Copyright 2006 - 2012 Unified EFI, Inc.
> -# Copyright (c) 2010 - 2012, 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 > @@ -53,4 +53,5 @@ > > [Protocols] > gEfiTestProfileLibraryGuid > + gEfiTestRecoveryLibraryGuid > gBlackBoxEfiHIIPackageListProtocolGuid > diff --git a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTest.h b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTest.h > index b1c35fee7435..008584577ed1 100644 > --- a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTest.h > +++ b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTest.h > @@ -1,7 +1,7 @@ > /** @file > > Copyright 2006 - 2017 Unified EFI, Inc.
> - Copyright (c) 2010 - 2017, 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 > @@ -35,6 +35,13 @@ Abstract: > #include EFI_PROTOCOL_DEFINITION (LoadFile) > > #include EFI_TEST_PROTOCOL_DEFINITION (TestProfileLibrary) > +#include EFI_TEST_PROTOCOL_DEFINITION (TestRecoveryLibrary) > + > +typedef struct _RESET_DATA { > + UINTN Step; > + UINTN TplIndex; > + UINT32 RepeatTimes; > +} RESET_DATA; > > #if (EFI_SPECIFICATION_VERSION >= 0x0002000A) > > diff --git a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTestConformance.c b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTestConformance.c > index 0a26d46847da..6ce1d2d72669 100644 > --- a/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTestConformance.c > +++ b/uefi- > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/ > ImageBBTestConformance.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 > @@ -30,7 +30,8 @@ Abstract: > #define TEST_VENDOR1_GUID \ > { 0xF6FAB04F, 0xACAF, 0x4af3, { 0xB9, 0xFA, 0xDC, 0xF9, 0x7F, > 0xB4, 0x42, 0x6F } } > > -#define MAX_BUFFER_SIZE 10 > +#define STATUS_BUFFER_SIZE 8 > +#define RECOVER_BUFFER_SIZE 1024 > > EFI_GUID gTestVendor1Guid = TEST_VENDOR1_GUID; > > @@ -778,19 +779,23 @@ BBTestExitBootServicesConsistencyTest ( > ) > { > EFI_STANDARD_TEST_LIBRARY_PROTOCOL *StandardLib; > + EFI_TEST_RECOVERY_LIBRARY_PROTOCOL *RecoveryLib; > EFI_STATUS Status; > EFI_TEST_ASSERTION AssertionType; > UINTN MapKey; > - > + UINTN Size; > UINTN Numbers; > UINTN DataSize; > - UINT8 Data[MAX_BUFFER_SIZE]; > + RESET_DATA *ResetData; > + UINT8 Data[STATUS_BUFFER_SIZE]; > + UINT8 Buffer[RECOVER_BUFFER_SIZE]; > EFI_STATUS ReturnStatus; > > // > // Init > // > StandardLib = NULL; > + RecoveryLib = NULL; > > // > // Get the Standard Library Interface > @@ -803,6 +808,14 @@ BBTestExitBootServicesConsistencyTest ( > return Status; > } > > + Status = gtBS->HandleProtocol ( > + SupportHandle, > + &gEfiTestRecoveryLibraryGuid, > + (VOID **) &RecoveryLib); > + if (EFI_ERROR(Status)) { > + return Status; > + } > + > Status = ImageTestCheckForCleanEnvironment (&Numbers); > if (EFI_ERROR(Status)) { > StandardLib->RecordAssertion ( > @@ -819,25 +832,101 @@ BBTestExitBootServicesConsistencyTest ( > return Status; > } > > - DataSize = MAX_BUFFER_SIZE; > - Status = gtRT->GetVariable ( > - L"ExitBootServicesTestVariable", // > VariableName > - &gTestVendor1Guid, // > VendorGuid > - NULL, // > Attributes > - &DataSize, // > DataSize > - &ReturnStatus // > Data > - ); > + // > + // Read reset record > + // > + Status = RecoveryLib->ReadResetRecord ( > + RecoveryLib, > + &Size, > + Buffer > + ); > + ResetData = (RESET_DATA *)Buffer; > > - if (Status == EFI_SUCCESS) { > - goto CheckResult; > + // > + // The workflow to check the recovery data > + // > + if (EFI_SUCCESS == Status) { > + // > + // To check the recovery data size > + // > + if (Size == sizeof(RESET_DATA)){ > + // > + // To Check the recovery data > + // > + if (ResetData->Step == 1) { > + // > + // Step 1: find the valid recovery data, to retrive presaved > status from variable > + // > + DataSize = STATUS_BUFFER_SIZE; > + Status = gtRT->GetVariable ( > + L"ExitBootServicesTestVariable", // > VariableName > + &gTestVendor1Guid, // > VendorGuid > + NULL, // > Attributes > + &DataSize, // > DataSize > + &ReturnStatus // Data > + ); > + > + if (EFI_ERROR(Status)) { > + StandardLib->RecordAssertion ( > + StandardLib, > + EFI_TEST_ASSERTION_FAILED, > + gTestGenericFailureGuid, > + L"GetVariable - Can't get the test variable - > ExitBootServicesTestVariable", > + L"%a:%d:Status - %r", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + return Status; > + } > + goto CheckResult; > + } else { > + // > + // It is invalid recovery data > + // > + return EFI_LOAD_ERROR; > + } > + } else { > + // > + // The size of recovery data is invalid > + // > + return EFI_LOAD_ERROR; > + } > } > > + // > + // Step 0: No recovery data is found. > + // > + > // > // Print out some information to avoid the user thought it is an > error > // > - SctPrint (L"System will cold reset after 2 second. please run this > test again..."); > + SctPrint (L"System will cold reset after 2 second and test will be > resumed after reboot."); > gtBS->Stall (2000000); > > + > + ResetData->Step = 1; > + ResetData->TplIndex = 0; > + Status = RecoveryLib->WriteResetRecord ( > + RecoveryLib, > + sizeof (RESET_DATA), > + (UINT8*)ResetData > + ); > + if (EFI_ERROR(Status)) { > + StandardLib->RecordAssertion ( > + StandardLib, > + EFI_TEST_ASSERTION_FAILED, > + gTestGenericFailureGuid, > + L"TestRecoveryLib - WriteResetRecord", > + L"%a:%d:Status - %r", > + __FILE__, > + (UINTN)__LINE__, > + Status > + ); > + return Status; > + } > + > + > // > // Checkpoint 1: > // 3.5.2.1 ExitBootServices should not succeed with an invalid > MapKey > @@ -885,7 +974,7 @@ BBTestExitBootServicesConsistencyTest ( > //reset system > gtRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL); > > - // get var to get the status > + > CheckResult: > > if (ReturnStatus == EFI_INVALID_PARAMETER) {