[edk2,RFC] ArmPlatformPkg: use virtual gRT at runtime

The cached copy gRT of SystemTable->RuntimeServices is not updated by
SetVirtualAddressMap (), which prevent us from using the RTC under the
OS, as we call [Get|Set]Variable() through gRT at runtime.

Replace gRT with mRT, which we update ourselves on a virtual address
change event.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

This is likely the wrong way to address this, hence the 'RFC' but it is
required for the RTC to be usable under Linux after installing the virtual
address map.

It appears that the cached copy gRT (owned by UefiRuntimeServicesTableLib)
is not wired up the virtual address change event, which results in us
dereferencing the physical address of the runtime services table. The contents
themselves have been converted, just not the value of gRT.

If there is a 'proper' way to deal with this (or just a better one), I would
love to hear about it.

Cheers,
Ard.


 .../PL031RealTimeClockLib/PL031RealTimeClockLib.c  | 26 +++++++++++++---------
 1 file changed, 15 insertions(+), 11 deletions(-)

On 09/06/14 13:42, Ard Biesheuvel wrote:
> The cached copy gRT of SystemTable->RuntimeServices is not updated by
> SetVirtualAddressMap (), which prevent us from using the RTC under the
> OS, as we call [Get|Set]Variable() through gRT at runtime.
> 
> Replace gRT with mRT, which we update ourselves on a virtual address
> change event.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> This is likely the wrong way to address this, hence the 'RFC' but it is
> required for the RTC to be usable under Linux after installing the virtual
> address map.
> 
> It appears that the cached copy gRT (owned by UefiRuntimeServicesTableLib)
> is not wired up the virtual address change event, which results in us
> dereferencing the physical address of the runtime services table. The contents
> themselves have been converted, just not the value of gRT.
> 
> If there is a 'proper' way to deal with this (or just a better one), I would
> love to hear about it.
> 
> Cheers,
> Ard.
> 
> 
>  .../PL031RealTimeClockLib/PL031RealTimeClockLib.c  | 26 +++++++++++++---------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
> index b43d8dc4ba2f..56204881ce86 100644
> --- a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
> +++ b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
> @@ -40,11 +40,12 @@
>  
>  #include <ArmPlatform.h>
>  
> -STATIC CONST CHAR16  mTimeZoneVariableName[] = L"PL031RtcTimeZone";
> -STATIC CONST CHAR16  mDaylightVariableName[] = L"PL031RtcDaylight";
> -STATIC BOOLEAN       mPL031Initialized = FALSE;
> -STATIC EFI_EVENT     mRtcVirtualAddrChangeEvent;
> -STATIC UINTN         mPL031RtcBase;
> +STATIC CONST CHAR16           mTimeZoneVariableName[] = L"PL031RtcTimeZone";
> +STATIC CONST CHAR16           mDaylightVariableName[] = L"PL031RtcDaylight";
> +STATIC BOOLEAN                mPL031Initialized = FALSE;
> +STATIC EFI_EVENT              mRtcVirtualAddrChangeEvent;
> +STATIC UINTN                  mPL031RtcBase;
> +STATIC EFI_RUNTIME_SERVICES   *mRT;
>  
>  EFI_STATUS
>  IdentifyPL031 (
> @@ -294,7 +295,7 @@ LibGetTime (
>  
>    // Get the current time zone information from non-volatile storage
>    Size = sizeof (TimeZone);
> -  Status = gRT->GetVariable (
> +  Status = mRT->GetVariable (
>                    (CHAR16 *)mTimeZoneVariableName,
>                    &gEfiCallerIdGuid,
>                    NULL,
> @@ -312,7 +313,7 @@ LibGetTime (
>      // The time zone variable does not exist in non-volatile storage, so create it.
>      Time->TimeZone = EFI_UNSPECIFIED_TIMEZONE;
>      // Store it
> -    Status = gRT->SetVariable (
> +    Status = mRT->SetVariable (
>                      (CHAR16 *)mTimeZoneVariableName,
>                      &gEfiCallerIdGuid,
>                      EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> @@ -346,7 +347,7 @@ LibGetTime (
>  
>    // Get the current daylight information from non-volatile storage
>    Size = sizeof (Daylight);
> -  Status = gRT->GetVariable (
> +  Status = mRT->GetVariable (
>                    (CHAR16 *)mDaylightVariableName,
>                    &gEfiCallerIdGuid,
>                    NULL,
> @@ -364,7 +365,7 @@ LibGetTime (
>      // The daylight variable does not exist in non-volatile storage, so create it.
>      Time->Daylight = 0;
>      // Store it
> -    Status = gRT->SetVariable (
> +    Status = mRT->SetVariable (
>                      (CHAR16 *)mDaylightVariableName,
>                      &gEfiCallerIdGuid,
>                      EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> @@ -498,7 +499,7 @@ LibSetTime (
>    // Do this after having set the RTC.
>  
>    // Save the current time zone information into non-volatile storage
> -  Status = gRT->SetVariable (
> +  Status = mRT->SetVariable (
>                    (CHAR16 *)mTimeZoneVariableName,
>                    &gEfiCallerIdGuid,
>                    EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> @@ -516,7 +517,7 @@ LibSetTime (
>    }
>  
>    // Save the current daylight information into non-volatile storage
> -  Status = gRT->SetVariable (
> +  Status = mRT->SetVariable (
>                    (CHAR16 *)mDaylightVariableName,
>                    &gEfiCallerIdGuid,
>                    EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> @@ -609,6 +610,7 @@ LibRtcVirtualNotifyEvent (
>    // runtime calls will be made in virtual mode.
>    //
>    EfiConvertPointer (0x0, (VOID**)&mPL031RtcBase);
> +  EfiConvertPointer (0x0, (VOID**)&mRT);
>    return;
>  }
>  
> @@ -656,6 +658,8 @@ LibRtcInitialize (
>    gRT->GetWakeupTime = LibGetWakeupTime;
>    gRT->SetWakeupTime = LibSetWakeupTime;
>  
> +  mRT = gRT;
> +
>    // Install the protocol
>    Handle = NULL;
>    Status = gBS->InstallMultipleProtocolInterfaces (
> 

Although maybe somewhat surprising (it was for me), this seems to be the
right thing to do.

I grepped the tree for a DXE_RUNTIME_DRIVER module that issued a
GetVariable() call. There are a few, but a particularly short one is:

MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystem.c

You'll notice that inside the ResetSystem() function (which gets
installed as gRT->ResetSystem in InitializeResetSystem()), it does not
say gRT->GetVariable. It calls EfiGetVariable().

EfiGetVariable() is from "MdePkg/Library/UefiRuntimeLib/RuntimeLib.c",
and goes like:

  return mInternalRT->GetVariable (VariableName, VendorGuid, Attributes,
DataSize, Data);

where "mInternalRT" behaves in the same way as your patch implements for
mRT:
- RuntimeDriverLibConstruct() sets it to gRT
- RuntimeLibVirtualNotifyEvent() converts it.

You can stick with this patch, or you could rebase the library on top of
UefiRuntimeLib. UefiRuntimeLib seems to wrap all of the runtime services
in similar convenience wrappers. There's a number of modules across the
tree that call the (sometimes) useful EfiGoneVirtual() function too.
Whatever is simpler for you.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/