|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [edk2-devel] [PATCH v2 29/31] OvmfPkg: Introduce XenIoPvhDxe to initialize Grant Tables
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@xxxxxxxxxx>
> ---
>
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |