On 03/02/15 18:13, Ard Biesheuvel wrote:
> This refactors the XenHypercallLib implementation for ARM and x86,
> and their callers, so that the init code for the XenHypercallLib
> is only called in places where we have already established that we
> are running as a Xen guest,
Hm, I think I disagree with this.
In particular for XenBusDxe, which is a UEFI driver model compliant
driver, we can talk about two points of "activation" -- (a) the driver
entry point (which always runs, and registers a driver binding protocol
instance), and (b) the Start() method, which can bind individual device
handles.
When XenHypercallLib cannot be initialized, we should not get to point
(b). We should fail the entry point (a) at once, which causes the DXE
core to unload the driver completely.
> or (in the XenConsoleSerialPortLib case)
> where ARM is the only user of the library, and its init code is a nop.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
> OvmfPkg/Include/Library/XenHypercallLib.h | 12 +++++++++
> .../XenConsoleSerialPortLib.c | 7 +++++
> OvmfPkg/Library/XenHypercallLib/XenHypercallArm.c | 30
> ++++++++++++++++++++++
> .../Library/XenHypercallLib/XenHypercallIntel.c | 8 +++---
> .../Library/XenHypercallLib/XenHypercallLibArm.inf | 1 +
> .../XenHypercallLib/XenHypercallLibIntel.inf | 1 -
> OvmfPkg/XenBusDxe/XenBusDxe.c | 7 +++++
> 7 files changed, 61 insertions(+), 5 deletions(-)
> create mode 100644 OvmfPkg/Library/XenHypercallLib/XenHypercallArm.c
>
> diff --git a/OvmfPkg/Include/Library/XenHypercallLib.h
> b/OvmfPkg/Include/Library/XenHypercallLib.h
> index 1a468ea7dcc5..0e4b13d5397a 100644
> --- a/OvmfPkg/Include/Library/XenHypercallLib.h
> +++ b/OvmfPkg/Include/Library/XenHypercallLib.h
> @@ -17,6 +17,18 @@
> #define __XEN_HYPERCALL_LIB_H__
>
> /**
> + Initialize the Xen hypercall library
> +
> + @return EFI_SUCCESS on success, or an EFI_xxx return code appropriate
> + for the failure mode on failure.
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenHypercallLibInit (
> + VOID
> + );
We should not use EFI_STATUS here -- please use RETURN_STATUS. The
reason is that we mean this library class to have BASE instances. For
that, the parameter list is good, but the return type should be
RETURN_STATUS. (It's syntactic sugar only, but still.) Also, the comment
should say RETURN_xxx.
> +
> +/**
> This function will put the two arguments in the right place (registers) and
> invoke the hypercall identified by HypercallID.
>
> diff --git
> a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
> b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
> index 98022354cf31..5bf588f75246 100644
> --- a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
> +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
> @@ -40,6 +40,13 @@ SerialPortInitialize (
> VOID
> )
> {
> + EFI_STATUS Status;
> +
> + Status = XenHypercallLibInit ();
> + if (EFI_ERROR (Status)) {
> + return RETURN_DEVICE_ERROR;
> + }
> +
Please replace EFI_STATUS with RETURN_STATUS, and EFI_ERROR () with
RETURN_ERROR ().
> if (!mXenConsoleInterface) {
> mXenConsoleEventChain.port = (UINT32)XenHypercallHvmGetParam
> (HVM_PARAM_CONSOLE_EVTCHN);
> mXenConsoleInterface = (struct xencons_interface *)(UINTN)
> diff --git a/OvmfPkg/Library/XenHypercallLib/XenHypercallArm.c
> b/OvmfPkg/Library/XenHypercallLib/XenHypercallArm.c
> new file mode 100644
Since you are creating a new file in this patch, please push it to a
public branch. Although edk2 uses CRLF line endings, "git am --keep-cr"
still mostly works, *except* when you have diff headers that create or
delete files. The /dev/null<CR> in those causes git to die.
> index 000000000000..d2a91b7ed6c7
> --- /dev/null
> +++ b/OvmfPkg/Library/XenHypercallLib/XenHypercallArm.c
> @@ -0,0 +1,30 @@
> +/** @file
> + Xen Hypercall Library implementation for ARM architecture
> +
> +Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
Please add a newline here.
> +This program and the accompanying materials are licensed and made available
> under
> +the terms and conditions of the BSD License that accompanies this
> distribution.
> +The full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php.
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
Please indent the copyright notice and the license block with "Xen
Hypercall Library...".
> +
> +#include <Uefi/UefiBaseType.h>
> +#include <Library/XenHypercallLib.h>
> +
> +/**
> + Library constructor: retrieves the Hyperpage address
> + from the gEfiXenInfoGuid HOB
> +**/
As you said on IRC, this should state: "Library initializer: NOP".
> +
> +EFI_STATUS
> +EFIAPI
> +XenHypercallLibInit (
> + VOID
> + )
> +{
> + return EFI_SUCCESS;
> +}
RETURN_xxx
> diff --git a/OvmfPkg/Library/XenHypercallLib/XenHypercallIntel.c
> b/OvmfPkg/Library/XenHypercallLib/XenHypercallIntel.c
> index fc52823f239a..a4ec4924bd7e 100644
> --- a/OvmfPkg/Library/XenHypercallLib/XenHypercallIntel.c
> +++ b/OvmfPkg/Library/XenHypercallLib/XenHypercallIntel.c
> @@ -35,9 +35,9 @@ __XenHypercall2 (
> from the gEfiXenInfoGuid HOB
> **/
>
> -RETURN_STATUS
> +EFI_STATUS
No; the other way around. :)
> EFIAPI
> -XenHypercallLibIntelInit (
> +XenHypercallLibInit (
> VOID
> )
> {
> @@ -46,11 +46,11 @@ XenHypercallLibIntelInit (
>
> GuidHob = GetFirstGuidHob (&gEfiXenInfoGuid);
> if (GuidHob == NULL) {
> - return RETURN_NOT_FOUND;
> + return EFI_NOT_FOUND;
> }
> XenInfo = (EFI_XEN_INFO *) GET_GUID_HOB_DATA (GuidHob);
> HyperPage = XenInfo->HyperPages;
> - return RETURN_SUCCESS;
> + return EFI_SUCCESS;
> }
Ditto.
>
> /**
> diff --git a/OvmfPkg/Library/XenHypercallLib/XenHypercallLibArm.inf
> b/OvmfPkg/Library/XenHypercallLib/XenHypercallLibArm.inf
> index 9cbbeb5d8789..e1e784a83d74 100644
> --- a/OvmfPkg/Library/XenHypercallLib/XenHypercallLibArm.inf
> +++ b/OvmfPkg/Library/XenHypercallLib/XenHypercallLibArm.inf
> @@ -34,6 +34,7 @@
>
> [Sources]
> XenHypercall.c
> + XenHypercallArm.c
>
> [Packages]
> MdePkg/MdePkg.dec
> diff --git a/OvmfPkg/Library/XenHypercallLib/XenHypercallLibIntel.inf
> b/OvmfPkg/Library/XenHypercallLib/XenHypercallLibIntel.inf
> index 2afd608f4a05..0afccac0994e 100644
> --- a/OvmfPkg/Library/XenHypercallLib/XenHypercallLibIntel.inf
> +++ b/OvmfPkg/Library/XenHypercallLib/XenHypercallLibIntel.inf
> @@ -19,7 +19,6 @@
> MODULE_TYPE = BASE
> VERSION_STRING = 1.0
> LIBRARY_CLASS = XenHypercallLib|DXE_DRIVER UEFI_DRIVER
> - CONSTRUCTOR = XenHypercallLibIntelInit
>
> #
> # The following information is for reference only and not required by the
> build tools.
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
> index 2c4a08673ce6..fd18a1707c2c 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.c
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
> @@ -361,6 +361,13 @@ XenBusDxeDriverBindingStart (
> mMyDevice = Dev;
> EfiReleaseLock (&mMyDeviceLock);
>
> + Status = XenHypercallLibInit ();
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_ERROR, "XenBus: XenHypercallLibInit () failed.\n"));
> + Status = EFI_UNSUPPORTED;
> + goto ErrorAllocated;
> + }
> +
As I wrote at the top, this check should not happen here; it's too late.
As I wrote earlier, please modify the XenBusDxeDriverEntryPoint()
function, in file "OvmfPkg/XenBusDxe/XenBusDxe.c". Call and check
XenHypercallLibInit() before calling
EfiLibInstallDriverBindingComponentName2().
I'd also downgrade the error message from EFI_D_ERROR to EFI_D_INFO,
since it is valid to run the code on a non-Xen platform.
Finally, using EFI_STATUS and EFI_ERROR () will be fine in
XenBusDxeDriverEntryPoint(). RETURN_xxx can be "converted" to EFI_xxx
(in practice it's just a plain assignment); it's the reverse direction
that's not allowed, in theory anyway.
Thanks!
Laszlo
> Status = XenGetSharedInfoPage (Dev);
> if (EFI_ERROR (Status)) {
> DEBUG ((EFI_D_ERROR, "XenBus: Unable to get the shared info page.\n"));
>
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
