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

Commit Message

Anthony PERARD Dec. 8, 2016, 3:33 p.m. UTC

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(+)

Comments

Laszlo Ersek Jan. 5, 2017, 4:38 p.m. UTC | #1

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

Patch

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