Re: [Qemu-devel] [PATCH v2 0/7] ramfb: simple boot framebuffer, no legacy vga

[Laszlo Ersek]

On 06/06/18 10:49, Gerd Hoffmann wrote:

> edk2:
>    QemuRamfbDxe driver, x86 wireup, incomplete arm wireup.

I'm looking at the patches very cursorily right now:

> Notes on QemuRamfbDxe:
>
> QemuRamfbDxe registers as VenHw device with a fresh generated uuid.
> The uuid should probably go to some header file, suggestions where to
> place it best?

(1) Under "OvmfPkg/Include/Guid/". The file "VirtioMmioTransport.h" is a
minimal example.

Please review commit 3765e030affb ("OvmfPkg: introduce
VIRTIO_MMIO_TRANSPORT_GUID", 2015-01-02) in its entirety though, as the
OvmfPkg.dec file should be modified as well.

> QemuRamfbDxe registers a acpi devpath node for the gop protocol.  It's
> appended to the VenHw path, but looks like that isn't enough to create
> it as child node of the VenHw device (according to devtree outout).
> Suggestions how to fix that?  Still this is good enough for
> consplitter to accept the device.

The UEFI Driver Writer's Guide, and the UEFI spec, treat this
extensively, but let's save ourselves some time:

(2) Please flip the MODULE_TYPE of the driver to DXE_DRIVER, from
UEFI_DRIVER. (This will also let you drop some of the other patches.)

(3) Try adding the following at the end of QemuRamfbDxe:

  VOID *DevicePath;

  ...

  gBS->OpenProtocol (
         RamfbHandle,
         &gEfiDevicePathProtocolGuid,
         &DevicePath,
         gImageHandle,
         GopHandle,
         EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
         );

See also iPXE commit d6817943d187 ("[efi] Install the HII config access
protocol on a child of the SNP handle", 2016-07-08), especially the call
to efi_child_add().

> The device path is added to the gPlatformConsole list,

Good idea.

Two notes on that:

(4) Once you add the venhw GUID as a standalone header file (see (1)),
you won't need to duplicate the constants between patches. In
"gQemuRamfbDevicePath", you'll be able to use the EFI_GUID structure
initializer macro from the header file.

(5) Does it work if you replace ACPI_ADR_DISPLAY_TYPE_VGA with
ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL? QemuVideoDxe uses the former,
but VirtioGpuDxe uses the latter. Windows has some really obscure
requirements dependent on "ACPI display type", if I remember correctly,
and "external" looked the most robust when I last checked.

> so consplitter will pick up the ramfb device even though it isn't a
> pci display device. The firmware display will show up on ramfb in
> addition to other display devices (if any).  Is that approach ok?

Absolutely, that's what we want.

> If so I'll do that for armvirt too.

(6) No extra steps should be necessary for ArmVirtPkg; in the
PlatformBootManagerBeforeConsole() function
[ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c], we have

>   //
>   // Find all display class PCI devices (using the handles from the previous
>   // step), and connect them non-recursively. This should produce a number of
>   // child handles with GOPs on them.
>   //
>   FilterAndProcess (&gEfiPciIoProtocolGuid, IsPciDisplay, Connect);
>
>   //
>   // Now add the device path of all handles with GOP on them to ConOut and
>   // ErrOut.
>   //
>   FilterAndProcess (&gEfiGraphicsOutputProtocolGuid, NULL, AddOutput);

The first call does not cover the new device, but that's fine: the new
child handle with the GOP instance on it is produced by a DXE_DRIVER in
its entry point function -- that is, not by a UEFI driver that follows
the UEFI driver model --, hence it need not be connected by platform BDS
explicitly. By the time BDS is entered in the first place, the GOP will
exist already.

Therefore, the second call will simply pick up the devpath from the new
child (GOP) handle automatically, and add it to both ConOut and ErrOut.

Does it not work for you?

Thanks
Laszlo