Re: [edk2-devel] [edk2-test][Patch 1/1] uefi-sct/SctPkg: Eliminate 2nd execution of ExitBootServices Test

["Eric Jin" <eric.jin@intel.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) {
> 
> 
>