| Message ID | 1436207186-15424-2-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 9 July 2015 at 19:21, Olivier Martin <Olivier.Martin@arm.com> wrote: > 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.
On 10 July 2015 at 17:13, Olivier Martin <Olivier.Martin@arm.com> wrote: > 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 :-) > Reviewed-By: Olivier Martin <Olivier.martin@arm.com> > > I pushed the patchset in SVN rev17925-17927. Thanks for the contribution :-) > Thanks, Ard. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: 09 July 2015 18:30 > To: Olivier Martin > Cc: edk2-devel@lists.sourceforge.net; ryan.harkin@linaro.org; leif.lindholm@linaro.org > Subject: Re: [PATCH 1/3] ArmPlatformPkg/ArmVExpressPkg: add ArmPlatformSysConfigLib for runtime > > On 9 July 2015 at 19:21, Olivier Martin <Olivier.Martin@arm.com> wrote: >> 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. > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: 06 July 2015 19:26 >> To: edk2-devel@lists.sourceforge.net; ryan.harkin@linaro.org; Olivier Martin; leif.lindholm@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 <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.<BR> >> +# Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR> >> # >> # 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 >> > > > -- 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 ------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/
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.<BR> +# Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR> # # 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
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 <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(-)