Re: [PATCH] Check return status on calls to GetTime()

["Samer El-Haj-Mahmoud" <samer.el-haj-mahmoud@arm.com>]

One issue here is that UEFI 2.8 spec language around allowing runtime services (like GetTime() ) to return EFI_UNSUPPORTED is a bit vague:

""
EFI_UNSUPPORTED
This call is not supported by this platform at the time the call is made. The platform should describe this runtime service as unsupported at runtime via an EFI_RT_PROPERTIES_TABLE configuration table.
""

The "at the time the call is made" language may be interpreted as the service is allowed to return EFI_UNSUPPORTED anytime (before or after ExitBootServices() ). But the next sentence (about declaring the unsupported at runtime) make it sounds like this is restricted to only being at runtime. While the use case makes sense, we may need some clarification in the UEFI spec language to make this clear. Otherwise, callers (like SCT) may assume the function is always supported.

> -----Original Message-----
> From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Sent: Tuesday, August 4, 2020 7:27 AM
> To: Grant Likely <Grant.Likely@arm.com>; devel@edk2.groups.io
> Cc: nd <nd@arm.com>; G Edhaya Chandran <Edhaya.Chandran@arm.com>;
> Heinrich Schuchardt <xypron.glpk@gmx.de>; Samer El-Haj-Mahmoud
> <Samer.El-Haj-Mahmoud@arm.com>
> Subject: RE: [PATCH] Check return status on calls to GetTime()
> 
> Reviewed-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> 
> > -----Original Message-----
> > From: Grant Likely <Grant.Likely@arm.com>
> > Sent: Friday, July 31, 2020 1:20 PM
> > To: devel@edk2.groups.io
> > Cc: nd <nd@arm.com>; Grant Likely <Grant.Likely@arm.com>; G Edhaya
> > Chandran <Edhaya.Chandran@arm.com>; Heinrich Schuchardt
> > <xypron.glpk@gmx.de>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > Mahmoud@arm.com>
> > Subject: [PATCH] Check return status on calls to GetTime()
> >
> > Not all platforms implement GetTime(), but the SCT just assumes calls
> > to GetTime will be successful. If GetTime() doesn't return
> > EFI_SUCCESS, then the EFI_TIME value will be uninitialized data.
> >
> > Fix by checking the GetTime() return code. If it doesn't return
> > EFI_SUCCESS, then use the traditional 1/1/1970 epoch so that the test
> > report at least looks sane, but it is obvious that we don't have a valid
> timestamp.
> >
> > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=2870
> >
> > Cc: G Edhaya Chandran <Edhaya.Chandran@arm.com>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>
> > Signed-off-by: Grant Likely <grant.likely@arm.com>
> > ---
> >  .../SimpleNetwork/SimpleNetworkENTSTestCase.c | 26 +++++++++++++----
> > --
> >  .../MiscBootServicesBBTestFunction.c          |  8 ++++--
> >  .../DriverBindingBBTestFunction.c             |  5 +++-
> >  .../SCT/Drivers/StandardTest/StandardTest.c   | 11 +++++---
> >  .../Framework/ENTS/EasDispatcher/Core/Eas.c   |  9 +++++--
> >  .../ENTS/EasDispatcher/Exec/EasCmdDisp.c      | 20 +++++++++-----
> >  6 files changed, 57 insertions(+), 22 deletions(-)
> >
> > diff --git a/uefi-
> > sct/SctPkg/TestCase/RIVL/Protocol/SimpleNetwork/SimpleNetworkENTSTes
> > tCase.c b/uefi-
> > sct/SctPkg/TestCase/RIVL/Protocol/SimpleNetwork/SimpleNetworkENTSTes
> > tCase.c
> > index 9c8d2a70..5579be7e 100644
> > --- a/uefi-
> > sct/SctPkg/TestCase/RIVL/Protocol/SimpleNetwork/SimpleNetworkENTSTes
> > tCase.c
> > +++ b/uefi-
> > sct/SctPkg/TestCase/RIVL/Protocol/SimpleNetwork/SimpleNetwork
> > +++ ENTSTestCase.c
> > @@ -24,6 +24,8 @@ Abstract:
> >
> >  #include "SimpleNetworkENTSTestCase.h"
> >
> > +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> > +
> >  //
> >  // SimpleNetwork.Start
> >  //
> > @@ -928,7 +930,8 @@ Returns:
> >    Status          = EFI_SUCCESS;
> >    tBS->Stall (5000);
> >
> > -  tRT->GetTime (&BeginTime, NULL);
> > +  if (tRT->GetTime (&BeginTime, NULL) != EFI_SUCCESS)
> > +    BeginTime = Epoch;
> >    for (Index = 0; Index < 1;) {
> >      Status = SimpleNetwork->Transmit (
> >                                SimpleNetwork, @@ -964,7 +967,8 @@ Returns:
> >      }
> >    }
> >
> > -  tRT->GetTime (&BeginTime, NULL);
> > +  if (tRT->GetTime (&BeginTime, NULL) != EFI_SUCCESS)
> > +    BeginTime = Epoch;
> >
> >    for (Index = 1; Index < TransmitPattern1Number;) {
> >      Status = SimpleNetwork->Transmit ( @@ -1002,7 +1006,8 @@ Returns:
> >    }
> >
> >  End:
> > -  tRT->GetTime (&EndTime, NULL);
> > +  if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> > +    EndTime = Epoch;
> >
> >    *TransmitPattern1Status = Status;
> >
> > @@ -1125,7 +1130,8 @@ Returns:
> >    Status          = EFI_SUCCESS;
> >    tBS->Stall (5000);
> >
> > -  tRT->GetTime (&BeginTime, NULL);
> > +  if (tRT->GetTime (&BeginTime, NULL) != EFI_SUCCESS)
> > +    BeginTime = Epoch;
> >    for (Index = 0; Index < 1;) {
> >      Status = SimpleNetwork->Transmit (
> >                                SimpleNetwork, @@ -1161,7 +1167,8 @@ Returns:
> >      }
> >    }
> >
> > -  tRT->GetTime (&BeginTime, NULL);
> > +  if (tRT->GetTime (&BeginTime, NULL) != EFI_SUCCESS)
> > +    BeginTime = Epoch;
> >
> >    for (Index = 1; Index < TransmitPattern2Number;) {
> >      Status = SimpleNetwork->Transmit ( @@ -1199,7 +1206,8 @@ Returns:
> >    }
> >
> >  End:
> > -  tRT->GetTime (&EndTime, NULL);
> > +  if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> > +    EndTime = Epoch;
> >
> >    *TransmitPattern1Status = Status;
> >
> > @@ -1326,7 +1334,8 @@ Returns:
> >      }
> >    }
> >
> > -  tRT->GetTime (&BeginTime, NULL);
> > +  if (tRT->GetTime (&BeginTime, NULL) != EFI_SUCCESS)
> > +    BeginTime = Epoch;
> >
> >    for (Index = 1; Index < ReceivePattern1Number;) {
> >      *ReceivePattern1BufferSize = BufferSizeOrg; @@ -1346,7 +1355,8 @@
> > Returns:
> >      }
> >    }
> >
> > -  tRT->GetTime (&EndTime, NULL);
> > +  if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> > +    EndTime = Epoch;
> >
> >    *ReceivePattern1Status = Status;
> >
> > diff --git a/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/BootServices/MiscBootServices/BlackBoxTes
> > t /MiscBootServicesBBTestFunction.c b/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/BootServices/MiscBootServices/BlackBoxTes
> > t
> > /MiscBootServicesBBTestFunction.c
> > index 1d231d8c..3a530282 100644
> > --- a/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/BootServices/MiscBootServices/BlackBoxTes
> > t
> > /MiscBootServicesBBTestFunction.c
> > +++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/BootServices/MiscBootServices/
> > +++ Bl ackBoxTest/MiscBootServicesBBTestFunction.c
> > @@ -27,6 +27,8 @@ Abstract:
> >  #include "SctLib.h"
> >  #include "MiscBootServicesBBTestMain.h"
> >
> > +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> > +
> >  /**
> >   *  Entrypoint for gtBS->SetWatchdogTimer() Interface Test.
> >   *  @param This a pointer of EFI_BB_TEST_PROTOCOL.
> > @@ -821,13 +823,15 @@ BBTestStallInterfaceTest (
> >      //
> >      // 4.2.2.1  Stall must succeed.
> >      //
> > -    gtRT->GetTime (&StartTime, NULL);
> > +    if (gtRT->GetTime (&StartTime, NULL) != EFI_SUCCESS)
> > +      StartTime = Epoch;
> >      OldTpl = gtBS->RaiseTPL (TplArray[Index]);
> >      Status = gtBS->Stall (
> >                       10000000
> >                       );
> >      gtBS->RestoreTPL (OldTpl);
> > -    gtRT->GetTime (&EndTime, NULL);
> > +    if (gtRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> > +      EndTime = Epoch;
> >      if (Status == EFI_SUCCESS) {
> >        AssertionType = EFI_TEST_ASSERTION_PASSED;
> >      } else {
> > diff --git a/uefi-
> > sct/SctPkg/TestCase/UEFI/IHV/Protocol/DriverBinding/BlackBoxTest/Drive
> > rB
> > indingBBTestFunction.c b/uefi-
> > sct/SctPkg/TestCase/UEFI/IHV/Protocol/DriverBinding/BlackBoxTest/Drive
> > rB
> > indingBBTestFunction.c
> > index bf675feb..4ab52dcd 100644
> > --- a/uefi-
> > sct/SctPkg/TestCase/UEFI/IHV/Protocol/DriverBinding/BlackBoxTest/Drive
> > rB
> > indingBBTestFunction.c
> > +++ b/uefi-sct/SctPkg/TestCase/UEFI/IHV/Protocol/DriverBinding/BlackBo
> > +++ xT
> > +++ est/DriverBindingBBTestFunction.c
> > @@ -36,6 +36,8 @@ static const UINTN  MonthLengths[2][12] = {
> >    { 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }  };
> >
> > +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> > +
> >  #define MINS_PER_HOUR       60
> >  #define HOURS_PER_DAY       24
> >  #define SECS_PER_MIN        60
> > @@ -1052,7 +1054,8 @@ EndLogging (
> >    WriteLogFile (Private, DashLine, SYSTEMLOG);
> >    WriteLogFile (Private, DashLine, CASELOG);
> >
> > -  gtRT->GetTime (&CurrentTime, NULL);
> > +  if (gtRT->GetTime (&CurrentTime, NULL) != EFI_SUCCESS)
> > +    CurrentTime = Epoch;
> >    DBSPrint (Buffer, EFI_MAX_PRINT_BUFFER, L"Test Finished: %t\n",
> > &CurrentTime);
> >
> >    WriteLogFile (Private, Buffer, SYSTEMLOG); diff --git a/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Drivers/StandardTest/StandardTest.c
> > b/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Drivers/StandardTest/StandardTest.c
> > index 84025457..836f072a 100644
> > --- a/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Drivers/StandardTest/StandardTest.c
> > +++ b/uefi-sct/SctPkg/TestInfrastructure/SCT/Drivers/StandardTest/Stan
> > +++ da
> > +++ rdTest.c
> > @@ -30,6 +30,8 @@ Abstract:
> >  #include "StandardTest.h"
> >  #include <Library/EntsLib.h>
> >
> > +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> > +
> >  //
> >  // Prototypes
> >  //
> > @@ -1081,7 +1083,8 @@ Returns:
> >      StslWriteLogFile (Private, Buffer);
> >
> >      CurrentTime = &Private->StartTime;
> > -    tRT->GetTime (CurrentTime, NULL);
> > +    if (tRT->GetTime (CurrentTime, NULL) != EFI_SUCCESS)
> > +      *CurrentTime = Epoch;
> >
> >    } else {
> >      StslWriteLogFile (Private, DashLine); @@ -1118,7 +1121,8 @@ Returns:
> >
> >      StslWriteLogFileName (Private);
> >      CurrentTime = &Private->StartTime;
> > -    tRT->GetTime (CurrentTime, NULL);
> > +    if (tRT->GetTime (CurrentTime, NULL) != EFI_SUCCESS)
> > +      *CurrentTime = Epoch;
> >      SctSPrint (Buffer, EFI_MAX_PRINT_BUFFER, L"Test Started: %t\n",
> > CurrentTime);
> >      StslWriteLogFile (Private, Buffer);
> >
> > @@ -1238,7 +1242,8 @@ Returns:
> >
> >    StslWriteLogFileName (Private);
> >
> > -  tRT->GetTime (&CurrentTime, NULL);
> > +  if (tRT->GetTime (&CurrentTime, NULL) != EFI_SUCCESS)
> > +    CurrentTime = Epoch;
> >
> >    SecondsElapsed = SecondsElapsedFromBaseYear (
> >                       Private->StartTime.Year, diff --git a/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Core/Ea
> > s.c b/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Core/Ea
> > s.c
> > index 28f5ed4a..60b1c4dc 100644
> > --- a/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Core/Ea
> > s.c
> > +++ b/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatche
> > +++ r/Core/Eas.c
> > @@ -23,9 +23,12 @@ Abstract:
> >  --*/
> >
> >
> > +#include "Sct.h"
> >  #include "Sct.h"
> >  #include EFI_TEST_PROTOCOL_DEFINITION (EntsMonitorProtocol)
> >
> > +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> > +
> >  STATIC
> >  EFI_STATUS
> >  AgentTestMain (
> > @@ -310,7 +313,8 @@ DelaySctAgentCmdPost (
> >    }
> >    SctAgentCmdDelayedPost->CmdReturn      = CmdReturn;
> >    SctAgentCmdDelayedPost->Cmd.ComdResult = CmdResult;
> > -  tRT->GetTime (&SctAgentCmdDelayedPost->StartTime, NULL);
> > +  if (tRT->GetTime (&SctAgentCmdDelayedPost->StartTime, NULL) !=
> > EFI_SUCCESS)
> > +    SctAgentCmdDelayedPost->StartTime = Epoch;
> >
> >    return Status;
> >  }
> > @@ -327,7 +331,8 @@ PostSctAgentDelayedCmd (
> >      return EFI_SUCCESS;
> >    }
> >
> > -  tRT->GetTime (&SctAgentCmdDelayedPost->EndTime, NULL);
> > +  if (tRT->GetTime (&SctAgentCmdDelayedPost->EndTime, NULL) !=
> > EFI_SUCCESS)
> > +    SctAgentCmdDelayedPost->EndTime = Epoch;
> >
> >    Status = RecordMessage (
> >              &SctAgentCmdDelayedPost->Cmd.ComdRuntimeInfo,
> > diff --git a/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Exec/Ea
> > sCmdDisp.c b/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Exec/Ea
> > sCmdDisp.c
> > index 1ff6d569..cb6f08cf 100644
> > --- a/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Exec/Ea
> > sCmdDisp.c
> > +++ b/uefi-
> > sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatche
> > +++ r/Exec/EasCmdDisp.c
> > @@ -50,6 +50,8 @@ Abstract:
> >
> >  EFI_CPU_ARCH_PROTOCOL *Cpu = NULL;
> >
> > +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> > +
> >  //
> >  // Local Function Definition
> >  //
> > @@ -132,9 +134,11 @@ Returns:
> >    //
> >    // Perform EFTP operation.
> >    //
> > -  tRT->GetTime (&StartTime, NULL);
> > +  if (tRT->GetTime (&StartTime, NULL) != EFI_SUCCESS)
> > +    StartTime = Epoch;
> >    Status = EftpDispatchFileTransferComd (FileCmdType);
> > -  tRT->GetTime (&EndTime, NULL);
> > +  if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> > +    EndTime = Epoch;
> >
> >    if (Status == EFI_OUT_OF_RESOURCES) {
> >      return EFI_OUT_OF_RESOURCES;
> > @@ -365,9 +369,11 @@ Returns:
> >    //
> >    // Execute Shell Command
> >    //
> > -  tRT->GetTime (&StartTime, NULL);
> > +  if (tRT->GetTime (&StartTime, NULL) != EFI_SUCCESS)
> > +    StartTime = Epoch;
> >    Status = SctShellExecute (&mImageHandle, (gEasFT->Cmd)->ComdArg,
> > FALSE, NULL, NULL);;
> > -  tRT->GetTime (&EndTime, NULL);
> > +  if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> > +    EndTime = Epoch;
> >    EFI_ENTS_DEBUG ((EFI_ENTS_D_TRACE, L"dispatch:(%s)", (gEasFT->Cmd)-
> > >ComdArg));
> >    SctPrint (L"dispatch:(%s) - %r\n", (gEasFT->Cmd)->ComdArg, Status);
> >    if (Status == EFI_OUT_OF_RESOURCES) { @@ -1483,9 +1489,11 @@
> > Returns:
> >    //
> >    // Resume SCT execution by executing "sct -c" in sct passive mode.
> >    //
> > -  tRT->GetTime (&StartTime, NULL);
> > +  if (tRT->GetTime (&StartTime, NULL) != EFI_SUCCESS)
> > +    StartTime = Epoch;
> >    Status = SctShellExecute (&mImageHandle, (gEasFT->Cmd)->ComdArg,
> > FALSE, NULL, NULL);;
> > -  tRT->GetTime (&EndTime, NULL);
> > +  if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> > +    EndTime = Epoch;
> >    EFI_ENTS_DEBUG ((EFI_ENTS_D_TRACE, L"dispatch:(%s)", (gEasFT->Cmd)-
> > >ComdArg));
> >    SctPrint (L"dispatch:(%s) - %r\n", (gEasFT->Cmd)->ComdArg, Status);
> >    if (Status == EFI_OUT_OF_RESOURCES) {
> > --
> > 2.20.1