From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.8473.1661327929387660795 for ; Wed, 24 Aug 2022 00:58:49 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=coUXln3D; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1661327928; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Z1EMd/PxM6HFpB7GmkQsKRdi0UGfsJ/z1Y+hAFYLYlQ=; b=coUXln3D/Ct91PowSMeP8ak1ydodeRlyM7kLMzFEwLdk5lY330Sil0xG5gUy+poo/tGW9A /5G/R+aUQlbj4oNKOOVsaoUc172Q2NvIG8md3NyOu4mzTDFOldB6aIgA1IGTqHz2Xbqb2d 3rfLMeju6p2IOoc6ZQKBWa9Ical0FWE= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-588-FssO-VLkOS-4dZEXh97U7w-1; Wed, 24 Aug 2022 03:58:44 -0400 X-MC-Unique: FssO-VLkOS-4dZEXh97U7w-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C02AC3C138A3; Wed, 24 Aug 2022 07:58:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.39.194.95]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4A9BC4010D2A; Wed, 24 Aug 2022 07:58:42 +0000 (UTC) Subject: Re: [PATCH v2 1/2] OvmfPkg: Introduce alternate UefiDriverEntrypoint to inhibit driver load To: Ard Biesheuvel Cc: devel@edk2.groups.io, Yuan Yu , Gerd Hoffmann , Pawel Polawski , Oliver Steffen , Jiewen Yao , "Brian J . Johnson" References: <20220817151157.1941409-1-ardb@kernel.org> <20220817151157.1941409-2-ardb@kernel.org> <08e278c5-4e7a-b91d-526f-315cfa3398b8@redhat.com> From: "Laszlo Ersek" Message-ID: <008b86dd-5dc3-e791-c6cb-4717971bc156@redhat.com> Date: Wed, 24 Aug 2022 09:58:41 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/22/22 18:59, Ard Biesheuvel wrote: > On Thu, 18 Aug 2022 at 07:58, Laszlo Ersek wrote: >> >> On 08/17/22 17:11, Ard Biesheuvel wrote: >>> Add a new library that can be incorporated into any driver built from >>> source, and which permits loading of the driver to be inhibited based on >>> the value of a QEMU fw_cfg boolean variable. This will be used in a >>> subsequent patch to allow dispatch of the IPv6 and IPv6 network protocol >> >> (1) Still a typo? Did you mean "IPv4 and IPv6"? >> >>> driver to be controlled from the QEMU command line. >>> >>> This approach is based on the notion that all UEFI and DXE drivers share >>> a single UefiDriverEntryPoint implementation, which we can easily swap >>> out at build time with one that will abort execution based on the value >>> of some QEMU fw_cfg variable. >>> >>> Signed-off-by: Ard Biesheuvel >>> --- >>> OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c | 147 ++++++++++++++++++++ >>> OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf | 57 ++++++++ >>> OvmfPkg/OvmfPkg.dec | 4 + >>> 3 files changed, 208 insertions(+) >>> >>> diff --git a/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c >>> new file mode 100644 >>> index 000000000000..6eaf0cfd16ad >>> --- /dev/null >>> +++ b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c >>> @@ -0,0 +1,147 @@ >>> +/** @file >>> + Entry point to a EFI/DXE driver. This version is specific to QEMU, and ties >>> + dispatch of the driver in question on the value of a QEMU fw_cfg boolean >>> + variable which is referenced by name via a fixed pointer PCD. >>> + >>> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
>>> +Copyright (c) 2022, Google LLC. All rights reserved.
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent >>> + >>> +**/ >>> + >>> +#include >>> + >>> +#include >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +/** >>> + Unloads an image from memory. >>> + >>> + This function is a callback that a driver registers to do cleanup >>> + when the UnloadImage boot service function is called. >>> + >>> + @param ImageHandle The handle to the image to unload. >>> + >>> + @return Status returned by all unload(). >>> + >>> +**/ >>> +STATIC >>> +EFI_STATUS >>> +EFIAPI >>> +_DriverUnloadHandler ( >>> + EFI_HANDLE ImageHandle >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + >>> + // >>> + // If an UnloadImage() handler is specified, then call it >>> + // >>> + Status = ProcessModuleUnloadList (ImageHandle); >>> + >>> + // >>> + // If the driver specific unload handler does not return an error, then call >>> + // all of the library destructors. If the unload handler returned an error, >>> + // then the driver can not be unloaded, and the library destructors should >>> + // not be called >>> + // >>> + if (!EFI_ERROR (Status)) { >>> + ProcessLibraryDestructorList (ImageHandle, gST); >>> + } >>> + >>> + // >>> + // Return the status from the driver specific unload handler >>> + // >>> + return Status; >>> +} >>> + >>> +/** >>> + The entry point of PE/COFF Image for a DXE Driver, DXE Runtime Driver, or >>> + UEFI Driver. >>> + >>> + @param ImageHandle The image handle of the DXE Driver, DXE >>> + Runtime Driver, or UEFI Driver. >>> + @param SystemTable A pointer to the EFI System Table. >>> + >>> + @retval EFI_SUCCESS The DXE Driver, DXE Runtime Driver, or >>> + UEFI Driver exited normally. >>> + @retval EFI_INCOMPATIBLE_VERSION _gUefiDriverRevision is greater than >>> + SystemTable->Hdr.Revision. >>> + @retval Other Return value from >>> + ProcessModuleEntryPointList(). >>> + >>> +**/ >>> +EFI_STATUS >>> +EFIAPI >>> +_ModuleEntryPoint ( >>> + IN EFI_HANDLE ImageHandle, >>> + IN EFI_SYSTEM_TABLE *SystemTable >>> + ) >>> +{ >>> + EFI_STATUS Status; >>> + EFI_LOADED_IMAGE_PROTOCOL *LoadedImage; >>> + RETURN_STATUS RetStatus; >>> + BOOLEAN Enabled; >>> + >>> + if (_gUefiDriverRevision != 0) { >>> + // >>> + // Make sure that the EFI/UEFI spec revision of the platform is >= EFI/UEFI >>> + // spec revision of the driver >>> + // >>> + if (SystemTable->Hdr.Revision < _gUefiDriverRevision) { >>> + return EFI_INCOMPATIBLE_VERSION; >>> + } >>> + } >>> + >>> + // >>> + // Call constructor for all libraries >>> + // >>> + ProcessLibraryConstructorList (ImageHandle, SystemTable); >>> + >>> + // >>> + // Install unload handler... >>> + // >>> + if (_gDriverUnloadImageCount != 0) { >>> + Status = gBS->HandleProtocol ( >>> + ImageHandle, >>> + &gEfiLoadedImageProtocolGuid, >>> + (VOID **)&LoadedImage >>> + ); >>> + ASSERT_EFI_ERROR (Status); >>> + LoadedImage->Unload = _DriverUnloadHandler; >>> + } >>> + >>> + RetStatus = QemuFwCfgParseBool ( >>> + FixedPcdGetPtr (PcdEntryPointOverrideFwCfgVarName), >>> + &Enabled); >>> + if (!RETURN_ERROR (RetStatus) && !Enabled) { >>> + // >>> + // The QEMU fw_cfg variable tells us not to load this image. So abort. >>> + // >>> + Status = EFI_ABORTED; >>> + } else { >>> + // >>> + // Call the driver entry point >>> + // >>> + Status = ProcessModuleEntryPointList (ImageHandle, SystemTable); >>> + } >> >> Right, I think this is the way to do it -- we've constructed all the lib >> instances, so now we can rely on PCDs and QemuFwCfg. >> >> I'm OK with this version, just one idea: in order to avoid the code >> duplication (and to benefit from future improvements to the original >> entry point lib in MdePkg), we could introduce a new hook API to the >> original -- forwarding the ImageHandle and SystemTable parameters to it >> --, provide a Null instance implementation for the API, for the general >> case, in MdePkg, and provide an fw_cfg-based implementation for OVMF / >> ArmVirt. >> >> I think it's worth a try; if Michael, Liming and Zhiguang don't like the >> bit of additional complexity in MdePkg, you can still merge this >> version. If you don't want to patch MdePkg though, I won't insist, of >> course. >> > > This means every single DSC that describes a driver, including > out-of-tree option ROMs for graphics and network cards will need to be > updated to include a resolution. I don't think this is worth the > hassle, to be honest, good point > at least not until we get support for defining > default resolutions as part of the library class declaration (which I > asked for a number of times) this one too > >> Reviewed-by: Laszlo Ersek >> > > Thanks. I highly appreciate that you have taken the time to join the discussion. > > Will you be in Dublin next month by any chance? > unfortunately not -- the location is good for me, but the time is not, it conflicts again with my kids' school schedule. Cheers! Laszlo