| Message ID | 20190409110844.14746-30-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: > This "device" use XenIoMmioLib to reserve some space to be use by the > Grant Tables. (1) can we replace "this device" with "XenIoPvhDxe"? > > The call is only done if it is necessary, we simply detect if the guest > is probably PVH, as in this case there is currently no PCI bus, and no (2) why "probably"? I've been under the impression that XenPvhDetected() is decisive. > PCI Xen platform device which would start the XenIoPciDxe and allocate > the space for the Grant Tables. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > > Notes: > v2: > - do allocation in EntryPoint like the other user of XenIoMmioLib. > - allocate memory instead of hardcoded addr. > - cleanup, add copyright > - detect if we are running in PVH mode > > OvmfPkg/XenOvmf.dsc | 2 ++ > OvmfPkg/XenOvmf.fdf | 1 + > OvmfPkg/{XenIoPciDxe/XenIoPciDxe.inf => XenIoPvhDxe/XenIoPvhDxe.inf} | 26 +++++--------- > OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c | 38 ++++++++++++++++++++ > 4 files changed, 49 insertions(+), 18 deletions(-) > > diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc > index a26f611058..72d6ea8b29 100644 > --- a/OvmfPkg/XenOvmf.dsc > +++ b/OvmfPkg/XenOvmf.dsc > @@ -199,6 +199,7 @@ [LibraryClasses] > OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf > XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf > XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf > + XenIoMmioLib|OvmfPkg/Library/XenIoMmioLib/XenIoMmioLib.inf > > Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf > > @@ -596,6 +597,7 @@ [Components] > NULL|IntelFrameworkModulePkg/Library/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf > !endif > } > + OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf > OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf > OvmfPkg/XenBusDxe/XenBusDxe.inf > OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf > diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf > index e078c7b405..9aa998f15f 100644 > --- a/OvmfPkg/XenOvmf.fdf > +++ b/OvmfPkg/XenOvmf.fdf > @@ -295,6 +295,7 @@ [FV.DXEFV] > INF MdeModulePkg/Universal/Metronome/Metronome.inf > INF PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf > > +INF OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf > INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf > INF OvmfPkg/XenBusDxe/XenBusDxe.inf > INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf So you keep both drivers in OvmfXen. Is there any chance that both drivers will launch successfully and the protocol database ends up with two instances of gXenIoProtocolGuid? ... If the PCI device PCI_VENDOR_ID_XEN/PCI_DEVICE_ID_XEN_PLATFORM is exclusive to HVM, then I guess the mutual exclusion is guaranteed. > diff --git a/OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf > similarity index 56% > copy from OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf > copy to OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf (this is a false positive of "--find-copies-harder", OK) > index b32075a381..985b6d54b7 100644 > --- a/OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf > +++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf > @@ -1,7 +1,7 @@ > ## @file > -# Driver for the virtual Xen PCI device > +# Driver for the XenIo protocol > # > -# Copyright (C) 2015, Linaro Ltd. > +# Copyright (c) 2019, Citrix Systems, Inc. > # > # This program and the accompanying materials > # are licensed and made available under the terms and conditions of the BSD License > @@ -15,31 +15,21 @@ > > [Defines] > INF_VERSION = 0x00010005 > - BASE_NAME = XenIoPciDxe > - FILE_GUID = cf569f50-de44-4f54-b4d7-f4ae25cda599 > + BASE_NAME = XenIoPvhDxe > + FILE_GUID = 7a567cc4-0e75-4d7a-a305-c3db109b53ad > MODULE_TYPE = UEFI_DRIVER (3) Please "downgrade" this to DXE_DRIVER. This driver doesn't follow the UEFI driver model (which is fine), and there is no reason to suggest it is more than a DXE (i.e., platform) driver. For that, you'll have to introduce a [Depex] section, but I think it can be just TRUE -- I don't think you need any particular protocols (architectural or not). > VERSION_STRING = 1.0 > - ENTRY_POINT = XenIoPciDeviceEntryPoint > + ENTRY_POINT = InitializeXenIoPvhDxe > > [Packages] > MdePkg/MdePkg.dec > OvmfPkg/OvmfPkg.dec > > [Sources] > - XenIoPciDxe.c > + XenIoPvhDxe.c > > [LibraryClasses] > UefiDriverEntryPoint > - UefiBootServicesTableLib > MemoryAllocationLib > - BaseMemoryLib > - BaseLib > - UefiLib > - DebugLib > - > -[Protocols] > - gEfiDriverBindingProtocolGuid > - gEfiPciIoProtocolGuid > - gEfiComponentName2ProtocolGuid > - gEfiComponentNameProtocolGuid > - gXenIoProtocolGuid > + XenIoMmioLib > + XenPlatformLib > diff --git a/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c > new file mode 100644 > index 0000000000..f2113b768c > --- /dev/null > +++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c > @@ -0,0 +1,38 @@ > +/** @file > + > + Driver for the XenIo protocol > + > + This driver simply allocate space for the grant tables. > + > + Copyright (c) 2019, Citrix Systems, Inc. > + > + This program and the accompanying materials are licensed and made available > + under the terms and conditions of the BSD License which accompanies this > + distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ (4) please update the license block in both new files to the SPDX ID format. > + > +#include <Library/MemoryAllocationLib.h> > +#include <Library/XenIoMmioLib.h> > +#include <Library/XenPlatformLib.h> > + > +EFI_STATUS > +EFIAPI > +InitializeXenIoPvhDxe ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + if (XenPvhDetected ()) { > + EFI_HANDLE Handle; > + > + Handle = NULL; > + return XenIoMmioInstall (&Handle, (UINTN) AllocateReservedPages (4)); > + } > + > + return EFI_SUCCESS; > +} > (5) Please check the return value of AllocateReservedPages(), and exit the driver with a failure (EFI_OUT_OF_RESOURCES) if that fails. (6) Do we really need to reserve the RAM away from the OS forever? (It's fine if we do, I'm just curious.) Because, if the OS sets up its own grant tables anyway, then we could allocate EfiBootServicesData type memory here -- and perhaps add an ExitBootServices() callback to disconnect from Xen (i.e. make sure that the firmware-allocated grant tables are no longer used). ... Not sure if this makes sense (I don't remember if we discussed it under v1). (7) Please check the return value of XenIoMmioInstall(), and if it fails, release the allocated memory first, and then propagate the error as the driver's exit status. (8) If XenPvhDetected() returns FALSE, I recommend exiting the driver with EFI_UNSUPPORTED. (There's an argument to be made for exiting the driver with an error code even if we successfully call XenIoMmioInstall(), as it's not really necessary to keep the driver image in memory after that. This kind of driver is called the "initializing driver". However, I do find that non-intuitive (in particular the DXE Core will report the startup failure, and it is confusing when analyzing logs); plus if you do decide to add the EBS() callback, then the driver will have to stay resident on success. Either way returning EFI_SUCCESS on success is OK.) Thanks, Laszlo
diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc index a26f611058..72d6ea8b29 100644 --- a/OvmfPkg/XenOvmf.dsc +++ b/OvmfPkg/XenOvmf.dsc @@ -199,6 +199,7 @@ [LibraryClasses] OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf + XenIoMmioLib|OvmfPkg/Library/XenIoMmioLib/XenIoMmioLib.inf Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf @@ -596,6 +597,7 @@ [Components] NULL|IntelFrameworkModulePkg/Library/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf !endif } + OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf OvmfPkg/XenBusDxe/XenBusDxe.inf OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf index e078c7b405..9aa998f15f 100644 --- a/OvmfPkg/XenOvmf.fdf +++ b/OvmfPkg/XenOvmf.fdf @@ -295,6 +295,7 @@ [FV.DXEFV] INF MdeModulePkg/Universal/Metronome/Metronome.inf INF PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf +INF OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf INF OvmfPkg/XenBusDxe/XenBusDxe.inf INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf diff --git a/OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf similarity index 56% copy from OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf copy to OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf index b32075a381..985b6d54b7 100644 --- a/OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf +++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf @@ -1,7 +1,7 @@ ## @file -# Driver for the virtual Xen PCI device +# Driver for the XenIo protocol # -# Copyright (C) 2015, Linaro Ltd. +# Copyright (c) 2019, Citrix Systems, Inc. # # This program and the accompanying materials # are licensed and made available under the terms and conditions of the BSD License @@ -15,31 +15,21 @@ [Defines] INF_VERSION = 0x00010005 - BASE_NAME = XenIoPciDxe - FILE_GUID = cf569f50-de44-4f54-b4d7-f4ae25cda599 + BASE_NAME = XenIoPvhDxe + FILE_GUID = 7a567cc4-0e75-4d7a-a305-c3db109b53ad MODULE_TYPE = UEFI_DRIVER VERSION_STRING = 1.0 - ENTRY_POINT = XenIoPciDeviceEntryPoint + ENTRY_POINT = InitializeXenIoPvhDxe [Packages] MdePkg/MdePkg.dec OvmfPkg/OvmfPkg.dec [Sources] - XenIoPciDxe.c + XenIoPvhDxe.c [LibraryClasses] UefiDriverEntryPoint - UefiBootServicesTableLib MemoryAllocationLib - BaseMemoryLib - BaseLib - UefiLib - DebugLib - -[Protocols] - gEfiDriverBindingProtocolGuid - gEfiPciIoProtocolGuid - gEfiComponentName2ProtocolGuid - gEfiComponentNameProtocolGuid - gXenIoProtocolGuid + XenIoMmioLib + XenPlatformLib diff --git a/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c new file mode 100644 index 0000000000..f2113b768c --- /dev/null +++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c @@ -0,0 +1,38 @@ +/** @file + + Driver for the XenIo protocol + + This driver simply allocate space for the grant tables. + + Copyright (c) 2019, Citrix Systems, Inc. + + This program and the accompanying materials are licensed and made available + under the terms and conditions of the BSD License which accompanies this + distribution. The full text of the license may be found at + http://opensource.org/licenses/bsd-license.php + + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. + +**/ + +#include <Library/MemoryAllocationLib.h> +#include <Library/XenIoMmioLib.h> +#include <Library/XenPlatformLib.h> + +EFI_STATUS +EFIAPI +InitializeXenIoPvhDxe ( + IN EFI_HANDLE ImageHandle, + IN EFI_SYSTEM_TABLE *SystemTable + ) +{ + if (XenPvhDetected ()) { + EFI_HANDLE Handle; + + Handle = NULL; + return XenIoMmioInstall (&Handle, (UINTN) AllocateReservedPages (4)); + } + + return EFI_SUCCESS; +}
This "device" use XenIoMmioLib to reserve some space to be use by the Grant Tables. The call is only done if it is necessary, we simply detect if the guest is probably PVH, as in this case there is currently no PCI bus, and no PCI Xen platform device which would start the XenIoPciDxe and allocate the space for the Grant Tables. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: v2: - do allocation in EntryPoint like the other user of XenIoMmioLib. - allocate memory instead of hardcoded addr. - cleanup, add copyright - detect if we are running in PVH mode OvmfPkg/XenOvmf.dsc | 2 ++ OvmfPkg/XenOvmf.fdf | 1 + OvmfPkg/{XenIoPciDxe/XenIoPciDxe.inf => XenIoPvhDxe/XenIoPvhDxe.inf} | 26 +++++--------- OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c | 38 ++++++++++++++++++++ 4 files changed, 49 insertions(+), 18 deletions(-)