On Thu, 2019-08-22 at 22:37 -0500, Jin, Eric wrote:
> Hi Supreeth,
>
> > -----Original Message-----
> > From: Supreeth Venkatesh [mailto:supreeth.venkat...@arm.com]
> > Sent: Friday, August 23, 2019 2:43 AM
> > To: devel@edk2.groups.io; Jin, Eric <eric....@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....@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.venkat...@arm.com>
> > > > > Signed-off-by: Eric Jin <eric....@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) {
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46310): https://edk2.groups.io/g/devel/message/46310
Mute This Topic: https://groups.io/mt/32975823/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
