Re: [PATCH v2 3/3] MdeModulePkg/BdsDxe: Set RuntimeServicesSupported variable

["Jeff Brasen" <jbrasen@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 9734 bytes --]

I will work on a patch to variable services and other runtime services that have centrallized implementations to have them check the value of this variable during ExitBootServices and then return unsupported if the bit is not set.


Thanks,

Jeff

________________________________
From: Gao, Zhichao <zhichao.gao@intel.com>
Sent: Wednesday, November 27, 2019 10:29 PM
To: Ni, Ray <ray.ni@intel.com>; Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Gao, Liming <liming.gao@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [PATCH v2 3/3] MdeModulePkg/BdsDxe: Set RuntimeServicesSupported variable

This bitmask value only affect the runtime service at runtime phase. So the implementation should be after the ExitBootServices() called.
I think the patch set is only implemented the basic setting of the variable but no implementation of the RuntimeServices.

Thanks,
Zhichao

> -----Original Message-----
> From: Ni, Ray
> Sent: Thursday, November 28, 2019 1:02 PM
> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [PATCH v2 3/3] MdeModulePkg/BdsDxe: Set
> RuntimeServicesSupported variable
>
> Jeff,
> I think I forgot to ask a very basic question on the new variable
> RuntimeServicesSupported.
>
> What if the variable claims SetVariable() is not supported but OS still calls
> SetVariable()?
> I think to behave in a consistent way, SetVariable() should reject to service.
> But I cannot find it in your patch.
>
> The similar thing was done for OsIndications variable.
> When firmware claims BOOT_TO_SETUP is not supported, the request is ignored
> even OS requests to boot to setup,.
>
> I suggest we change all runtime services implementation to return unsupported
> when the accordingly bit in the PCD is not set.
>
> Thanks,
> Ray
>
>
> > -----Original Message-----
> > From: Jeff Brasen <jbrasen@nvidia.com>
> > Sent: Thursday, November 28, 2019 7:25 AM
> > To: devel@edk2.groups.io
> > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> > jbrasen@nvidia.com
> > Subject: [PATCH v2 3/3] MdeModulePkg/BdsDxe: Set
> > RuntimeServicesSupported variable
> >
> > Add support for initializing and setting the UEFI 2.8 global variable
> > RuntimeServicesSupported based on the value of a PCD.
> >
> > Change-Id: I8fbd404d492ff8278466edde8aa37d203537318c
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> >  MdeModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 +
> > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35
> > +++++++++++++++++++++++++++++++-
> >  MdePkg/MdePkg.dec                        | 18 ++++++++++++++++
> >  MdePkg/MdePkg.uni                        | 17 ++++++++++++++++
> >  4 files changed, 70 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > index 9310b4d..52ec04f 100644
> > --- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > +++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > @@ -90,6 +90,7 @@
> >    gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLang         ##
> > CONSUMES
> >    gEfiMdePkgTokenSpaceGuid.PcdHardwareErrorRecordLevel                ##
> > CONSUMES
> >    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut                     ##
> > CONSUMES
> > +  gEfiMdePkgTokenSpaceGuid.PcdRuntimeServicesSupported                ##
> > CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor                    ##
> > CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision                  ##
> > CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand
> > ## CONSUMES
> > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > index d387dbe..16bc593 100644
> > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > @@ -40,7 +40,8 @@ CHAR16  *mReadOnlyVariables[] = {
> >    EFI_LANG_CODES_VARIABLE_NAME,
> >    EFI_BOOT_OPTION_SUPPORT_VARIABLE_NAME,
> >    EFI_HW_ERR_REC_SUPPORT_VARIABLE_NAME,
> > -  EFI_OS_INDICATIONS_SUPPORT_VARIABLE_NAME
> > +  EFI_OS_INDICATIONS_SUPPORT_VARIABLE_NAME,
> > +  EFI_RUNTIME_SERVICES_SUPPORTED_VARIABLE_NAME
> >    };
> >
> >  CHAR16 *mBdsLoadOptionName[] = {
> > @@ -626,6 +627,33 @@ BdsFormalizeOSIndicationVariable (
> >
> >  /**
> >
> > +  Formalize RuntimeServicesSupported variable.
> > +
> > +**/
> > +VOID
> > +BdsFormalizeRuntimeServicesSupportedVariable (
> > +  VOID
> > +  )
> > +{
> > +  EFI_STATUS                      Status;
> > +  UINT16                          RuntimeServicesSupported;
> > +
> > +  RuntimeServicesSupported = PcdGet16 (PcdRuntimeServicesSupported);
> > + Status = gRT->SetVariable (
> > +                  EFI_RUNTIME_SERVICES_SUPPORTED_VARIABLE_NAME,
> > +                  &gEfiGlobalVariableGuid,
> > +                  EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > EFI_VARIABLE_RUNTIME_ACCESS,
> > +                  sizeof(RuntimeServicesSupported),
> > +                  &RuntimeServicesSupported
> > +                  );
> > +  //
> > +  // Platform needs to make sure setting volatile variable before
> > + calling 3rd
> > party code shouldn't fail.
> > +  //
> > +  ASSERT_EFI_ERROR (Status);
> > +}
> > +
> > +/**
> > +
> >    Validate variables.
> >
> >  **/
> > @@ -645,6 +673,11 @@ BdsFormalizeEfiGlobalVariable (
> >    // Validate OSIndication related variable.
> >    //
> >    BdsFormalizeOSIndicationVariable ();
> > +
> > +  //
> > +  // Validate Runtime Services Supported variable.
> > +  //
> > +  BdsFormalizeRuntimeServicesSupportedVariable ();
> >  }
> >
> >  /**
> > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index
> > d022cc5..cdcb2f9 100644
> > --- a/MdePkg/MdePkg.dec
> > +++ b/MdePkg/MdePkg.dec
> > @@ -2297,5 +2297,23 @@
> >    # @Prompt Boot Timeout (s)
> >
> > gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x00
> > 00002c
> >
> > +  ## Bitmask of supported runtime services<BR>  #  BIT0  - GetTime  #
> > + BIT1  - SetTime  #  BIT2  - GetWakeupTime  #  BIT3  - SetWakeupTime
> > + #  BIT4  - GetVariable  #  BIT5  - GetNextVariableName  #  BIT6  -
> > + SetVariable  #  BIT7  - SetVirtualAddressMap  #  BIT8  -
> > + ConvertPointer  #  BIT9  - GetNextHighMonotonicCount  #  BIT10 -
> > + ResetSystem  #  BIT11 - UpdateCapsule  #  BIT12 -
> > + QueryCapsuleCapabilites  #  BIT13 - QueryVariableInfo  # @Prompt
> > + Supported Runtime services bitmask.
> > +
> > gEfiMdePkgTokenSpaceGuid.PcdRuntimeServicesSupported|0x3FFF|UINT16
> > |0x0000002e
> > +
> >  [UserExtensions.TianoCore."ExtraFiles"]
> >    MdePkgExtra.uni
> > diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni index
> > 5c1fa24..1edf681 100644
> > --- a/MdePkg/MdePkg.uni
> > +++ b/MdePkg/MdePkg.uni
> > @@ -413,3 +413,20 @@
> >
> >  #string
> > STR_gEfiMdePkgTokenSpaceGuid_PcdUartDefaultReceiveFifoDepth_HELP
> > #language en-US "Indicates the receive FIFO depth of UART
> > controller.<BR><BR>"
> >
> > +#string
> > STR_gEfiMdePkgTokenSpaceGuid_PcdRuntimeServicesSupported_PROMPT
> > #language en-US "Supported Runtime Services bitmask"
> > +
> > +#string
> > STR_gEfiMdePkgTokenSpaceGuid_PcdRuntimeServicesSupported_HELP
> > #language en-US "Bitmask of supported runtime services<BR><BR>\n"
> > +                                                                                      "BIT0  - GetTime<BR>\n"
> > +                                                                                      "BIT1  - SetTime<BR>\n"
> > +                                                                                      "BIT2  -
> GetWakeupTime<BR>\n"
> > +                                                                                      "BIT3  -
> SetWakeupTime<BR>\n"
> > +                                                                                      "BIT4  - GetVariable<BR>\n"
> > +
> > + "BIT5  -
> > GetNextVariableName<BR>\n"
> > +                                                                                      "BIT6  - SetVariable<BR>\n"
> > +
> > + "BIT7  -
> > SetVirtualAddressMap<BR>\n"
> > +                                                                                      "BIT8  -
> ConvertPointer<BR>\n"
> > +
> > + "BIT9  -
> > GetNextHighMonotonicCount<BR>\n"
> > +                                                                                      "BIT10 - ResetSystem<BR>\n"
> > +                                                                                      "BIT11 -
> UpdateCapsule<BR>\n"
> > +
> > + "BIT12 -
> > QueryCapsuleCapabilities<BR>\n"
> > +                                                                                      "BIT13 -
> QueryVariableInfo<BR>"
> > --
> > 2.7.4


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

[-- Attachment #2: Type: text/html, Size: 19857 bytes --]