| Message ID | 20190409110844.14746-29-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Specific platform to run OVMF in Xen PVH and HVM guests | expand |
On 04/09/19 13:08, 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 > implemeted via SerialDxe. > > That a simple console implementation that can works on both PVH guest (1) ITYM "that *is* a simple console implementation..." > 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. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > > Notes: > 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/XenOvmf.dsc | 4 ++ > OvmfPkg/XenOvmf.fdf | 1 + > OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 4 ++ > OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 1 + > OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 10 +++- > OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c | 59 ++++++++++++++++++++ > 6 files changed, 78 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc > index 35af05715b..a26f611058 100644 > --- a/OvmfPkg/XenOvmf.dsc > +++ b/OvmfPkg/XenOvmf.dsc > @@ -599,6 +599,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/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf > index d614bdce1d..e078c7b405 100644 > --- a/OvmfPkg/XenOvmf.fdf > +++ b/OvmfPkg/XenOvmf.fdf > @@ -298,6 +298,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 (2) Do you really need all three of: - MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf - IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/IsaSerialDxe.inf - MdeModulePkg/Universal/SerialDxe/SerialDxe.inf for supporting both HVM and PVH? If possible I'd suggest just the last one, plus a general resolution of SerialPortLib to XenConsoleSerialPortLib. Or would that force HVM users to change their domU configs? (Anyway I don't feel strongly about this question, with regard to the OvmfXen platform.) > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > index c41aaeef3f..7dd0683cd2 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > @@ -67,6 +67,10 @@ [Pcd] > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut > gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## CONSUMES > > [Pcd.IA32, Pcd.X64] > gEfiMdePkgTokenSpaceGuid.PcdFSBClock (3) If possible, please keep PCDs from the space token space GUID (gEfiMdePkgTokenSpaceGuid) clustered near each other. ("Fully sorted" is optimal, but in many cases the PCD lists we have already aren't sorted, and then I prefer sorting to be a separate patch -- but that's really not necessary here.) > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h > index 4948ca6518..2ab1a76f6a 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h > +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h > @@ -172,6 +172,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 1a6d47732e..f186060f34 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > @@ -85,6 +85,13 @@ InstallDevicePathCallback ( > VOID > ); > > +STATIC > +BOOLEAN > +XenDetected ( > + VOID > + ); > + > + > VOID > PlatformRegisterFvBootOption ( > EFI_GUID *FileGuid, (4) OK, this has convinced me: (4a) please rebase PlatformBootManagerLib to XenPlatformLib first, removing the open-coded XenDetected() function -- this should be a separate patch (4b) once you do that, my concern for "v2 25/31" can also be addressed easily -- you can simply repeat the XenPvhDetected() restriction from XenPlatformPei() in PlatformBootManagerLib too. > @@ -404,7 +411,8 @@ PlatformBootManagerBeforeConsole ( > // > EfiBootManagerDispatchDeferredImages (); > > - PlatformInitializeConsole (gPlatformConsole); > + PlatformInitializeConsole ( > + XenDetected() ? gXenPlatformConsole : gPlatformConsole); > PcdStatus = PcdSet16S (PcdPlatformBootTimeOut, > GetFrontPageTimeoutFromQemu ()); > ASSERT_RETURN_ERROR (PcdStatus); Aha, so my question under (2) does make sense! If you are switching to the Xen console just based on XenDetected(), that includes HVM domUs too, and so you should be able to drop both PciSioSerialDxe and IsaSerialDxe from the OvmfXen platform. > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c > index 1250a6d351..4179370c81 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c > +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c > @@ -16,6 +16,11 @@ > #include "BdsPlatform.h" > #include <Guid/QemuRamfb.h> > > +#define SERIAL_DXE_FILE_GUID { \ > + 0xD3987D4B, 0x971A, 0x435F, \ > + { 0x8C, 0xAF, 0x49, 0x67, 0xEB, 0x62, 0x72, 0x41 } \ > + } > + > // > // Debug Agent UART Device Path structure > // Ah cool, now we have *four* separate #define's for this GUID :( - ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c - ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c - CorebootPayloadPkg/Library/PlatformBootManagerLib/PlatformConsole.c - OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c This is obviously an omission in MdeModulePkg at this point. (5) Please write a separate patch for "MdeModulePkg.dec" and "MdeModulePkg/Include/Guid/" -- given that the FILE_GUID of "MdeModulePkg/Universal/SerialDxe" has to be placed into device paths by multiple platforms, and SerialDxe calls itself "Universal", MdeModulePkg itself should offer the GUID ready for consumption. Then all three of the above (preexistent) consumers should be updated to take the GUID from that location. I suggest discussing this with the MdeModulePkg maintainers, and if they agree, I recommend implementing this GUID-extraction in an independent series, before spinning v3 of XenPVH. (In v3, the GUID should be consumed like described above.) ... Yes, it's quite arbitrary that I'm asking for the extraction of the GUID at this point, and not earlier. Sorry about that. > @@ -49,6 +54,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() > + > ACPI_HID_DEVICE_PATH gPnpPs2KeyboardDeviceNode = gPnpPs2Keyboard; > ACPI_HID_DEVICE_PATH gPnp16550ComPortDeviceNode = gPnp16550ComPort; > UART_DEVICE_PATH gUartDeviceNode = gUart; (6) Can you please insert a space between "pack" and "(1)"? > @@ -147,6 +164,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) > + } > + }, > + SERIAL_DXE_FILE_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 > // > @@ -169,6 +217,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 > // > The rest looks OK to me, although at this point my brain is mush and you could sell me anything. Stopping with the review for today. Thanks, Laszlo
diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc index 35af05715b..a26f611058 100644 --- a/OvmfPkg/XenOvmf.dsc +++ b/OvmfPkg/XenOvmf.dsc @@ -599,6 +599,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/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf index d614bdce1d..e078c7b405 100644 --- a/OvmfPkg/XenOvmf.fdf +++ b/OvmfPkg/XenOvmf.fdf @@ -298,6 +298,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 c41aaeef3f..7dd0683cd2 100644 --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf @@ -67,6 +67,10 @@ [Pcd] gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile + 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 4948ca6518..2ab1a76f6a 100644 --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h @@ -172,6 +172,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 1a6d47732e..f186060f34 100644 --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c @@ -85,6 +85,13 @@ InstallDevicePathCallback ( VOID ); +STATIC +BOOLEAN +XenDetected ( + VOID + ); + + VOID PlatformRegisterFvBootOption ( EFI_GUID *FileGuid, @@ -404,7 +411,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 1250a6d351..4179370c81 100644 --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c @@ -16,6 +16,11 @@ #include "BdsPlatform.h" #include <Guid/QemuRamfb.h> +#define SERIAL_DXE_FILE_GUID { \ + 0xD3987D4B, 0x971A, 0x435F, \ + { 0x8C, 0xAF, 0x49, 0x67, 0xEB, 0x62, 0x72, 0x41 } \ + } + // // Debug Agent UART Device Path structure // @@ -49,6 +54,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() + ACPI_HID_DEVICE_PATH gPnpPs2KeyboardDeviceNode = gPnpPs2Keyboard; ACPI_HID_DEVICE_PATH gPnp16550ComPortDeviceNode = gPnp16550ComPort; UART_DEVICE_PATH gUartDeviceNode = gUart; @@ -147,6 +164,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) + } + }, + SERIAL_DXE_FILE_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 // @@ -169,6 +217,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 //
On a Xen PVH guest, none of the existing serial or console interface works, so we add a new one, based on XenConsoleSerialPortLib, and implemeted via SerialDxe. That 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. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: 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/XenOvmf.dsc | 4 ++ OvmfPkg/XenOvmf.fdf | 1 + OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 4 ++ OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 1 + OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 10 +++- OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c | 59 ++++++++++++++++++++ 6 files changed, 78 insertions(+), 1 deletion(-)