Re: [PATCH v2 2/4] OvmfPkg: add QemuRamfbDxe

[Laszlo Ersek <lersek@redhat.com>]

Hi Gerd,

On 06/12/18 11:31, Gerd Hoffmann wrote:
> Add a driver for the qemu ramfb display device.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/QemuRamfbDxe/QemuRamfb.c      | 396 ++++++++++++++++++++++++++++++++++
>  OvmfPkg/OvmfPkgIa32.dsc               |   1 +
>  OvmfPkg/OvmfPkgIa32.fdf               |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc            |   1 +
>  OvmfPkg/OvmfPkgIa32X64.fdf            |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                |   1 +
>  OvmfPkg/OvmfPkgX64.fdf                |   1 +
>  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf |  52 +++++
>  8 files changed, 454 insertions(+)
>  create mode 100644 OvmfPkg/QemuRamfbDxe/QemuRamfb.c
>  create mode 100644 OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
> 
> diff --git a/OvmfPkg/QemuRamfbDxe/QemuRamfb.c b/OvmfPkg/QemuRamfbDxe/QemuRamfb.c
> new file mode 100644
> index 0000000000..00a2875e99
> --- /dev/null
> +++ b/OvmfPkg/QemuRamfbDxe/QemuRamfb.c
> @@ -0,0 +1,396 @@
> +/** @file
> +  This driver is a implementation of the Graphics Output Protocol
> +  for the QEMU ramfb device.
> +
> +  Copyright (c) 2018, 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

(1) These lines are overlong, can you please wrap them to 80 chars?

> +  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 <Protocol/GraphicsOutput.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DevicePathLib.h>
> +#include <Library/FrameBufferBltLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/QemuFwCfgLib.h>
> +
> +#include <Guid/QemuRamfb.h>
> +
> +#define RAMFB_FORMAT  0x34325258 /* DRM_FORMAT_XRGB8888 */
> +#define RAMFB_BPP     4
> +
> +#pragma pack (1)
> +typedef struct RAMFB_CONFIG {
> +  UINT64 Address;
> +  UINT32 FourCC;
> +  UINT32 Flags;
> +  UINT32 Width;
> +  UINT32 Height;
> +  UINT32 Stride;
> +} RAMFB_CONFIG;
> +#pragma pack ()
> +
> +static EFI_HANDLE                    mRamfbHandle;

(2) So, when I mentioned "STATIC" in my last email, I meant "STATIC",
not "static" :) I see you couldn't believe me! :) I apologize; yes,
indeed in edk2 we spell "static" "STATIC". Can you please update all
occurrences?

> +static EFI_HANDLE                    mGopHandle;
> +static FRAME_BUFFER_CONFIGURE        *mQemuRamfbFrameBufferBltConfigure;
> +static UINTN                         mQemuRamfbFrameBufferBltConfigureSize;
> +static FIRMWARE_CONFIG_ITEM          mRamFbFwCfgItem;

(3) My bad, I suggested inconsistent spelling for "mRamFbFwCfgItem". You
spelled it everywhere consistently "Ramfb", in identifiers.

Can you please update this to "mRamfbFwCfgItem" too?

> +
> +static EFI_GRAPHICS_OUTPUT_MODE_INFORMATION mQemuRamfbModeInfo[] = {
> +  {
> +    0,    // Version
> +    640,  // HorizontalResolution
> +    480,  // VerticalResolution
> +  },{
> +    0,    // Version
> +    800,  // HorizontalResolution
> +    600,  // VerticalResolution
> +  },{
> +    0,    // Version
> +    1024, // HorizontalResolution
> +    768,  // VerticalResolution
> +  }
> +};
> +#define mQemuRamfbModeCount ARRAY_SIZE(mQemuRamfbModeInfo)

(4) Sorry I was unclear about this in my last email. What I meant is to
eliminate the "mQemuRamfbModeCount" macro entirely, and use the
ARRAY_SIZE() in its place. I count two instances in the code.

Alternatively, if you really like a dedicated macro for this, please
spell it QEMU_RAMFB_MODE_COUNT.

