From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2C0D7201B0430 for ; Wed, 20 Feb 2019 01:21:18 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id v72so13915780itc.0 for ; Wed, 20 Feb 2019 01:21:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=FMRF71QCmtixN2rfmGTfEUjXzgu0LP0G6qIPzY8m5c8=; b=GEz2+nQr+eIO0yKDYFgFqcQRDAq/Yi/YN9aZqLVfOVePbMjRNh/MvDIEUum9NW6POk bu0P8Aocc79JYsHKWCX21wvl3fGv3DMZDxt5mXiWC/dtZE1tviXuNHG34hSf65MnS7w8 aX0mLCaQ6mghnqD3PvoIY+5fpvctQeILilFS389JUlXbuOtBfHhQYWEnjwN5ypLsvPQc wD3RxdQWf8trOuJQs6/CjKiNCZ2ZJvQMQDfYQPrn/2oJgYbxaG6kFnOBpd5TmI7hb+dQ QdlzfGYRy86Yc9myGqw0tcim6Zs7kBp81/EOf5wRflvJo18LezMh7RyBzdqfb8TsdUAo tSjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=FMRF71QCmtixN2rfmGTfEUjXzgu0LP0G6qIPzY8m5c8=; b=ROcwMbh/SxuHS+fwpu13QNhVJlOf3RpeZDHp6QFD36OW1Tmny5NLbX6SOZB3MlZek/ +rI+aIiSWs/0UE1EfGJvwYcHSK4tXjgKTFZGi0HLNgR3u881K0EY69ZrUOWk/02TZqoh OdPh9R9gKWbI6zAr5ldJgSK4jXoFYWC8Vs9hnXPrbvDMB74T27DBz7hCYU2es7wOFz3Z hPDk5Qw780Ele/5i1eGZL05HWKUM05hC35SHsrXkTZtJf63bpzSSCx5DlfWXMOKms4Oq +w0JbquC0tVcVXgBCX5/9G9+1wmY/fa/Vo3nER9DYypzAbujtT9aitzIYBym9ybuagaN S8HQ== X-Gm-Message-State: AHQUAuaDa3rO6Pn3ULCsEPtNo3WgwbTN07lQIbvldJCJ+tXiOUbtmsc8 dcWc/nS8TIKCYQWzOl7QIbBuN6El17OTxS2R37QqYQ== X-Google-Smtp-Source: AHgI3IaKhnDC99/YQhyWmP/GpxrWa7PKHMFv2k+yY6E/OHKPFdZ0wDzJm0pUqzAHTx3R20KTbdFJ2rOmd/a/F1+Wkds= X-Received: by 2002:a24:45dd:: with SMTP id c90mr5167563itd.71.1550654477242; Wed, 20 Feb 2019 01:21:17 -0800 (PST) MIME-Version: 1.0 References: <20190220081644.8238-1-lersek@redhat.com> <20190220081644.8238-3-lersek@redhat.com> In-Reply-To: <20190220081644.8238-3-lersek@redhat.com> From: Ard Biesheuvel Date: Wed, 20 Feb 2019 10:21:06 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Anthony Perard , Jordan Justen , Julien Grall , Ray Ni Subject: Re: [PATCH v2 2/5] OvmfPkg: add library to track boot option loading/starting on the console X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Feb 2019 09:21:18 -0000 Content-Type: text/plain; charset="UTF-8" On Wed, 20 Feb 2019 at 09:16, Laszlo Ersek wrote: > > Introduce the Platform Boot Manager Print Status Code Library (for short, > PlatformBmPrintScLib) class for catching and printing the LoadImage() / > StartImage() preparations, and return statuses, that are reported by > UefiBootManagerLib. > > In the primary library instance, catch only such status codes that > UefiBootManagerLib reports from the same module that contains > PlatformBmPrintScLib. The intent is to establish a reporting-printing > channel within BdsDxe, between UefiBootManagerLib and > PlatformBmPrintScLib. Ignore status codes originating elsewhence, e.g. > from UiApp's copy of UefiBootManagerLib. > > Cc: Anthony Perard > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Julien Grall > Cc: Ray Ni > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1515418 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > > Notes: > v2: > > - Split the status code handling to a separate library, so that it's > easy to reuse in ArmVirtPkg. > > - Rework the logic based on > and > , and follow Ray's > advice in > : > > - The boot option details are fetched via BootCurrent. > > - For reporting LoadImage() and StartImage() preparations, replace the > originally proposed PcdDebugCodeOsLoaderDetail status code with the > existent (edk2-specific) PcdProgressCodeOsLoaderLoad and > PcdProgressCodeOsLoaderStart status codes. > > - For reporting LoadImage() and StartImage() return values, replace > the originally proposed PcdDebugCodeOsLoaderDetail status code with > the standard EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR and > EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED status codes. > > - For all four kinds of reports, replace the originally proposed "OS > Loader Detail" structure (and GUID) with the recently standardized > EFI_RETURN_STATUS_EXTENDED_DATA structure. > > OvmfPkg/OvmfPkg.dec | 5 + > OvmfPkg/OvmfPkgIa32.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/Include/Library/PlatformBmPrintScLib.h | 41 +++ > OvmfPkg/Library/PlatformBmPrintScLib/PlatformBmPrintScLib.inf | 66 +++++ > OvmfPkg/Library/PlatformBmPrintScLib/StatusCodeHandler.c | 310 ++++++++++++++++++++ > 7 files changed, 425 insertions(+) > ... > diff --git a/OvmfPkg/Library/PlatformBmPrintScLib/StatusCodeHandler.c b/OvmfPkg/Library/PlatformBmPrintScLib/StatusCodeHandler.c > new file mode 100644 > index 000000000000..9806d3c7411e > --- /dev/null > +++ b/OvmfPkg/Library/PlatformBmPrintScLib/StatusCodeHandler.c > @@ -0,0 +1,310 @@ > +/** @file > + Register a status code handler for printing the Boot Manager's LoadImage() > + and StartImage() preparations, and return codes, to the UEFI console. > + > + This feature enables users that are not accustomed to analyzing the firmware > + log to glean some information about UEFI boot option processing (loading and > + starting). > + > + This library instance filters out (ignores) status codes that are not > + reported by the containing firmware module. The intent is to link this > + library instance into BdsDxe via PlatformBootManagerLib (which BdsDxe depends > + upon), then catch only those status codes that BdsDxe reports (which happens > + via UefiBootManagerLib). Status codes reported by other modules (such as > + UiApp), via UefiBootManagerLib or otherwise, are meant to be ignored. > + > + Copyright (C) 2019, Red Hat, Inc. > + > + This program and the accompanying materials are licensed and made available > + under the terms and conditions of the BSD License which 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. > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include > +#include > + > +#include > + > + > +// > +// Convenience variables for the status codes that are relevant for LoadImage() > +// and StartImage() preparations and return codes. > +// > +STATIC EFI_STATUS_CODE_VALUE mLoadPrep; > +STATIC EFI_STATUS_CODE_VALUE mLoadFail; > +STATIC EFI_STATUS_CODE_VALUE mStartPrep; > +STATIC EFI_STATUS_CODE_VALUE mStartFail; > + > + > +/** > + Handle status codes reported through ReportStatusCodeLib / > + EFI_STATUS_CODE_PROTOCOL.ReportStatusCode(). Format matching status codes to > + the system console. > + > + The highest TPL at which this handler can be registered with > + EFI_RSC_HANDLER_PROTOCOL.Register() is TPL_NOTIFY. That's because Print() > + uses EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL internally. > + > + The parameter list of this function precisely matches that of > + EFI_STATUS_CODE_PROTOCOL.ReportStatusCode(). > + > + The return status of this function is ignored by the caller, but the function > + still returns sensible codes: > + > + @retval EFI_SUCCESS The status code has been processed; either > + as a no-op, due to filtering, or by > + formatting it to the system console. > + > + @retval EFI_INVALID_PARAMETER Unknown or malformed contents have been > + detected in Data. > + > + @retval EFI_INCOMPATIBLE_VERSION Unexpected UEFI variable behavior has been > + encountered. > + > + @return Error codes propagated from underlying > + services. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +HandleStatusCode ( > + IN EFI_STATUS_CODE_TYPE CodeType, > + IN EFI_STATUS_CODE_VALUE Value, > + IN UINT32 Instance, > + IN EFI_GUID *CallerId, > + IN EFI_STATUS_CODE_DATA *Data > + ) > +{ > + UINTN VariableSize; > + UINT16 BootCurrent; > + EFI_STATUS Status; > + CHAR16 BootOptionName[ARRAY_SIZE (L"Boot####")]; > + EFI_BOOT_MANAGER_LOAD_OPTION BmBootOption; > + BOOLEAN DevPathStringIsDynamic; > + CHAR16 *DevPathString; > + > + // > + // Ignore all status codes that are irrelevant for LoadImage() and > + // StartImage() preparations and return codes. > + // > + if (Value != mLoadPrep && Value != mLoadFail && > + Value != mStartPrep && Value != mStartFail) { > + return EFI_SUCCESS; > + } > + // > + // Ignore status codes that are not reported by the same containing module. > + // > + if (!CompareGuid (CallerId, &gEfiCallerIdGuid)) { > + return EFI_SUCCESS; > + } > + > + // > + // Sanity-check Data in case of failure reports. > + // > + if ((Value == mLoadFail || Value == mStartFail) && > + (Data == NULL || > + Data->HeaderSize != sizeof *Data || > + Data->Size != sizeof (EFI_RETURN_STATUS_EXTENDED_DATA) - sizeof *Data || > + !CompareGuid (&Data->Type, &gEfiStatusCodeSpecificDataGuid))) { > + DEBUG ((DEBUG_ERROR, "%a:%a: malformed Data\n", gEfiCallerBaseName, > + __FUNCTION__)); > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // Get the number of the Boot#### option that the status code applies to. > + // > + VariableSize = sizeof BootCurrent; > + Status = gRT->GetVariable (EFI_BOOT_CURRENT_VARIABLE_NAME, > + &gEfiGlobalVariableGuid, NULL /* Attributes */, > + &VariableSize, &BootCurrent); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a:%a: failed to get %g:\"%s\": %r\n", > + gEfiCallerBaseName, __FUNCTION__, &gEfiGlobalVariableGuid, > + EFI_BOOT_CURRENT_VARIABLE_NAME, Status)); > + return Status; > + } > + if (VariableSize != sizeof BootCurrent) { > + DEBUG ((DEBUG_ERROR, "%a:%a: got %Lu bytes for %g:\"%s\", expected %Lu\n", > + gEfiCallerBaseName, __FUNCTION__, (UINT64)VariableSize, > + &gEfiGlobalVariableGuid, EFI_BOOT_CURRENT_VARIABLE_NAME, > + (UINT64)sizeof BootCurrent)); > + return EFI_INCOMPATIBLE_VERSION; > + } > + > + // > + // Get the Boot#### option that the status code applies to. > + // > + UnicodeSPrint (BootOptionName, sizeof BootOptionName, L"Boot%04x", > + BootCurrent); > + Status = EfiBootManagerVariableToLoadOption (BootOptionName, &BmBootOption); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, > + "%a:%a: EfiBootManagerVariableToLoadOption(\"%s\"): %r\n", > + gEfiCallerBaseName, __FUNCTION__, BootOptionName, Status)); > + return Status; > + } > + > + // > + // Format the device path. > + // > + DevPathStringIsDynamic = TRUE; > + DevPathString = ConvertDevicePathToText ( > + BmBootOption.FilePath, > + FALSE, // DisplayOnly > + FALSE // AllowShortcuts > + ); > + if (DevPathString == NULL) { > + DevPathStringIsDynamic = FALSE; > + DevPathString = L""; > + } > + > + // > + // Print the message to the console. > + // > + if (Value == mLoadPrep || Value == mStartPrep) { > + Print ( > + L"%a: %a %s \"%s\" from %s\n", > + gEfiCallerBaseName, > + Value == mLoadPrep ? "loading" : "starting", > + BootOptionName, > + BmBootOption.Description, > + DevPathString > + ); > + } else { > + Print ( > + L"%a: failed to %a %s \"%s\" from %s: %r\n", > + gEfiCallerBaseName, > + Value == mLoadFail ? "load" : "start", > + BootOptionName, > + BmBootOption.Description, > + DevPathString, > + ((EFI_RETURN_STATUS_EXTENDED_DATA *)Data)->ReturnStatus > + ); > + } > + > + // > + // Done. > + // > + if (DevPathStringIsDynamic) { > + FreePool (DevPathString); > + } > + EfiBootManagerFreeLoadOption (&BmBootOption); > + return EFI_SUCCESS; > +} > + > + > +/** > + Unregister HandleStatusCode() at ExitBootServices(). > + > + (See EFI_RSC_HANDLER_PROTOCOL in Volume 3 of the Platform Init spec.) > + > + @param[in] Event Event whose notification function is being invoked. > + > + @param[in] Context Pointer to EFI_RSC_HANDLER_PROTOCOL, originally looked up > + when HandleStatusCode() was registered. > +**/ > +STATIC > +VOID > +EFIAPI > +UnregisterAtExitBootServices ( > + IN EFI_EVENT Event, > + IN VOID *Context > + ) > +{ > + EFI_RSC_HANDLER_PROTOCOL *StatusCodeRouter; > + > + StatusCodeRouter = Context; > + StatusCodeRouter->Unregister (HandleStatusCode); > +} > + > + > +/** > + Register a status code handler for printing the Boot Manager's LoadImage() > + and StartImage() preparations, and return codes, to the UEFI console. > + > + @retval EFI_SUCCESS The status code handler has been successfully > + registered. > + > + @return Error codes propagated from boot services and from > + EFI_RSC_HANDLER_PROTOCOL. > +**/ > +EFI_STATUS > +EFIAPI > +PlatformBmPrintScRegisterHandler ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + EFI_RSC_HANDLER_PROTOCOL *StatusCodeRouter; > + EFI_EVENT ExitBootEvent; > + > + Status = gBS->LocateProtocol (&gEfiRscHandlerProtocolGuid, > + NULL /* Registration */, (VOID **)&StatusCodeRouter); > + ASSERT_EFI_ERROR (Status); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // > + // Set the EFI_STATUS_CODE_VALUE convenience variables. > + // > + mLoadPrep = PcdGet32 (PcdProgressCodeOsLoaderLoad); > + mLoadFail = (EFI_SOFTWARE_DXE_BS_DRIVER | > + EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR); > + mStartPrep = PcdGet32 (PcdProgressCodeOsLoaderStart); > + mStartFail = (EFI_SOFTWARE_DXE_BS_DRIVER | > + EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED); > + This bit looks somewhat dodgy to me, but I suppose the asymmetry is 'prior art' from EDK2, no? In any case, this looks good to me otherwise, so for the series Reviewed-by: Ard Biesheuvel > + // > + // Register the handler callback. > + // > + Status = StatusCodeRouter->Register (HandleStatusCode, TPL_CALLBACK); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a:%a: failed to register status code handler: %r\n", > + gEfiCallerBaseName, __FUNCTION__, Status)); > + return Status; > + } > + > + // > + // Status code reporting and routing/handling extend into OS runtime. Since > + // we don't want our handler to survive the BDS phase, we have to unregister > + // the callback at ExitBootServices(). (See EFI_RSC_HANDLER_PROTOCOL in > + // Volume 3 of the Platform Init spec.) > + // > + Status = gBS->CreateEvent ( > + EVT_SIGNAL_EXIT_BOOT_SERVICES, // Type > + TPL_CALLBACK, // NotifyTpl > + UnregisterAtExitBootServices, // NotifyFunction > + StatusCodeRouter, // NotifyContext > + &ExitBootEvent // Event > + ); > + if (EFI_ERROR (Status)) { > + // > + // We have to unregister the callback right now, and fail the function. > + // > + DEBUG ((DEBUG_ERROR, "%a:%a: failed to create ExitBootServices() event: " > + "%r\n", gEfiCallerBaseName, __FUNCTION__, Status)); > + StatusCodeRouter->Unregister (HandleStatusCode); > + return Status; > + } > + > + return EFI_SUCCESS; > +} > -- > 2.19.1.3.g30247aa5d201 > >