[edk2] [PATCH 0/3] ArmVExpressPkg: avoid PSCI reboot/poweroff on 32-bit platforms

Discussion:

Ard Biesheuvel

2015-07-06 18:26:23 UTC

This fixes an issue reported by Ryan Harkin where 32-bit ARM platforms without
PSCI compliant firmware cannot be rebooted or powered off successfully at
boot time.

This is caused by a change which aimed to prevent system register accesses at
runtime. These registers are not virtually remapped, so their availability is
not guaranteed in that case. However, since the reboot/poweroff driver now
uses PSCI unconditionally, these non-PSCI platforms break.

This series addresses this regression by introducing a new system register
access library which switches into a non-functional mode at runtime, and wiring
it up for 32-bit VExpress platforms.

Ard Biesheuvel (3):
ArmPlatformPkg/ArmVExpressPkg: add ArmPlatformSysConfigLib for runtime
ArmVExpressPkg: use ArmVExpressSysConfigRuntimeLib by default
ArmVExpressPkg: use PSCI for system reset only on AARCH64 platforms

ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 3 ++-
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c} | 10 ++++++++++
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} | 12 +++++++-----
3 files changed, 19 insertions(+), 6 deletions(-)
copy ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c} (93%)
copy ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} (65%)

--
1.9.1

Ard Biesheuvel

2015-07-06 18:26:26 UTC

Permalink

The PSCI specification covers both ARM and AARCH64, however, the
ARM Trusted Firmware (ATF) reference implementation is only available
for AARCH64, and PSCI firmware is not widely available for ARM platforms.

So use the EfiResetSystemLib implementation that uses PSCI calls only
on AARCH64, and revert to the Versatile Express-specific system register
interface (which is only available during boot time) on ARM platforms.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 1 +
1 file changed, 1 insertion(+)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 706861d5e2ec..d9e06781d2a5 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -223,6 +223,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
ArmPlatformSysConfigLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf

+[LibraryClasses.AARCH64.DXE_RUNTIME_DRIVER]
#
# PSCI support in EL3 may not be available if we are not running under a PSCI
# compliant secure firmware, but since the default VExpress EfiResetSystemLib

--
1.9.1

Olivier Martin

2015-07-09 17:24:46 UTC

Permalink

Reviewed-By: Olivier Martin <***@arm.com>

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: 06 July 2015 19:26
To: edk2-***@lists.sourceforge.net; ***@linaro.org; Olivier Martin; ***@linaro.org
Cc: Ard Biesheuvel
Subject: [PATCH 3/3] ArmVExpressPkg: use PSCI for system reset only on AARCH64 platforms

The PSCI specification covers both ARM and AARCH64, however, the ARM Trusted Firmware (ATF) reference implementation is only available for AARCH64, and PSCI firmware is not widely available for ARM platforms.

So use the EfiResetSystemLib implementation that uses PSCI calls only on AARCH64, and revert to the Versatile Express-specific system register interface (which is only available during boot time) on ARM platforms.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 1 +
1 file changed, 1 insertion(+)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 706861d5e2ec..d9e06781d2a5 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -223,6 +223,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
ArmPlatformSysConfigLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf

+[LibraryClasses.AARCH64.DXE_RUNTIME_DRIVER]
#
# PSCI support in EL3 may not be available if we are not running under a PSCI
# compliant secure firmware, but since the default VExpress EfiResetSystemLib
--
1.9.1

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Ard Biesheuvel

2015-07-06 18:26:25 UTC

Permalink

Instead of using a NULL implementation of ArmPlatformSysConfigLib for
DXE_RUNTIME_DRIVER modules, which prevents them from accessing system
control registers even at boot time, use the new implementation that
only switches into a non-functional mode at runtime, and operates as
before otherwise.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 1ca9b2de6550..706861d5e2ec 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -221,7 +221,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
- ArmPlatformSysConfigLib|ArmPlatformPkg/Library/ArmPlatformSysConfigLibNull/ArmPlatformSysConfigLibNull.inf
+ ArmPlatformSysConfigLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf

