From: "Eric Jin" <eric.jin@intel.com>
To: Supreeth Venkatesh <supreeth.venkatesh@arm.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Subject: Re: [edk2-test][Patch V2 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test
Date: Mon, 26 Aug 2019 09:24:32 +0000 [thread overview]
Message-ID: <DA72DC7456565B47808A57108259571F637FF6EB@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <57e19cdac7501ab96f27512260bc3b65acd9930e.camel@arm.com>
Pushed 4d3395a9a54d575850fa12ca04326c797b7e1fe9
Re-order the statements as Supreeth's comment.
Best Regards
Eric
-----Original Message-----
From: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Sent: Saturday, August 24, 2019 12:52 AM
To: Jin, Eric <eric.jin@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-test][Patch V2 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test
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 <supreeth.venkatesh@arm.com>
> Signed-off-by: Eric Jin <eric.jin@intel.com>
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 <supreeth.venkatesh@arm.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 | 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.<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..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.<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,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) {
prev parent reply other threads:[~2019-08-26 9:24 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-23 5:46 [edk2-test][Patch V2 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test Eric Jin
2019-08-23 16:51 ` Supreeth Venkatesh
2019-08-26 9:24 ` Eric Jin [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DA72DC7456565B47808A57108259571F637FF6EB@SHSMSX103.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox