Re: [PATCH 3/4] OvmfPkg: add QemuRamfb to platform console

[Laszlo Ersek <lersek@redhat.com>]

On 06/08/18 13:39, Gerd Hoffmann wrote:
> Add QemuRamfbDxe device path to the list of platform console devices,
> so ConSplitter will pick up the device even though it isn't a PCI GPU.

This explanation is not wrong, but I'll list more details, just in case.

By adding the devpath to "gPlatformConsole" with CONSOLE_OUT,
technically we push the devpath into the ConOut UEFI variable, as
follows:

  BdsEntry()                                  [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]
    PlatformBootManagerBeforeConsole()        [OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
      PlatformInitializeConsole()             [OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c]
        EfiBootManagerUpdateConsoleVariable() [MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c]

After this, BdsEntry goes on to actually connect the device (i.e., it
makes the QemuRamfbDxe driver bind the device):

  BdsEntry()                                  [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]
    EfiBootManagerConnectAllDefaultConsoles() [MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c]

Binding drivers to keyboard, serial and graphics devices, so that text
input, text output, and graphics output protocols are produced, isn't
per se sufficient for ConSplitterDxe to multiplex to/from those
protocols. For that, ConPlatformDxe needs to "tag" the handles with
EfiConsole(In|Out)DeviceGuid|EfiStandardErrorDeviceGuid --
ConSplitterDxe depends on those protocols. It's ConPlatformDxe that
needs the devpath to be in ConIn/ConOut/ErrOut, for the "tagging" to
occur.

In total, we add the ramfb devpath to "gPlatformConsole" so that our
PlatformInitializeConsole() function puts it in ConOut, despite ramfb
not being a PCI display device. Binding the device to QemuRamFbDxe, and
multiplexing from/to it happen "automatically" from there, thanks to
BdsDxe, and ConPlatformDxe/ConSplitterDxe respectively.

(1) I don't mind if you stick with the current commit message; I just
wanted to provide more details for this discussion.

More comments below:

>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  .../Library/PlatformBootManagerLib/PlatformData.c  | 44 ++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> index a50cd7bcaf..6202516971 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> @@ -14,6 +14,7 @@
>  **/
>
>  #include "BdsPlatform.h"
> +#include <Guid/QemuRamfb.h>
>
>  //
>  // Debug Agent UART Device Path structure
> @@ -37,6 +38,17 @@ typedef struct {
>  } USB_KEYBOARD_DEVICE_PATH;
>  #pragma pack ()
>
> +//
> +// QemuRamfb Device Path structure
> +//
> +#pragma pack(1)
> +typedef struct {
> +  VENDOR_DEVICE_PATH        Vendor;
> +  ACPI_ADR_DEVICE_PATH      AcpiAdr;
> +  EFI_DEVICE_PATH_PROTOCOL  End;
> +} VENDOR_RAMFB_DEVICE_PATH;
> +#pragma pack()

(2) Please add a space character between each "pack" and "(".

> +
>  ACPI_HID_DEVICE_PATH       gPnpPs2KeyboardDeviceNode  = gPnpPs2Keyboard;
>  ACPI_HID_DEVICE_PATH       gPnp16550ComPortDeviceNode = gPnp16550ComPort;
>  UART_DEVICE_PATH           gUartDeviceNode            = gUart;
> @@ -100,6 +112,34 @@ STATIC USB_KEYBOARD_DEVICE_PATH gUsbKeyboardDevicePath = {
>    gEndEntire
>  };
>
> +STATIC VENDOR_RAMFB_DEVICE_PATH gQemuRamfbDevicePath = {
> +  {
> +    {
> +      HARDWARE_DEVICE_PATH,
> +      HW_VENDOR_DP,
> +      {
> +        (UINT8) (sizeof (VENDOR_DEVICE_PATH)),
> +        (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> +      }
> +    },
> +    QEMU_RAMFB_GUID,
> +  },
> +  {
> +    {
> +      ACPI_DEVICE_PATH,
> +      ACPI_ADR_DP,
> +      {
> +        (UINT8) (sizeof (ACPI_ADR_DEVICE_PATH)),
> +        (UINT8) ((sizeof (ACPI_ADR_DEVICE_PATH)) >> 8)
> +      }
> +    },
> +    ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
> +                      ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL,
> +                      0, 0)

(3) Urgh. On one hand, I dislike this dump of constants; on the other
hand, I don't want to ask you to repeat all the parameter comments that
I asked for in the previous patch.

OK, let's stick with the dump of constants, just make sure the
indentation is idiomatic.

> +  },
> +  gEndEntire
> +};
> +
>  //
>  // Predefined platform default console device path
>  //
> @@ -113,6 +153,10 @@ PLATFORM_CONSOLE_CONNECT_ENTRY   gPlatformConsole[] = {
>      CONSOLE_IN
>    },
>    {
> +    (EFI_DEVICE_PATH_PROTOCOL *)&gQemuRamfbDevicePath,
> +    CONSOLE_OUT
> +  },

Right, this is consistent with PreparePciDisplayDevicePath().

Thanks,
Laszlo

> +  {
>      NULL,
>      0
>    }
>