#
# PSCI support in EL3 may not be available if we are not running under a PSCI

--
1.9.1

Olivier Martin

2015-07-09 17:23:59 UTC

Permalink

Reviewed-By: Olivier Martin <***@arm.com>

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: 06 July 2015 19:26
To: edk2-***@lists.sourceforge.net; ***@linaro.org; Olivier Martin; ***@linaro.org
Cc: Ard Biesheuvel
Subject: [PATCH 2/3] ArmVExpressPkg: use ArmVExpressSysConfigRuntimeLib by default

Instead of using a NULL implementation of ArmPlatformSysConfigLib for DXE_RUNTIME_DRIVER modules, which prevents them from accessing system control registers even at boot time, use the new implementation that only switches into a non-functional mode at runtime, and operates as before otherwise.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 1ca9b2de6550..706861d5e2ec 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -221,7 +221,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
ReportStatusCodeLib|IntelFrameworkModulePkg/Library/DxeReportStatusCodeLibFramework/DxeReportStatusCodeLib.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
- ArmPlatformSysConfigLib|ArmPlatformPkg/Library/ArmPlatformSysConfigLibNull/ArmPlatformSysConfigLibNull.inf
+
+ ArmPlatformSysConfigLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpr
+ essSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf

#
# PSCI support in EL3 may not be available if we are not running under a PSCI
--
1.9.1

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Ard Biesheuvel

2015-07-06 18:26:24 UTC

Permalink

This adds a ArmPlatformSysConfigLib implementation that is usable by
DXE_RUNTIME_DRIVER modules. Since the system registers that this library
encapsulates are not usable at runtime, this driver allows access to
those registers only at boot time.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c} | 10 ++++++++++
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} | 12 +++++++-----
2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfig.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c
similarity index 93%
copy from ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfig.c
copy to ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c
index 6dfbacd11762..1f915e3b0225 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfig.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c
@@ -19,6 +19,9 @@
#include <Library/ArmPlatformSysConfigLib.h>
#include <ArmPlatform.h>

