Re: [PATCH RFC 12/14] OvmfPkg/PlatformBootManagerLib: Use a Xen console for ConOut/ConIn

[Laszlo Ersek <lersek@redhat.com>]

On 12/08/16 16:33, Anthony PERARD wrote:
> and add OvmfPkg/XenConsoleIo/XenConsoleIo to XenOvmf platform.
> 
> It actually look for gEfiSerialIoProtocolGuid.
> ---
>  .../Library/PlatformBootManagerLib/BdsPlatform.c   | 33 ++++++++++++++++++++++
>  .../PlatformBootManagerLib.inf                     |  2 ++
>  OvmfPkg/XenOvmf.dsc                                |  4 +++
>  OvmfPkg/XenOvmf.fdf                                |  1 +
>  4 files changed, 40 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index bd64cc3..b8972f7 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -904,6 +904,31 @@ DetectAndPreparePlatformPciDevicePaths (
>    return VisitAllPciInstances (DetectAndPreparePlatformPciDevicePath);
>  }
>  
> +#include <Protocol/SerialIo.h>
> +EFI_STATUS
> +EFIAPI
> +add_serial (
> +  IN EFI_HANDLE  DeviceHandle,
> +  IN VOID        *Instance,
> +  IN VOID        *Context
> +  )
> +{
> +  EFI_DEVICE_PATH_PROTOCOL  *DevicePath = NULL;
> +
> +  DevicePath = DevicePathFromHandle(DeviceHandle);
> +  if (DevicePath == NULL) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gTerminalTypeDeviceNode);
> +  DEBUG((EFI_D_ERROR, "%a %d: full path: %s\n", __FUNCTION__, __LINE__,
> +         ConvertDevicePathToText(DevicePath, TRUE, FALSE)
> +         ));
> +  EfiBootManagerUpdateConsoleVariable (ConOut, DevicePath, NULL);
> +  EfiBootManagerUpdateConsoleVariable (ConIn, DevicePath, NULL);
> +  EfiBootManagerUpdateConsoleVariable (ErrOut, DevicePath, NULL);
> +  return EFI_SUCCESS;
> +}
>  
>  VOID
>  PlatformInitializeConsole (
> @@ -931,6 +956,14 @@ Arguments:
>    GetEfiGlobalVariable2 (EFI_CON_OUT_VARIABLE_NAME, (VOID **) &VarConout, NULL);
>    GetEfiGlobalVariable2 (EFI_CON_IN_VARIABLE_NAME, (VOID **) &VarConin, NULL);
>  
> +  // do xen console
> +  //VISIT_PCI_INSTANCE_CALLBACK CallBackFunction
> +  VisitAllInstancesOfProtocol (
> +           &gEfiSerialIoProtocolGuid,
> +           add_serial,
> +           (VOID*)NULL
> +           );
> +
>    if (VarConout == NULL || VarConin == NULL) {
>      //
>      // Do platform specific PCI Device check and add them to ConOut, ConIn, ErrOut
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index 4a6bece..74ab6b1 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -73,6 +73,8 @@
>    gEfiLoadedImageProtocolGuid                   # PROTOCOL SOMETIMES_PRODUCED
>    gEfiFirmwareVolume2ProtocolGuid               # PROTOCOL SOMETIMES_CONSUMED
>  
> +  gEfiSerialIoProtocolGuid
> +
>  [Guids]
>    gEfiXenInfoGuid
>    gEfiEndOfDxeEventGroupGuid
> diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc
> index 31a2185..8bce996 100644
> --- a/OvmfPkg/XenOvmf.dsc
> +++ b/OvmfPkg/XenOvmf.dsc
> @@ -590,6 +590,10 @@
>    OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> +  OvmfPkg/XenConsoleIo/XenConsoleIo.inf {
> +    <LibraryClasses>
> +      SerialPortLib|OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf
> +  }
>    MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>    MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf
> index f6876d7..a40d186 100644
> --- a/OvmfPkg/XenOvmf.fdf
> +++ b/OvmfPkg/XenOvmf.fdf
> @@ -223,6 +223,7 @@ INF  OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf
>  INF  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>  INF  OvmfPkg/XenBusDxe/XenBusDxe.inf
>  INF  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> +INF  OvmfPkg/XenConsoleIo/XenConsoleIo.inf
>  
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    INF  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> 

Okay, so this patch forced me to review the current additions of serial
port devpaths to ConIn, ConOut, ErrOut in

  OvmfPkg/Library/PlatformBootManagerLib/

I think I understand what you intend to do. I'd like to propose an
alternative I feel would be better.

You are introducing a new driver called "XenConsoleIo" which I assume is
a DXE_DRIVER module that produces gEfiSerialIoProtocolGuid. The reason I
can only "assume" is that you apparently forgot to include the driver in
the patch, with "git add". Nonetheless, without having seen the driver,
I claim that it should be unnecessary.

Namely, MdeModulePkg provides a generic platform serial driver, under

  MdeModulePkg/Universal/SerialDxe/SerialDxe.inf

This driver is already used in the ARM Xen guest platform:

  ArmVirtPkg/ArmVirtXen.dsc

with a dependency on

  OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf

which you also employ above.

Delegating the actual serial port access to the SerialPortLib class, the
SerialDxe driver produces one instance of EFI_SERIAL_IO_PROTOCOL. The
device path protocol instance that it installs on the handle is
constant, and known to other edk2 modules.

(
In turn, the TerminalDxe driver layers on top of the
EFI_SERIAL_IO_PROTOCOL instance, and produces simple text in/out
protocols on a child handle. The device path node appended specifically
for the child handle determines the terminal type (e.g., vt100, pc-ansi,
etc) to emulate. The terminal type can be selected by the platform by
adding a devpath to the Con* variables that ends with the desired device
path node, or else the TerminalDxe driver can fall back to a default
type. But, I digress.
)

What this all boils down to is:

(1) Rather than introducing a new driver (again, not having it seen),
please include "MdeModulePkg/Universal/SerialDxe/SerialDxe.inf" in the
XenOvmf DSC and FDF files.

You can resolve the SerialPortLib class to XenConsoleSerialPortLib
either for this one module only, or generally (same as in
"ArmVirtPkg/ArmVirtXen.dsc").

(I assume that in a Xen HVM guest, both the emulated serial port and the
paravirt xen console are available )

(2) In the code, we shouldn't locate the appropriate handle dynamically.
The full device path is known statically, in advance. Please refer to
"mSerialConsole" in

  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c

Therefore I suggest that we add a "gXenPlatformConsole" array to

  OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c

with an initializer that effects the following:

- gXenPlatformConsole[1] is entirely zeroed,

- gXenPlatformConsole[0].ConnectType = CONSOLE_IN |
                                       CONSOLE_OUT |
                                       STD_ERROR;

- gXenPlatformConsole[0].DevicePath should be what's seen in
"mSerialConsole" above, but with the terminal type GUID pre-filled to
PC-ANSI ("gPcAnsiTerminal").

Then, we shouldn't touch the PlatformInitializeConsole() function;
instead modify the PlatformInitializeConsole() call in
PlatformBootManagerBeforeConsole() like this:

  PlatformInitializeConsole (
    XenDetected() ? gXenPlatformConsole : gPlatformConsole);

This will cause PlatformInitializeConsole() to do the right thing, and
it won't introduce special handling for gEfiSerialIoProtocolGuid that
would superfluously run on QEMU as well.

Furthermore, in a Xen HVM guest, I reckon this will multiplex terminal
IO to both emulated serial and Xen console.

Thanks
Laszlo