Re: [PATCH v3 32/35] OvmfPkg/PlatformBootManagerLib: Use a Xen console for ConOut/ConIn

["Laszlo Ersek" <lersek@redhat.com>]

On 07/04/19 16:42, Anthony PERARD wrote:
> On a Xen PVH guest, none of the existing serial or console interface
> works, so we add a new one, based on XenConsoleSerialPortLib, and
> implemented via SerialDxe.
> 
> That is a simple console implementation that can works on both PVH
> guest and HVM guests, even if it rarely going to be use on HVM.
> 
> Have PlatformBootManagerLib look for the new console, when running as a
> Xen guest.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 
> Notes:
>     v3:
>     - removed PciSioSerialDxe and IsaSerialDxe from OvmfXen, since they
>       would not be used, maybe, to check.
>     - some coding style fix
>     
>     - not changed: PciSioSerialDxe: even if we add SerialDxe, we still needs
>       PciSioSerialDxe to have OVMF use the emulated serial port on HVM.
>     
>     v2:
>     - Use MdeModulePkg/Universal/SerialDxe instead of something new.
>     - Have PlatformInitializeConsole() look for it by using the
>       known-in-advance device path for the xen console in the
>       PLATFORM_CONSOLE_CONNECT_ENTRY.
> 
>  OvmfPkg/OvmfXen.dsc                           |  4 ++
>  OvmfPkg/OvmfXen.fdf                           |  1 +
>  .../PlatformBootManagerLib.inf                |  4 ++
>  .../PlatformBootManagerLib/BdsPlatform.h      |  1 +
>  .../PlatformBootManagerLib/BdsPlatform.c      |  3 +-
>  .../PlatformBootManagerLib/PlatformData.c     | 59 ++++++++++++++++++-
>  6 files changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 1ecae3fb45..487bada64d 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -586,6 +586,10 @@ [Components]
>    OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> +  MdeModulePkg/Universal/SerialDxe/SerialDxe.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/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
> index fa0830a324..5c1a925d6a 100644
> --- a/OvmfPkg/OvmfXen.fdf
> +++ b/OvmfPkg/OvmfXen.fdf
> @@ -312,6 +312,7 @@ [FV.DXEFV]
>  INF  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>  INF  OvmfPkg/XenBusDxe/XenBusDxe.inf
>  INF  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> +INF  MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
>  
>  INF  MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>  INF  MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index b2d3b4fb4d..646a1c522c 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -61,6 +61,10 @@ [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate         ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits         ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity           ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits         ## CONSUMES
>  
>  [Pcd.IA32, Pcd.X64]
>    gEfiMdePkgTokenSpaceGuid.PcdFSBClock
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> index 49a072b400..153e215101 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> @@ -165,6 +165,7 @@ typedef struct {
>  #define CONSOLE_IN  BIT1
>  #define STD_ERROR   BIT2
>  extern PLATFORM_CONSOLE_CONNECT_ENTRY  gPlatformConsole[];
> +extern PLATFORM_CONSOLE_CONNECT_ENTRY  gXenPlatformConsole[];
>  
>  //
>  // Platform BDS Functions
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index 9ae590293a..ee6ee6608f 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -399,7 +399,8 @@ PlatformBootManagerBeforeConsole (
>    //
>    EfiBootManagerDispatchDeferredImages ();
>  
> -  PlatformInitializeConsole (gPlatformConsole);
> +  PlatformInitializeConsole (
> +    XenDetected() ? gXenPlatformConsole : gPlatformConsole);
>    PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
>                  GetFrontPageTimeoutFromQemu ());
>    ASSERT_RETURN_ERROR (PcdStatus);
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> index 36aab784d7..a9b1fe274a 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c
> @@ -9,18 +9,19 @@
>  
>  #include "BdsPlatform.h"
>  #include <Guid/QemuRamfb.h>
> +#include <Guid/SerialPortLibVendor.h>
>  
>  //
>  // Debug Agent UART Device Path structure
>  //
> -#pragma pack(1)
> +#pragma pack (1)
>  typedef struct {
>    VENDOR_DEVICE_PATH        VendorHardware;
>    UART_DEVICE_PATH          Uart;
>    VENDOR_DEVICE_PATH        TerminalType;
>    EFI_DEVICE_PATH_PROTOCOL  End;
>  } VENDOR_UART_DEVICE_PATH;
> -#pragma pack()
> +#pragma pack ()
>  
>  //
>  // USB Keyboard Device Path structure
> @@ -43,6 +44,18 @@ typedef struct {
>  } VENDOR_RAMFB_DEVICE_PATH;
>  #pragma pack ()
>  
> +//
> +// Xen Console Device Path structure
> +//
> +#pragma pack(1)
> +typedef struct {
> +  VENDOR_DEVICE_PATH        VendorHardware;
> +  UART_DEVICE_PATH          Uart;
> +  VENDOR_DEVICE_PATH        TerminalType;
> +  EFI_DEVICE_PATH_PROTOCOL  End;
> +} XEN_CONSOLE_DEVICE_PATH;
> +#pragma pack()
> +

This version of the patch addresses all of my v2 review comments (either
by code changes or by explanations in the Notes section) -- thanks for that.

However, when you arrived at my reuqest (6) in
<http://mid.mail-archive.com/7d6adf5d-baca-7e9c-68ef-2f8479bbd902@redhat.com>,
and searched the source file for "pack(" -- in order to insert a space
character before the opening paren --, the match was *not* around the
new XEN_CONSOLE_DEVICE_PATH structure. Instead, it was around the
preexistent VENDOR_UART_DEVICE_PATH structure. And so you fixed the
style for the old code, and not the new code.

But: that's actually useful. Because now that I'm looking at
VENDOR_UART_DEVICE_PATH, it seems that we don't need the new type
XEN_CONSOLE_DEVICE_PATH at all. Is that right? So:

(1) Please drop XEN_CONSOLE_DEVICE_PATH.

(2) Please replace the comment

  Debug Agent UART Device Path structure

with

  Vendor UART Device Path structure

on VENDOR_UART_DEVICE_PATH.

(3) Please preserve the "misplaced" whitespace fix, for "pack(", around
VENDOR_UART_DEVICE_PATH.

(4) Please use VENDOR_UART_DEVICE_PATH as the type of gXenConsoleDevicePath.

With those:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

>  ACPI_HID_DEVICE_PATH       gPnpPs2KeyboardDeviceNode  = gPnpPs2Keyboard;
>  ACPI_HID_DEVICE_PATH       gPnp16550ComPortDeviceNode = gPnp16550ComPort;
>  UART_DEVICE_PATH           gUartDeviceNode            = gUart;
> @@ -141,6 +154,37 @@ STATIC VENDOR_RAMFB_DEVICE_PATH gQemuRamfbDevicePath = {
>    gEndEntire
>  };
>  
> +STATIC XEN_CONSOLE_DEVICE_PATH gXenConsoleDevicePath = {
> +  {
> +    {
> +      HARDWARE_DEVICE_PATH,
> +      HW_VENDOR_DP,
> +      {
> +        (UINT8) (sizeof (VENDOR_DEVICE_PATH)),
> +        (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> +      }
> +    },
> +    EDKII_SERIAL_PORT_LIB_VENDOR_GUID
> +  },
> +  {
> +    {
> +      MESSAGING_DEVICE_PATH,
> +      MSG_UART_DP,
> +      {
> +        (UINT8) (sizeof (UART_DEVICE_PATH)),
> +        (UINT8) ((sizeof (UART_DEVICE_PATH)) >> 8)
> +      }
> +    },
> +    0,
> +    FixedPcdGet64 (PcdUartDefaultBaudRate),
> +    FixedPcdGet8 (PcdUartDefaultDataBits),
> +    FixedPcdGet8 (PcdUartDefaultParity),
> +    FixedPcdGet8 (PcdUartDefaultStopBits),
> +  },
> +  gPcAnsiTerminal,
> +  gEndEntire
> +};
> +
>  //
>  // Predefined platform default console device path
>  //
> @@ -163,6 +207,17 @@ PLATFORM_CONSOLE_CONNECT_ENTRY   gPlatformConsole[] = {
>    }
>  };
>  
> +PLATFORM_CONSOLE_CONNECT_ENTRY   gXenPlatformConsole[] = {
> +  {
> +    (EFI_DEVICE_PATH_PROTOCOL *)&gXenConsoleDevicePath,
> +    (CONSOLE_OUT | CONSOLE_IN | STD_ERROR)
> +  },
> +  {
> +    NULL,
> +    0
> +  }
> +};
> +
>  //
>  // Predefined platform connect sequence
>  //
>