+#include <Uefi.h>
+#include <Library/UefiRuntimeLib.h>
+
//
// SYS_CFGCTRL Bits
//
@@ -72,6 +75,10 @@ AccessSysCfgRegister (
{
UINT32 SysCfgCtrl;

+ if (EfiAtRuntime ()) {
+ return RETURN_UNSUPPORTED;
+ }
+
// Clear the COMPLETE bit
MmioAnd32(ARM_VE_SYS_CFGSTAT_REG, ~SYS_CFGSTAT_COMPLETE);

@@ -229,6 +236,9 @@ ArmPlatformSysConfigSetDevice (
switch(Function) {
case SYS_CFG_SCC:
#ifdef ARM_VE_SCC_BASE
+ if (EfiAtRuntime ()) {
+ return RETURN_UNSUPPORTED;
+ }
MmioWrite32 ((ARM_VE_SCC_BASE + (Device * 4)),Value);
return RETURN_SUCCESS;
#else
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf
similarity index 65%
copy from ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
copy to ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf
index b89455a421c3..988250d930cb 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf
@@ -1,8 +1,9 @@
#/** @file
#
-# Component description file for ArmVExpressSysConfigLib module
+# Component description file for ArmVExpressSysConfigRuntimeLib module
#
# Copyright (c) 2011-2012, ARM Ltd. All rights reserved. 
+# Copyright (c) 2015, Linaro Ltd. All rights reserved. 
#
# This program and the accompanying materials
# are licensed and made available under the terms and conditions of the BSD License
@@ -16,14 +17,14 @@

[Defines]
INF_VERSION = 0x00010005
- BASE_NAME = ArmVExpressSysConfigLib
- FILE_GUID = a05b5cc0-82d2-11e0-82cb-0002a5d5c51b
+ BASE_NAME = ArmVExpressSysConfigRuntimeLib
+ FILE_GUID = 6275b819-615c-4a36-814a-c1f330b4e5d9
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmPlatformSysConfigLib|SEC DXE_DRIVER
+ LIBRARY_CLASS = ArmPlatformSysConfigLib|DXE_RUNTIME_DRIVER

[Sources.common]
- ArmVExpressSysConfig.c
+ ArmVExpressSysConfigRuntimeLib.c

[Packages]
MdePkg/MdePkg.dec
@@ -33,3 +34,4 @@ [Packages]
[LibraryClasses]
BaseLib
IoLib
+ UefiRuntimeLib

--
1.9.1

Olivier Martin

2015-07-09 17:21:01 UTC

Permalink

I can see few coding style issue in this patch - mainly missing space between function name and '('.

Out of curiosity why you do not convert the base address of the System Register when we switch to Runtime - like in ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c?
Doing that we could reset the board from Linux using UEFI Runtime Services.
Are you worry by a race condition when the System Registers are accessed by Linux and UEFI firmware in the same time?

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: 06 July 2015 19:26
To: edk2-***@lists.sourceforge.net; ***@linaro.org; Olivier Martin; ***@linaro.org
Cc: Ard Biesheuvel
Subject: [PATCH 1/3] ArmPlatformPkg/ArmVExpressPkg: add ArmPlatformSysConfigLib for runtime

This adds a ArmPlatformSysConfigLib implementation that is usable by DXE_RUNTIME_DRIVER modules. Since the system registers that this library encapsulates are not usable at runtime, this driver allows access to those registers only at boot time.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c} | 10 ++++++++++
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} | 12 +++++++-----
2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfig.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c
similarity index 93%
copy from ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfig.c
copy to ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c
index 6dfbacd11762..1f915e3b0225 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfig.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeL
+++ ib/ArmVExpressSysConfigRuntimeLib.c
@@ -19,6 +19,9 @@
#include <Library/ArmPlatformSysConfigLib.h>
#include <ArmPlatform.h>

+#include <Uefi.h>
+#include <Library/UefiRuntimeLib.h>
+
//
// SYS_CFGCTRL Bits
//
@@ -72,6 +75,10 @@ AccessSysCfgRegister ( {
UINT32 SysCfgCtrl;

+ if (EfiAtRuntime ()) {
+ return RETURN_UNSUPPORTED;
+ }
+
// Clear the COMPLETE bit
MmioAnd32(ARM_VE_SYS_CFGSTAT_REG, ~SYS_CFGSTAT_COMPLETE);

@@ -229,6 +236,9 @@ ArmPlatformSysConfigSetDevice (
switch(Function) {
case SYS_CFG_SCC:
#ifdef ARM_VE_SCC_BASE
+ if (EfiAtRuntime ()) {
+ return RETURN_UNSUPPORTED;
+ }
MmioWrite32 ((ARM_VE_SCC_BASE + (Device * 4)),Value);
return RETURN_SUCCESS;
#else
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf
similarity index 65%
copy from ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
copy to ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf
index b89455a421c3..988250d930cb 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeL
+++ ib/ArmVExpressSysConfigRuntimeLib.inf
@@ -1,8 +1,9 @@
#/** @file
#
-# Component description file for ArmVExpressSysConfigLib module
+# Component description file for ArmVExpressSysConfigRuntimeLib module
#
# Copyright (c) 2011-2012, ARM Ltd. All rights reserved. 
+# Copyright (c) 2015, Linaro Ltd. All rights reserved. 
#
# This program and the accompanying materials # are licensed and made available under the terms and conditions of the BSD License @@ -16,14 +17,14 @@

[Defines]
INF_VERSION = 0x00010005
- BASE_NAME = ArmVExpressSysConfigLib
- FILE_GUID = a05b5cc0-82d2-11e0-82cb-0002a5d5c51b
+ BASE_NAME = ArmVExpressSysConfigRuntimeLib
+ FILE_GUID = 6275b819-615c-4a36-814a-c1f330b4e5d9
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmPlatformSysConfigLib|SEC DXE_DRIVER
+ LIBRARY_CLASS = ArmPlatformSysConfigLib|DXE_RUNTIME_DRIVER

[Sources.common]
- ArmVExpressSysConfig.c
+ ArmVExpressSysConfigRuntimeLib.c

[Packages]
MdePkg/MdePkg.dec
@@ -33,3 +34,4 @@ [Packages]
[LibraryClasses]
BaseLib
IoLib
+ UefiRuntimeLib
--
1.9.1

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Ard Biesheuvel

2015-07-09 17:29:37 UTC

Permalink

Post by Olivier Martin
I can see few coding style issue in this patch - mainly missing space between function name and '('.
Out of curiosity why you do not convert the base address of the System Register when we switch to Runtime - like in ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c?
Doing that we could reset the board from Linux using UEFI Runtime Services.
Are you worry by a race condition when the System Registers are accessed by Linux and UEFI firmware in the same time?

Indeed. The system register block is a multi-function device that is
described by a single node in the device tree, and Linux binds a
single driver to it. Ideally, runtime MMIO regions should be disjoint
from MMIO owned by the kernel, since there is no coordination possible
at all between the firmware and the OS..

Perhaps in this particular case, we could get away with it, but I
think it is a pattern that we want to discourage, so it does not
belong in a reference implementation.
--
Ard.

Post by Olivier Martin
-----Original Message-----
Sent: 06 July 2015 19:26
Cc: Ard Biesheuvel
Subject: [PATCH 1/3] ArmPlatformPkg/ArmVExpressPkg: add ArmPlatformSysConfigLib for runtime
This adds a ArmPlatformSysConfigLib implementation that is usable by DXE_RUNTIME_DRIVER modules. Since the system registers that this library encapsulates are not usable at runtime, this driver allows access to those registers only at boot time.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c} | 10 ++++++++++
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} | 12 +++++++-----
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfig.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c
similarity index 93%
copy from ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfig.c
copy to ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c
index 6dfbacd11762..1f915e3b0225 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfig.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeL
+++ ib/ArmVExpressSysConfigRuntimeLib.c
@@ -19,6 +19,9 @@
#include <Library/ArmPlatformSysConfigLib.h>
#include <ArmPlatform.h>
+#include <Uefi.h>
+#include <Library/UefiRuntimeLib.h>
+
//
// SYS_CFGCTRL Bits
//
@@ -72,6 +75,10 @@ AccessSysCfgRegister ( {
UINT32 SysCfgCtrl;
+ if (EfiAtRuntime ()) {
+ return RETURN_UNSUPPORTED;
+ }
+
// Clear the COMPLETE bit
MmioAnd32(ARM_VE_SYS_CFGSTAT_REG, ~SYS_CFGSTAT_COMPLETE);
@@ -229,6 +236,9 @@ ArmPlatformSysConfigSetDevice (
switch(Function) {
#ifdef ARM_VE_SCC_BASE
+ if (EfiAtRuntime ()) {
+ return RETURN_UNSUPPORTED;
+ }
MmioWrite32 ((ARM_VE_SCC_BASE + (Device * 4)),Value);
return RETURN_SUCCESS;
#else
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf
similarity index 65%
copy from ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
copy to ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf
index b89455a421c3..988250d930cb 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigRuntimeL
+++ ib/ArmVExpressSysConfigRuntimeLib.inf
@@ -1,8 +1,9 @@
#
-# Component description file for ArmVExpressSysConfigLib module
+# Component description file for ArmVExpressSysConfigRuntimeLib module
#
# Copyright (c) 2011-2012, ARM Ltd. All rights reserved. 
+# Copyright (c) 2015, Linaro Ltd. All rights reserved. 
#
[Defines]
INF_VERSION = 0x00010005
- BASE_NAME = ArmVExpressSysConfigLib
- FILE_GUID = a05b5cc0-82d2-11e0-82cb-0002a5d5c51b
+ BASE_NAME = ArmVExpressSysConfigRuntimeLib
+ FILE_GUID = 6275b819-615c-4a36-814a-c1f330b4e5d9
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = ArmPlatformSysConfigLib|SEC DXE_DRIVER
+ LIBRARY_CLASS = ArmPlatformSysConfigLib|DXE_RUNTIME_DRIVER
[Sources.common]
- ArmVExpressSysConfig.c
+ ArmVExpressSysConfigRuntimeLib.c
[Packages]
MdePkg/MdePkg.dec
@@ -33,3 +34,4 @@ [Packages]
[LibraryClasses]
BaseLib
IoLib
+ UefiRuntimeLib
--
1.9.1
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Olivier Martin

2015-07-10 15:13:36 UTC

Permalink

Thanks for confirming the explanation.

I was initially thinking to ask you for a new patchset to fix the coding style. But the original version (ie: ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib) has the same coding style issue.

Reviewed-By: Olivier Martin <***@arm.com>

I pushed the patchset in SVN rev17925-17927. Thanks for the contribution :-)

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: 09 July 2015 18:30
To: Olivier Martin
Cc: edk2-***@lists.sourceforge.net; ***@linaro.org; ***@linaro.org
Subject: Re: [PATCH 1/3] ArmPlatformPkg/ArmVExpressPkg: add ArmPlatformSysConfigLib for runtime

Indeed. The system register block is a multi-function device that is described by a single node in the device tree, and Linux binds a single driver to it. Ideally, runtime MMIO regions should be disjoint from MMIO owned by the kernel, since there is no coordination possible at all between the firmware and the OS..

Perhaps in this particular case, we could get away with it, but I think it is a pattern that we want to discourage, so it does not belong in a reference implementation.

--
Ard.

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Ard Biesheuvel

2015-07-10 15:17:32 UTC

Permalink

Post by Olivier Martin
Thanks for confirming the explanation.
I was initially thinking to ask you for a new patchset to fix the coding style. But the original version (ie: ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib) has the same coding style issue.

Yes. I wonder who wrote it :-)

Post by Olivier Martin
I pushed the patchset in SVN rev17925-17927. Thanks for the contribution :-)

Thanks,
Ard.

Post by Olivier Martin
-----Original Message-----
Sent: 09 July 2015 18:30
To: Olivier Martin
Subject: Re: [PATCH 1/3] ArmPlatformPkg/ArmVExpressPkg: add ArmPlatformSysConfigLib for runtime

Indeed. The system register block is a multi-function device that is described by a single node in the device tree, and Linux binds a single driver to it. Ideally, runtime MMIO regions should be disjoint from MMIO owned by the kernel, since there is no coordination possible at all between the firmware and the OS..
Perhaps in this particular case, we could get away with it, but I think it is a pattern that we want to discourage, so it does not belong in a reference implementation.
--
Ard.

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Ard Biesheuvel

2015-07-07 12:22:48 UTC

Permalink

Post by Ard Biesheuvel
This fixes an issue reported by Ryan Harkin where 32-bit ARM platforms without
PSCI compliant firmware cannot be rebooted or powered off successfully at
boot time.
This is caused by a change which aimed to prevent system register accesses at
runtime. These registers are not virtually remapped, so their availability is
not guaranteed in that case. However, since the reboot/poweroff driver now
uses PSCI unconditionally, these non-PSCI platforms break.
This series addresses this regression by introducing a new system register
access library which switches into a non-functional mode at runtime, and wiring
it up for 32-bit VExpress platforms.

OK, I have tested these changes myself using the RTSM-A15 build under QEMU.

@Ryan: it would be helpful if you could verify that it fixes the
original problem as well.

Thanks,
Ard.

Post by Ard Biesheuvel
ArmPlatformPkg/ArmVExpressPkg: add ArmPlatformSysConfigLib for runtime
ArmVExpressPkg: use ArmVExpressSysConfigRuntimeLib by default
ArmVExpressPkg: use PSCI for system reset only on AARCH64 platforms
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc | 3 ++-
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c} | 10 ++++++++++
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} | 12 +++++++-----
3 files changed, 19 insertions(+), 6 deletions(-)
copy ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c} (93%)
copy ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf => ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} (65%)
--
1.9.1

Ryan Harkin

2015-07-07 17:37:16 UTC

Permalink

Hi Ard,

Post by Ard Biesheuvel

Post by Ard Biesheuvel
This fixes an issue reported by Ryan Harkin where 32-bit ARM platforms

without

Post by Ard Biesheuvel
PSCI compliant firmware cannot be rebooted or powered off successfully at
boot time.
This is caused by a change which aimed to prevent system register

accesses at

Post by Ard Biesheuvel
runtime. These registers are not virtually remapped, so their

availability is

Post by Ard Biesheuvel
not guaranteed in that case. However, since the reboot/poweroff driver

now

Post by Ard Biesheuvel
uses PSCI unconditionally, these non-PSCI platforms break.
This series addresses this regression by introducing a new system

Post by Ard Biesheuvel
access library which switches into a non-functional mode at runtime, and

wiring

Post by Ard Biesheuvel
it up for 32-bit VExpress platforms.

OK, I have tested these changes myself using the RTSM-A15 build under QEMU.
@Ryan: it would be helpful if you could verify that it fixes the
original problem as well.

Thanks, yes, this fixes the problem I found with TC2. I also tested it on
Juno and FVP AEMv8 and Foundation to make sure it all still worked - and it
does.

Well, the FVP AEMv8 model reboots instead of shutting down, but that isn't
your fault ;-)

