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

[Laszlo Ersek <lersek@redhat.com>]

On 06/11/18 18:24, Laszlo Ersek wrote:
> 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.

Whoops, managed to confuse myself a little here; some correction should
be in order:

The ramfb driver does not follow the UEFI driver model. The GOP is
produced right in the entry point of the driver, not when platform BDS
connects the device.

It remains true however that ConPlatformDxe / ConSplitterDxe, which do
follow the UEFI driver model, bind the GOP in
EfiBootManagerConnectAllDefaultConsoles(). I guess I would re-formulate,

"""
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. Multiplexing from/to the ramfb GOP
happens "automatically" from there, thanks to
ConPlatformDxe/ConSplitterDxe.
"""

Which is basically what the current commit message says. :)

Sorry for any confusion caused!
Laszlo

> 
> (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
>>    }
>>
>