* [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test @ 2019-08-21 6:24 Eric Jin 2019-08-21 16:43 ` Supreeth Venkatesh 0 siblings, 1 reply; 6+ messages in thread From: Eric Jin @ 2019-08-21 6:24 UTC (permalink / raw) To: devel; +Cc: Supreeth Venkatesh REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2098 Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> Signed-off-by: Eric Jin <eric.jin@intel.com> --- 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------- 3 files changed, 93 insertions(+), 17 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.<BR> -# Copyright (c) 2010 - 2012, 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 @@ -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.<BR> - Copyright (c) 2010 - 2017, 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 @@ -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..e90afe7ecae0 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.<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 @@ -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,80 @@ 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) { + if (EFI_ERROR(Status) || (Size < sizeof(RESET_DATA))) { + // + // Step 1 + // + } else if (ResetData->Step == 1) { + // + // Step 2 + // + 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 { + return EFI_LOAD_ERROR; } // // 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), + Buffer + ); + 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 +953,7 @@ BBTestExitBootServicesConsistencyTest ( //reset system gtRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL); - // get var to get the status + CheckResult: if (ReturnStatus == EFI_INVALID_PARAMETER) { -- 2.20.0.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test 2019-08-21 6:24 [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test Eric Jin @ 2019-08-21 16:43 ` Supreeth Venkatesh 2019-08-22 1:50 ` [edk2-devel] " Eric Jin 0 siblings, 1 reply; 6+ messages in thread From: Supreeth Venkatesh @ 2019-08-21 16:43 UTC (permalink / raw) To: Eric Jin, devel@edk2.groups.io On Wed, 2019-08-21 at 01:24 -0500, Eric Jin wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2098 Please add the details of the patch to the commit message. "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 multiple 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" More comments inline... > > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> > Signed-off-by: Eric Jin <eric.jin@intel.com> > --- > 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 | 98 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > ++++++++++++++--------------- > 3 files changed, 93 insertions(+), 17 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.<BR> > -# Copyright (c) 2010 - 2012, 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 > @@ -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.<BR> > - Copyright (c) 2010 - 2017, 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 > @@ -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..e90afe7ecae0 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.<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 > @@ -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 Why is the size being reduced by 2 bytes? Was the earlier size not optimal? > +#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,80 @@ 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 > + ); Status should be checked before proceeding further. Unhandled error condition. > + ResetData = (RESET_DATA *)Buffer; Before initializing, size check should be performed. Unhandled error condition. > > - if (Status == EFI_SUCCESS) { > + if (EFI_ERROR(Status) || (Size < sizeof(RESET_DATA))) { > + // > + // Step 1 > + // This check is unnecessary if the above comments regarding unhandled error condition are resolved. > + } else if (ResetData->Step == 1) { > + // > + // Step 2 > + // > + 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 { > + return EFI_LOAD_ERROR; > } > > // > // 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), > + Buffer To make it clear it should be (UINT8 *)ResetData. As Buffer is of size 1024, where as ResetData is not. i.e., Buffer should be used above. > + ); > + 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 +953,7 @@ BBTestExitBootServicesConsistencyTest ( > //reset system > gtRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL); > > - // get var to get the status > + > CheckResult: > > if (ReturnStatus == EFI_INVALID_PARAMETER) { ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test 2019-08-21 16:43 ` Supreeth Venkatesh @ 2019-08-22 1:50 ` Eric Jin 2019-08-22 18:43 ` Supreeth Venkatesh 0 siblings, 1 reply; 6+ messages in thread From: Eric Jin @ 2019-08-22 1:50 UTC (permalink / raw) To: devel@edk2.groups.io, supreeth.venkatesh@arm.com Hij Supreeth, > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Supreeth Venkatesh > Sent: Thursday, August 22, 2019 12:43 AM > To: Jin, Eric <eric.jin@intel.com>; devel@edk2.groups.io > Subject: Re: [edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate > 2nd execution of ExitBootServices Test > > On Wed, 2019-08-21 at 01:24 -0500, Eric Jin wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2098 > Please add the details of the patch to the commit message. > "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 > multiple execution of the test to retrieve the value from variable and this > design was not straightforward from end user perspective. > I would like to change "multiple execution" to "one additional execution" > This patch enhances the test by leveraging RecoveryLib to restore execution > after reset automatically, thus requiring only one execution" > I am ok with this suggestion when I push the code to repo > More comments inline... > > > > > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> > > Signed-off-by: Eric Jin <eric.jin@intel.com> > > --- > > 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 | 98 > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > +++++++++++ > > ++++++++++++++--------------- > > 3 files changed, 93 insertions(+), 17 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.<BR> -# Copyright (c) 2010 > > - 2012, 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 @@ > > -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.<BR> > > - Copyright (c) 2010 - 2017, 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 @@ -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..e90afe7ecae0 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.<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 @@ -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 > Why is the size being reduced by 2 bytes? > Was the earlier size not optimal? The buffer here is to save the returned status code from ExitBootServices(), so 8 bytes is enough with UEFI Spec's perceptive. > > > +#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,80 @@ 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 > > + ); > Status should be checked before proceeding further. Unhandled error > condition. The code needn't handle possible error status here, and it will be handled in the judgement statement later. In fact, Error status code doesn't mean bad, just means no recovery data exists this time/is written before. > > > + ResetData = (RESET_DATA *)Buffer; > Before initializing, size check should be performed. > Unhandled error condition. Size is checked in the judgement statement later, the same time as the status check. Initializing here doesn't have issue because the Buffer is local buf already defined/allocated in this function. > > > > > - if (Status == EFI_SUCCESS) { > > + if (EFI_ERROR(Status) || (Size < sizeof(RESET_DATA))) { > > + // > > + // Step 1 > > + // > This check is unnecessary if the above comments regarding unhandled error > condition are resolved. Here, the code needn't handle anything. It means no recovery data is found. But I would like to adjust the comment to 'Step 0' and add " no recovery data is found " to make it clear. Correspondingly, adjust the comment 'Step 2' in judgement branch below to 'Step 1, recovery data is found'. > > > + } else if (ResetData->Step == 1) { > > + // > > + // Step 2 > > + // > > + 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 { > > + return EFI_LOAD_ERROR; > > } > > > > // > > // 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), > > + Buffer > To make it clear it should be (UINT8 *)ResetData. > As Buffer is of size 1024, where as ResetData is not. > i.e., Buffer should be used above. > I agree on this suggestion. Will follow when commit this patch to repo. Best Regards Eric > > > + ); > > + 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 +953,7 @@ BBTestExitBootServicesConsistencyTest ( > > //reset system > > gtRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL); > > > > - // get var to get the status > > + > > CheckResult: > > > > if (ReturnStatus == EFI_INVALID_PARAMETER) { > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test 2019-08-22 1:50 ` [edk2-devel] " Eric Jin @ 2019-08-22 18:43 ` Supreeth Venkatesh 2019-08-23 3:37 ` Eric Jin 0 siblings, 1 reply; 6+ messages in thread From: Supreeth Venkatesh @ 2019-08-22 18:43 UTC (permalink / raw) To: devel@edk2.groups.io, eric.jin@intel.com On Wed, 2019-08-21 at 20:50 -0500, Eric Jin via Groups.Io wrote: > Hij Supreeth, Hi Eric, > > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf > > Of > > Supreeth Venkatesh > > Sent: Thursday, August 22, 2019 12:43 AM > > To: Jin, Eric <eric.jin@intel.com>; devel@edk2.groups.io > > Subject: Re: [edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: > > Eliminate > > 2nd execution of ExitBootServices Test > > > > On Wed, 2019-08-21 at 01:24 -0500, Eric Jin wrote: > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2098 > > > > Please add the details of the patch to the commit message. > > "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 > > multiple execution of the test to retrieve the value from variable > > and this > > design was not straightforward from end user perspective. > > > > I would like to change "multiple execution" to "one additional > execution" > > > This patch enhances the test by leveraging RecoveryLib to restore > > execution > > after reset automatically, thus requiring only one execution" > > > > I am ok with this suggestion when I push the code to repo Thanks. > > > More comments inline... > > > > > > > > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> > > > Signed-off-by: Eric Jin <eric.jin@intel.com> > > > --- > > > uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTest.inf | 3 ++- > > > uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTest.h | 9 ++++++++- > > > uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTestConformance.c | 98 > > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > +++++++++++ > > > ++++++++++++++--------------- > > > 3 files changed, 93 insertions(+), 17 deletions(-) > > > > > > diff --git a/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTest.inf b/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTest.inf > > > index 49ad79915934..3de43a20e8a4 100644 > > > --- a/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTest.inf > > > +++ b/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTest.inf > > > @@ -1,7 +1,7 @@ > > > ## @file > > > # > > > # Copyright 2006 - 2012 Unified EFI, Inc.<BR> -# Copyright (c) > > > 2010 > > > - 2012, 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 > > > @@ > > > -53,4 +53,5 @@ > > > > > > [Protocols] > > > gEfiTestProfileLibraryGuid > > > + gEfiTestRecoveryLibraryGuid > > > gBlackBoxEfiHIIPackageListProtocolGuid > > > diff --git a/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTest.h b/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTest.h > > > index b1c35fee7435..008584577ed1 100644 > > > --- a/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTest.h > > > +++ b/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTest.h > > > @@ -1,7 +1,7 @@ > > > /** @file > > > > > > Copyright 2006 - 2017 Unified EFI, Inc.<BR> > > > - Copyright (c) 2010 - 2017, 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 @@ -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/BlackBoxT > > > est/ > > > ImageBBTestConformance.c b/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTestConformance.c > > > index 0a26d46847da..e90afe7ecae0 100644 > > > --- a/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTestConformance.c > > > +++ b/uefi- > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > est/ > > > ImageBBTestConformance.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 @@ -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 > > > > Why is the size being reduced by 2 bytes? > > Was the earlier size not optimal? > > The buffer here is to save the returned status code from > ExitBootServices(), so 8 bytes is enough with UEFI Spec's > perceptive. Ok. Sounds good. > > > > > > +#define RECOVER_BUFFER_SIZE 1024 Not sure why such a large Recovery Buffer is required? When this test case is writing only the _RESET_DATA structure into the recovery library. if this is to handle the case where there are other types of recovery data being written in some other place, How do you distinguish? > > > > > > 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_SIZ > > > E]; > > > EFI_STATUS ReturnStatus; > > > > > > // > > > // Init > > > // > > > StandardLib = NULL; > > > + RecoveryLib = NULL; > > > > > > //_RESET_DATA > > > // 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,80 @@ 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 > > > + ); > > > > Status should be checked before proceeding further. Unhandled error > > condition. > > The code needn't handle possible error status here, and it will be > handled in the judgeme_RESET_DATAnt statement later. > In fact, Error status code doesn't mean bad, just means no recovery > data exists this time/is written before. As I see it, if Status is "error", there is no point in assigning ResetData to Buffer as Buffer is not valid anymore. Hence the reason for the comment. My expectation was just rearrangement of "if" conditional statements. > > > > > > + ResetData = (RESET_DATA *)Buffer; > > > > Before initializing, size check should be performed. > > Unhandled error condition. > > Size is checked in the judgement statement later, the same time as > the status check. > Initializing here doesn't have issue because the Buffer is local buf > already defined/allocated in this function. > > > > > > > > > - if (Status == EFI_SUCCESS) { > > > + if (EFI_ERROR(Status) || (Size < sizeof(RESET_DATA))) { > > > + // > > > + // Step 1 > > > + // > > > > This check is unnecessary if the above comments regarding unhandled > > error > > condition are resolved. > > Here, the code needn't handle anything. It means no recovery data is > found. Right, if there is no action (nothing to handle) after the "if" statement, then there is no point in having the "if" condition. Please rearrange the conditional statements so that for every condition there is an action to follow. > But I would like to adjust the comment to 'Step 0' and add " no > recovery data is found " to make it clear. > Correspondingly, adjust the comment 'Step 2' in judgement branch > below to 'Step 1, recovery data is found'. > > > > > > + } else if (ResetData->Step == 1) { > > > + // > > > + // Step 2 > > > + // > > > + 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 { > > > + return EFI_LOAD_ERROR; > > > } > > > > > > // > > > // 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), > > > + Buffer > > > > To make it clear it should be (UINT8 *)ResetData. > > As Buffer is of size 1024, where as ResetData is not. > > i.e., Buffer should be used above. > > > > I agree on this suggestion. Will follow when commit this patch to > repo. Thanks. > > Best Regards > Eric > > > > > > + ); > > > + 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 +953,7 @@ BBTestExitBootServicesConsistencyTest > > > ( > > > //reset system > > > gtRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL); > > > > > > - // get var to get the status > > > + > > > CheckResult: > > > > > > if (ReturnStatus == EFI_INVALID_PARAMETER) { > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test 2019-08-22 18:43 ` Supreeth Venkatesh @ 2019-08-23 3:37 ` Eric Jin 2019-08-23 16:48 ` Supreeth Venkatesh 0 siblings, 1 reply; 6+ messages in thread From: Eric Jin @ 2019-08-23 3:37 UTC (permalink / raw) To: Supreeth Venkatesh, devel@edk2.groups.io Hi Supreeth, > -----Original Message----- > From: Supreeth Venkatesh [mailto:supreeth.venkatesh@arm.com] > Sent: Friday, August 23, 2019 2:43 AM > To: devel@edk2.groups.io; Jin, Eric <eric.jin@intel.com> > Subject: Re: [edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate > 2nd execution of ExitBootServices Test > > On Wed, 2019-08-21 at 20:50 -0500, Eric Jin via Groups.Io wrote: > > Hij Supreeth, > Hi Eric, > > > > > > -----Original Message----- > > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf > > > Of Supreeth Venkatesh > > > Sent: Thursday, August 22, 2019 12:43 AM > > > To: Jin, Eric <eric.jin@intel.com>; devel@edk2.groups.io > > > Subject: Re: [edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: > > > Eliminate > > > 2nd execution of ExitBootServices Test > > > > > > On Wed, 2019-08-21 at 01:24 -0500, Eric Jin wrote: > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2098 > > > > > > Please add the details of the patch to the commit message. > > > "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 multiple execution of the test to retrieve the value from > > > variable and this design was not straightforward from end user > > > perspective. > > > > > > > I would like to change "multiple execution" to "one additional > > execution" > > > > > This patch enhances the test by leveraging RecoveryLib to restore > > > execution > > > after reset automatically, thus requiring only one execution" > > > > > > > I am ok with this suggestion when I push the code to repo > Thanks. > > > > > > More comments inline... > > > > > > > > > > > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> > > > > Signed-off-by: Eric Jin <eric.jin@intel.com> > > > > --- > > > > uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTest.inf | 3 ++- > > > > uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTest.h | 9 ++++++++- > > > > uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTestConformance.c | 98 > > > > > > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > +++++++++++ > > > > ++++++++++++++--------------- > > > > 3 files changed, 93 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTest.inf b/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTest.inf > > > > index 49ad79915934..3de43a20e8a4 100644 > > > > --- a/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTest.inf > > > > +++ b/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTest.inf > > > > @@ -1,7 +1,7 @@ > > > > ## @file > > > > # > > > > # Copyright 2006 - 2012 Unified EFI, Inc.<BR> -# Copyright (c) > > > > 2010 > > > > - 2012, 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 > > > > @@ > > > > -53,4 +53,5 @@ > > > > > > > > [Protocols] > > > > gEfiTestProfileLibraryGuid > > > > + gEfiTestRecoveryLibraryGuid > > > > gBlackBoxEfiHIIPackageListProtocolGuid > > > > diff --git a/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTest.h b/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTest.h > > > > index b1c35fee7435..008584577ed1 100644 > > > > --- a/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTest.h > > > > +++ b/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTest.h > > > > @@ -1,7 +1,7 @@ > > > > /** @file > > > > > > > > Copyright 2006 - 2017 Unified EFI, Inc.<BR> > > > > - Copyright (c) 2010 - 2017, 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 @@ -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/BlackBoxT > > > > est/ > > > > ImageBBTestConformance.c b/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTestConformance.c > > > > index 0a26d46847da..e90afe7ecae0 100644 > > > > --- a/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTestConformance.c > > > > +++ b/uefi- > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxT > > > > est/ > > > > ImageBBTestConformance.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 @@ -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 > > > > > > Why is the size being reduced by 2 bytes? > > > Was the earlier size not optimal? > > > > The buffer here is to save the returned status code from > > ExitBootServices(), so 8 bytes is enough with UEFI Spec's > > perceptive. > Ok. Sounds good. > > > > > > > > > > +#define RECOVER_BUFFER_SIZE 1024 > Not sure why such a large Recovery Buffer is required? > When this test case is writing only the _RESET_DATA structure into the > recovery library. if this is to handle the case where there are other > types of recovery data being written in some other place, How do you > distinguish? > The recovery data will be saved on the system physical storage with file protocol and 512 or 1024 bytes is block size as experience value. The design of RecoveryLib and SCT infrastructure ensure the recovery data is belong to case granularity, the data will not be impacted by others until current case complete. > > > > > > > > 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_SIZ > > > > E]; > > > > EFI_STATUS ReturnStatus; > > > > > > > > // > > > > // Init > > > > // > > > > StandardLib = NULL; > > > > + RecoveryLib = NULL; > > > > > > > > //_RESET_DATA > > > > // 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,80 @@ 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 > > > > + ); > > > > > > Status should be checked before proceeding further. Unhandled error > > > condition. > > > > The code needn't handle possible error status here, and it will be > > handled in the judgeme_RESET_DATAnt statement later. > > In fact, Error status code doesn't mean bad, just means no recovery > > data exists this time/is written before. > As I see it, if Status is "error", there is no point in assigning > ResetData to Buffer as Buffer is not valid anymore. Hence the reason > for the comment. > My expectation was just rearrangement of "if" conditional statements. > Yes, Buffer needn't be validated here and can be assigned to ResetData directly. > > > > > > > > > + ResetData = (RESET_DATA *)Buffer; > > > > > > Before initializing, size check should be performed. > > > Unhandled error condition. > > > > Size is checked in the judgement statement later, the same time as > > the status check. > > Initializing here doesn't have issue because the Buffer is local buf > > already defined/allocated in this function. > > > > > > > > > > > > > - if (Status == EFI_SUCCESS) { > > > > + if (EFI_ERROR(Status) || (Size < sizeof(RESET_DATA))) { > > > > + // > > > > + // Step 1 > > > > + // > > > > > > This check is unnecessary if the above comments regarding unhandled > > > error > > > condition are resolved. > > > > Here, the code needn't handle anything. It means no recovery data is > > found. > Right, if there is no action (nothing to handle) after the "if" > statement, then there is no point in having the "if" condition. > Please rearrange the conditional statements so that for every condition > there is an action to follow. > Your concern is no action in 1st “if” statement, and want to has action for each condition statement. I can consider this, but the logic(nest statement) may be increased. > > But I would like to adjust the comment to 'Step 0' and add " no > > recovery data is found " to make it clear. > > Correspondingly, adjust the comment 'Step 2' in judgement branch > > below to 'Step 1, recovery data is found'. > > > > > > > > > + } else if (ResetData->Step == 1) { > > > > + // > > > > + // Step 2 > > > > + // > > > > + 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 { > > > > + return EFI_LOAD_ERROR; > > > > } > > > > > > > > // > > > > // 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), > > > > + Buffer > > > > > > To make it clear it should be (UINT8 *)ResetData. > > > As Buffer is of size 1024, where as ResetData is not. > > > i.e., Buffer should be used above. > > > > > > > I agree on this suggestion. Will follow when commit this patch to > > repo. > Thanks. > > > > > Best Regards > > Eric > > > > > > > > > + ); > > > > + 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 +953,7 @@ BBTestExitBootServicesConsistencyTest > > > > ( > > > > //reset system > > > > gtRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL); > > > > > > > > - // get var to get the status > > > > + > > > > CheckResult: > > > > > > > > if (ReturnStatus == EFI_INVALID_PARAMETER) { > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test 2019-08-23 3:37 ` Eric Jin @ 2019-08-23 16:48 ` Supreeth Venkatesh 0 siblings, 0 replies; 6+ messages in thread From: Supreeth Venkatesh @ 2019-08-23 16:48 UTC (permalink / raw) To: Jin, Eric, devel@edk2.groups.io On Thu, 2019-08-22 at 22:37 -0500, Jin, Eric wrote: > Hi Supreeth, > > > -----Original Message----- > > From: Supreeth Venkatesh [mailto:supreeth.venkatesh@arm.com] > > Sent: Friday, August 23, 2019 2:43 AM > > To: devel@edk2.groups.io; Jin, Eric <eric.jin@intel.com> > > Subject: Re: [edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: > > Eliminate > > 2nd execution of ExitBootServices Test > > > > On Wed, 2019-08-21 at 20:50 -0500, Eric Jin via Groups.Io wrote: > > > Hij Supreeth, > > > > Hi Eric, > > > > > > > > > -----Original Message----- > > > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On > > > > Behalf > > > > Of Supreeth Venkatesh > > > > Sent: Thursday, August 22, 2019 12:43 AM > > > > To: Jin, Eric <eric.jin@intel.com>; devel@edk2.groups.io > > > > Subject: Re: [edk2-devel] [edk2-test][Patch 1/1] uefi- > > > > sct/SctPkg: > > > > Eliminate > > > > 2nd execution of ExitBootServices Test > > > > > > > > On Wed, 2019-08-21 at 01:24 -0500, Eric Jin wrote: > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2098 > > > > > > > > Please add the details of the patch to the commit message. > > > > "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 multiple execution of the test to retrieve the value > > > > from > > > > variable and this design was not straightforward from end user > > > > perspective. > > > > > > > > > > I would like to change "multiple execution" to "one additional > > > execution" > > > > > > > This patch enhances the test by leveraging RecoveryLib to > > > > restore > > > > execution > > > > after reset automatically, thus requiring only one execution" > > > > > > > > > > I am ok with this suggestion when I push the code to repo > > > > Thanks. > > > > > > > > > More comments inline... > > > > > > > > > > > > > > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> > > > > > Signed-off-by: Eric Jin <eric.jin@intel.com> > > > > > --- > > > > > uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTest.inf | 3 ++- > > > > > uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTest.h | 9 ++++++++- > > > > > uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTestConformance.c | 98 > > > > > > > > > > > > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > +++++++++++ > > > > > ++++++++++++++--------------- > > > > > 3 files changed, 93 insertions(+), 17 deletions(-) > > > > > > > > > > diff --git a/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTest.inf b/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTest.inf > > > > > index 49ad79915934..3de43a20e8a4 100644 > > > > > --- a/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTest.inf > > > > > +++ b/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTest.inf > > > > > @@ -1,7 +1,7 @@ > > > > > ## @file > > > > > # > > > > > # Copyright 2006 - 2012 Unified EFI, Inc.<BR> -# Copyright > > > > > (c) > > > > > 2010 > > > > > - 2012, 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 > > > > > @@ > > > > > -53,4 +53,5 @@ > > > > > > > > > > [Protocols] > > > > > gEfiTestProfileLibraryGuid > > > > > + gEfiTestRecoveryLibraryGuid > > > > > gBlackBoxEfiHIIPackageListProtocolGuid > > > > > diff --git a/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTest.h b/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTest.h > > > > > index b1c35fee7435..008584577ed1 100644 > > > > > --- a/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTest.h > > > > > +++ b/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTest.h > > > > > @@ -1,7 +1,7 @@ > > > > > /** @file > > > > > > > > > > Copyright 2006 - 2017 Unified EFI, Inc.<BR> > > > > > - Copyright (c) 2010 - 2017, 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 @@ -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/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTestConformance.c b/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTestConformance.c > > > > > index 0a26d46847da..e90afe7ecae0 100644 > > > > > --- a/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTestConformance.c > > > > > +++ b/uefi- > > > > > sct/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/Black > > > > > BoxT > > > > > est/ > > > > > ImageBBTestConformance.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 @@ -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 > > > > > > > > Why is the size being reduced by 2 bytes? > > > > Was the earlier size not optimal? > > > > > > The buffer here is to save the returned status code from > > > ExitBootServices(), so 8 bytes is enough with UEFI Spec's > > > perceptive. > > > > Ok. Sounds good. > > > > > > > > > > > > > > +#define RECOVER_BUFFER_SIZE 1024 > > > > Not sure why such a large Recovery Buffer is required? > > When this test case is writing only the _RESET_DATA structure into > > the > > recovery library. if this is to handle the case where there are > > other > > types of recovery data being written in some other place, How do > > you > > distinguish? > > > > The recovery data will be saved on the system physical storage with > file > protocol and 512 or 1024 bytes is block size as experience value. > The design of RecoveryLib and SCT infrastructure ensure the recovery > data > is belong to case granularity, the data will not be impacted by > others until > current case complete. Thanks for the clarification. > > > > > > > > > > > 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_SI > > > > > ZE]; > > > > > + UINT8 Buffer[RECOVER_BUFFER > > > > > _SIZ > > > > > E]; > > > > > EFI_STATUS ReturnStatus; > > > > > > > > > > // > > > > > // Init > > > > > // > > > > > StandardLib = NULL; > > > > > + RecoveryLib = NULL; > > > > > > > > > > //_RESET_DATA > > > > > // 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,80 @@ 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 > > > > > + ); > > > > > > > > Status should be checked before proceeding further. Unhandled > > > > error > > > > condition. > > > > > > The code needn't handle possible error status here, and it will > > > be > > > handled in the judgeme_RESET_DATAnt statement later. > > > In fact, Error status code doesn't mean bad, just means no > > > recovery > > > data exists this time/is written before. > > > > As I see it, if Status is "error", there is no point in assigning > > ResetData to Buffer as Buffer is not valid anymore. Hence the > > reason > > for the comment. > > My expectation was just rearrangement of "if" conditional > > statements. > > > > Yes, Buffer needn't be validated here and can be assigned to > ResetData directly. The order of statements here is misleading. 1. RecoveryLib->ReadResetRecord(RecoveryLib, Size, Buffer); 2. ResetData = (RESET_DATA *)Buffer; Re-order the statements like below to make it more clear since it is a just a pointer to Buffer which is statically allocated to 1024 and ResetData is not being really initialized to Reset Record after reading reset record. 1. ResetData = (RESET_DATA *)Buffer; 2. RecoveryLib->ReadResetRecord(RecoveryLib, Size, Buffer); > > > > > > > > > > > > > + ResetData = (RESET_DATA *)Buffer; > > > > > > > > Before initializing, size check should be performed. > > > > Unhandled error condition. > > > > > > Size is checked in the judgement statement later, the same time > > > as > > > the status check. > > > Initializing here doesn't have issue because the Buffer is local > > > buf > > > already defined/allocated in this function. > > > > > > > > > > > > > > > > > - if (Status == EFI_SUCCESS) { > > > > > + if (EFI_ERROR(Status) || (Size < sizeof(RESET_DATA))) { > > > > > + // > > > > > + // Step 1 > > > > > + // > > > > > > > > This check is unnecessary if the above comments regarding > > > > unhandled > > > > error > > > > condition are resolved. > > > > > > Here, the code needn't handle anything. It means no recovery data > > > is > > > found. > > > > Right, if there is no action (nothing to handle) after the "if" > > statement, then there is no point in having the "if" condition. > > Please rearrange the conditional statements so that for every > > condition > > there is an action to follow. > > > > Your concern is no action in 1st “if” statement, and want to has > action for each condition statement. > I can consider this, but the logic(nest statement) may be increased. Thanks. I understand it would result in nested "if". > > > > But I would like to adjust the comment to 'Step 0' and add " no > > > recovery data is found " to make it clear. > > > Correspondingly, adjust the comment 'Step 2' in judgement branch > > > below to 'Step 1, recovery data is found'. > > > > > > > > > > > > + } else if (ResetData->Step == 1) { > > > > > + // > > > > > + // Step 2 > > > > > + // > > > > > + 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 { > > > > > + return EFI_LOAD_ERROR; > > > > > } > > > > > > > > > > // > > > > > // 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), > > > > > + Buffer > > > > > > > > To make it clear it should be (UINT8 *)ResetData. > > > > As Buffer is of size 1024, where as ResetData is not. > > > > i.e., Buffer should be used above. > > > > > > > > > > I agree on this suggestion. Will follow when commit this patch to > > > repo. > > > > Thanks. > > > > > > > > Best Regards > > > Eric > > > > > > > > > > > > + ); > > > > > + 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 +953,7 @@ > > > > > BBTestExitBootServicesConsistencyTest > > > > > ( > > > > > //reset system > > > > > gtRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL); > > > > > > > > > > - // get var to get the status > > > > > + > > > > > CheckResult: > > > > > > > > > > if (ReturnStatus == EFI_INVALID_PARAMETER) { > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-23 16:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-21 6:24 [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test Eric Jin 2019-08-21 16:43 ` Supreeth Venkatesh 2019-08-22 1:50 ` [edk2-devel] " Eric Jin 2019-08-22 18:43 ` Supreeth Venkatesh 2019-08-23 3:37 ` Eric Jin 2019-08-23 16:48 ` Supreeth Venkatesh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox