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

["Anthony PERARD" <anthony.perard@citrix.com>]

On Wed, Jul 10, 2019 at 12:48:57PM +0200, Laszlo Ersek wrote:
> 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>
> > ---

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

I'm going to add the following to the commit message:

  Since we use VENDOR_UART_DEVICE_PATH, fix its description and
  coding style.


-- 
Anthony PERARD