[edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test
Supreeth Venkatesh
supreeth.venkatesh at arm.com
Wed Aug 21 16:43:02 UTC 2019
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 at arm.com>
> Signed-off-by: Eric Jin <eric.jin at 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) {
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46168): https://edk2.groups.io/g/devel/message/46168
Mute This Topic: https://groups.io/mt/32975823/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list