Thanks for sorting this out so quickly.

Post by Ard Biesheuvel
Thanks,
Ard.

| 3 ++-
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c
=> ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c}
| 10 ++++++++++
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
=> ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} | 12
+++++++-----

Post by Ard Biesheuvel
3 files changed, 19 insertions(+), 6 deletions(-)
copy

ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c
=> ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c} (93%)

Post by Ard Biesheuvel
copy

ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
=> ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} (65%)

Post by Ard Biesheuvel
--
1.9.1

Ard Biesheuvel

2015-07-09 08:35:47 UTC

Permalink

Post by Ryan Harkin
Hi Ard,

Post by Ard Biesheuvel

OK, I have tested these changes myself using the RTSM-A15 build under QEMU.
@Ryan: it would be helpful if you could verify that it fixes the
original problem as well.

Thanks, yes, this fixes the problem I found with TC2. I also tested it on
Juno and FVP AEMv8 and Foundation to make sure it all still worked - and it
does.
Well, the FVP AEMv8 model reboots instead of shutting down, but that isn't
your fault ;-)
Thanks for sorting this out so quickly.

Hello Olivier,

Are you picking this up? Any changes needed?

Thanks,
Ard.

Post by Ryan Harkin

Post by Ard Biesheuvel

Post by Ard Biesheuvel
ArmPlatformPkg/ArmVExpressPkg: add ArmPlatformSysConfigLib for runtime
ArmVExpressPkg: use ArmVExpressSysConfigRuntimeLib by default
ArmVExpressPkg: use PSCI for system reset only on AARCH64 platforms
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
| 3 ++-
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c
=> ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c} |
10 ++++++++++
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
=> ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} | 12
+++++++-----
3 files changed, 19 insertions(+), 6 deletions(-)
copy
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfig.c
=> ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.c} (93%)
copy
ArmPlatformPkg/ArmVExpressPkg/Library/{ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
=> ArmVExpressSysConfigRuntimeLib/ArmVExpressSysConfigRuntimeLib.inf} (65%)
--
1.9.1