On 08/22/22 18:59, Ard Biesheuvel wrote:
> On Thu, 18 Aug 2022 at 07:58, Laszlo Ersek <ler...@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 <a...@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,
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 <ler...@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?
>
unfortunately not -- the location is good for me, but the time is not,
it conflicts again with my kids' school schedule.
Cheers!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92746): https://edk2.groups.io/g/devel/message/92746
Mute This Topic: https://groups.io/mt/93083118/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
