XenIoPvhDxe 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 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.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
v3:
- downgrade type to DXE_DRIVER
- use SPDX
- rework InitializeXenIoPvhDxe, and handle errors properly.
- Free the reserved allocation in ExitBootServices even if the XenIo
protocol could successfully been uninstalled.
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/OvmfXen.dsc | 2 +
OvmfPkg/OvmfXen.fdf | 1 +
OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf | 34 +++++++++
OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c | 108 ++++++++++++++++++++++++++++
4 files changed, 145 insertions(+)
create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 487bada64d..af92ce3ed2 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -196,6 +196,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
@@ -583,6 +584,7 @@ [Components]
NULL|OvmfPkg/Csm/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf
!endif
}
+ OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
OvmfPkg/XenBusDxe/XenBusDxe.inf
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
index 5c1a925d6a..517a492f14 100644
--- a/OvmfPkg/OvmfXen.fdf
+++ b/OvmfPkg/OvmfXen.fdf
@@ -309,6 +309,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/XenIoPvhDxe/XenIoPvhDxe.inf b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
new file mode 100644
index 0000000000..a093d48fde
--- /dev/null
+++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
@@ -0,0 +1,34 @@
+## @file
+# Driver for the XenIo protocol
+#
+# Copyright (c) 2019, Citrix Systems, Inc.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = XenIoPvhDxe
+ FILE_GUID = 7a567cc4-0e75-4d7a-a305-c3db109b53ad
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = InitializeXenIoPvhDxe
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[Sources]
+ XenIoPvhDxe.c
+
+[LibraryClasses]
+ DebugLib
+ MemoryAllocationLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+ XenIoMmioLib
+ XenPlatformLib
+
+[Depex]
+ TRUE
diff --git a/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c
new file mode 100644
index 0000000000..88a394bf91
--- /dev/null
+++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c
@@ -0,0 +1,108 @@
+/** @file
+
+ Driver for the XenIo protocol
+
+ This driver simply allocate space for the grant tables.
+
+ Copyright (c) 2019, Citrix Systems, Inc.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/DebugLib.h>
+#include <Library/XenIoMmioLib.h>
+#include <Library/XenPlatformLib.h>
+
+typedef struct {
+ EFI_HANDLE XenIoHandle;
+ EFI_EVENT ExitBootEvent;
+ VOID *Allocation;
+} XEN_IO_PVH_STATE;
+
+//
+// Value should be the same as NR_GRANT_FRAMES in XenBusDxe
+//
+#define XEN_GRANT_FRAMES 4
+
+STATIC
+VOID
+EFIAPI
+XenIoPvhDxeNotifyExitBoot (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ XEN_IO_PVH_STATE *State;
+ EFI_STATUS Status;
+
+ State = Context;
+
+ gBS->CloseEvent(&State->ExitBootEvent);
+ Status = XenIoMmioUninstall(State->XenIoHandle);
+ if (Status == EFI_SUCCESS) {
+ //
+ // Only free the reserved space for grant table if no driver is using it.
+ //
+ FreePages (State->Allocation, XEN_GRANT_FRAMES);
+ }
+ FreePool (State);
+}
+
+EFI_STATUS
+EFIAPI
+InitializeXenIoPvhDxe (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ VOID *Allocation;
+ EFI_STATUS Status;
+ XEN_IO_PVH_STATE *State;
+
+ State = NULL;
+ Allocation = NULL;
+
+ if (! XenPvhDetected ()) {
+ return EFI_UNSUPPORTED;
+ }
+
+ State = AllocatePool (sizeof (*State));
+ if (State == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Error;
+ }
+
+ Allocation = AllocateReservedPages (XEN_GRANT_FRAMES);
+ if (Allocation == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Error;
+ }
+
+ State->XenIoHandle = NULL;
+ Status = XenIoMmioInstall (&State->XenIoHandle, (UINTN) Allocation);
+ if (EFI_ERROR (Status)) {
+ goto Error;
+ }
+
+ State->Allocation = Allocation;
+ Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
+ XenIoPvhDxeNotifyExitBoot, State, &State->ExitBootEvent);
+ ASSERT_EFI_ERROR (Status);
+
+ return EFI_SUCCESS;
+
+Error:
+ if (State != NULL && State->XenIoHandle != NULL) {
+ XenIoMmioUninstall(State->XenIoHandle);
+ }
+ if (Allocation != NULL) {
+ FreePages (Allocation, XEN_GRANT_FRAMES);
+ }
+ if (State != NULL) {
+ FreePool (State);
+ }
+ return Status;
+}
--
Anthony PERARD
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#43310): https://edk2.groups.io/g/devel/message/43310
Mute This Topic: https://groups.io/mt/32308734/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 07/04/19 16:42, Anthony PERARD wrote:
> XenIoPvhDxe 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 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.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>
> Notes:
> v3:
> - downgrade type to DXE_DRIVER
> - use SPDX
> - rework InitializeXenIoPvhDxe, and handle errors properly.
> - Free the reserved allocation in ExitBootServices even if the XenIo
> protocol could successfully been uninstalled.
>
> 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/OvmfXen.dsc | 2 +
> OvmfPkg/OvmfXen.fdf | 1 +
> OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf | 34 +++++++++
> OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c | 108 ++++++++++++++++++++++++++++
> 4 files changed, 145 insertions(+)
> create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
> create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c
>
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 487bada64d..af92ce3ed2 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -196,6 +196,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
>
> @@ -583,6 +584,7 @@ [Components]
> NULL|OvmfPkg/Csm/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf
> !endif
> }
> + OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
> OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
> OvmfPkg/XenBusDxe/XenBusDxe.inf
> OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
> index 5c1a925d6a..517a492f14 100644
> --- a/OvmfPkg/OvmfXen.fdf
> +++ b/OvmfPkg/OvmfXen.fdf
> @@ -309,6 +309,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/XenIoPvhDxe/XenIoPvhDxe.inf b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
> new file mode 100644
> index 0000000000..a093d48fde
> --- /dev/null
> +++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
> @@ -0,0 +1,34 @@
> +## @file
> +# Driver for the XenIo protocol
> +#
> +# Copyright (c) 2019, Citrix Systems, Inc.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010005
> + BASE_NAME = XenIoPvhDxe
> + FILE_GUID = 7a567cc4-0e75-4d7a-a305-c3db109b53ad
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> + ENTRY_POINT = InitializeXenIoPvhDxe
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[Sources]
> + XenIoPvhDxe.c
> +
> +[LibraryClasses]
> + DebugLib
> + MemoryAllocationLib
> + UefiBootServicesTableLib
> + UefiDriverEntryPoint
> + XenIoMmioLib
> + XenPlatformLib
> +
> +[Depex]
> + TRUE
> diff --git a/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c
> new file mode 100644
> index 0000000000..88a394bf91
> --- /dev/null
> +++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c
> @@ -0,0 +1,108 @@
> +/** @file
> +
> + Driver for the XenIo protocol
> +
> + This driver simply allocate space for the grant tables.
> +
> + Copyright (c) 2019, Citrix Systems, Inc.
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/XenIoMmioLib.h>
> +#include <Library/XenPlatformLib.h>
> +
> +typedef struct {
> + EFI_HANDLE XenIoHandle;
> + EFI_EVENT ExitBootEvent;
> + VOID *Allocation;
> +} XEN_IO_PVH_STATE;
> +
> +//
> +// Value should be the same as NR_GRANT_FRAMES in XenBusDxe
> +//
> +#define XEN_GRANT_FRAMES 4
In v2, this was a "naked" integer constant, and I didn't realize it was
supposed to match XenBusDxe.
(1) Please insert a separate patch before this patch, that turns the
value 4 in XenBusDxe into a FixedAtBuild PCD, in "OvmfPkg.dec":
[PcdsFixedAtBuild]
## Number of page frames to use for storing grant table entries.
# Must be less than or equal to what is configured in the Xen HV.
gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
In "XenBusDxe.inf", introduce a [FixedPcd] section, for expressing the
dependency on the PCD.
And in the code, use
FixedPcdGet32 (PcdXenGrantFrames)
This will expand to an integer constant, and so it is suitable for use
in further #define's.
> +
> +STATIC
> +VOID
> +EFIAPI
> +XenIoPvhDxeNotifyExitBoot (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> + )
> +{
> + XEN_IO_PVH_STATE *State;
> + EFI_STATUS Status;
> +
> + State = Context;
> +
> + gBS->CloseEvent(&State->ExitBootEvent);
> + Status = XenIoMmioUninstall(State->XenIoHandle);
> + if (Status == EFI_SUCCESS) {
> + //
> + // Only free the reserved space for grant table if no driver is using it.
> + //
> + FreePages (State->Allocation, XEN_GRANT_FRAMES);
> + }
> + FreePool (State);
> +}
In my v2 review at
http://mid.mail-archive.com/2c386393-b886-a7e7-e8b9-c5e339c94727@redhat.com
I asked, in point (6), whether we needed the memory allocation to be
"reserved". I guess this change (in v3) is supposed to address that.
(And, indeed, version 3 of this patch correctly addresses all other
points from my v2 review.)
However: an ExitBootServices notification function *must not* perform
actions that could change the UEFI memory map. Thus, you can not release
memory there -- you can't call CloseEvent(),
UninstallMultipleProtocolInterfaces(), FreePool(), FreePages(), and so on.
In my earlier review, I wrote,
> [...] then we could allocate EfiBootServicesData type memory here --
> and perhaps add an ExitBootServices() callback to disconnect from Xen
> [...]
By "disconnect", I meant actions that do *not* include memory allocation
/ release, but only make the hypervisor *forget* its guest RAM
references that it currently holds.
ExitBootServices() handlers (notification functions) are different from
normal "teardown" functions. A "teardown" function generally mirrors a
"construct" function, where you remove all references, and release all
memory allocations, in precise reverse order of construction.
ExitBootServices() handlers perform a *subsequence* of that (usually
preserving the same relative order between the steps): you only erase
external references to RAM -- that is, RAM references that are held by
external entities, such as PCI devices (in-flight DMA), the hypervisor,
and so on. The referred-to RAM, originally allocated as "boot services
data", will be released later, whole-sale, by the OS.
In that sense, ExitBootServices() callbacks implement a kind of
ownership transfer. You no longer own the UEFI memory map, you just have
to kick out such external references into it that the OS would not know
about.
To summarize:
- If this memory *needs* to be reserved past ExitBootServices(), then
stick with AllocateReservedPages(). Otherwise, use AllocatePages(); then
the pages will be freed automatically by the OS, soon after it is started.
- In the latter case, if necessary, you can make the hypervisor forget
about the area, as part of ExitBootServices(), in a notification
function. But that can only happen without allocating or releasing
memory in the notification function.
... I guess you could make a XENMEM_remove_from_physmap hypercall, for
example. But, it's *entirely* possible that said hypercall doesn't
belong in *this* driver. After all, this is not the driver that calls
XENMEM_add_to_physmap either!
... In fact, I think XenBusDxe has a bug. Right now, you have the
following call chain, when the OS calls ExitBootServices():
NotifyExitBoot()
gBS->DisconnectController()
XenBusDxeDriverBindingStop()
XenGrantTableDeinit()
XenHypercallMemoryOp (XENMEM_remove_from_physmap)
Unfortunately, this call tree releases memory left and right, and so it
is not valid on the stack of gBS->ExitBootServices().
Instead, NotifyExitBoot() should collect just the subsequence of
XenBusDxeDriverBindingStop() -- interpreting that function as a flatened
sequence of operations -- that removes external references to RAM.
Making hypercalls (using parameters constructed on the stack) is fine.
Using dynamically *pre*allocated memory (for constructing hypercall
arguments) is also fine. Dynamically allocating or releasing memory *on
the spot* is not fine.
(2) So, for now, I'd recommend simply removing
XenIoPvhDxeNotifyExitBoot(), in this patch.
You might want to fix XenBusDxe's NotifyExitBoot(), separately.
> +
> +EFI_STATUS
> +EFIAPI
> +InitializeXenIoPvhDxe (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + VOID *Allocation;
> + EFI_STATUS Status;
> + XEN_IO_PVH_STATE *State;
> +
> + State = NULL;
> + Allocation = NULL;
> +
> + if (! XenPvhDetected ()) {
(3) Please drop the space character. It's not invalid, but quite foreign
in edk2, I'd say.
> + return EFI_UNSUPPORTED;
> + }
> +
> + State = AllocatePool (sizeof (*State));
(4) What do we gain by allocating "State" dynamically? IMO, it could be
a local variable (= "automatic storage duration").
> + if (State == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto Error;
> + }
> +
> + Allocation = AllocateReservedPages (XEN_GRANT_FRAMES);
(5) So, again, please evaluate if this could simply be AllocatePages().
Thanks
Laszlo
> + if (Allocation == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto Error;
> + }
> +
> + State->XenIoHandle = NULL;
> + Status = XenIoMmioInstall (&State->XenIoHandle, (UINTN) Allocation);
> + if (EFI_ERROR (Status)) {
> + goto Error;
> + }
> +
> + State->Allocation = Allocation;
> + Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> + XenIoPvhDxeNotifyExitBoot, State, &State->ExitBootEvent);
> + ASSERT_EFI_ERROR (Status);
> +
> + return EFI_SUCCESS;
> +
> +Error:
> + if (State != NULL && State->XenIoHandle != NULL) {
> + XenIoMmioUninstall(State->XenIoHandle);
> + }
> + if (Allocation != NULL) {
> + FreePages (Allocation, XEN_GRANT_FRAMES);
> + }
> + if (State != NULL) {
> + FreePool (State);
> + }
> + return Status;
> +}
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#43480): https://edk2.groups.io/g/devel/message/43480
Mute This Topic: https://groups.io/mt/32308734/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Wed, Jul 10, 2019 at 04:05:02PM +0200, Laszlo Ersek wrote:
> On 07/04/19 16:42, Anthony PERARD wrote:
> > +
> > +STATIC
> > +VOID
> > +EFIAPI
> > +XenIoPvhDxeNotifyExitBoot (
> > + IN EFI_EVENT Event,
> > + IN VOID *Context
> > + )
> > +{
> > + XEN_IO_PVH_STATE *State;
> > + EFI_STATUS Status;
> > +
> > + State = Context;
> > +
> > + gBS->CloseEvent(&State->ExitBootEvent);
> > + Status = XenIoMmioUninstall(State->XenIoHandle);
> > + if (Status == EFI_SUCCESS) {
> > + //
> > + // Only free the reserved space for grant table if no driver is using it.
> > + //
> > + FreePages (State->Allocation, XEN_GRANT_FRAMES);
> > + }
> > + FreePool (State);
> > +}
>
> In my v2 review at
>
> http://mid.mail-archive.com/2c386393-b886-a7e7-e8b9-c5e339c94727@redhat.com
>
> I asked, in point (6), whether we needed the memory allocation to be
> "reserved". I guess this change (in v3) is supposed to address that.
> (And, indeed, version 3 of this patch correctly addresses all other
> points from my v2 review.)
>
> However: an ExitBootServices notification function *must not* perform
> actions that could change the UEFI memory map. Thus, you can not release
> memory there -- you can't call CloseEvent(),
> UninstallMultipleProtocolInterfaces(), FreePool(), FreePages(), and so on.
>
> In my earlier review, I wrote,
>
> > [...] then we could allocate EfiBootServicesData type memory here --
> > and perhaps add an ExitBootServices() callback to disconnect from Xen
> > [...]
>
> By "disconnect", I meant actions that do *not* include memory allocation
> / release, but only make the hypervisor *forget* its guest RAM
> references that it currently holds.
>
> ExitBootServices() handlers (notification functions) are different from
> normal "teardown" functions. A "teardown" function generally mirrors a
> "construct" function, where you remove all references, and release all
> memory allocations, in precise reverse order of construction.
>
> ExitBootServices() handlers perform a *subsequence* of that (usually
> preserving the same relative order between the steps): you only erase
> external references to RAM -- that is, RAM references that are held by
> external entities, such as PCI devices (in-flight DMA), the hypervisor,
> and so on. The referred-to RAM, originally allocated as "boot services
> data", will be released later, whole-sale, by the OS.
>
> In that sense, ExitBootServices() callbacks implement a kind of
> ownership transfer. You no longer own the UEFI memory map, you just have
> to kick out such external references into it that the OS would not know
> about.
>
> To summarize:
>
> - If this memory *needs* to be reserved past ExitBootServices(), then
> stick with AllocateReservedPages(). Otherwise, use AllocatePages(); then
> the pages will be freed automatically by the OS, soon after it is started.
>
> - In the latter case, if necessary, you can make the hypervisor forget
> about the area, as part of ExitBootServices(), in a notification
> function. But that can only happen without allocating or releasing
> memory in the notification function.
>
>
> ... I guess you could make a XENMEM_remove_from_physmap hypercall, for
> example. But, it's *entirely* possible that said hypercall doesn't
> belong in *this* driver. After all, this is not the driver that calls
> XENMEM_add_to_physmap either!
>
> ... In fact, I think XenBusDxe has a bug. Right now, you have the
> following call chain, when the OS calls ExitBootServices():
>
> NotifyExitBoot()
> gBS->DisconnectController()
> XenBusDxeDriverBindingStop()
> XenGrantTableDeinit()
> XenHypercallMemoryOp (XENMEM_remove_from_physmap)
>
> Unfortunately, this call tree releases memory left and right, and so it
> is not valid on the stack of gBS->ExitBootServices().
>
> Instead, NotifyExitBoot() should collect just the subsequence of
> XenBusDxeDriverBindingStop() -- interpreting that function as a flatened
> sequence of operations -- that removes external references to RAM.
> Making hypercalls (using parameters constructed on the stack) is fine.
> Using dynamically *pre*allocated memory (for constructing hypercall
> arguments) is also fine. Dynamically allocating or releasing memory *on
> the spot* is not fine.
>
> (2) So, for now, I'd recommend simply removing
> XenIoPvhDxeNotifyExitBoot(), in this patch.
Ok, I'll remove it.
> You might want to fix XenBusDxe's NotifyExitBoot(), separately.
I'll put that on my TODO list.
> > + return EFI_UNSUPPORTED;
> > + }
> > +
> > + State = AllocatePool (sizeof (*State));
>
> (4) What do we gain by allocating "State" dynamically? IMO, it could be
> a local variable (= "automatic storage duration").
Without the exit boot notification, State isn't needed anymore, so I'll
remove it.
> > + if (State == NULL) {
> > + Status = EFI_OUT_OF_RESOURCES;
> > + goto Error;
> > + }
> > +
> > + Allocation = AllocateReservedPages (XEN_GRANT_FRAMES);
>
> (5) So, again, please evaluate if this could simply be AllocatePages().
I would prefer to let know the operating system that those pages are
potentially used, unless I'm sure OVMF has let know Xen that those
pages don't contain the grant table anymore.
I'll probably try to make some changes in OVMF to allow to give back
those pages, but for now I think keeping those pages as reserved will
be good enough.
Thanks,
--
Anthony PERARD
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44462): https://edk2.groups.io/g/devel/message/44462
Mute This Topic: https://groups.io/mt/32308734/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 07/26/19 18:06, Anthony PERARD wrote:
> On Wed, Jul 10, 2019 at 04:05:02PM +0200, Laszlo Ersek wrote:
>> On 07/04/19 16:42, Anthony PERARD wrote:
>>> + if (State == NULL) {
>>> + Status = EFI_OUT_OF_RESOURCES;
>>> + goto Error;
>>> + }
>>> +
>>> + Allocation = AllocateReservedPages (XEN_GRANT_FRAMES);
>>
>> (5) So, again, please evaluate if this could simply be AllocatePages().
>
> I would prefer to let know the operating system that those pages are
> potentially used, unless I'm sure OVMF has let know Xen that those
> pages don't contain the grant table anymore.
>
> I'll probably try to make some changes in OVMF to allow to give back
> those pages, but for now I think keeping those pages as reserved will
> be good enough.
Fair enough!
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44464): https://edk2.groups.io/g/devel/message/44464
Mute This Topic: https://groups.io/mt/32308734/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.