Either way: please insert a space character after ARRAY_SIZE and before "(".

> +
> +static EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE mQemuRamfbMode = {
> +  mQemuRamfbModeCount,                            // MaxMode
> +  0,                                              // Mode
> +  mQemuRamfbModeInfo,                             // Info
> +  sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION),  // SizeOfInfo
> +};
> +
> +static
> +EFI_STATUS
> +EFIAPI
> +QemuRamfbGraphicsOutputQueryMode (
> +  IN  EFI_GRAPHICS_OUTPUT_PROTOCOL          *This,
> +  IN  UINT32                                ModeNumber,
> +  OUT UINTN                                 *SizeOfInfo,
> +  OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  **Info
> +  )
> +{
> +  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *ModeInfo;
> +
> +  if (Info == NULL || SizeOfInfo == NULL || ModeNumber >= mQemuRamfbMode.MaxMode) {

(5) This line is too long; please use a 80 chars width.

> +    return EFI_INVALID_PARAMETER;
> +  }
> +  ModeInfo = &mQemuRamfbModeInfo[ModeNumber];
> +
> +  *Info = AllocateCopyPool (sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION),
> +            ModeInfo);
> +  if (*Info == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +  *SizeOfInfo = sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +static
> +EFI_STATUS
> +EFIAPI
> +QemuRamfbGraphicsOutputSetMode (
> +  IN  EFI_GRAPHICS_OUTPUT_PROTOCOL *This,
> +  IN  UINT32                       ModeNumber
> +  )
> +{
> +  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *ModeInfo;
> +  RAMFB_CONFIG                          Config;
> +  EFI_GRAPHICS_OUTPUT_BLT_PIXEL         Black;
> +  RETURN_STATUS                         Status;
> +
> +  if (ModeNumber >= mQemuRamfbMode.MaxMode) {
> +    return EFI_UNSUPPORTED;
> +  }
> +  ModeInfo = &mQemuRamfbModeInfo[ModeNumber];
> +
> +  DEBUG ((DEBUG_INFO, "Ramfb: SetMode %u (%ux%u)\n", ModeNumber,
> +    ModeInfo->HorizontalResolution, ModeInfo->VerticalResolution));
> +
> +  Config.Address = SwapBytes64 (mQemuRamfbMode.FrameBufferBase);
> +  Config.FourCC  = SwapBytes32 (RAMFB_FORMAT);
> +  Config.Flags   = SwapBytes32 (0);
> +  Config.Width   = SwapBytes32 (ModeInfo->HorizontalResolution);
> +  Config.Height  = SwapBytes32 (ModeInfo->VerticalResolution);
> +  Config.Stride  = SwapBytes32 (ModeInfo->HorizontalResolution * RAMFB_BPP);
> +
> +  Status = FrameBufferBltConfigure (
> +             (VOID*)(UINTN)mQemuRamfbMode.FrameBufferBase,
> +             ModeInfo,
> +             mQemuRamfbFrameBufferBltConfigure,
> +             &mQemuRamfbFrameBufferBltConfigureSize
> +             );
> +
> +  if (Status == RETURN_BUFFER_TOO_SMALL) {
> +    if (mQemuRamfbFrameBufferBltConfigure != NULL) {
> +      FreePool (mQemuRamfbFrameBufferBltConfigure);
> +    }
> +    mQemuRamfbFrameBufferBltConfigure =
> +      AllocatePool (mQemuRamfbFrameBufferBltConfigureSize);
> +    if (mQemuRamfbFrameBufferBltConfigure == NULL) {
> +      return EFI_OUT_OF_RESOURCES;

(6) Small logic issue here: at this point
"mQemuRamfbFrameBufferBltConfigure" is NULL (which is good), but
"mQemuRamfbFrameBufferBltConfigureSize" is nonzero (in fact it is "large
enough"; it's just been returned to us with the correct size).

As a result, at the next attempt to set the same mode,
FrameBufferBltConfigure() will dereference NULL.

So please, before returning, assign
"mQemuRamfbFrameBufferBltConfigureSize" zero.

> +    }
> +
> +    Status = FrameBufferBltConfigure (
> +               (VOID*)(UINTN)mQemuRamfbMode.FrameBufferBase,
> +               ModeInfo,
> +               mQemuRamfbFrameBufferBltConfigure,
> +               &mQemuRamfbFrameBufferBltConfigureSize);

(7) Please break off the ");" to a separate line, as discussed before.

> +    if (RETURN_ERROR (Status)) {
> +      ASSERT (Status == RETURN_UNSUPPORTED);
> +      return Status;
> +    }

(8) I think I was unclear about this in my last email, sorry about that
again. I meant for this RETURN_ERROR() check to be placed *after* (not
inside) the RETURN_BUFFER_TOO_SMALL branch.

With the idea being, after the end of the "too small" branch, regardless
of us actually having taken the "too small" branch, Status must either
be "success", or "unsupported".

Currently the code, as-is, will not catch the first
FrameBufferBltConfigure() call returning "unsupported".

> +  }
> +
> +  mQemuRamfbMode.Mode = ModeNumber;
> +  mQemuRamfbMode.Info = ModeInfo;
> +
> +  QemuFwCfgSelectItem (mRamFbFwCfgItem);
> +  QemuFwCfgWriteBytes (sizeof (Config), &Config);
> +
> +  //
> +  // clear screen
> +  //
> +  ZeroMem (&Black, sizeof (Black));
> +  Status = FrameBufferBlt (
> +             mQemuRamfbFrameBufferBltConfigure,
> +             &Black,
> +             EfiBltVideoFill,
> +             0,                               // SourceX -- ignored
> +             0,                               // SourceY -- ignored
> +             0,                               // DestinationX
> +             0,                               // DestinationY
> +             ModeInfo->HorizontalResolution,  // Width
> +             ModeInfo->VerticalResolution,    // Height
> +             0                                // Delta -- ignored
> +             );
> +  if (EFI_ERROR (Status)) {

(9) For stylistic reasons, please use "RETURN_ERROR (Status)" here. (See
the previous discussion on RETURN_STATUS vs. EFI_STATUS.)

> +    DEBUG ((DEBUG_WARN, "%a: clearing the screen failed: %r\n",
> +            __FUNCTION__, Status));

(10) Please fix the indentation.

> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +static
> +EFI_STATUS
> +EFIAPI
> +QemuRamfbGraphicsOutputBlt (
> +  IN  EFI_GRAPHICS_OUTPUT_PROTOCOL          *This,
> +  IN  EFI_GRAPHICS_OUTPUT_BLT_PIXEL         *BltBuffer, OPTIONAL
> +  IN  EFI_GRAPHICS_OUTPUT_BLT_OPERATION     BltOperation,
> +  IN  UINTN                                 SourceX,
> +  IN  UINTN                                 SourceY,
> +  IN  UINTN                                 DestinationX,
> +  IN  UINTN                                 DestinationY,
> +  IN  UINTN                                 Width,
> +  IN  UINTN                                 Height,
> +  IN  UINTN                                 Delta
> +  )
> +{
> +  return FrameBufferBlt (
> +           mQemuRamfbFrameBufferBltConfigure,
> +           BltBuffer,
> +           BltOperation,
> +           SourceX,
> +           SourceY,
> +           DestinationX,
> +           DestinationY,
> +           Width,
> +           Height,
> +           Delta);

(11) Please break off the ");" to a separate line.

> +}
> +
> +EFI_GRAPHICS_OUTPUT_PROTOCOL QemuRamfbGraphicsOutput = {

(12) Please make this STATIC.

(13) Please rename it to "mQemuRamfbGraphicsOutput".

> +  QemuRamfbGraphicsOutputQueryMode,
> +  QemuRamfbGraphicsOutputSetMode,
> +  QemuRamfbGraphicsOutputBlt,
> +  &mQemuRamfbMode,
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +InitializeQemuRamfb (
> +  IN EFI_HANDLE           ImageHandle,
> +  IN EFI_SYSTEM_TABLE     *SystemTable
> +  )
> +{
> +  EFI_DEVICE_PATH_PROTOCOL  *RamfbDevicePath;
> +  EFI_DEVICE_PATH_PROTOCOL  *GopDevicePath;
> +  VOID                      *DevicePath;
> +  VENDOR_DEVICE_PATH        VendorDeviceNode;
> +  ACPI_ADR_DEVICE_PATH      AcpiDeviceNode;
> +  EFI_STATUS                Status;
> +  EFI_PHYSICAL_ADDRESS      FbBase;
> +  UINTN                     FbSize, MaxFbSize;
> +  UINTN                     Size, Pages, Index;

(14) Can you rename "Size" to "FwCfgSize" as I requested earlier please?

(15) I see that you are reluctant accepting that edk2 really prefers to
break almost all variable definitions to separate lines :) I suggest the
following compromise:

  UINTN                     FbSize, MaxFbSize, Pages;
  UINTN                     FwCfgSize;
  UINTN                     Index;

Because, an argument can be made that FbSize, MaxFbSize, and Pages
belong together. However, FwCfgSize and Index should each stand alone.

> +
> +  if (!QemuFwCfgIsAvailable()) {

(16) Oops, I missed this last time; please add a space between function
identifier and ().

> +    DEBUG ((DEBUG_INFO, "Ramfb: no FwCfg\n"));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  Status = QemuFwCfgFindFile("etc/ramfb", &mRamFbFwCfgItem, &Size);

(17) Ditto.

> +  if (EFI_ERROR (Status)) {
> +    return EFI_NOT_FOUND;
> +  }
> +  if (Size != sizeof (RAMFB_CONFIG)) {
> +    DEBUG ((DEBUG_ERROR, "Ramfb: FwCfg size mismatch (expected %d, got %d)\n",
> +             sizeof (RAMFB_CONFIG), Size));

(18) Indentation.

(19) Additionally, "sizeof" has type UINTN, and "Size" (FwCfgSize) has
type UINTN too. Please cast them both to UINT64 and print them with %lu.

> +    return EFI_NOT_FOUND;

(20) I generally return EFI_PROTOCOL_ERROR whenever fw_cfg produces
garbage, but EFI_NOT_FOUND is perfectly acceptable as well. Up to you.

> +  }
> +
> +  MaxFbSize = 0;
> +  for (Index = 0; Index < mQemuRamfbModeCount; Index++) {
> +    mQemuRamfbModeInfo[Index].PixelsPerScanLine =
> +      mQemuRamfbModeInfo[Index].HorizontalResolution;
> +    mQemuRamfbModeInfo[Index].PixelFormat =
> +      PixelBlueGreenRedReserved8BitPerColor;
> +    FbSize = RAMFB_BPP *
> +      mQemuRamfbModeInfo[Index].HorizontalResolution *
> +      mQemuRamfbModeInfo[Index].VerticalResolution;
> +    if (MaxFbSize < FbSize) {
> +      MaxFbSize = FbSize;
> +    }
> +    DEBUG ((DEBUG_INFO, "Ramfb: Mode %u: %ux%u, %lu kB\n", (UINT64)Index,

(21) The cast is OK, but the conversion specifier should be %lu.

> +      mQemuRamfbModeInfo[Index].HorizontalResolution,
> +      mQemuRamfbModeInfo[Index].VerticalResolution,
> +      (UINT64)FbSize / 1024));

(22) Sorry, this division is no longer portable (between 64-bit and
32-bit edk2 builds) in this form. The reason is that UINT64 values can't
*generally* be divided in 32-bit builds without compiler intrinsics; in
order to avoid those, we use named helper functions for such divisions.

However, using those helpers wasn't what I meant; I meant

  (UINT64)(FbSize / 1024)

Because, FbSize is UINTN (i.e. UINT32 in 32-bit builds, UINT64 in 64-bit
builds), and so it can be divided by 1024 (of type INT32) without
helpers. It's the UINTN *quotient* that we should cast, for portable
printing.

I apologize if my previous comment on this was misleading.

> +  }
> +
> +  Pages = EFI_SIZE_TO_PAGES (MaxFbSize);
> +  MaxFbSize = EFI_PAGES_TO_SIZE (Pages);
> +  FbBase = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateReservedPages (Pages);
> +  if (FbBase == 0) {
> +    DEBUG ((DEBUG_INFO, "Ramfb: memory allocation failed\n"));

(23) Sorry, missed this last time, should be DEBUG_ERROR.

> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +  DEBUG ((DEBUG_INFO, "Ramfb: Framebuffer at 0x%lx, %ld kB, %ld pages\n",
> +          (UINT64)FbBase, (UINT64)MaxFbSize / 1024, (UINT64)Pages));

(24) Indentation.

(25) Regarding the MaxFbSize division, please see (22).

(26) Please use %lu for printing both the MaxFbSize quotient (see above)
and Pages. %ld is for INT64.

> +  mQemuRamfbMode.FrameBufferSize = MaxFbSize;
> +  mQemuRamfbMode.FrameBufferBase = FbBase;
> +
> +  //
> +  // 800 x 600
> +  //
> +  QemuRamfbGraphicsOutputSetMode (&QemuRamfbGraphicsOutput, 1);
> +
> +  //
> +  // ramfb vendor devpath
> +  //
> +  VendorDeviceNode.Header.Type = HARDWARE_DEVICE_PATH;
> +  VendorDeviceNode.Header.SubType = HW_VENDOR_DP;
> +  CopyGuid(&VendorDeviceNode.Guid, &gQemuRamfbGuid);

(27) Please add a space after "CopyGuid".

> +  SetDevicePathNodeLength (&VendorDeviceNode.Header,
> +    sizeof (VENDOR_DEVICE_PATH));
> +
> +  RamfbDevicePath = AppendDevicePathNode (NULL,
> +    (EFI_DEVICE_PATH_PROTOCOL *) &VendorDeviceNode);
> +  if (RamfbDevicePath == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto FreeFramebuffer;
> +  }
> +
> +  Status = gBS->InstallMultipleProtocolInterfaces (
> +                  &mRamfbHandle,
> +                  &gEfiDevicePathProtocolGuid,
> +                  RamfbDevicePath,
> +                  NULL
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Ramfb: install Ramfb Vendor DevicePath failed: %r\n",
> +      Status));
> +    goto FreeRamfbDevicePath;
> +  }
> +
> +  //
> +  // gop devpath + protocol
> +  //
> +  ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));

(28) I think you can drop the ZeroMem(); all fields are set explicitly.

> +  AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;
> +  AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;
> +  AcpiDeviceNode.ADR = ACPI_DISPLAY_ADR (
> +    1,                                       // DeviceIdScheme
> +    0,                                       // HeadId
> +    0,                                       // NonVgaOutput
> +    1,                                       // BiosCanDetect
> +    0,                                       // VendorInfo
> +    ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL,  // Type
> +    0,                                       // Port
> +    0                                        // Index
> +    );

Sigh, this is one of those situations when different coding style
guidelines conflict. One style requirement would be to indent the
arguments two columns to the right of the beginning of
"ACPI_DISPLAY_ADR". However, in that case, we wouldn't fit in 80
characters; and in my book, that's a lot worse. So, just leave this as
it is. :/

> +  SetDevicePathNodeLength (&AcpiDeviceNode.Header,
> +    sizeof (ACPI_ADR_DEVICE_PATH));
> +
> +  GopDevicePath = AppendDevicePathNode (RamfbDevicePath,
> +    (EFI_DEVICE_PATH_PROTOCOL *) &AcpiDeviceNode);
> +  if (GopDevicePath == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto FreeRamfbHandle;
> +  }
> +
> +  Status = gBS->InstallMultipleProtocolInterfaces (
> +                  &mGopHandle,
> +                  &gEfiDevicePathProtocolGuid,
> +                  GopDevicePath,
> +                  &gEfiGraphicsOutputProtocolGuid,
> +                  &QemuRamfbGraphicsOutput,
> +                  NULL
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Ramfb: install GOP DevicePath failed: %r\n",
> +      Status));
> +    goto FreeGopDevicePath;
> +  }
> +
> +  Status = gBS->OpenProtocol (
> +                  mRamfbHandle,
> +                  &gEfiDevicePathProtocolGuid,
> +                  &DevicePath,
> +                  gImageHandle,
> +                  mGopHandle,
> +                  EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Ramfb: OpenProtocol failed: %r\n", Status));
> +    goto FreeGopHandle;
> +  }
> +
> +  return EFI_SUCCESS;
> +
> +FreeGopHandle:
> +  gBS->UninstallMultipleProtocolInterfaces (
> +         mGopHandle,
> +         &gEfiDevicePathProtocolGuid,
> +         GopDevicePath,
> +         &gEfiGraphicsOutputProtocolGuid,
> +         &QemuRamfbGraphicsOutput,
> +         NULL
> +         );
> +FreeGopDevicePath:
> +  FreePool (GopDevicePath);
> +FreeRamfbHandle:
> +  gBS->UninstallMultipleProtocolInterfaces (
> +         mRamfbHandle,
> +         &gEfiDevicePathProtocolGuid,
> +         RamfbDevicePath,
> +         NULL
> +         );
> +FreeRamfbDevicePath:
> +  FreePool (RamfbDevicePath);
> +FreeFramebuffer:
> +  FreePool ((VOID*)(UINTN)mQemuRamfbMode.FrameBufferBase);

(29) Oops, completely missed this in the previous review, multiple times
-- please use FreePages() for releasing FrameBufferBase, not FreePool()!

> +  return Status;
> +}
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index a2c995b910..7ddda89999 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -745,6 +745,7 @@
>    MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>  
>    OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> +  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
>    OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>  
>    #
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index b199713925..52b8b1fea1 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -351,6 +351,7 @@ INF  RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf
>  !endif
>  
>  INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> +INF  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
>  INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>  INF  OvmfPkg/PlatformDxe/Platform.inf
>  INF  OvmfPkg/IoMmuDxe/IoMmuDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index bc7db229d2..3481cdc36b 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -754,6 +754,7 @@
>    MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>  
>    OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> +  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
>    OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>  
>    #
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index 4ebf64b2b9..70845d6972 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -357,6 +357,7 @@ INF  RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf
>  !endif
>  
>  INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> +INF  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
>  INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>  INF  OvmfPkg/PlatformDxe/Platform.inf
>  INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 0767b34d18..8b0895b0ff 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -752,6 +752,7 @@
>    MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>  
>    OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> +  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
>    OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>  
>    #
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index 9ca96f9282..1eb46ac9a2 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -357,6 +357,7 @@ INF  RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf
>  !endif
>  
>  INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> +INF  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
>  INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>  INF  OvmfPkg/PlatformDxe/Platform.inf
>  INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> diff --git a/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf b/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
> new file mode 100644
> index 0000000000..5a9fd42c32
> --- /dev/null
> +++ b/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
> @@ -0,0 +1,52 @@
> +## @file
> +#  This driver is a implementation of the Graphics Output Protocol
> +#  for the QEMU ramfb device.
> +#
> +#  Copyright (c) 2018, 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

(30) These two lines are too long, can you please rewrap them to 80 chars?

> +#  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.
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = QemuRamfbDxe
> +  FILE_GUID                      = dce1b094-7dc6-45d0-9fdd-d7fc3cc3e4ef
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +
> +  ENTRY_POINT                    = InitializeQemuRamfb
> +
> +[Sources]
> +  QemuRamfb.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  BaseMemoryLib
> +  DebugLib
> +  DevicePathLib
> +  FrameBufferBltLib
> +  MemoryAllocationLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +  QemuFwCfgLib
> +
> +[Protocols]
> +  gEfiGraphicsOutputProtocolGuid                # BY_START

(31) As requested previously, drop "# BY_START" and say ""## PRODUCES"
please.

> +
> +[Guids]
> +  gQemuRamfbGuid
> +
> +[Depex]
> +  TRUE
> 

Thanks!
Laszlo