From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web09.20670.1661187609875831238 for ; Mon, 22 Aug 2022 10:00:10 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=Qk8xSJ1A; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 35BEEB81629 for ; Mon, 22 Aug 2022 17:00:07 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D8533C433D7 for ; Mon, 22 Aug 2022 17:00:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1661187605; bh=IVDMRhsv7QA5N6L6F8e0i/ezLQYOU2gg/H0usCt6mVM=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=Qk8xSJ1ApzS5RWgaGVglShCW6Y42UbqdlMxtgeXuGsHTwKXKywWhtGloXnK5YHEPF vn9Yxaaq+KblYlqm4GOSYWitl2VxDIwItYfPNfVvkVupOT7lEoxUaS9HOIaHbe3Hz2 t2TZm2IfS61EHIvJ6/sTEg7cMmWYNAwt0HbFXQQBlvf8F3v9OQPnhSfb83ur/smQ9i +2rT2tDJRfdJnxo6a0yuCZLIXt0zwFHHRd6sXp/B3tXQb2LD6/qDp8REm0AEiCX+Hm j9QJuBPSvWTmMq8uQ1IQNFWHWp0lPECO0VararkGrxMGzdnyVKyOnaH8IeVUmr/1Ix sHB1x+5+YYxpA== Received: by mail-wm1-f43.google.com with SMTP id c187-20020a1c35c4000000b003a30d88fe8eso8142565wma.2 for ; Mon, 22 Aug 2022 10:00:05 -0700 (PDT) X-Gm-Message-State: ACgBeo2tMmjD3Om+5980VXDUu6qMN5poxD4EWUhKiGD11B3Sc77/ZfNV cP5fF1GCNIiRaU4shbh/1/ZtxOv5V0UOZoaEAqk= X-Google-Smtp-Source: AA6agR6mw8WggI443eelWxC7VQ2f8vxhnPJQy1AZwo2qM9Ka66Uv4ZEfVGRmMI2NxTGhlK+5hiWdFyFGmYztKp2Khnw= X-Received: by 2002:a05:600c:384f:b0:3a6:603c:4338 with SMTP id s15-20020a05600c384f00b003a6603c4338mr4578207wmr.192.1661187604054; Mon, 22 Aug 2022 10:00:04 -0700 (PDT) MIME-Version: 1.0 References: <20220817151157.1941409-1-ardb@kernel.org> <20220817151157.1941409-2-ardb@kernel.org> <08e278c5-4e7a-b91d-526f-315cfa3398b8@redhat.com> In-Reply-To: <08e278c5-4e7a-b91d-526f-315cfa3398b8@redhat.com> From: "Ard Biesheuvel" Date: Mon, 22 Aug 2022 18:59:52 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 1/2] OvmfPkg: Introduce alternate UefiDriverEntrypoint to inhibit driver load To: Laszlo Ersek Cc: devel@edk2.groups.io, Yuan Yu , Gerd Hoffmann , Pawel Polawski , Oliver Steffen , Jiewen Yao , "Brian J . Johnson" Content-Type: text/plain; charset="UTF-8" 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, 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) > 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?