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

["Supreeth Venkatesh" <supreeth.venkatesh@arm.com>]

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) {
> > > > 
> > > > 
> > > > 
> > > 
> > > 
> > > 
> > > 
> 
>