Re: [edk2-devel] [PATCH 3/3] MdeModulePkg/BdsDxe: Set RuntimeServicesSupported variable

["Wang, Sunny (HPS SW)" <sunnywang@hpe.com>]

Hi Ray, 

May I know why we need to put this PCD to [PcdsFixedAtBuild, PcdsPatchableInModule] section only? If the reason is the security concern, Locking the variable (value of PCD) at the EndOfDxe should be secure enough. For the platforms that want to make it more secure (don't want the PCD to be modified), they can override the PCD type in their .dsc file. 
I can imagine that there are still some use cases that need to modify the PCD during boot. Can we put this PCD in [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] to make it more flexible? 

Regards,
Sunny Wang

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Gao, Zhichao
Sent: Thursday, November 21, 2019 2:12 PM
To: Ni, Ray <ray.ni@intel.com>; Jeff Brasen <jbrasen@nvidia.com>; edk2-devel@lists.01.org; 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>
Subject: Re: [edk2-devel] [PATCH 3/3] MdeModulePkg/BdsDxe: Set RuntimeServicesSupported variable

Agree with Ray, and we should update the uni file at the same time when add the new pcd.

Thanks,
Zhichao

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, November 21, 2019 11:13 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; edk2-devel@lists.01.org; 
> 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>; Gao, 
> Zhichao <zhichao.gao@intel.com>
> Subject: RE: [PATCH 3/3] MdeModulePkg/BdsDxe: Set 
> RuntimeServicesSupported variable
> 
> Jeff,
> I suggest you add the PCD definition to MdePkg.dec because this PCD 
> just maps to the spec defined variable RuntimeServicesSupported.
> 
> And can you put this PCD to [PcdsFixedAtBuild, PcdsPatchableInModule] 
> section only?
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Jeff Brasen <jbrasen@nvidia.com>
> > Sent: Saturday, November 16, 2019 1:43 AM
> > To: edk2-devel@lists.01.org; devel@edk2.groups.io
> > Cc: Jeff Brasen <jbrasen@nvidia.com>; 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>
> > Subject: [PATCH 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.
> >
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> >  MdeModulePkg/MdeModulePkg.dec            | 18 ++++++++++++++++
> >  MdeModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 + 
> > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35
> > +++++++++++++++++++++++++++++++-
> >  3 files changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec 
> > b/MdeModulePkg/MdeModulePkg.dec index 41b9e70..a1767e4 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -2003,6 +2003,24 @@
> >    # @Prompt Capsule On Disk relocation device path.
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdCodRelocationDevPath|{0xFF}|VOI
> > D*|0x0000002f
> >
> > +  ## 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.
> > +
> > +
> > gEfiMdeModulePkgTokenSpaceGuid.PcdRuntimeServicesSupported|0x3FFF
> > |UINT
> > + 16|0x00000030
> > +
> >  [PcdsPatchableInModule]
> >    ## Specify memory size with page number for PEI code when
> >    #  Loading Module at Fixed Address feature is enabled.
> > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > index 9310b4d..e4ba9be 100644
> > --- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > +++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > @@ -97,6 +97,7 @@
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed                       ##
> > CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleOnDiskSupport              ##
> > CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport           ##
> > CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdRuntimeServicesSupported
> > ## CONSUMES
> >
> >  [Depex]
> >    TRUE
> > 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 ();
> >  }
> >
> >  /**
> > --
> > 2.7.4