Re: [PATCH v2 1/2] OvmfPkg: Introduce alternate UefiDriverEntrypoint to inhibit driver load

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 18 Aug 2022 at 07:58, Laszlo Ersek <lersek@redhat.com> 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 <ardb@kernel.org>
> > ---
> >  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.<BR>
> > +Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include <Uefi.h>
> > +
> > +#include <Protocol/LoadedImage.h>
> > +
> > +#include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/QemuFwCfgSimpleParserLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/UefiDriverEntryPoint.h>
> > +
> > +/**
> > +  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 <lersek@redhat.com>
>

Thanks. I highly appreciate that you have taken the time to join the discussion.

Will you be in Dublin next month by any chance?