* [PATCH V2 1/4] MdePkg/UefiSpec.h: Add define of runtime services support
2019-07-19 8:09 [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder Gao, Zhichao
@ 2019-07-19 8:09 ` Gao, Zhichao
2019-07-19 8:09 ` [PATCH V2 2/4] MdePkg: Add new pcd PcdRuntimeServicesSupport Gao, Zhichao
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Gao, Zhichao @ 2019-07-19 8:09 UTC (permalink / raw)
To: devel
Cc: Michael D Kinney, Liming Gao, Sean Brogan, Michael Turner,
Bret Barkelew
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1907
UEFI spec 2.8, Section 8.1.1 define some MACROs for
RuntimeServicesSupported variable. Add them to UefiSpec.h.
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
MdePkg/Include/Uefi/UefiSpec.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
index 44a0a6a7fa..20ecbff26e 100644
--- a/MdePkg/Include/Uefi/UefiSpec.h
+++ b/MdePkg/Include/Uefi/UefiSpec.h
@@ -1783,6 +1783,21 @@ EFI_STATUS
#define EFI_RUNTIME_SERVICES_SIGNATURE SIGNATURE_64 ('R','U','N','T','S','E','R','V')
#define EFI_RUNTIME_SERVICES_REVISION EFI_SPECIFICATION_VERSION
+#define EFI_RT_SUPPORTED_GET_TIME 0x0001
+#define EFI_RT_SUPPORTED_SET_TIME 0x0002
+#define EFI_RT_SUPPORTED_GET_WAKEUP_TIME 0x0004
+#define EFI_RT_SUPPORTED_SET_WAKEUP_TIME 0x0008
+#define EFI_RT_SUPPORTED_GET_VARIABLE 0x0010
+#define EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME 0x0020
+#define EFI_RT_SUPPORTED_SET_VARIABLE 0x0040
+#define EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP 0x0080
+#define EFI_RT_SUPPORTED_CONVERT_POINTER 0x0100
+#define EFI_RT_SUPPORTED_GET_NEXT_HIGH_MONOTONIC_COUNT 0x0200
+#define EFI_RT_SUPPORTED_RESET_SYSTEM 0x0400
+#define EFI_RT_SUPPORTED_UPDATE_CAPSULE 0x0800
+#define EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES 0x1000
+#define EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO 0x2000
+
///
/// EFI Runtime Services Table.
///
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH V2 2/4] MdePkg: Add new pcd PcdRuntimeServicesSupport
2019-07-19 8:09 [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder Gao, Zhichao
2019-07-19 8:09 ` [PATCH V2 1/4] MdePkg/UefiSpec.h: Add define of runtime services support Gao, Zhichao
@ 2019-07-19 8:09 ` Gao, Zhichao
2019-07-19 8:09 ` [PATCH V2 3/4] MdeModulePkg/RuntimeDxe: Set RuntimeServicesSupport base on Pcd Gao, Zhichao
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Gao, Zhichao @ 2019-07-19 8:09 UTC (permalink / raw)
To: devel
Cc: Michael D Kinney, Liming Gao, Sean Brogan, Michael Turner,
Bret Barkelew
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1907
Add pcd PcdRuntimeServicesSupport to control whether runtime
services is supported at runtime phase. It is a UINT16 type
bitmask value. Refer to Uefi Spec 2.8, Section 8.1.1.
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
MdePkg/MdePkg.dec | 19 +++++++++++++++++++
MdePkg/MdePkg.uni | 19 +++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index b382efd578..c66c88970e 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2079,6 +2079,25 @@
# @Prompt Speculation Barrier Type.
gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType|0x01|UINT8|0x30001018
+ ## Indicates the supported runtime services after exit boot services.
+ # Uefi Spec 2.8, Section 8.1.1, RuntimeServicesSupported variable realted definitions.<BR><BR>
+ # 0x0001 (EFI_RT_SUPPORTED_GET_TIME)<BR>
+ # 0x0002 (EFI_RT_SUPPORTED_SET_TIME)<BR>
+ # 0x0004 (EFI_RT_SUPPORTED_GET_WAKEUP_TIME)<BR>
+ # 0x0008 (EFI_RT_SUPPORTED_SET_WAKEUP_TIME)<BR>
+ # 0x0010 (EFI_RT_SUPPORTED_GET_VARIABLE)<BR>
+ # 0x0020 (EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME)<BR>
+ # 0x0040 (EFI_RT_SUPPORTED_SET_VARIABLE)<BR>
+ # 0x0080 (EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP)<BR>
+ # 0x0100 (EFI_RT_SUPPORTED_CONVERT_POINTER)<BR>
+ # 0x0200 (EFI_RT_SUPPORTED_GET_NEXT_HIGH_MONOTONIC_COUNT)<BR>
+ # 0x0400 (EFI_RT_SUPPORTED_RESET_SYSTEM)<BR>
+ # 0x0800 (EFI_RT_SUPPORTED_UPDATE_CAPSULE)<BR>
+ # 0x1000 (EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES)<BR>
+ # 0x2000 (EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO)
+ # @Prompt Runtime Services support.
+ gEfiMdePkgTokenSpaceGuid.PcdRuntimeServicesSupport|0x3FFF|UINT16|0x30001019
+
[PcdsFixedAtBuild,PcdsPatchableInModule]
## Indicates the maximum length of unicode string used in the following
# BaseLib functions: StrLen(), StrSize(), StrCmp(), StrnCmp(), StrCpy(), StrnCpy()<BR><BR>
diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
index 5c1fa24065..09e0cf22f1 100644
--- a/MdePkg/MdePkg.uni
+++ b/MdePkg/MdePkg.uni
@@ -157,6 +157,25 @@
"0x02 - CPUID (IA32/X64).<BR>\n"
"Other - reserved"
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdRuntimeServicesSupport_PROMPT #language en-US "Runtime Services support."
+
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdRuntimeServicesSupport_HELP #language en-US "Indicates the supported runtime services after exit boot services."
+ "Uefi Spec 2.8, Section 8.1.1, RuntimeServicesSupported variable realted definitions.<BR><BR>"
+ " 0x0001 (EFI_RT_SUPPORTED_GET_TIME)<BR>"
+ " 0x0002 (EFI_RT_SUPPORTED_SET_TIME)<BR>"
+ " 0x0004 (EFI_RT_SUPPORTED_GET_WAKEUP_TIME)<BR>"
+ " 0x0008 (EFI_RT_SUPPORTED_SET_WAKEUP_TIME)<BR>"
+ " 0x0010 (EFI_RT_SUPPORTED_GET_VARIABLE)<BR>"
+ " 0x0020 (EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME)<BR>"
+ " 0x0040 (EFI_RT_SUPPORTED_SET_VARIABLE)<BR>"
+ " 0x0080 (EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP)<BR>"
+ " 0x0100 (EFI_RT_SUPPORTED_CONVERT_POINTER)<BR>"
+ " 0x0200 (EFI_RT_SUPPORTED_GET_NEXT_HIGH_MONOTONIC_COUNT)<BR>"
+ " 0x0400 (EFI_RT_SUPPORTED_RESET_SYSTEM)<BR>"
+ " 0x0800 (EFI_RT_SUPPORTED_UPDATE_CAPSULE)<BR>"
+ " 0x1000 (EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES)<BR>"
+ " 0x2000 (EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO)"
+
#string STR_gEfiMdePkgTokenSpaceGuid_PcdMaximumAsciiStringLength_PROMPT #language en-US "Maximum Length of Ascii String"
#string STR_gEfiMdePkgTokenSpaceGuid_PcdMaximumAsciiStringLength_HELP #language en-US "Sets the maximum number of ASCII characters used for string functions. This affects the following BaseLib functions: AsciiStrLen(), AsciiStrSize(), AsciiStrCmp(), AsciiStrnCmp(), AsciiStrCpy(), AsciiStrnCpy(). <BR><BR>\n"
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH V2 3/4] MdeModulePkg/RuntimeDxe: Set RuntimeServicesSupport base on Pcd
2019-07-19 8:09 [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder Gao, Zhichao
2019-07-19 8:09 ` [PATCH V2 1/4] MdePkg/UefiSpec.h: Add define of runtime services support Gao, Zhichao
2019-07-19 8:09 ` [PATCH V2 2/4] MdePkg: Add new pcd PcdRuntimeServicesSupport Gao, Zhichao
@ 2019-07-19 8:09 ` Gao, Zhichao
2019-07-19 16:01 ` [edk2-devel] " Michael D Kinney
2019-07-19 8:09 ` [PATCH V2 4/4] MdeModulePkg/CapsuleRuntimeDxe: Implement RuntimeServicesSupported Gao, Zhichao
2019-07-19 14:15 ` [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder Laszlo Ersek
4 siblings, 1 reply; 12+ messages in thread
From: Gao, Zhichao @ 2019-07-19 8:09 UTC (permalink / raw)
To: devel
Cc: Jian J Wang, Hao A Wu, Ray Ni, Star Zeng, Liming gao, Sean Brogan,
Michael Turner, Bret Barkelew, Laszlo Ersek
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1907
Add a pcd named PcdRuntimeServicesSupport, RuntimeDxe driver would
set variable L"RuntimeServicesSupported" base on this pcd. The varialbe
would indicate whether the runtime services is supported or not in
runtime phase. If the pcd's value is 0x3FFF, that means all runtime
services is supported at runtime, then the variable wouldn't be set.
Refer to UEFI spec 2.8, Section 8.1.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
MdeModulePkg/Core/RuntimeDxe/Runtime.c | 65 ++++++++++++++++++++-
MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf | 8 ++-
2 files changed, 71 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Core/RuntimeDxe/Runtime.c b/MdeModulePkg/Core/RuntimeDxe/Runtime.c
index c52b2b7ecf..394baf230a 100644
--- a/MdeModulePkg/Core/RuntimeDxe/Runtime.c
+++ b/MdeModulePkg/Core/RuntimeDxe/Runtime.c
@@ -35,7 +35,7 @@ Revision History:
Table now contains an item named CalculateCrc32.
-Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -359,6 +359,65 @@ RuntimeDriverSetVirtualAddressMap (
return EFI_SUCCESS;
}
+/**
+ Set the L"RuntimeServicesSupported" variable depend on the pcd PcdRuntimeServicesSupport.
+
+ Firstly try to get the variable, it may be set in other dxe driver. If it is set, then return
+ EFI_SUCCESS. If it isn't present, try to set it.
+
+ @retval EFI_SUCCESS The variable is already set.
+ EFI_NOT_FOUND All runtime services are supported at runtime. No variable is set.
+ Others Error to get variable or set variable. Unexpected.
+**/
+static
+EFI_STATUS
+SetRuntimeServicesSupported (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ UINT16 RuntimeServicesSupported;
+ UINT32 Attributes;
+ UINTN DataSize;
+
+ //
+ // Firstly try to get L"RuntimeServicesSupported" variable
+ //
+ Attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS;
+ DataSize = sizeof (UINT16);
+ Status = gRT->GetVariable (
+ L"RuntimeServicesSupported",
+ &gEfiGlobalVariableGuid,
+ &Attributes,
+ &DataSize,
+ &RuntimeServicesSupported
+ );
+
+ if (Status == EFI_NOT_FOUND) {
+ //
+ // L"RuntimeServicesSupported" isn't set yet. Then set it if
+ // some of the RuntimeServices is unsupported.
+ //
+ RuntimeServicesSupported = PcdGet16 (PcdRuntimeServicesSupport);
+ if (RuntimeServicesSupported != 0x3FFF) {
+ Status = gRT->SetVariable (
+ L"RuntimeServicesSupported",
+ &gEfiGlobalVariableGuid,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+ sizeof (UINT16),
+ &RuntimeServicesSupported
+ );
+ } else {
+ //
+ // Set Status to EFI_NOT_FOUND to indicate not such variable
+ //
+ Status = EFI_NOT_FOUND;
+ }
+ }
+
+ return Status;
+}
+
/**
Entry Point for Runtime driver.
@@ -401,6 +460,10 @@ RuntimeDriverInitialize (
gRT->SetVirtualAddressMap = RuntimeDriverSetVirtualAddressMap;
gRT->ConvertPointer = RuntimeDriverConvertPointer;
+ Status = SetRuntimeServicesSupported ();
+
+ ASSERT (Status == EFI_NOT_FOUND || Status == EFI_SUCCESS);
+
//
// Install the Runtime Architectural Protocol onto a new handle
//
diff --git a/MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf b/MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
index 694c7690fa..48ecec9e99 100644
--- a/MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
+++ b/MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
@@ -5,7 +5,7 @@
# CalculateCrc32 boot services table, SetVirtualAddressMap & ConvertPointer
# runtime services table.
#
-# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -53,6 +53,12 @@
gEfiRuntimeArchProtocolGuid ## PRODUCES
gEfiLoadedImageProtocolGuid ## CONSUMES
+[Guids]
+ gEfiGlobalVariableGuid ## SOMETIMES_CONSUMES ## Variable L"RuntimeServicesSupported"
+
+[Pcd]
+ gEfiMdePkgTokenSpaceGuid.PcdRuntimeServicesSupport ## SOME_TIMES_CONSUMES
+
[depex]
TRUE
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH V2 3/4] MdeModulePkg/RuntimeDxe: Set RuntimeServicesSupport base on Pcd
2019-07-19 8:09 ` [PATCH V2 3/4] MdeModulePkg/RuntimeDxe: Set RuntimeServicesSupport base on Pcd Gao, Zhichao
@ 2019-07-19 16:01 ` Michael D Kinney
0 siblings, 0 replies; 12+ messages in thread
From: Michael D Kinney @ 2019-07-19 16:01 UTC (permalink / raw)
To: devel@edk2.groups.io, Gao, Zhichao, Kinney, Michael D,
Rothman, Michael A
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Zeng, Star, Gao, Liming,
Sean Brogan, Michael Turner, Bret Barkelew, Laszlo Ersek
Zhichao,
I realized that MdeModulePkg/Core/RuntimeDxe may not be the
best place to set this variable. This module has a depex of
TRUE, so there is no guarantee that the variable services are
available when this module is dispatched.
This new variable is only intended to be consumed by OS Loaders
and OS Kernels, so the latest time that this variable can be
set is Ready To Boot in BDS.
All UEFI Services are available when gBds->Entry() is called
from the DXE Core, so maybe the best place to set this new
variable is in gBds->Entry().
Thanks,
Mike
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Gao, Zhichao
> Sent: Friday, July 19, 2019 1:09 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH V2 3/4]
> MdeModulePkg/RuntimeDxe: Set RuntimeServicesSupport base
> on Pcd
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1907
>
> Add a pcd named PcdRuntimeServicesSupport, RuntimeDxe
> driver would set variable L"RuntimeServicesSupported"
> base on this pcd. The varialbe would indicate whether
> the runtime services is supported or not in runtime
> phase. If the pcd's value is 0x3FFF, that means all
> runtime services is supported at runtime, then the
> variable wouldn't be set.
> Refer to UEFI spec 2.8, Section 8.1.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
> MdeModulePkg/Core/RuntimeDxe/Runtime.c | 65
> ++++++++++++++++++++-
> MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf | 8 ++-
> 2 files changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Core/RuntimeDxe/Runtime.c
> b/MdeModulePkg/Core/RuntimeDxe/Runtime.c
> index c52b2b7ecf..394baf230a 100644
> --- a/MdeModulePkg/Core/RuntimeDxe/Runtime.c
> +++ b/MdeModulePkg/Core/RuntimeDxe/Runtime.c
> @@ -35,7 +35,7 @@ Revision History:
> Table now contains an item named CalculateCrc32.
>
>
> -Copyright (c) 2006 - 2015, Intel Corporation. All
> rights reserved.<BR>
> +Copyright (c) 2006 - 2019, Intel Corporation. All
> rights reserved.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -359,6 +359,65 @@ RuntimeDriverSetVirtualAddressMap (
> return EFI_SUCCESS;
> }
>
> +/**
> + Set the L"RuntimeServicesSupported" variable depend
> on the pcd PcdRuntimeServicesSupport.
> +
> + Firstly try to get the variable, it may be set in
> other dxe driver.
> + If it is set, then return EFI_SUCCESS. If it isn't
> present, try to set it.
> +
> + @retval EFI_SUCCESS The variable is already
> set.
> + EFI_NOT_FOUND All runtime services are
> supported at runtime. No variable is set.
> + Others Error to get variable or
> set variable. Unexpected.
> +**/
> +static
> +EFI_STATUS
> +SetRuntimeServicesSupported (
> + VOID
> + )
> +{
> + EFI_STATUS Status;
> + UINT16 RuntimeServicesSupported;
> + UINT32 Attributes;
> + UINTN DataSize;
> +
> + //
> + // Firstly try to get L"RuntimeServicesSupported"
> variable //
> + Attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS; DataSize = sizeof
> (UINT16); Status =
> + gRT->GetVariable (
> + L"RuntimeServicesSupported",
> + &gEfiGlobalVariableGuid,
> + &Attributes,
> + &DataSize,
> + &RuntimeServicesSupported
> + );
> +
> + if (Status == EFI_NOT_FOUND) {
> + //
> + // L"RuntimeServicesSupported" isn't set yet. Then
> set it if
> + // some of the RuntimeServices is unsupported.
> + //
> + RuntimeServicesSupported = PcdGet16
> (PcdRuntimeServicesSupport);
> + if (RuntimeServicesSupported != 0x3FFF) {
> + Status = gRT->SetVariable (
> + L"RuntimeServicesSupported",
> + &gEfiGlobalVariableGuid,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> EFI_VARIABLE_RUNTIME_ACCESS,
> + sizeof (UINT16),
> + &RuntimeServicesSupported
> + );
> + } else {
> + //
> + // Set Status to EFI_NOT_FOUND to indicate not
> such variable
> + //
> + Status = EFI_NOT_FOUND;
> + }
> + }
> +
> + return Status;
> +}
> +
> /**
> Entry Point for Runtime driver.
>
> @@ -401,6 +460,10 @@ RuntimeDriverInitialize (
> gRT->SetVirtualAddressMap =
> RuntimeDriverSetVirtualAddressMap;
> gRT->ConvertPointer =
> RuntimeDriverConvertPointer;
>
> + Status = SetRuntimeServicesSupported ();
> +
> + ASSERT (Status == EFI_NOT_FOUND || Status ==
> EFI_SUCCESS);
> +
> //
> // Install the Runtime Architectural Protocol onto a
> new handle
> //
> diff --git a/MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
> b/MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
> index 694c7690fa..48ecec9e99 100644
> --- a/MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
> +++ b/MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
> @@ -5,7 +5,7 @@
> # CalculateCrc32 boot services table,
> SetVirtualAddressMap & ConvertPointer # runtime
> services table.
> #
> -# Copyright (c) 2006 - 2018, Intel Corporation. All
> rights reserved.<BR>
> +# Copyright (c) 2006 - 2019, Intel Corporation. All
> rights
> +reserved.<BR>
> #
> # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -
> 53,6 +53,12 @@
> gEfiRuntimeArchProtocolGuid ##
> PRODUCES
> gEfiLoadedImageProtocolGuid ##
> CONSUMES
>
> +[Guids]
> + gEfiGlobalVariableGuid ##
> SOMETIMES_CONSUMES ## Variable
> L"RuntimeServicesSupported"
> +
> +[Pcd]
> + gEfiMdePkgTokenSpaceGuid.PcdRuntimeServicesSupport
> ## SOME_TIMES_CONSUMES
> +
> [depex]
> TRUE
>
> --
> 2.21.0.windows.1
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2 4/4] MdeModulePkg/CapsuleRuntimeDxe: Implement RuntimeServicesSupported
2019-07-19 8:09 [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder Gao, Zhichao
` (2 preceding siblings ...)
2019-07-19 8:09 ` [PATCH V2 3/4] MdeModulePkg/RuntimeDxe: Set RuntimeServicesSupport base on Pcd Gao, Zhichao
@ 2019-07-19 8:09 ` Gao, Zhichao
2019-07-19 16:08 ` [edk2-devel] " Michael D Kinney
2019-07-19 14:15 ` [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder Laszlo Ersek
4 siblings, 1 reply; 12+ messages in thread
From: Gao, Zhichao @ 2019-07-19 8:09 UTC (permalink / raw)
To: devel
Cc: Jian J Wang, Hao A Wu, Ray Ni, Star Zeng, Liming gao, Sean Brogan,
Michael Turner, Bret Barkelew, Laszlo Ersek
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1907
Control the capsule services supported at runtime base on
the variable L"RuntimeServicesSupported".
It would update a global variable mRuntimeServicesSupported
at ExitBootServices event.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf | 2 +
.../CapsuleRuntimeDxe/CapsuleService.c | 68 +++++++++++++++++++
2 files changed, 70 insertions(+)
diff --git a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
index 9da450722b..15d498863a 100644
--- a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
+++ b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
@@ -72,6 +72,8 @@
## SOMETIMES_PRODUCES ## Variable:L"CapsuleLongModeBuffer" # The long mode buffer used by IA32 Capsule PEIM to call X64 CapsuleCoalesce code to handle >4GB capsule blocks
gEfiCapsuleVendorGuid
gEfiFmpCapsuleGuid ## SOMETIMES_CONSUMES ## GUID # FMP capsule GUID
+ gEfiGlobalVariableGuid ## CONSUMES ## Variable L"RuntimeServicesSupported"
+ gEfiEventExitBootServicesGuid ## CONSUMES
[Protocols]
gEfiCapsuleArchProtocolGuid ## PRODUCES
diff --git a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c
index 77b8f00062..e4e700764b 100644
--- a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c
+++ b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c
@@ -24,6 +24,13 @@ UINTN mTimes = 0;
UINT32 mMaxSizePopulateCapsule = 0;
UINT32 mMaxSizeNonPopulateCapsule = 0;
+//
+// Runtime Services Supported Flag
+// Initialize it to 0x3FFF to indicate it is all supported before runtime
+//
+static UINT16 mRuntimeServicesSupported = 0x3FFF;
+
+
/**
Passes capsules to the firmware with both virtual and physical mapping. Depending on the intended
consumption, the firmware may process the capsule immediately. If the payload should persist
@@ -71,6 +78,10 @@ UpdateCapsule (
CHAR16 CapsuleVarName[30];
CHAR16 *TempVarName;
+ if (!(mRuntimeServicesSupported | EFI_RT_SUPPORTED_UPDATE_CAPSULE)) {
+ return EFI_UNSUPPORTED;
+ }
+
//
// Check if platform support Capsule In RAM or not.
// Platform could choose to drop CapsulePei/CapsuleX64 and do not support Capsule In RAM.
@@ -259,6 +270,10 @@ QueryCapsuleCapabilities (
EFI_CAPSULE_HEADER *CapsuleHeader;
BOOLEAN NeedReset;
+ if (!(mRuntimeServicesSupported | EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES)) {
+ return EFI_UNSUPPORTED;
+ }
+
//
// Capsule Count can't be less than one.
//
@@ -343,6 +358,48 @@ QueryCapsuleCapabilities (
return EFI_SUCCESS;
}
+/**
+ ExitBootServices Event to update the mRuntimeServicesSupported to
+ affect the runtime services.
+
+ @param[in] Event Event whose notification function is being invoked
+ @param[in] Context Pointer to the notification function's context
+**/
+static
+VOID
+EFIAPI
+UpdateRuntimeServicesSupported (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ EFI_STATUS Status;
+ UINT16 RuntimeServicesSupported;
+ UINT32 Attributes;
+ UINTN DataSize;
+
+ Attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS;
+ DataSize = sizeof (UINT16);
+ Status = gRT->GetVariable (
+ L"RuntimeServicesSupported",
+ &gEfiGlobalVariableGuid,
+ &Attributes,
+ &DataSize,
+ &RuntimeServicesSupported
+ );
+
+ if (!EFI_ERROR (Status)) {
+ mRuntimeServicesSupported = RuntimeServicesSupported;
+ } else if (Status == EFI_NOT_FOUND) {
+ mRuntimeServicesSupported = 0x3FFF;
+ } else {
+ //
+ // Should never arrive here.
+ //
+ ASSERT_EFI_ERROR (Status);
+ }
+}
+
/**
@@ -362,6 +419,7 @@ CapsuleServiceInitialize (
)
{
EFI_STATUS Status;
+ EFI_EVENT Event;
mMaxSizePopulateCapsule = PcdGet32(PcdMaxSizePopulateCapsule);
mMaxSizeNonPopulateCapsule = PcdGet32(PcdMaxSizeNonPopulateCapsule);
@@ -381,6 +439,16 @@ CapsuleServiceInitialize (
gRT->UpdateCapsule = UpdateCapsule;
gRT->QueryCapsuleCapabilities = QueryCapsuleCapabilities;
+ Status = gBS->CreateEventEx(
+ EVT_NOTIFY_SIGNAL,
+ TPL_NOTIFY,
+ UpdateRuntimeServicesSupported,
+ NULL,
+ &gEfiEventExitBootServicesGuid,
+ &Event
+ );
+ ASSERT_EFI_ERROR (Status);
+
//
// Install the Capsule Architectural Protocol on a new handle
// to signify the capsule runtime services are ready.
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH V2 4/4] MdeModulePkg/CapsuleRuntimeDxe: Implement RuntimeServicesSupported
2019-07-19 8:09 ` [PATCH V2 4/4] MdeModulePkg/CapsuleRuntimeDxe: Implement RuntimeServicesSupported Gao, Zhichao
@ 2019-07-19 16:08 ` Michael D Kinney
2019-07-22 2:53 ` Gao, Zhichao
0 siblings, 1 reply; 12+ messages in thread
From: Michael D Kinney @ 2019-07-19 16:08 UTC (permalink / raw)
To: devel@edk2.groups.io, Gao, Zhichao, Kinney, Michael D
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Zeng, Star, Gao, Liming,
Sean Brogan, Michael Turner, Bret Barkelew, Laszlo Ersek
Zhichao,
The GetVariable and ExitBootServices events should be
removed from this patch. I recommend RT drivers that
produce RT services consume the new PCD to determine
if the services return EFI_UNSUPPORTED or not after
ExitBootServices(). This keeps RT Driver as simple as
possible and removes the need for the UEFI Variable to
be in a known state earlier in the boot and removes the
need for RT Drivers to setup extra notification events.
The new UEFI Variable is intended to be used by OS Loaders
and OS Kernels that call ExitBootServices() and need to
know if specific RT services are available or not after
ExitBootServices().
Thanks,
Mike
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Gao, Zhichao
> Sent: Friday, July 19, 2019 1:09 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH V2 4/4]
> MdeModulePkg/CapsuleRuntimeDxe: Implement
> RuntimeServicesSupported
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1907
>
> Control the capsule services supported at runtime base
> on the variable L"RuntimeServicesSupported".
> It would update a global variable
> mRuntimeServicesSupported at ExitBootServices event.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
> .../CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf | 2 +
> .../CapsuleRuntimeDxe/CapsuleService.c | 68
> +++++++++++++++++++
> 2 files changed, 70 insertions(+)
>
> diff --git
> a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> eDxe.inf
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> eDxe.inf
> index 9da450722b..15d498863a 100644
> ---
> a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> eDxe.inf
> +++
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> eDxe.inf
> @@ -72,6 +72,8 @@
> ## SOMETIMES_PRODUCES ##
> Variable:L"CapsuleLongModeBuffer" # The long mode buffer
> used by IA32 Capsule PEIM to call X64 CapsuleCoalesce
> code to handle >4GB capsule blocks
> gEfiCapsuleVendorGuid
> gEfiFmpCapsuleGuid ##
> SOMETIMES_CONSUMES ## GUID # FMP capsule GUID
> + gEfiGlobalVariableGuid ##
> CONSUMES ## Variable
> L"RuntimeServicesSupported"
> + gEfiEventExitBootServicesGuid ##
> CONSUMES
>
> [Protocols]
> gEfiCapsuleArchProtocolGuid ##
> PRODUCES
> diff --git
> a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> e.c
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> e.c
> index 77b8f00062..e4e700764b 100644
> ---
> a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> e.c
> +++
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> e.c
> @@ -24,6 +24,13 @@ UINTN mTimes = 0;
> UINT32 mMaxSizePopulateCapsule = 0;
> UINT32 mMaxSizeNonPopulateCapsule = 0;
>
> +//
> +// Runtime Services Supported Flag
> +// Initialize it to 0x3FFF to indicate it is all
> supported before
> +runtime //
> +static UINT16 mRuntimeServicesSupported = 0x3FFF;
> +
> +
> /**
> Passes capsules to the firmware with both virtual and
> physical mapping. Depending on the intended
> consumption, the firmware may process the capsule
> immediately. If the payload should persist @@ -71,6
> +78,10 @@ UpdateCapsule (
> CHAR16 CapsuleVarName[30];
> CHAR16 *TempVarName;
>
> + if (!(mRuntimeServicesSupported |
> EFI_RT_SUPPORTED_UPDATE_CAPSULE)) {
> + return EFI_UNSUPPORTED;
> + }
> +
> //
> // Check if platform support Capsule In RAM or not.
> // Platform could choose to drop
> CapsulePei/CapsuleX64 and do not support Capsule In RAM.
> @@ -259,6 +270,10 @@ QueryCapsuleCapabilities (
> EFI_CAPSULE_HEADER *CapsuleHeader;
> BOOLEAN NeedReset;
>
> + if (!(mRuntimeServicesSupported |
> EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES)) {
> + return EFI_UNSUPPORTED;
> + }
> +
> //
> // Capsule Count can't be less than one.
> //
> @@ -343,6 +358,48 @@ QueryCapsuleCapabilities (
> return EFI_SUCCESS;
> }
>
> +/**
> + ExitBootServices Event to update the
> mRuntimeServicesSupported to
> + affect the runtime services.
> +
> + @param[in] Event Event whose notification
> function is being invoked
> + @param[in] Context Pointer to the notification
> function's context
> +**/
> +static
> +VOID
> +EFIAPI
> +UpdateRuntimeServicesSupported (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> + )
> +{
> + EFI_STATUS Status;
> + UINT16 RuntimeServicesSupported;
> + UINT32 Attributes;
> + UINTN DataSize;
> +
> + Attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS; DataSize = sizeof
> (UINT16); Status =
> + gRT->GetVariable (
> + L"RuntimeServicesSupported",
> + &gEfiGlobalVariableGuid,
> + &Attributes,
> + &DataSize,
> + &RuntimeServicesSupported
> + );
> +
> + if (!EFI_ERROR (Status)) {
> + mRuntimeServicesSupported =
> RuntimeServicesSupported;
> + } else if (Status == EFI_NOT_FOUND) {
> + mRuntimeServicesSupported = 0x3FFF;
> + } else {
> + //
> + // Should never arrive here.
> + //
> + ASSERT_EFI_ERROR (Status);
> + }
> +}
> +
>
> /**
>
> @@ -362,6 +419,7 @@ CapsuleServiceInitialize (
> )
> {
> EFI_STATUS Status;
> + EFI_EVENT Event;
>
> mMaxSizePopulateCapsule =
> PcdGet32(PcdMaxSizePopulateCapsule);
> mMaxSizeNonPopulateCapsule =
> PcdGet32(PcdMaxSizeNonPopulateCapsule);
> @@ -381,6 +439,16 @@ CapsuleServiceInitialize (
> gRT->UpdateCapsule =
> UpdateCapsule;
> gRT->QueryCapsuleCapabilities =
> QueryCapsuleCapabilities;
>
> + Status = gBS->CreateEventEx(
> + EVT_NOTIFY_SIGNAL,
> + TPL_NOTIFY,
> + UpdateRuntimeServicesSupported,
> + NULL,
> + &gEfiEventExitBootServicesGuid,
> + &Event
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> //
> // Install the Capsule Architectural Protocol on a
> new handle
> // to signify the capsule runtime services are ready.
> --
> 2.21.0.windows.1
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH V2 4/4] MdeModulePkg/CapsuleRuntimeDxe: Implement RuntimeServicesSupported
2019-07-19 16:08 ` [edk2-devel] " Michael D Kinney
@ 2019-07-22 2:53 ` Gao, Zhichao
2019-07-22 3:39 ` Michael D Kinney
0 siblings, 1 reply; 12+ messages in thread
From: Gao, Zhichao @ 2019-07-22 2:53 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Zeng, Star, Gao, Liming,
Sean Brogan, Michael Turner, Bret Barkelew, Laszlo Ersek
Hi Mike,
I agree to remove the GetVariable. But why for ExitBootServices? If we remove that, the services would be disabled at both boot phase and runtime phase.
Thanks,
Zhichao
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Saturday, July 20, 2019 12:09 AM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael
> Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [edk2-devel] [PATCH V2 4/4] MdeModulePkg/CapsuleRuntimeDxe:
> Implement RuntimeServicesSupported
>
> Zhichao,
>
> The GetVariable and ExitBootServices events should be removed from this
> patch. I recommend RT drivers that produce RT services consume the new PCD
> to determine if the services return EFI_UNSUPPORTED or not after
> ExitBootServices(). This keeps RT Driver as simple as possible and removes the
> need for the UEFI Variable to be in a known state earlier in the boot and
> removes the need for RT Drivers to setup extra notification events.
>
> The new UEFI Variable is intended to be used by OS Loaders and OS Kernels
> that call ExitBootServices() and need to know if specific RT services are
> available or not after ExitBootServices().
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Gao, Zhichao
> > Sent: Friday, July 19, 2019 1:09 AM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> > <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean Brogan
> > <sean.brogan@microsoft.com>; Michael Turner
> > <Michael.Turner@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: [edk2-devel] [PATCH V2 4/4]
> > MdeModulePkg/CapsuleRuntimeDxe: Implement RuntimeServicesSupported
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1907
> >
> > Control the capsule services supported at runtime base on the variable
> > L"RuntimeServicesSupported".
> > It would update a global variable
> > mRuntimeServicesSupported at ExitBootServices event.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Liming gao <liming.gao@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Michael Turner <Michael.Turner@microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > ---
> > .../CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf | 2 +
> > .../CapsuleRuntimeDxe/CapsuleService.c | 68
> > +++++++++++++++++++
> > 2 files changed, 70 insertions(+)
> >
> > diff --git
> > a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> > eDxe.inf
> > b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> > eDxe.inf
> > index 9da450722b..15d498863a 100644
> > ---
> > a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> > eDxe.inf
> > +++
> > b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> > eDxe.inf
> > @@ -72,6 +72,8 @@
> > ## SOMETIMES_PRODUCES ##
> > Variable:L"CapsuleLongModeBuffer" # The long mode buffer used by IA32
> > Capsule PEIM to call X64 CapsuleCoalesce code to handle >4GB capsule
> > blocks
> > gEfiCapsuleVendorGuid
> > gEfiFmpCapsuleGuid ##
> > SOMETIMES_CONSUMES ## GUID # FMP capsule GUID
> > + gEfiGlobalVariableGuid ##
> > CONSUMES ## Variable
> > L"RuntimeServicesSupported"
> > + gEfiEventExitBootServicesGuid ##
> > CONSUMES
> >
> > [Protocols]
> > gEfiCapsuleArchProtocolGuid ##
> > PRODUCES
> > diff --git
> > a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> > e.c
> > b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> > e.c
> > index 77b8f00062..e4e700764b 100644
> > ---
> > a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> > e.c
> > +++
> > b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> > e.c
> > @@ -24,6 +24,13 @@ UINTN mTimes = 0;
> > UINT32 mMaxSizePopulateCapsule = 0;
> > UINT32 mMaxSizeNonPopulateCapsule = 0;
> >
> > +//
> > +// Runtime Services Supported Flag
> > +// Initialize it to 0x3FFF to indicate it is all
> > supported before
> > +runtime //
> > +static UINT16 mRuntimeServicesSupported = 0x3FFF;
> > +
> > +
> > /**
> > Passes capsules to the firmware with both virtual and physical
> > mapping. Depending on the intended
> > consumption, the firmware may process the capsule immediately. If
> > the payload should persist @@ -71,6
> > +78,10 @@ UpdateCapsule (
> > CHAR16 CapsuleVarName[30];
> > CHAR16 *TempVarName;
> >
> > + if (!(mRuntimeServicesSupported |
> > EFI_RT_SUPPORTED_UPDATE_CAPSULE)) {
> > + return EFI_UNSUPPORTED;
> > + }
> > +
> > //
> > // Check if platform support Capsule In RAM or not.
> > // Platform could choose to drop
> > CapsulePei/CapsuleX64 and do not support Capsule In RAM.
> > @@ -259,6 +270,10 @@ QueryCapsuleCapabilities (
> > EFI_CAPSULE_HEADER *CapsuleHeader;
> > BOOLEAN NeedReset;
> >
> > + if (!(mRuntimeServicesSupported |
> > EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES)) {
> > + return EFI_UNSUPPORTED;
> > + }
> > +
> > //
> > // Capsule Count can't be less than one.
> > //
> > @@ -343,6 +358,48 @@ QueryCapsuleCapabilities (
> > return EFI_SUCCESS;
> > }
> >
> > +/**
> > + ExitBootServices Event to update the
> > mRuntimeServicesSupported to
> > + affect the runtime services.
> > +
> > + @param[in] Event Event whose notification
> > function is being invoked
> > + @param[in] Context Pointer to the notification
> > function's context
> > +**/
> > +static
> > +VOID
> > +EFIAPI
> > +UpdateRuntimeServicesSupported (
> > + IN EFI_EVENT Event,
> > + IN VOID *Context
> > + )
> > +{
> > + EFI_STATUS Status;
> > + UINT16 RuntimeServicesSupported;
> > + UINT32 Attributes;
> > + UINTN DataSize;
> > +
> > + Attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS; DataSize = sizeof
> > (UINT16); Status =
> > + gRT->GetVariable (
> > + L"RuntimeServicesSupported",
> > + &gEfiGlobalVariableGuid,
> > + &Attributes,
> > + &DataSize,
> > + &RuntimeServicesSupported
> > + );
> > +
> > + if (!EFI_ERROR (Status)) {
> > + mRuntimeServicesSupported =
> > RuntimeServicesSupported;
> > + } else if (Status == EFI_NOT_FOUND) {
> > + mRuntimeServicesSupported = 0x3FFF;
> > + } else {
> > + //
> > + // Should never arrive here.
> > + //
> > + ASSERT_EFI_ERROR (Status);
> > + }
> > +}
> > +
> >
> > /**
> >
> > @@ -362,6 +419,7 @@ CapsuleServiceInitialize (
> > )
> > {
> > EFI_STATUS Status;
> > + EFI_EVENT Event;
> >
> > mMaxSizePopulateCapsule =
> > PcdGet32(PcdMaxSizePopulateCapsule);
> > mMaxSizeNonPopulateCapsule =
> > PcdGet32(PcdMaxSizeNonPopulateCapsule);
> > @@ -381,6 +439,16 @@ CapsuleServiceInitialize (
> > gRT->UpdateCapsule =
> > UpdateCapsule;
> > gRT->QueryCapsuleCapabilities =
> > QueryCapsuleCapabilities;
> >
> > + Status = gBS->CreateEventEx(
> > + EVT_NOTIFY_SIGNAL,
> > + TPL_NOTIFY,
> > + UpdateRuntimeServicesSupported,
> > + NULL,
> > + &gEfiEventExitBootServicesGuid,
> > + &Event
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > +
> > //
> > // Install the Capsule Architectural Protocol on a new handle
> > // to signify the capsule runtime services are ready.
> > --
> > 2.21.0.windows.1
> >
> >
> >
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH V2 4/4] MdeModulePkg/CapsuleRuntimeDxe: Implement RuntimeServicesSupported
2019-07-22 2:53 ` Gao, Zhichao
@ 2019-07-22 3:39 ` Michael D Kinney
0 siblings, 0 replies; 12+ messages in thread
From: Michael D Kinney @ 2019-07-22 3:39 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io, Kinney, Michael D
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray, Zeng, Star, Gao, Liming,
Sean Brogan, Michael Turner, Bret Barkelew, Laszlo Ersek
The UefiRuntimeLib has an EfiAtRuntime() service. Pease
use that instead. The CapsuleRuntimeDxe module is already
using the UefiRuntimeLib library. The patch below adds a
2nd ExitBootServices event and global that is not needed.
Thanks,
Mike
> -----Original Message-----
> From: Gao, Zhichao
> Sent: Sunday, July 21, 2019 7:54 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [edk2-devel] [PATCH V2 4/4]
> MdeModulePkg/CapsuleRuntimeDxe: Implement
> RuntimeServicesSupported
>
> Hi Mike,
>
> I agree to remove the GetVariable. But why for
> ExitBootServices? If we remove that, the services would
> be disabled at both boot phase and runtime phase.
>
>
> Thanks,
> Zhichao
>
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: Saturday, July 20, 2019 12:09 AM
> > To: devel@edk2.groups.io; Gao, Zhichao
> <zhichao.gao@intel.com>;
> > Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng,
> Star
> > <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean Brogan
> > <sean.brogan@microsoft.com>; Michael Turner
> > <Michael.Turner@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Laszlo Ersek
> <lersek@redhat.com>
> > Subject: RE: [edk2-devel] [PATCH V2 4/4]
> MdeModulePkg/CapsuleRuntimeDxe:
> > Implement RuntimeServicesSupported
> >
> > Zhichao,
> >
> > The GetVariable and ExitBootServices events should be
> removed from
> > this patch. I recommend RT drivers that produce RT
> services consume
> > the new PCD to determine if the services return
> EFI_UNSUPPORTED or not
> > after ExitBootServices(). This keeps RT Driver as
> simple as possible
> > and removes the need for the UEFI Variable to be in a
> known state
> > earlier in the boot and removes the need for RT Drivers
> to setup extra notification events.
> >
> > The new UEFI Variable is intended to be used by OS
> Loaders and OS
> > Kernels that call ExitBootServices() and need to know
> if specific RT
> > services are available or not after ExitBootServices().
> >
> > Thanks,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf
> > > Of Gao, Zhichao
> > > Sent: Friday, July 19, 2019 1:09 AM
> > > To: devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Zeng, Star
> > > <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean
> > > Brogan <sean.brogan@microsoft.com>; Michael Turner
> > > <Michael.Turner@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Laszlo Ersek
> <lersek@redhat.com>
> > > Subject: [edk2-devel] [PATCH V2 4/4]
> > > MdeModulePkg/CapsuleRuntimeDxe: Implement
> RuntimeServicesSupported
> > >
> > > REF:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1907
> > >
> > > Control the capsule services supported at runtime
> base on the
> > > variable L"RuntimeServicesSupported".
> > > It would update a global variable
> > > mRuntimeServicesSupported at ExitBootServices event.
> > >
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Star Zeng <star.zeng@intel.com>
> > > Cc: Liming gao <liming.gao@intel.com>
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > Cc: Michael Turner <Michael.Turner@microsoft.com>
> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > > ---
> > > .../CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf | 2 +
> > > .../CapsuleRuntimeDxe/CapsuleService.c | 68
> > > +++++++++++++++++++
> > > 2 files changed, 70 insertions(+)
> > >
> > > diff --git
> > >
> a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> > > eDxe.inf
> > >
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> > > eDxe.inf
> > > index 9da450722b..15d498863a 100644
> > > ---
> > >
> a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> > > eDxe.inf
> > > +++
> > >
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntim
> > > eDxe.inf
> > > @@ -72,6 +72,8 @@
> > > ## SOMETIMES_PRODUCES ##
> > > Variable:L"CapsuleLongModeBuffer" # The long mode
> buffer used by
> > > IA32 Capsule PEIM to call X64 CapsuleCoalesce code to
> handle >4GB
> > > capsule blocks
> > > gEfiCapsuleVendorGuid
> > > gEfiFmpCapsuleGuid ##
> > > SOMETIMES_CONSUMES ## GUID # FMP capsule GUID
> > > + gEfiGlobalVariableGuid ##
> > > CONSUMES ## Variable
> > > L"RuntimeServicesSupported"
> > > + gEfiEventExitBootServicesGuid ##
> > > CONSUMES
> > >
> > > [Protocols]
> > > gEfiCapsuleArchProtocolGuid ##
> > > PRODUCES
> > > diff --git
> > >
> a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> > > e.c
> > >
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> > > e.c
> > > index 77b8f00062..e4e700764b 100644
> > > ---
> > >
> a/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> > > e.c
> > > +++
> > >
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleServic
> > > e.c
> > > @@ -24,6 +24,13 @@ UINTN mTimes = 0;
> > > UINT32 mMaxSizePopulateCapsule = 0;
> > > UINT32 mMaxSizeNonPopulateCapsule = 0;
> > >
> > > +//
> > > +// Runtime Services Supported Flag
> > > +// Initialize it to 0x3FFF to indicate it is all
> > > supported before
> > > +runtime //
> > > +static UINT16 mRuntimeServicesSupported =
> 0x3FFF;
> > > +
> > > +
> > > /**
> > > Passes capsules to the firmware with both virtual
> and physical
> > > mapping. Depending on the intended
> > > consumption, the firmware may process the capsule
> immediately. If
> > > the payload should persist @@ -71,6
> > > +78,10 @@ UpdateCapsule (
> > > CHAR16 CapsuleVarName[30];
> > > CHAR16 *TempVarName;
> > >
> > > + if (!(mRuntimeServicesSupported |
> > > EFI_RT_SUPPORTED_UPDATE_CAPSULE)) {
> > > + return EFI_UNSUPPORTED;
> > > + }
> > > +
> > > //
> > > // Check if platform support Capsule In RAM or
> not.
> > > // Platform could choose to drop
> > > CapsulePei/CapsuleX64 and do not support Capsule In
> RAM.
> > > @@ -259,6 +270,10 @@ QueryCapsuleCapabilities (
> > > EFI_CAPSULE_HEADER *CapsuleHeader;
> > > BOOLEAN NeedReset;
> > >
> > > + if (!(mRuntimeServicesSupported |
> > > EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES)) {
> > > + return EFI_UNSUPPORTED;
> > > + }
> > > +
> > > //
> > > // Capsule Count can't be less than one.
> > > //
> > > @@ -343,6 +358,48 @@ QueryCapsuleCapabilities (
> > > return EFI_SUCCESS;
> > > }
> > >
> > > +/**
> > > + ExitBootServices Event to update the
> > > mRuntimeServicesSupported to
> > > + affect the runtime services.
> > > +
> > > + @param[in] Event Event whose notification
> > > function is being invoked
> > > + @param[in] Context Pointer to the notification
> > > function's context
> > > +**/
> > > +static
> > > +VOID
> > > +EFIAPI
> > > +UpdateRuntimeServicesSupported (
> > > + IN EFI_EVENT Event,
> > > + IN VOID *Context
> > > + )
> > > +{
> > > + EFI_STATUS Status;
> > > + UINT16 RuntimeServicesSupported;
> > > + UINT32 Attributes;
> > > + UINTN DataSize;
> > > +
> > > + Attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > + EFI_VARIABLE_RUNTIME_ACCESS; DataSize = sizeof
> > > (UINT16); Status =
> > > + gRT->GetVariable (
> > > + L"RuntimeServicesSupported",
> > > + &gEfiGlobalVariableGuid,
> > > + &Attributes,
> > > + &DataSize,
> > > + &RuntimeServicesSupported
> > > + );
> > > +
> > > + if (!EFI_ERROR (Status)) {
> > > + mRuntimeServicesSupported =
> > > RuntimeServicesSupported;
> > > + } else if (Status == EFI_NOT_FOUND) {
> > > + mRuntimeServicesSupported = 0x3FFF;
> > > + } else {
> > > + //
> > > + // Should never arrive here.
> > > + //
> > > + ASSERT_EFI_ERROR (Status);
> > > + }
> > > +}
> > > +
> > >
> > > /**
> > >
> > > @@ -362,6 +419,7 @@ CapsuleServiceInitialize (
> > > )
> > > {
> > > EFI_STATUS Status;
> > > + EFI_EVENT Event;
> > >
> > > mMaxSizePopulateCapsule =
> > > PcdGet32(PcdMaxSizePopulateCapsule);
> > > mMaxSizeNonPopulateCapsule =
> > > PcdGet32(PcdMaxSizeNonPopulateCapsule);
> > > @@ -381,6 +439,16 @@ CapsuleServiceInitialize (
> > > gRT->UpdateCapsule =
> > > UpdateCapsule;
> > > gRT->QueryCapsuleCapabilities =
> > > QueryCapsuleCapabilities;
> > >
> > > + Status = gBS->CreateEventEx(
> > > + EVT_NOTIFY_SIGNAL,
> > > + TPL_NOTIFY,
> > > + UpdateRuntimeServicesSupported,
> > > + NULL,
> > > + &gEfiEventExitBootServicesGuid,
> > > + &Event
> > > + );
> > > + ASSERT_EFI_ERROR (Status);
> > > +
> > > //
> > > // Install the Capsule Architectural Protocol on a
> new handle
> > > // to signify the capsule runtime services are
> ready.
> > > --
> > > 2.21.0.windows.1
> > >
> > >
> > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder
2019-07-19 8:09 [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder Gao, Zhichao
` (3 preceding siblings ...)
2019-07-19 8:09 ` [PATCH V2 4/4] MdeModulePkg/CapsuleRuntimeDxe: Implement RuntimeServicesSupported Gao, Zhichao
@ 2019-07-19 14:15 ` Laszlo Ersek
2019-07-22 3:17 ` [edk2-devel] " Gao, Zhichao
4 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-07-19 14:15 UTC (permalink / raw)
To: Zhichao Gao, devel
Cc: Michael D Kinney, Liming Gao, Jian J Wang, Hao A Wu, Ray Ni,
Star Zeng, Sean Brogan, Michael Turner, Bret Barkelew
Hi Zhichao,
On 07/19/19 10:09, Zhichao Gao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1979
>
> V1:
> UEFI spec 2.8 introduce a new variable L"RuntimeServicesSupported". If
> some runtime sevices is not supported at runtime phase, the variable
> should present at boot services. It is a bitmask value, the bit value
> of zero indicate the related runtime services is not supported at
> runtime phase.
> Add the difinition and use it to control Capsule runtime services.
>
> V2:
> Adjust the indent of uni file.
> Move the set variable function from CapsuleRuntimeDxe to RuntimeDxe.
> Add 'EFIAPI' to the event function "UpdateRuntimeServicesSupported",
> lacking of it would cause the GCC build failure.
(1) First of all, I think something must have gone wrong with your
posting. Your cover letter carries the subject
Add a pcd PcdBootManagerInBootOrder to control whether BootManager is
in BootOrder
and references TianoCore#1979.
However, all four patches in the series belong to TianoCore#1907, and
the *contents* of the cover letter are also related to TianoCore#1907.
So basically I think the subject line and the BZ reference in your cover
letter are incorrect.
(2) I have read your answers at:
http://mid.mail-archive.com/3CE959C139B4C44DBEA1810E3AA6F9000B808772@SHSMSX101.ccr.corp.intel.com
https://edk2.groups.io/g/devel/message/43899
If I understand correctly, you said that the new PCD / standardized UEFI
variable is a pure addition, and that platforms can *transparently*
inherit this feature enablement in the runtime DXE core and
CapsuleRuntimeDxe.
Did I understand your answer correctly?
If so, then I disagree. In my opinion, this is *not* a transparent
change for platforms. And that's because of the following change in the
UEFI specification:
* In UEFI v2.7 Errata B, the EFI_UNSUPPORTED return status is documented
as follows, for the UpdateCapsule() runtime service:
"The capsule type is not supported on this platform."
And for the QueryCapsuleCapabilities() runtime service:
"The capsule type is not supported on this platform, and
/MaximumCapsuleSize/ and /ResetType/ are undefined."
* In UEFI v2.8, the same return status specifications are preserved, but
the following ones are added too (for EFI_UNSUPPORTED), under both
UpdateCapsule() and QueryCapsuleCapabilities():
"This call is not supported by this platform at the time the call is
made. The platform must correctly reflect this behavior in the
/RuntimeServicesSupported/ variable."
Therefore, if a platform knows that it will return EFI_UNSUPPORTED
*consistently* (due to platform limitations) from these runtime
services, then UEFI-2.8 *requires* the platform to advertize that fact
in the new UEFI variable.
(3) If a platform links DxeCapsuleLibNull into CapsuleRuntimeDxe, that
has the following consequences:
- QueryCapsuleCapabilities()
[MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c] calls
SupportCapsuleImage()
[MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.c].
The return status is EFI_UNSUPPORTED, consistently.
- UpdateCapsule()
[MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c] calls both
SupportCapsuleImage() -- see above -- and ProcessCapsuleImage()
[MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.c].
The return status is EFI_UNSUPPORTED, consistently.
Meaning that, if a platform uses DxeCapsuleLibNull, it *must* clear the
EFI_RT_SUPPORTED_UPDATE_CAPSULE and
EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES bits in the
"RuntimeServicesSupported" variable.
Now, your patch introduces "PcdRuntimeServicesSupport" in the
[PcdsFixedAtBuild] section of "MdePkg.dec". Based on that, I think we
should add a CONSTRUCTOR function to DxeCapsuleLibNull, as a separate
patch.
The constructor function should do:
if (((FixedPcdGet16 (PcdRuntimeServicesSupport) &
EFI_RT_SUPPORTED_UPDATE_CAPSULE) != 0) ||
((FixedPcdGet16 (PcdRuntimeServicesSupport) &
EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES) != 0)) {
//
// This library instance is unsuitable for implementing the
// UpdateCapsule() and SupportCapsuleImage() runtime services.
//
return EFI_UNSUPPORTED;
}
return EFI_SUCCESS;
Why is this important? Because it will *force* platforms to expose their
lack of capsule support in the new PCD. Otherwise, the firmware will not
boot -- and that is impossible to miss.
(4) The situation is somewhat similar with "PcdCapsuleInRamSupport". If
"PcdCapsuleInRamSupport" is FALSE, then UpdateCapsule() will always
return EFI_UNSUPPORTED.
Therefore, the entry point function of CapsuleRuntimeDxe --
CapsuleServiceInitialize() -- should get the following assertion:
ASSERT (
PcdGetBool (PcdCapsuleInRamSupport) ||
((FixedPcdGet16 (PcdRuntimeServicesSupport) &
EFI_RT_SUPPORTED_UPDATE_CAPSULE) == 0)
);
(5) For each platform in the edk2 tree that either uses
DxeCapsuleLibNull or sets "PcdCapsuleInRamSupport" to FALSE, the
corresponding bits should be cleared in "PcdRuntimeServicesSupport", in
the platform DSC files.
This would mean a number of new patches for this series.
(6) With the sanity checks from points (3) and (4) implemented, I agree
that CapsuleRuntimeDxe is permitted to consume
"PcdRuntimeServicesSupport", in patch#4, and to introduce new
EFI_UNSUPPORTED exit points into QueryCapsuleCapabilities() and
UpdateCapsule().
However:
(6a) In patch#4, CapsuleRuntimeDxe consumes the new *UEFI variable*,
and not the new *PCD*. I think that's wrong; or at least sub-optiomal.
Earlier Mike wrote, in
http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F5B9D77345@ORSMSX113.amr.corp.intel.com
https://edk2.groups.io/g/devel/message/43890
that the runtime DXE Core should set the variable, and that individual
runtime drivers providing some runtime services should consume the
*PCD*. See the quote below, from Mike:
> I agree that each RT driver that populates the RT Services Table with
> a RT services can consume the new bitmask PCD and use the PCD to
> determine if the RT Service should return EFI_UNSUPPORTED after
> ExitBootServices().
So, CapsuleRuntimeDxe should base those new exit points on the PCD, and
the GetVariable() call should be removed.
(6b) The current bitmask checks in patch #4 are wrong:
> + if (!(mRuntimeServicesSupported | EFI_RT_SUPPORTED_UPDATE_CAPSULE)) {
> + return EFI_UNSUPPORTED;
> + }
> +
>
> + if (!(mRuntimeServicesSupported | EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES)) {
> + return EFI_UNSUPPORTED;
> + }
> +
First, the relevant bits should be extracted with the bitwise AND
operator, not the bitwise OR operator.
Second, after the extraction, the edk2 coding style dictates an explicit
comparison with zero, to my understanding. The logical negation operator
is only acceptable with BOOLEAN variables, and with such sub-expressions
that evaluate to FALSE/TRUE.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder
2019-07-19 14:15 ` [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder Laszlo Ersek
@ 2019-07-22 3:17 ` Gao, Zhichao
2019-07-22 17:28 ` Laszlo Ersek
0 siblings, 1 reply; 12+ messages in thread
From: Gao, Zhichao @ 2019-07-22 3:17 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com
Cc: Kinney, Michael D, Gao, Liming, Wang, Jian J, Wu, Hao A, Ni, Ray,
Zeng, Star, Sean Brogan, Michael Turner, Bret Barkelew
Hi Laszlo,
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Friday, July 19, 2019 10:15 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH V2 0/4] Add a pcd
> PcdBootManagerInBootOrder to control whether BootManager is in
> BootOrder
>
> Hi Zhichao,
>
> On 07/19/19 10:09, Zhichao Gao wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1979
> >
> > V1:
> > UEFI spec 2.8 introduce a new variable L"RuntimeServicesSupported". If
> > some runtime sevices is not supported at runtime phase, the variable
> > should present at boot services. It is a bitmask value, the bit value
> > of zero indicate the related runtime services is not supported at
> > runtime phase.
> > Add the difinition and use it to control Capsule runtime services.
> >
> > V2:
> > Adjust the indent of uni file.
> > Move the set variable function from CapsuleRuntimeDxe to RuntimeDxe.
> > Add 'EFIAPI' to the event function "UpdateRuntimeServicesSupported",
> > lacking of it would cause the GCC build failure.
>
> (1) First of all, I think something must have gone wrong with your posting.
> Your cover letter carries the subject
>
> Add a pcd PcdBootManagerInBootOrder to control whether BootManager is
> in BootOrder
>
> and references TianoCore#1979.
>
> However, all four patches in the series belong to TianoCore#1907, and the
> *contents* of the cover letter are also related to TianoCore#1907.
>
> So basically I think the subject line and the BZ reference in your cover letter
> are incorrect.
Sorry I mixed the two patch I am working on. The BZ link should be https://bugzilla.tianocore.org/show_bug.cgi?id=1907.
And the title should be MdePkg/MdeModulePkg: Introduce a pcd to control runtime capsule servives.
>
>
> (2) I have read your answers at:
>
> http://mid.mail-
> archive.com/3CE959C139B4C44DBEA1810E3AA6F9000B808772@SHSMSX101.c
> cr.corp.intel.com
> https://edk2.groups.io/g/devel/message/43899
>
> If I understand correctly, you said that the new PCD / standardized UEFI
> variable is a pure addition, and that platforms can *transparently* inherit this
> feature enablement in the runtime DXE core and CapsuleRuntimeDxe.
>
> Did I understand your answer correctly?
I didn't think of * transparent* things before.
>
> If so, then I disagree. In my opinion, this is *not* a transparent change for
> platforms. And that's because of the following change in the UEFI
> specification:
>
> * In UEFI v2.7 Errata B, the EFI_UNSUPPORTED return status is documented
> as follows, for the UpdateCapsule() runtime service:
>
> "The capsule type is not supported on this platform."
>
> And for the QueryCapsuleCapabilities() runtime service:
>
> "The capsule type is not supported on this platform, and
> /MaximumCapsuleSize/ and /ResetType/ are undefined."
>
> * In UEFI v2.8, the same return status specifications are preserved, but
> the following ones are added too (for EFI_UNSUPPORTED), under both
> UpdateCapsule() and QueryCapsuleCapabilities():
>
> "This call is not supported by this platform at the time the call is
> made. The platform must correctly reflect this behavior in the
> /RuntimeServicesSupported/ variable."
>
> Therefore, if a platform knows that it will return EFI_UNSUPPORTED
> *consistently* (due to platform limitations) from these runtime services,
> then UEFI-2.8 *requires* the platform to advertize that fact in the new UEFI
> variable.
The new pcd is set by the platform and the platform should aware that the pcd would set a new variable L"RuntimeServicesSupported".
It there some implementations that conflict with your description above?
>
>
> (3) If a platform links DxeCapsuleLibNull into CapsuleRuntimeDxe, that has
> the following consequences:
>
> - QueryCapsuleCapabilities()
> [MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c] calls
> SupportCapsuleImage()
> [MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.c].
>
> The return status is EFI_UNSUPPORTED, consistently.
>
> - UpdateCapsule()
> [MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c] calls
> both
> SupportCapsuleImage() -- see above -- and ProcessCapsuleImage()
> [MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.c].
>
> The return status is EFI_UNSUPPORTED, consistently.
>
> Meaning that, if a platform uses DxeCapsuleLibNull, it *must* clear the
> EFI_RT_SUPPORTED_UPDATE_CAPSULE and
> EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES bits in the
> "RuntimeServicesSupported" variable.
>
> Now, your patch introduces "PcdRuntimeServicesSupport" in the
> [PcdsFixedAtBuild] section of "MdePkg.dec". Based on that, I think we
> should add a CONSTRUCTOR function to DxeCapsuleLibNull, as a separate
> patch.
>
> The constructor function should do:
>
> if (((FixedPcdGet16 (PcdRuntimeServicesSupport) &
> EFI_RT_SUPPORTED_UPDATE_CAPSULE) != 0) ||
> ((FixedPcdGet16 (PcdRuntimeServicesSupport) &
> EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES) != 0)) {
> //
> // This library instance is unsuitable for implementing the
> // UpdateCapsule() and SupportCapsuleImage() runtime services.
> //
> return EFI_UNSUPPORTED;
> }
> return EFI_SUCCESS;
>
> Why is this important? Because it will *force* platforms to expose their lack
> of capsule support in the new PCD. Otherwise, the firmware will not boot --
> and that is impossible to miss.
I see your point. The platforms which use null version CapsuleLib should setting the related bit in the new PCD. That's right.
But changing the whole related platforms which use the null version is a challenge. If I missed some, those platforms would not boot because of the patch.
And I think miss this change for DxeCapsuleLibNull wouldn't violate the spec. I'd better to hear more comments about this.
>
>
> (4) The situation is somewhat similar with "PcdCapsuleInRamSupport". If
> "PcdCapsuleInRamSupport" is FALSE, then UpdateCapsule() will always
> return EFI_UNSUPPORTED.
>
> Therefore, the entry point function of CapsuleRuntimeDxe --
> CapsuleServiceInitialize() -- should get the following assertion:
>
> ASSERT (
> PcdGetBool (PcdCapsuleInRamSupport) ||
> ((FixedPcdGet16 (PcdRuntimeServicesSupport) &
> EFI_RT_SUPPORTED_UPDATE_CAPSULE) == 0)
> );
>
>
> (5) For each platform in the edk2 tree that either uses DxeCapsuleLibNull or
> sets "PcdCapsuleInRamSupport" to FALSE, the corresponding bits should be
> cleared in "PcdRuntimeServicesSupport", in the platform DSC files.
>
> This would mean a number of new patches for this series.
(4) and (5) would force the platform to set PcdRuntimeServicesSupport base on PcdCapsuleInRamSupport. That' fine. But I should know the specific platforms that already set "PcdCapsuleInRamSupport". If the PcdCapsuleInRamSupport is only an introduction, that means no platform sets it, no patch is required.
>
>
> (6) With the sanity checks from points (3) and (4) implemented, I agree that
> CapsuleRuntimeDxe is permitted to consume "PcdRuntimeServicesSupport",
> in patch#4, and to introduce new EFI_UNSUPPORTED exit points into
> QueryCapsuleCapabilities() and UpdateCapsule().
>
> However:
>
> (6a) In patch#4, CapsuleRuntimeDxe consumes the new *UEFI variable*, and
> not the new *PCD*. I think that's wrong; or at least sub-optiomal.
>
> Earlier Mike wrote, in
>
> http://mid.mail-
> archive.com/E92EE9817A31E24EB0585FDF735412F5B9D77345@ORSMSX113.a
> mr.corp.intel.com
> https://edk2.groups.io/g/devel/message/43890
>
> that the runtime DXE Core should set the variable, and that individual
> runtime drivers providing some runtime services should consume the *PCD*.
> See the quote below, from Mike:
>
> > I agree that each RT driver that populates the RT Services Table with
> > a RT services can consume the new bitmask PCD and use the PCD to
> > determine if the RT Service should return EFI_UNSUPPORTED after
> > ExitBootServices().
>
> So, CapsuleRuntimeDxe should base those new exit points on the PCD, and
> the GetVariable() call should be removed.
Agree.
>
> (6b) The current bitmask checks in patch #4 are wrong:
>
> > + if (!(mRuntimeServicesSupported |
> EFI_RT_SUPPORTED_UPDATE_CAPSULE)) {
> > + return EFI_UNSUPPORTED;
> > + }
> > +
> >
> > + if (!(mRuntimeServicesSupported |
> EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES)) {
> > + return EFI_UNSUPPORTED;
> > + }
> > +
>
> First, the relevant bits should be extracted with the bitwise AND operator,
> not the bitwise OR operator.
Agree. My mistake.
>
> Second, after the extraction, the edk2 coding style dictates an explicit
> comparison with zero, to my understanding. The logical negation operator is
> only acceptable with BOOLEAN variables, and with such sub-expressions that
> evaluate to FALSE/TRUE.
I would follow that in the next patch.
Before I make next version patch, I want to hear more comments. Expecially for your (3).
Thanks,
Zhichao
>
> Thanks,
> Laszlo
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder
2019-07-22 3:17 ` [edk2-devel] " Gao, Zhichao
@ 2019-07-22 17:28 ` Laszlo Ersek
0 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2019-07-22 17:28 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Kinney, Michael D, Gao, Liming, Wang, Jian J, Wu, Hao A, Ni, Ray,
Zeng, Star, Sean Brogan, Michael Turner, Bret Barkelew
On 07/22/19 05:17, Gao, Zhichao wrote:
> Hi Laszlo,
>
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Friday, July 19, 2019 10:15 PM
>> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
>> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
>> Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
>> <Bret.Barkelew@microsoft.com>
>> Subject: Re: [edk2-devel] [PATCH V2 0/4] Add a pcd
>> PcdBootManagerInBootOrder to control whether BootManager is in
>> BootOrder
>> (3) If a platform links DxeCapsuleLibNull into CapsuleRuntimeDxe, that has
>> the following consequences:
>>
>> - QueryCapsuleCapabilities()
>> [MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c] calls
>> SupportCapsuleImage()
>> [MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.c].
>>
>> The return status is EFI_UNSUPPORTED, consistently.
>>
>> - UpdateCapsule()
>> [MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c] calls
>> both
>> SupportCapsuleImage() -- see above -- and ProcessCapsuleImage()
>> [MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.c].
>>
>> The return status is EFI_UNSUPPORTED, consistently.
>>
>> Meaning that, if a platform uses DxeCapsuleLibNull, it *must* clear the
>> EFI_RT_SUPPORTED_UPDATE_CAPSULE and
>> EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES bits in the
>> "RuntimeServicesSupported" variable.
>>
>> Now, your patch introduces "PcdRuntimeServicesSupport" in the
>> [PcdsFixedAtBuild] section of "MdePkg.dec". Based on that, I think we
>> should add a CONSTRUCTOR function to DxeCapsuleLibNull, as a separate
>> patch.
>>
>> The constructor function should do:
>>
>> if (((FixedPcdGet16 (PcdRuntimeServicesSupport) &
>> EFI_RT_SUPPORTED_UPDATE_CAPSULE) != 0) ||
>> ((FixedPcdGet16 (PcdRuntimeServicesSupport) &
>> EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES) != 0)) {
>> //
>> // This library instance is unsuitable for implementing the
>> // UpdateCapsule() and SupportCapsuleImage() runtime services.
>> //
>> return EFI_UNSUPPORTED;
>> }
>> return EFI_SUCCESS;
>>
>> Why is this important? Because it will *force* platforms to expose their lack
>> of capsule support in the new PCD. Otherwise, the firmware will not boot --
>> and that is impossible to miss.
>
> I see your point. The platforms which use null version CapsuleLib should setting the related bit in the new PCD. That's right.
> But changing the whole related platforms which use the null version is a challenge.
You don't have to change all platforms in existence, in this patch
series, just those that live inside the core edk2 repository.
> If I missed some, those platforms would not boot because of the patch.
Yes, and that's exactly the point.
The above code will cause an assertion failure for such platforms.
People will look at the error message, will locate the relevant source
code, will run "git blame" and "git log" on the source file, and they
will learn about the subject TianoCore BZ, and the new responsibility
for their platform DSC.
Openly forcing downstream platforms to implement a very simple change (a
PCD setting in the platform DSC) is a whole lot better than silently
breaking spec conformance for them.
(Obviously, it would even be better if we could write code that kept
those platforms spec-conformant by default. But that's not possible,
because the change in UEFI-2.8 spells out a new requirement.)
> And I think miss this change for DxeCapsuleLibNull wouldn't violate the spec.
Well, I disagree. :)
> I'd better to hear more comments about this.
Sure, absolutely! Feedback is welcome, like always.
>> (4) The situation is somewhat similar with "PcdCapsuleInRamSupport". If
>> "PcdCapsuleInRamSupport" is FALSE, then UpdateCapsule() will always
>> return EFI_UNSUPPORTED.
>>
>> Therefore, the entry point function of CapsuleRuntimeDxe --
>> CapsuleServiceInitialize() -- should get the following assertion:
>>
>> ASSERT (
>> PcdGetBool (PcdCapsuleInRamSupport) ||
>> ((FixedPcdGet16 (PcdRuntimeServicesSupport) &
>> EFI_RT_SUPPORTED_UPDATE_CAPSULE) == 0)
>> );
>>
>>
>> (5) For each platform in the edk2 tree that either uses DxeCapsuleLibNull or
>> sets "PcdCapsuleInRamSupport" to FALSE, the corresponding bits should be
>> cleared in "PcdRuntimeServicesSupport", in the platform DSC files.
>>
>> This would mean a number of new patches for this series.
>
> (4) and (5) would force the platform to set PcdRuntimeServicesSupport base on PcdCapsuleInRamSupport. That' fine. But I should know the specific platforms that already set "PcdCapsuleInRamSupport". If the PcdCapsuleInRamSupport is only an introduction, that means no platform sets it, no patch is required.
Even if no platform sets PcdCapsuleInRamSupport to FALSE at this time, a
platform can choose to do so later. And, at that later point, any
inconsistency between PcdCapsuleInRamSupport and
PcdRuntimeServicesSupport should be caught, and reported.
Whether you should identify and fix up such individual inconsistencies
in specific platforms, as part of this patch series, is a different
question. For platforms that live inside the edk2 tree, the answer is
"yes". For other platforms, the answer is "no" -- they will have to fix
up the inconsistency for themselves. But, at least, the above ASSERT
will notify them, so they will learn about the new task.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread